fix(slack-oauth-backend): deliver refresh token on re-auth (DM + page fallback) - #545
Conversation
… fallback) A Jun 2026 security commit hardcoded the success-page refresh token arg to undefined, and DM delivery was gated on the users.info enrichment result. So a user re-authing received only an access token and had no way to obtain their refresh token whenever the DM didn't go out, which in turn breaks the token keeper's ability to rotate. - routes/oauth.ts: render the refresh token on the no-store success-page fallback again (consistent with the access token already shown there under the same no-store/no-cache headers); send the DM using authed_user.id (OAuthResult.userId) rather than the enrichment result, so a users.info failure no longer silently skips delivery; log the underlying Slack error on DM failure for diagnosability. - oauth/handler.ts + oauth/types.ts: surface authed_user.id as OAuthResult.userId. - tests: guard the refresh-token-on-page regression and the DM-without- enrichment path; update the integration test that asserted the old (gated) skip-the-DM behavior. Claude-Session: https://claude.ai/code/session_012LLEFGoEDgLpQXRubeMK7w
…livery Round 1+2 review-fix-loop findings on the refresh-token-delivery fix: - types.ts: correct the userId docstring. It claimed "always present on success", but a bot-only install (no user scopes) returns success with no authed_user, so userId is undefined. Document the bot-only caveat. - handler.ts: align the inline userId comment with the same caveat (was also claiming "always present here"). - oauth.ts: add an else branch logging a warn when a successful exchange has no userId/accessToken, so the bot-only DM-skip is diagnosable instead of inferable only from an info breadcrumb. Logs booleans only, never a token. - oauth.spec.ts: add a negative test (no token rendered when the DM succeeds, pinning the !dmSent gate) and a test asserting the DM-failure log surfaces the Slack error code, not the token. Refactor the logger mock to a stable singleton so its spies survive jest.isolateModules. - oauth-flow.spec.ts: fix a stale comment that contradicted the now- asserted "DM is sent" behavior. Tests 106 pass, typecheck clean, lint 0 errors. Claude-Session: https://claude.ai/code/session_012LLEFGoEDgLpQXRubeMK7w
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🤖 Claude Code Review
Review: DM delivery via
|
📚 Documentation Check ✅Verdict: Passed No plugin files modified (all changes are in apps/slack-oauth-backend/), so the mandatory version-bump rule does not apply. fail_on_missing_docs is false, so the documentation gaps below are advisory warnings only. PR #545 — Documentation ValidationChanged scope: Plugin version bumpNot required. Zero plugin files were modified. Key behavioral changes in this PR
Documentation gaps found
Neither gap is blocking given Missing Updates
Suggestions (2)
🤖 Generated by Claude Documentation Validator | Mode: |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
📋 Review verdict: APPROVE
👆 The main review comment above is the source of truth for this PR review. It is automatically updated on each review cycle, so always refer to it for the most current feedback.
This formal review submission is for the verdict only. 1 inline comment(s) are attached below.
| // instead of vanishing into a generic message. | ||
| const slackError = | ||
| (dmError as { details?: { originalError?: { data?: { error?: string } } } })?.details | ||
| ?.originalError?.data?.error ?? |
There was a problem hiding this comment.
The first extraction (details.originalError.data.error) covers the common case where the Slack SDK throws a platform error (wrapped as DM_ERROR), which the test pins. But when conversations.open / chat.postMessage return ok: false, sendDirectMessage throws SlackApiError(..., 'OPEN_DM_FAILED'|'SEND_MESSAGE_FAILED', response) — here the granular Slack code is at details.error (the response's error), and details.originalError is undefined, so this falls through to code and logs the generic wrapper code instead of e.g. user_not_found. Adding a details?.error step before the code fallback would capture both shapes. Non-blocking — these ok: false branches are rarely hit with the modern throwing SDK.
| ?.originalError?.data?.error ?? | |
| const slackError = | |
| (dmError as { details?: { originalError?: { data?: { error?: string } } } })?.details | |
| ?.originalError?.data?.error ?? | |
| (dmError as { details?: { error?: string } })?.details?.error ?? | |
| (dmError as { code?: string })?.code ?? | |
| (dmError instanceof Error ? dmError.message : 'unknown'); |
1 participant
Problem
Re-authing against the Slack OAuth backend returns an access token but no refresh token, which breaks the downstream token-rotation keeper (it can no longer refresh, surfacing as opaque 500s).
Two linked causes, both from the Jun 25 2026 security hardening:
routes/oauth.tshardcoded the success-page refresh-token arg toundefined, so the HTML fallback (shown when the DM fails) no longer carries the refresh token.users.infoenrichment result (result.user?.id). When enrichment fails, the DM is silently skipped even thoughauthed_user.idis available — so neither channel delivers the refresh token.Fix
dmSent ? undefined : result.refreshToken), under the sameCache-Control: no-store, no-cache, must-revalidate, privateheaders that already protect the access token (also a secret) on that page. Both are HTML-escaped.users.info. Surfaceauthed_user.idasOAuthResult.userIdand send the DM to it, so an enrichment failure no longer skips the DM. When the precondition genuinely can't be met (bot-only install, noauthed_user), log awarn(booleans only) instead of skipping silently.Security note
Rendering the refresh token in the success-page fallback is consistent with the access token already rendered there under no-store headers; the "page can be cached" concern the prior commit cited is mitigated by those headers. The prior change broke a real delivery path for a marginal, inconsistent gain.
Test plan
nx test slack-oauth-backend: 106 passed (+2 new regression guards: refresh-token-on-page when DM fails; DM sent viaauthed_user.idwhenusers.infofails; plus a negative test that the refresh token is NOT rendered when the DM succeeds, and aSlackApiError-shaped DM-failure log assertion).nx typecheck slack-oauth-backend: clean.nx lint slack-oauth-backend: 0 errors (38 pre-existingno-explicit-anywarnings, out of scope).nx build slack-oauth-backend: compiles./review-fix-loop(specialist reviewers, two rounds → zero findings); the regression-guard tests were verified to fail against the pre-fix code.https://claude.ai/code/session_012LLEFGoEDgLpQXRubeMK7w