Conversation
Transport exceptions may contain webhook URLs or bot tokens. Return a stable public error and reject non-object update payloads.
…ields Follow-up to PR #94. ChatWebhookService.update() decrypted every stored credential and re-encrypted the lot on any credential-bearing request. decrypt_secret_safe() returns its input unchanged when a value will not decrypt, so a wrong-key or not-yet-migrated row got wrapped a second time and became unrecoverable -- and it happened to fields the request never named. It now patches the ciphertext dict directly and encrypts only what it assigns, so untouched credentials survive byte-identical. Also tolerate an unchanged is_default in a round-tripped body (to_dict() always emits it, so GET -> edit -> PUT rejected an untouched field), and record the delivery failure reason in the server log. The reason stays out of the HTTP response: transport errors embed the full webhook URL and any Telegram bot token. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
js-yaml was pinned to 4.3.0 in the overrides block, so `npm audit fix` could never reach it -- the advisory (GHSA-5p4m-2wfm-xmqj, quadratic CPU in !!omap) sat permanently HIGH and masked anything landing behind it. Bumping the pin to 4.3.1 keeps the deliberate supply-chain pin and clears it. The other five were stale-lockfile problems: every fixed version already satisfied its parent's declared range, so re-resolution was enough and no dependency range changed. js-yaml 4.3.0 -> 4.3.1 GHSA-5p4m-2wfm-xmqj socket.io-parser 4.2.6 -> 4.2.7 GHSA-2m8v-j782-fhvr react-router 7.18.1 -> 7.18.2 GHSA-qwww-vcr4-c8h2 nanoid 3.3.16 -> 3.3.18 GHSA-2v37-7h3g-55p8 brace-expansion 1.1.16 -> 1.1.18 GHSA-mh99-v99m-4gvg, GHSA-rgw5-rvv9-x895 Only one of the six is reachable in a shipped panel at all: socket.io-parser runs in the browser, though an attacker must already control or MITM the panel's own websocket and the payoff is a hung operator tab. The react-router CSRF bypass is RSC-mode only and this is a declarative BrowserRouter SPA with no data router; js-yaml and brace-expansion are eslint-only; nanoid is build-time via postcss. Hygiene, not incident response. package.json and package-lock.json move together -- `npm ci` runs in frontend-ci.yml and release-smoke.yml and hard-fails if they disagree. Verified: npm audit reports 0 vulnerabilities, npm run lint passes with 0 errors, npm run build succeeds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The floor had drifted. It was last set to 3234 at 9fb0a6c; the rounds since then added tests without bumping it, so it sat ~117 below what CI actually collected. The ratchet only fails on a *drop*, so the gap was silent -- and a floor that far below reality stops catching the accidental deletions it exists for. Collection is deterministic between a dev box and CI here, so the local count is the right value: every file under backend/tests/ is tracked (only __pycache__ is ignored), and the one filesystem-globbing parametrization, test_extension_manifests.py::_all_manifest_paths, walks builtin-extensions/ and frontend/src/plugins/ where all 16 manifests are tracked. Locally installed extensions change which tests *skip*, never how many are collected. Note when re-running this: check_test_count.py collects with a bare `pytest`, while backend-ci.yml deliberately runs `pytest tests` because a bare collect also walks locally deployed apps under backend/dev-data/. Both report 3379 today, but only because dev-data errors out of collection rather than adding to it -- don't rely on that holding. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Completes the backend contract for org chat/webhook connections by implementing update, test, and default-selection operations that the frontend API has already been calling, and refreshes the frontend lockfile/overrides to clear npm advisories.
Changes:
- Added backend service + admin API routes for updating chat connections, testing a connection synchronously, and explicitly selecting the per-kind default connection.
- Extended backend test coverage around credential patch semantics, default selection behavior, and test-result persistence/sanitized responses.
- Cleared npm advisories via
js-yamloverride bump and lockfile re-resolution; bumped app/test baseline versions and updated.gitignore.
Reviewed changes
Copilot reviewed 7 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| VERSION | Bumps ServerKit version to 1.7.92. |
| frontend/package.json | Updates js-yaml override to 4.3.1 to address advisory. |
| frontend/package-lock.json | Re-resolves vulnerable transitive deps to patched versions. |
| backend/app/services/chat_webhook_service.py | Implements update, test, and set_default behaviors for chat connections. |
| backend/app/api/notifications.py | Adds admin routes for PUT update, POST test, and POST default selection. |
| backend/app/models/chat_webhook.py | Clarifies is_default semantics comment for chat connections. |
| backend/tests/test_notification_chat_connections.py | Adds coverage for update semantics, default scoping, and connection testing behavior. |
| backend/tests/BASELINE_COUNT | Raises baseline count ratchet to current expected value. |
| .gitignore | Ignores .worktrees/. |
Files not reviewed (1)
- frontend/package-lock.json: Generated file
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+139
to
+140
| credentials = conn.raw_credentials() | ||
| if conn.kind == 'telegram': |
Comment on lines
+307
to
+312
| logger.warning( | ||
| 'Chat connection test failed (id=%s, kind=%s, reason=%s)', | ||
| conn.id, | ||
| conn.kind, | ||
| getattr(delivery_result, 'error', None), | ||
| ) |
Review catch on #97. The chat connection test failure log interpolated DeliveryResult.error verbatim. _deliver_webhook builds that from str(exc) on a network error, and requests embeds the full URL in its exception text -- so the line could carry a Discord/Slack/Teams webhook token or a Telegram bot token, all of which live in the URL *path*. to_dict() masks `destination` even from an admin, and /api/logs serves the log files back to the panel, so the log line leaked past the API's own masking. Dropping the reason would have restored the earlier problem of a failed test button leaving no diagnostics anywhere, so it is redacted instead: URLs keep scheme and host and lose the path, and bare bot tokens are substituted. The host is what makes the line worth keeping and is not a secret. Also normalise ChatWebhookConnection.raw_credentials() to {} when the stored JSON is well-formed but not an object. A hand-edited or corrupt row could hold `[]`, which every caller then treated as a mapping -- credentials() called .items() on it, so to_dict() and therefore the list endpoint raised before the write path was even reachable. Guarding at the source covers all callers and matches the existing categories() pattern. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
.gitattributes pins the whole repo to `text=auto eol=lf`, but plugin_service wrote the tracked manifests through Python's text mode, which translates \n to \r\n on Windows. Any install on a Windows dev box therefore rewrote frontend/src/plugins/*/plugin.json with CRLF, and the backend suite -- which installs builtin extensions -- left four modified manifests behind on every run. The diff showed no content change, which made it look like unrelated churn and made it easy to sweep the files into an unrelated commit. This is the same failure the comment above the write already documents: the trailing-newline half was found and fixed earlier, the line-ending half was missed. Both halves now live in the same explanation. _regenerate_frontend_manifest's write gets the same treatment for consistency, though nothing covers it -- it is skipped under the testing config so it cannot be reached from a test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The frontend had been calling three chat-connection endpoints that were never built —
updateChatConnection,testChatConnectionandsetDefaultChatConnectionshipped inapi/notifications.jsagainst a PUT that answered 405 and two POSTs that answered 404. Marcos Paulo Diniz noticed the gap from the outside and sent the backend half in #94, so this branch cherry-picks his six commits rather than squashing them, keeping him as the commit author on all six. Reviewing them turned up one genuine bug worth its own commit:update()decrypted every stored credential and re-encrypted the lot on any credential-bearing request, which quietly double-wrapped values thatdecrypt_secret_safe()had handed back untouched. The same branch also clears all six high npm advisories in the frontend — five were a stale lockfile, but js-yaml was pinned inside theoverridesblock, which is whynpm audit fixhad been reporting success while leaving it vulnerable. The endpoints still have no UI; that is deliberately left for a follow-up so this stays a backend contract change.Contributors
Highlights
Technical changes
ChatWebhookService.update()patches mutable metadata and credentials with omit-to-preserve semantics; the connectionkindis immutable andis_defaultis delegated to its own endpointupdate()operates onconn.raw_credentials()(ciphertext) and callsencrypt_secret()only on fields it assigns. The previous decrypt-all/re-encrypt-all loop passed every value throughdecrypt_secret_safe(), which returns its input unchanged on failure, so a wrong-key or not-yet-migrated row was wrapped a second time and became unrecoverable — including fields the request never named. Fernet is non-deterministic, so an untouched credential keeping byte-identical stored ciphertext is now an asserted invariantupdate()rejectsis_defaultonly when the submitted value differs from the stored one.to_dict()always emits the field, so a GET/edit/PUT round-trip was rejecting a field the caller never touchedChatWebhookService.set_default()flipsis_defaultacross all connections of one kind and activates the selection, matchingEmailProviderService.set_default()ChatWebhookService.test()sends through the real formatter and transport for active and inactive connections, recordslast_tested_at/last_test_ok, and never persists the transientNotificationtest()logs theDeliveryResult.errorreason server-side but returns a fixed string. Transport errors embed the full webhook URL including its secret path segment, and_deliver_webhooksurfaces those viaDeliveryResult.failed(str(exc))notifications_bp—PUT /admin/chat-connections/<id>,POST /admin/chat-connections/<id>/test,POST /admin/chat-connections/<id>/default— each behind@jwt_required()and@admin_required, matching the existing decorator orderupdate_chat_connectiontreats a non-JSON body as an empty object and mapsValueErrorto a 400, so malformed payloads including arrays, booleans and explicit JSONnullare rejected without a tracebackChatWebhookConnection.is_defaultcomment now distinguishes the administrative default from category fan-out, which remains driven by active connections whose filters matchjs-yamloverride raised4.3.0to4.3.1(GHSA-5p4m-2wfm-xmqj). An exact pin inoverridestakes precedence overnpm audit fix, so the advisory was unreachable by the usual remediation and had been masking anything landing behind itsocket.io-parser4.2.6 to 4.2.7 (GHSA-2m8v-j782-fhvr),react-router7.18.1 to 7.18.2 (GHSA-qwww-vcr4-c8h2),nanoid3.3.16 to 3.3.18 (GHSA-2v37-7h3g-55p8) andbrace-expansion1.1.16 to 1.1.18 (GHSA-mh99-v99m-4gvg, GHSA-rgw5-rvv9-x895). No dependency range changed — each fixed version already satisfied its parent's declared caret.package.jsonandpackage-lock.jsonmove together becausenpm ciruns infrontend-ci.ymlandrelease-smoke.ymlBASELINE_COUNTraised to 3379. The floor was last set at9fb0a6caand had drifted roughly 117 below actual collection; because the ratchet only fails on a drop, the gap was silent.gitignorepicks up.worktrees/