Browse Source

fix(notifications): accept discordapp.com webhook URLs (#1363)

  Discord's "Copy Webhook URL" button emits discordapp.com URLs; both
  hostnames serve the same webhooks. The validation now accepts either
  prefix while keeping the check itself in place to catch the
  paste-the-wrong-thing error.
maziggy 1 week ago
parent
commit
5e88ce13f0

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 4 - 1
backend/app/services/notification_service.py

@@ -418,7 +418,10 @@ class NotificationService:
         if not webhook_url:
         if not webhook_url:
             return False, "Webhook URL is required"
             return False, "Webhook URL is required"
 
 
-        if not webhook_url.startswith("https://discord.com/api/webhooks/"):
+        if not (
+            webhook_url.startswith("https://discord.com/api/webhooks/")
+            or webhook_url.startswith("https://discordapp.com/api/webhooks/")
+        ):
             return False, "Invalid Discord webhook URL"
             return False, "Invalid Discord webhook URL"
 
 
         # Discord embed format for nicer messages
         # Discord embed format for nicer messages

+ 26 - 3
backend/tests/integration/test_security.py

@@ -1319,7 +1319,7 @@ class TestEncryptLegacyMigration:
 
 
     @pytest.mark.asyncio
     @pytest.mark.asyncio
     @pytest.mark.integration
     @pytest.mark.integration
-    async def test_init_db_propagates_unexpected_migration_error(self, monkeypatch):
+    async def test_init_db_propagates_unexpected_migration_error(self, monkeypatch, tmp_path):
         """B3: an unexpected error from _migrate_encrypt_legacy_secrets must
         """B3: an unexpected error from _migrate_encrypt_legacy_secrets must
         surface (re-raise) instead of being silently swallowed.
         surface (re-raise) instead of being silently swallowed.
 
 
@@ -1331,7 +1331,27 @@ class TestEncryptLegacyMigration:
         rather than poking the inner read phase, because that is the contract
         rather than poking the inner read phase, because that is the contract
         boundary the rest of the codebase relies on (init_db -> migration).
         boundary the rest of the codebase relies on (init_db -> migration).
         """
         """
+        from sqlalchemy import event
+        from sqlalchemy.ext.asyncio import create_async_engine
+
         import backend.app.core.database as db_mod
         import backend.app.core.database as db_mod
+        from backend.app.core.config import settings
+
+        # init_db() uses the module-level `engine`, which was bound at import
+        # time to settings.database_url — that resolves to the real shared
+        # bambuddy.db at the project root (or, when DATABASE_URL is set, the
+        # configured Postgres). The autouse DATA_DIR fixture runs too late to
+        # influence either. Letting this test write to that real DB makes it
+        # (a) non-hermetic and (b) flake under `-n 30` with "database is
+        # locked" when two workers race on the file. Substitute an isolated
+        # per-test SQLite engine — and override settings.database_url for
+        # this test so the is_sqlite() / is_postgres() dialect guards inside
+        # run_migrations pick the SQLite path against this engine.
+        test_db_url = f"sqlite+aiosqlite:///{tmp_path / 'init_db_test.db'}"
+        test_engine = create_async_engine(test_db_url, echo=False)
+        event.listen(test_engine.sync_engine, "connect", db_mod._set_sqlite_pragmas)
+        monkeypatch.setattr(db_mod, "engine", test_engine)
+        monkeypatch.setattr(settings, "database_url", test_db_url)
 
 
         async def boom():
         async def boom():
             raise RuntimeError("simulated startup-fatal failure")
             raise RuntimeError("simulated startup-fatal failure")
@@ -1346,8 +1366,11 @@ class TestEncryptLegacyMigration:
         monkeypatch.setattr(db_mod, "seed_spool_catalog", lambda: _noop_async())
         monkeypatch.setattr(db_mod, "seed_spool_catalog", lambda: _noop_async())
         monkeypatch.setattr(db_mod, "seed_color_catalog", lambda: _noop_async())
         monkeypatch.setattr(db_mod, "seed_color_catalog", lambda: _noop_async())
 
 
-        with pytest.raises(RuntimeError, match="simulated startup-fatal failure"):
-            await db_mod.init_db()
+        try:
+            with pytest.raises(RuntimeError, match="simulated startup-fatal failure"):
+                await db_mod.init_db()
+        finally:
+            await test_engine.dispose()
 
 
 
 
 async def _noop_async():
 async def _noop_async():

+ 52 - 0
backend/tests/unit/services/test_notification_service.py

@@ -670,6 +670,58 @@ class TestNotificationProviderTypes:
             assert "image" not in payload
             assert "image" not in payload
 
 
 
 
+class TestDiscordProvider:
+    """Discord webhook URL host validation (#1363)."""
+
+    @pytest.fixture
+    def service(self):
+        return NotificationService()
+
+    @pytest.mark.asyncio
+    async def test_discord_accepts_discord_com_url(self, service):
+        config = {"webhook_url": "https://discord.com/api/webhooks/123/abc"}
+        mock_response = MagicMock()
+        mock_response.status_code = 204
+        mock_client = AsyncMock()
+        mock_client.post = AsyncMock(return_value=mock_response)
+
+        with patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get_client:
+            mock_get_client.return_value = mock_client
+            success, _ = await service._send_discord(config, "Title", "Body")
+
+        assert success is True
+        mock_client.post.assert_called_once()
+
+    @pytest.mark.asyncio
+    async def test_discord_accepts_legacy_discordapp_com_url(self, service):
+        """Discord's 'Copy Webhook URL' button emits discordapp.com URLs (#1363)."""
+        config = {"webhook_url": "https://discordapp.com/api/webhooks/123/abc"}
+        mock_response = MagicMock()
+        mock_response.status_code = 204
+        mock_client = AsyncMock()
+        mock_client.post = AsyncMock(return_value=mock_response)
+
+        with patch.object(service, "_get_client", new_callable=AsyncMock) as mock_get_client:
+            mock_get_client.return_value = mock_client
+            success, _ = await service._send_discord(config, "Title", "Body")
+
+        assert success is True
+        mock_client.post.assert_called_once()
+
+    @pytest.mark.asyncio
+    async def test_discord_rejects_non_discord_host(self, service):
+        config = {"webhook_url": "https://evil.example.com/api/webhooks/123/abc"}
+        success, message = await service._send_discord(config, "Title", "Body")
+        assert success is False
+        assert "Invalid Discord webhook URL" in message
+
+    @pytest.mark.asyncio
+    async def test_discord_rejects_empty_url(self, service):
+        success, message = await service._send_discord({"webhook_url": ""}, "Title", "Body")
+        assert success is False
+        assert "required" in message.lower()
+
+
 class TestNtfyPriority:
 class TestNtfyPriority:
     """Per-event ntfy Priority header (#990)."""
     """Per-event ntfy Priority header (#990)."""
 
 

Some files were not shown because too many files changed in this diff