Repository navigation
fix(integrations): no-store the callback hand-off and trace chat callbacks as server spans - #1673
Conversation
…rted them Every connect, app-install and chat account-link callback is public, and it used to redeem its state row and store the provider's grant under whatever org and user the row named. The authorize URL is transferable, so a tenant admin could send theirs to somebody else and receive that person's GitHub install, Cloudflare/PlanetScale/Supabase/Hazel tokens or chat workspace, and a member could attach somebody else's chat account to their own Maple user. The public callbacks now complete nothing. They redirect to the dashboard with the provider's query in the URL fragment, and the signed-in integrations page finishes the flow through `oauthComplete` / `chatComplete`. Every service redeems a state row only for the session's user and org (`OAuthStateInitiator`), burning it on a mismatch, so a forwarded URL finished by another account fails without storing anything. Random state, TTL, provider scoping, PKCE and external-resource uniqueness are unchanged.
Maple review🟡 Confidence 6/10 · needs attention Public provider and chat callbacks now only 302 their query into the dashboard fragment; the signed-in page finishes the flow through new session-only endpoints whose services refuse a state row started by another org or user. The design holds up — the gap is test coverage on the new completer.
Before merge
Findings🟠 Warning · F1 ·
|
| File | Calls/day | Busiest operations |
|---|---|---|
apps/api/src/routes/chat-integration.http.ts |
936 | GET 795/day, 2.3% errGET 141/day, 0.1% err |
apps/api/src/routes/integrations-callback.http.ts |
936 | GET 795/day, 2.3% errGET 141/day, 0.1% err |
Telemetry this change adds and removes (5)
- ➖ attribute
http.response.status_code(~2.6M/day today) ·apps/api/src/routes/integrations-callback.http.ts:78 - ➕ span name
IntegrationCompleter.oauth·apps/api/src/routes/internal/integrations-complete.ts:176 - ➕ span name
IntegrationCompleter.chat·apps/api/src/routes/internal/integrations-complete.ts:232 - ➕ attribute
maple.chat.callback·apps/api/src/routes/internal/integrations-complete.ts:238 - ➕ attribute
vcs.connect.reason·packages/backend/src/services/integrations/vcs/vendor/github/GithubConnectService.ts:324
Removed here but still emitted elsewhere at this commit:
http.response.status_code·apps/api/src/routes/webhooks/clerk.http.ts:67
What was checked
- The state row is consumed before the initiator check, so a forwarded link cannot be replayed (
ChatWorkspaceService.ts:391,GithubConnectService.ts:298) - Every state row is written with the session's
initiatedByUserId, so no flow creates a row its initiator cannot redeem - The popup's
postMessagetargets the app origin andisCallbackOriginaccepts it (subscription.ts:234)
Observability coverage: 2 of 2 changes observable
| Change | Kind | Observable | Evidence |
|---|---|---|---|
Public /api/integrations/*/callback and /oauth/chat/:connector[/identity]/callback (redirect only) |
HTTP entrypoint | yes | Server span with http.route and maple.integration.provider (apps/api/src/routes/integrations-callback.http.ts:67); chat callbacks annotate maple.chat.connector (chat-integration.http.ts:47) |
POST /internal/integrations/oauth/complete and /internal/integrations/chat/complete |
HTTP entrypoint | yes | HttpApi group endpoint, span annotated with maple.integration.provider / maple.chat.connector (apps/api/src/routes/internal/integrations-complete.ts:181,238) |
402f8e5 · Updated on every push. Reply "won't fix" to dismiss a finding, or mention @maple-review-bot to ask about one. Check ids refer to Maple's instrumentation audit.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
…ions The completer that wires the dashboard session into each provider's completion had no test: cover the initiator hand-off per provider, the failure-to-copy and chat reason mappings, the pre-exchange refusals (issuer mismatch, missing code/state, provider error, pending GitHub approval) and the PlanetScale single-org finalize. Refusals before any exchange now log a warning with a fixed reason and stamp maple.integration.outcome, as do completions and failures, so a refused completion leaves a trace. The initiator-binding test asserts its finisher differs from the initiator in exactly one field.
Maple review🟢 Confidence 9/10 · safe to merge Adds a test suite for
Before merge
Fixed since the last review
Production impactProduction traffic of the changed files (last 7 days)
Telemetry this change adds and removes (6)
Removed here but still emitted elsewhere at this commit:
What was checked
Observability coverage: 1 of 1 changes observable
|
The callback spans are the only trace root for these routes; the forwarding rewrite dropped http.response.status_code from them. Every branch is a 302.
Maple review🟢 Confidence 8/10 · likely safe to merge Public OAuth and chat callbacks now only 302 the provider's query into the dashboard fragment; the signed-in page finishes the flow through new session-only endpoints that redeem a state row only for its initiator. The two files changed since the last review just stamp the redirect status; safe to merge. Before merge
Findings🔵 Note · F4 · Chat callbacks get no Server span, unlike the integration callbacksobservability · SPAN-01 ·
🤖 Prompt to fix this finding with an AI agentProduction impactProduction traffic of the changed files (last 7 days)
Telemetry this change adds and removes (5)
What was checked
Observability coverage: 1 of 2 changes observable
|
🍁 Maple PR previewNote Preview resources were removed when this pull request closed. Final commit |
The auto server span is suppressed for these paths, so the handler span is the trace root; make it server-kind with http.route like the provider callbacks.
Maple review🟢 Confidence 9/10 · safe to merge The chat callbacks now only 302 into the dashboard with the provider query in the fragment, and each carries its own Server span. Safe to merge; the OAuth flow change is the one thing to exercise end to end.
Fixed since the last review
Production impactProduction traffic of the changed files (last 7 days)
Telemetry this change adds and removes (5)
What was checked
Observability coverage: 2 of 2 changes observable
|
…er-binding # Conflicts: # .oxlintrc.json # apps/api/src/routes/chat-integration.http.ts # apps/api/src/routes/integrations-callback.http.test.ts # apps/api/src/routes/integrations-callback.http.ts # apps/api/src/routes/internal/integrations.http.ts # apps/web/src/page/integrations/update.ts # packages/backend/src/services/auth/CloudflareOAuthService.test.ts # packages/backend/src/services/auth/CloudflareOAuthService.ts # packages/backend/src/services/auth/HazelOAuthService.ts # packages/backend/src/services/auth/PlanetScaleOAuthService.test.ts # packages/backend/src/services/auth/PlanetScaleOAuthService.ts # packages/backend/src/services/auth/SupabaseOAuthService.test.ts # packages/backend/src/services/auth/SupabaseOAuthService.ts # packages/backend/src/services/auth/oauth/connection-helpers.test.ts # packages/backend/src/services/auth/oauth/connection-helpers.ts # packages/backend/src/services/integrations/ChatWorkspaceService.test.ts # packages/backend/src/services/integrations/ChatWorkspaceService.ts # packages/backend/src/services/integrations/PlanetScaleConnectionService.test.ts # packages/backend/src/services/integrations/PlanetScaleDiscoveryService.test.ts # packages/backend/src/services/integrations/PlanetScaleService.test.ts # packages/backend/src/services/integrations/SupabaseConnectionService.test.ts # packages/backend/src/services/integrations/SupabaseInsightsService.test.ts # packages/backend/src/services/integrations/SupabaseLogsService.test.ts # packages/backend/src/services/integrations/supabase/scrape-targets.test.ts # packages/backend/src/services/integrations/vcs/vendor/github/GithubConnectService.ts # packages/backend/src/services/integrations/vcs/vendor/github/__tests__/GithubConnectService.test.ts
…callbacks as server spans The redirect into the dashboard carries the provider's code in its fragment, so it is sent with no-store and no-referrer. The chat callbacks' auto server span is suppressed, so their handler span is now server-kind with http.route, like the provider callbacks.
Maple review🟢 Confidence 9/10 · safe to merge Routes every callback redirect through a shared
Production impactProduction traffic of the changed files (last 7 days)
What was checked
Observability coverage: 1 of 1 changes observable
|
Follow-up to #1675, which landed the initiator binding for OAuth, app-install and chat-identity callbacks. This branch first carried its own version of that fix; after #1675 merged it was reduced to the two pieces #1675 does not have.
cache-control: no-storeandreferrer-policy: no-referrer(forwardToDashboard), so no cache keeps that redirect and the landing request sends noReferer.ApiObservabilityLivesuppresses the auto server span for/oauth/chat/*/callback, so the handler span is the trace root. It is now server-kind withhttp.route,http.request.methodandhttp.response.status_code, likeIntegrationsCallbackRouter.oauthCallback.Tests:
integrations-callback.http.test.tsasserts both headers on a provider redirect and a chat redirect (6/6 pass). The api typecheck and lint are clean.🤖 Generated with Claude Code