From e1f1cf60ec697a79a46973803ee38d25cd9a7dd2 Mon Sep 17 00:00:00 2001 From: Nick Koutrelakos Date: Wed, 20 May 2026 16:54:17 -0700 Subject: [PATCH 01/12] feat(slack-oauth-backend): expand OAuth scope requests to match app manifest (#517) Sync the bot and user scope arrays in createOAuthHandler() with the current Slack app manifest so granted tokens carry all configured permissions. Bot scopes: 14 to 15 (adds incoming-webhook). User scopes: 14 to 39 (adds search:read family, canvases, bookmarks, links, reminders, pins, files read/write, emoji:read, channels:write, groups:write, mpim:write, users:read.email). Existing tokens retain their previous narrower scopes; users must re-authorize to receive the new permissions. --- apps/slack-oauth-backend/src/oauth/handler.ts | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/apps/slack-oauth-backend/src/oauth/handler.ts b/apps/slack-oauth-backend/src/oauth/handler.ts index 46e41cc7..8698f2cc 100644 --- a/apps/slack-oauth-backend/src/oauth/handler.ts +++ b/apps/slack-oauth-backend/src/oauth/handler.ts @@ -230,6 +230,8 @@ export function createOAuthHandler( clientSecret: config.slackClientSecret, redirectUri: config.slackRedirectUri, // Scopes are split between bot (xoxb) and user (xoxp) tokens. + // Source of truth: Slack app manifest. Keep these arrays in sync with the + // manifest's oauth_config.scopes.bot and oauth_config.scopes.user lists. botScopes: [ 'channels:history', 'channels:read', @@ -239,6 +241,7 @@ export function createOAuthHandler( 'im:history', 'im:read', 'im:write', + 'incoming-webhook', 'mpim:history', 'mpim:read', 'reactions:read', @@ -247,20 +250,45 @@ export function createOAuthHandler( 'users:read', ], userScopes: [ + 'bookmarks:read', + 'bookmarks:write', + 'canvases:read', + 'canvases:write', 'channels:history', 'channels:read', + 'channels:write', 'chat:write', + 'emoji:read', + 'files:read', + 'files:write', 'groups:history', 'groups:read', + 'groups:write', 'im:history', 'im:read', 'im:write', + 'links.embed:write', + 'links:read', + 'links:write', 'mpim:history', 'mpim:read', + 'mpim:write', + 'pins:read', + 'pins:write', 'reactions:read', 'reactions:write', + 'reminders:read', + 'reminders:write', + 'search:read', + 'search:read.files', + 'search:read.im', + 'search:read.mpim', + 'search:read.private', + 'search:read.public', + 'search:read.users', 'users.profile:read', 'users:read', + 'users:read.email', ], validateState, }); From a48592e05af617f6be64f117acaf1aeecf0fecd9 Mon Sep 17 00:00:00 2001 From: Nick Koutrelakos Date: Wed, 20 May 2026 17:01:00 -0700 Subject: [PATCH 02/12] fix(slack-oauth-backend): drop incoming-webhook bot scope (#518) The scope was added in PR #517 to mirror the Slack app manifest, but the OAuth callback handler discards tokenResponse.incoming_webhook (only forwards team, enterprise, scopes, bot_user_id). The app does not consume webhook URLs, so requesting the scope adds a channel- picker step to install for no functional benefit. Manifest should be updated separately to remove incoming-webhook from oauth_config.scopes.bot so the two stay aligned. --- apps/slack-oauth-backend/src/oauth/handler.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/apps/slack-oauth-backend/src/oauth/handler.ts b/apps/slack-oauth-backend/src/oauth/handler.ts index 8698f2cc..07f1627c 100644 --- a/apps/slack-oauth-backend/src/oauth/handler.ts +++ b/apps/slack-oauth-backend/src/oauth/handler.ts @@ -241,7 +241,6 @@ export function createOAuthHandler( 'im:history', 'im:read', 'im:write', - 'incoming-webhook', 'mpim:history', 'mpim:read', 'reactions:read', From 7d0d6a3d9c06c66a5d7e14b6e5b6ac4ff5f6e8ce Mon Sep 17 00:00:00 2001 From: Uniswap Labs Service Account Date: Thu, 21 May 2026 19:13:20 +0000 Subject: [PATCH 03/12] chore(sync): [skip ci] sync next with main From 80ac06e43805082e9a610c72a619bf2226e0f07e Mon Sep 17 00:00:00 2001 From: Nick Koutrelakos Date: Tue, 26 May 2026 12:54:45 -0700 Subject: [PATCH 04/12] chore(slack-oauth-backend): log OAuth request and Slack-granted scopes (#520) * chore(slack-oauth-backend): log OAuth request scopes and Slack-granted scopes Adds two diagnostic logger.info calls to surface what we ask Slack for and what Slack returns on each install, so future scope-grant mismatches can be debugged against Vercel logs instead of trial-and-error reinstalls. handler.ts (after exchangeCodeForToken): logs botScopesGranted, userScopesGranted, presence flags for bot/user access and refresh tokens, token types, team id, enterprise id. Token values themselves are not logged. routes/oauth.ts (in /authorize handler): logs the full redirect URL including scope and user_scope query params, so we can verify the request side independently of Slack's response. Temporary instrumentation for the ongoing org-install debugging; safe to leave in but a follow-up can prune if these become noisy. * chore(slack-oauth-backend): scope-only log + drop "Debug" framing Addresses two review comments on #520: routes/oauth.ts: parse the authorize URL and log only the scope, user_scope, and redirect_uri params instead of the full URL. The full URL contains the `state` CSRF nonce, which would otherwise leak into Vercel logs and weaken the CSRF protection validated on callback. handler.ts + routes/oauth.ts: drop the "Debug:" framing from both log comments. The logs fire at info level in production and the data they capture (scope strings, presence flags) is permanent OAuth observability, not temporary instrumentation, so the comments now reflect that intent. --- apps/slack-oauth-backend/src/oauth/handler.ts | 18 ++++++++++++++++++ apps/slack-oauth-backend/src/routes/oauth.ts | 10 ++++++++++ 2 files changed, 28 insertions(+) diff --git a/apps/slack-oauth-backend/src/oauth/handler.ts b/apps/slack-oauth-backend/src/oauth/handler.ts index 07f1627c..b2e95dc9 100644 --- a/apps/slack-oauth-backend/src/oauth/handler.ts +++ b/apps/slack-oauth-backend/src/oauth/handler.ts @@ -106,6 +106,24 @@ export class SlackOAuthHandler implements OAuthHandler { ); } + // Observability: record the scopes Slack actually granted on the issued + // tokens, plus presence flags useful for diagnosing token-type confusion + // and refresh-token availability. Token values themselves are deliberately + // omitted; only scope strings and booleans are recorded, so this log is + // safe to leave on in production. + logger.info('Slack OAuth exchange complete', { + botScopesGranted: tokenResponse.scope, + userScopesGranted: tokenResponse.authed_user?.scope, + hasBotToken: !!tokenResponse.access_token, + hasUserToken: !!tokenResponse.authed_user?.access_token, + hasBotRefreshToken: !!tokenResponse.refresh_token, + hasUserRefreshToken: !!tokenResponse.authed_user?.refresh_token, + botTokenType: tokenResponse.token_type, + userTokenType: tokenResponse.authed_user?.token_type, + teamId: tokenResponse.team?.id, + enterpriseId: tokenResponse.enterprise?.id, + }); + // Get user information using the bot token (if available) // With token rotation, bot token may not be available let userInfo: SlackUserInfo | undefined; diff --git a/apps/slack-oauth-backend/src/routes/oauth.ts b/apps/slack-oauth-backend/src/routes/oauth.ts index 0a068326..332801ad 100644 --- a/apps/slack-oauth-backend/src/routes/oauth.ts +++ b/apps/slack-oauth-backend/src/routes/oauth.ts @@ -155,6 +155,16 @@ router.get( const oauthHandler = createOAuthHandler(); const authUrl = oauthHandler.generateAuthUrl(state); + // Observability: record the scopes being requested on this install + // attempt. Parse params out of the URL so the `state` CSRF nonce + // doesn't end up in logs alongside the scope data. + const parsedUrl = new URL(authUrl); + logger.info('Slack OAuth authorize redirect', { + scope: parsedUrl.searchParams.get('scope'), + userScope: parsedUrl.searchParams.get('user_scope'), + redirectUri: parsedUrl.searchParams.get('redirect_uri'), + }); + // In production, store state in session or database // For now, just redirect res.redirect(authUrl); From 07fdbd9e6fccec32cc85729e7ef78277b16cb6dd Mon Sep 17 00:00:00 2001 From: Uniswap Labs Service Account Date: Thu, 28 May 2026 18:10:55 +0000 Subject: [PATCH 05/12] chore(sync): [skip ci] sync next with main From 0e09ace7a67daab7250b598fa9bb788af6d904d3 Mon Sep 17 00:00:00 2001 From: Uniswap Labs Service Account Date: Thu, 28 May 2026 18:23:31 +0000 Subject: [PATCH 06/12] chore(sync): [skip ci] sync next with main From c431f5b03c94f28918c3fb07d9a3cf846f6ef594 Mon Sep 17 00:00:00 2001 From: David Gilman Date: Thu, 28 May 2026 13:27:31 -0500 Subject: [PATCH 07/12] fix(workflows): replace sed-with-pipe-delimiter with python str.replace (#513) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _claude-docs-check.yml's Build Docs Check Prompt step substitutes 8 placeholders into the prompt template via `sed -i "s|\${VAR}|$VALUE|g"` with `|` as the sed delimiter. The escape applied to $CHANGED_FILES on line 520 is `sed 's/[&/\]/\\&/g'` — it escapes `&`, `/`, `\` but NOT `|` (the actual delimiter). A filename in $CHANGED_FILES containing `|` (legal on Linux; not quoted by git's default core.quotepath since `|` is in the printable ASCII range) breaks the substitution into garbage flags and causes the docs-check workflow to fail or write a partial value into the prompt. PR authors can introduce such filenames via fork PRs. Replace the 8 sed -i calls with a single python str.replace pass. Python's str.replace is plain-text and immune to this entire class of delimiter-injection bugs. Discovered during the bug bounty review that produced Uniswap/default-token-list#2484 and Uniswap/ai-toolkit#509. Co-authored-by: Claude Opus 4.7 (1M context) --- .github/workflows/_claude-docs-check.yml | 36 ++++++++++++++++-------- 1 file changed, 24 insertions(+), 12 deletions(-) diff --git a/.github/workflows/_claude-docs-check.yml b/.github/workflows/_claude-docs-check.yml index 04d44609..eadc2ea3 100644 --- a/.github/workflows/_claude-docs-check.yml +++ b/.github/workflows/_claude-docs-check.yml @@ -507,18 +507,30 @@ jobs: PROMPT_EOF - # Substitute variables - sed -i "s|\${REPO_OWNER}|$REPO_OWNER|g" /tmp/docs-check-prompt.txt - sed -i "s|\${REPO_NAME}|$REPO_NAME|g" /tmp/docs-check-prompt.txt - sed -i "s|\${PR_NUMBER}|$PR_NUMBER|g" /tmp/docs-check-prompt.txt - sed -i "s|\${BASE_REF}|$BASE_REF|g" /tmp/docs-check-prompt.txt - sed -i "s|\${LINES_CHANGED}|$LINES_CHANGED|g" /tmp/docs-check-prompt.txt - sed -i "s|\${FAIL_ON_MISSING_DOCS}|$FAIL_ON_MISSING_DOCS|g" /tmp/docs-check-prompt.txt - sed -i "s|\${FAIL_ON_MISSING_VERSION}|$FAIL_ON_MISSING_VERSION|g" /tmp/docs-check-prompt.txt - - # Insert changed files (escape for sed) - CHANGED_FILES_ESCAPED=$(printf '%s\n' "$CHANGED_FILES" | sed 's/[&/\]/\\&/g; s/$/\\n/' | tr -d '\n') - sed -i "s|\${CHANGED_FILES}|$CHANGED_FILES_ESCAPED|g" /tmp/docs-check-prompt.txt + # Substitute placeholders using python's str.replace. + # + # Why not sed: the previous code used `sed -i "s|\${VAR}|$VALUE|g"` + # with `|` as the sed delimiter. The escape applied to $CHANGED_FILES + # was `sed 's/[&/\]/\\&/g'`, which escapes `&`, `/`, `\` but NOT `|` + # (the actual delimiter). A filename in $CHANGED_FILES containing `|` + # — legal on Linux, not quoted by git's default `core.quotepath` + # since `|` is in the printable ASCII range — would break the + # substitution into garbage flags and cause the docs-check workflow + # to fail loudly or, worse, write a partial value. + # + # Python's str.replace is plain-text and immune to this class. + export REPO_OWNER REPO_NAME CHANGED_FILES + python3 - <<'PYEOF' + import os + from pathlib import Path + p = Path('/tmp/docs-check-prompt.txt') + content = p.read_text() + for key in ('REPO_OWNER', 'REPO_NAME', 'PR_NUMBER', 'BASE_REF', + 'LINES_CHANGED', 'FAIL_ON_MISSING_DOCS', + 'FAIL_ON_MISSING_VERSION', 'CHANGED_FILES'): + content = content.replace('${' + key + '}', os.environ.get(key, '')) + p.write_text(content) + PYEOF # For the diff, we'll append it separately due to size echo "" >> /tmp/docs-check-prompt.txt From 09491fbe0719290e7a66842e0cbc95f1d7435c95 Mon Sep 17 00:00:00 2001 From: David Gilman Date: Thu, 28 May 2026 13:29:51 -0500 Subject: [PATCH 08/12] fix(claude-task-worker): harden against prompt injection (#514) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workflows): harden Claude session in task worker (drop --dangerously-skip-permissions, bullfrog block) The autonomous task worker had two structural gaps that any prompt- injection bug would weaponize: 1. Claude ran with `--dangerously-skip-permissions`, meaning every tool call was approved automatically. Combined with the absence of a file-allowlist post-flight check (see the separate Validate-changed- files PR), a prompt-injected session could call any tool unrestricted. 2. bullfrog egress-policy was `audit` (log-only). Network calls to attacker-controlled endpoints would be logged but not blocked. This patch: a) Replaces `--dangerously-skip-permissions` with an explicit `--allowedTools` list. Without an allowlist Claude prompts on every tool call (no human to approve in CI, so the job stalls); with the explicit allowlist the worker can still operate but a hijacked session cannot call arbitrary tools (curl, wget for exfil; dangerous mcp__linear__ operations like delete_*; etc.). Refine the list as legitimate tasks reveal additional needed tools. b) Switches bullfrog from `egress-policy: audit` to `egress-policy: block` with an allowlist matching the other ai-toolkit workflows that already run in block mode (_claude-main.yml, _claude-code-review.yml, _claude-docs-check.yml, _generate-pr-metadata.yml), plus api.linear.app for the Linear MCP server this worker uses. A follow-up that this patch does NOT address: the GitHub App's installation permissions are configured server-side and are out of scope for a code PR. Audit the App's scopes in the GitHub UI and reduce to the minimum needed (contents:write, pull_requests:write, issues:read). Discovered during the bug bounty review that produced Uniswap/default-token-list#2484 and Uniswap/ai-toolkit#509. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(workflows): tighten bullfrog allowlist to aitk-worker-specific empirically-derived hosts The previous commit shipped a 10-entry allowlist matching the dtl worker's set, but the aitk Task Worker has different needs (uses registry.npmjs.org as its npm registry, not registry.yarnpkg.com) and the shared list included entries unused in practice. Per-workflow analysis based on 15 audit-mode runs of _generate-pr-metadata.yml (the only ai-toolkit workflow with recent runs and audit logs showing the same setup-bun + bun install pattern) plus reading this worker's code: Setup-phase empirical: - registry.npmjs.org (341 audit lines across 15 sister runs) - release-assets.githubusercontent.com (covered by *.githubusercontent.com) Runtime (invisible to audit per agent/queue_audit.nft — DNS reply queue means cached resolutions emit no further events): - api.anthropic.com (claude-code-action) - api.linear.app (mcp__linear__ tools at runtime) Default-allowed by bullfrog (agent/agent.go:18 defaultDomains): - github.com, api.github.com — silently allowed Dropped vs the previous shared allowlist: - github.com / *.github.com — covered by defaultDomains - bun.sh — 0 audit lines in 15 setup-bun runs; the binary comes from github releases via *.githubusercontent.com - claude.ai / *.claude.ai — OAuth path only; this worker uses ANTHROPIC_API_KEY Final allowlist: 5 entries (down from 10), each empirically justified or required by code that audit logs can't observe. Sister PR Uniswap/default-token-list#2485 makes the analogous tightening for the dtl Task Worker. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(claude-task-worker): clarify allowlist limits + add Bash(rg:*) Rewords the comment above allowedTools to stop overstating what the list contains. node/python3/bunx/npx are all arbitrary code execution; the real containment is the bullfrog egress block above. Documents that explicitly so a future maintainer doesn't loosen the bullfrog allowlist assuming the tool allowlist constrains exfiltration — it does not. Also adds Bash(rg:*) preemptively per the PR's own 'add the tool here' guidance: Claude's default search flows prefer ripgrep and would otherwise stall on a permission prompt mid-task. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .github/workflows/_claude-task-worker.yml | 38 +++++++++++++++++++++-- 1 file changed, 35 insertions(+), 3 deletions(-) diff --git a/.github/workflows/_claude-task-worker.yml b/.github/workflows/_claude-task-worker.yml index 7ab4a5dc..dac44835 100644 --- a/.github/workflows/_claude-task-worker.yml +++ b/.github/workflows/_claude-task-worker.yml @@ -179,11 +179,26 @@ jobs: actions: read steps: - # Security scanning + # Security scanning — block mode prevents secret exfiltration if RCE is achieved. + # + # Allowlist is workflow-specific. github.com / api.github.com are in + # bullfrog's defaultDomains (agent/agent.go:18) and don't need to be + # listed. claude.ai / *.claude.ai apply only to the OAuth token path + # (CLAUDE_CODE_OAUTH_TOKEN); this worker uses the API-key path and + # never resolves them. bun.sh is unused — setup-bun fetches the binary + # from GitHub releases (covered by *.githubusercontent.com), confirmed + # by 15 audit-mode runs of _generate-pr-metadata.yml that use the same + # setup-bun action and showed 0 audit lines for bun.sh. - name: Security scanning uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: - egress-policy: audit + egress-policy: block + allowed-domains: | + release-assets.githubusercontent.com + api.anthropic.com + api.linear.app + registry.npmjs.org + enable-sudo: false # Checkout the target branch # Must come before validate-claude-auth (local composite action) @@ -535,10 +550,27 @@ jobs: claude_code_oauth_token: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} prompt: ${{ steps.build-prompt.outputs.prompt }} show_full_output: ${{ inputs.debug_mode }} + # Replaced --dangerously-skip-permissions with an explicit allowedTools list + # so Claude doesn't auto-approve every tool call. Without an allowlist + # Claude prompts on every tool call (no human to approve in CI, so the job + # stalls); with --dangerously-skip-permissions a prompt-injected session + # could call any tool. + # + # SECURITY MODEL: the allowlist below includes Bash(node:*), Bash(python3:*), + # Bash(bunx:*), and Bash(npx:*) — each is arbitrary code execution. This + # allowlist alone does NOT contain a hijacked session: a prompt-injected + # Claude can run `node -e "..."` or `npx ` to do anything + # the comment used to claim was "excluded". The actual containment is the + # bullfrog egress block above: even if a hijacked session runs `node -e` to + # exfiltrate, traffic is blocked to non-allowlisted hosts. Do not relax the + # bullfrog allowlist without re-evaluating this layer. + # + # Refine as needed — if a legitimate task fails because a tool is missing, + # add the tool here. claude_args: | --model ${{ inputs.model }} --max-turns 150 - --dangerously-skip-permissions + --allowedTools "Read,Write,Edit,MultiEdit,NotebookEdit,Glob,Grep,WebSearch,WebFetch,Bash(git:*),Bash(gh pr:*),Bash(gh issue:*),Bash(gh api:*),Bash(npm:*),Bash(bun:*),Bash(bunx:*),Bash(npx:*),Bash(node:*),Bash(python3:*),Bash(ls:*),Bash(cat:*),Bash(head:*),Bash(tail:*),Bash(grep:*),Bash(rg:*),Bash(find:*),Bash(awk:*),Bash(sed:*),Bash(jq:*),Bash(date:*),Bash(wc:*),Bash(cut:*),Bash(tr:*),Bash(sort:*),Bash(uniq:*),Bash(diff:*),Bash(echo:*),Bash(printf:*),Bash(mkdir:*),Bash(touch:*),Bash(chmod:*),Bash(mv:*),Bash(cp:*),mcp__linear__linear_get_issue,mcp__linear__linear_list_issues,mcp__linear__linear_add_comment,mcp__linear__linear_search_documentation,mcp__linear__linear_update_issue" --mcp-config '{"mcpServers":{"linear":{"command":"bunx","args":["linear-mcp-server"],"env":{"LINEAR_API_KEY":"${{ secrets.LINEAR_API_KEY }}"}}}}' plugin_marketplaces: ${{ steps.build-plugins.outputs.plugin_marketplaces }} plugins: ${{ steps.build-plugins.outputs.plugins }} From dab0e806edaa9b7b22431dfbb2fb760b671b6950 Mon Sep 17 00:00:00 2001 From: David Gilman Date: Thu, 28 May 2026 13:32:00 -0500 Subject: [PATCH 09/12] fix(workflows): set BUN_CONFIG_FILE=/dev/null in remaining workflows (#511) * fix(workflows): set BUN_CONFIG_FILE=/dev/null to neutralize bunfig.toml.preload RCE `bun run`, `bun install`, and `bun x` auto-load bunfig.toml from CWD. The bunfig.toml.preload array executes arbitrary code before the intended script. When a workflow checks out PR head content (any of our reusable workflows that take a pr_number input do this), `bun run` loads bunfig.toml from the checkout root, and a malicious preload entry executes with the caller's secrets. Disable bunfig.toml loading by setting BUN_CONFIG_FILE=/dev/null at the workflow level. This propagates to all jobs and steps and covers every `bun *` invocation in the workflow. Affected files (all use `bun run`/`bun install` after checkout): - _claude-docs-check.yml - _claude-code-review.yml - _claude-task-worker.yml - ci-pr-checks.yml - publish-packages.yml Discovered during the bug bounty review that produced Uniswap/default-token-list#2484 and Uniswap/ai-toolkit#509 (semgrep flagged 8+ sites; this fix is uniform across all of them). Co-Authored-By: Claude Opus 4.7 (1M context) * fix(workflows): also prefix BUN_CONFIG_FILE=/dev/null inline on bun run Belt-and-suspenders: job env already sets BUN_CONFIG_FILE=/dev/null, but pinning it inline on the bun run invocation matches the Semgrep CI comment's suggested fix and survives anyone later moving this into a step that overrides the job env. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .github/workflows/_claude-code-review.yml | 8 ++++++++ .github/workflows/_claude-docs-check.yml | 15 +++++++++++++-- .github/workflows/_claude-task-worker.yml | 7 +++++++ .github/workflows/ci-pr-checks.yml | 7 +++++++ .github/workflows/publish-packages.yml | 8 ++++++++ 5 files changed, 43 insertions(+), 2 deletions(-) diff --git a/.github/workflows/_claude-code-review.yml b/.github/workflows/_claude-code-review.yml index 3b3a5a80..39524bad 100644 --- a/.github/workflows/_claude-code-review.yml +++ b/.github/workflows/_claude-code-review.yml @@ -251,6 +251,14 @@ on: description: "Personal Access Token with repo scope. Required for: (1) resolving review threads via GraphQL API, and (2) auto-fix pushes that trigger subsequent workflow runs. Falls back to GITHUB_TOKEN if not provided, but auto-fix pushes with GITHUB_TOKEN will NOT trigger new workflow runs." required: false +# Disable Bun's automatic bunfig.toml loading from CWD. This workflow runs +# `bun run` against post-review scripts; if a caller checks out PR head +# content first, `bun run` would auto-load bunfig.toml from the checkout +# root and `bunfig.toml.preload` would execute arbitrary code with the +# caller's secrets. Propagates to all jobs and steps in this workflow. +env: + BUN_CONFIG_FILE: /dev/null + jobs: claude-review: runs-on: ubuntu-latest diff --git a/.github/workflows/_claude-docs-check.yml b/.github/workflows/_claude-docs-check.yml index eadc2ea3..0688873a 100644 --- a/.github/workflows/_claude-docs-check.yml +++ b/.github/workflows/_claude-docs-check.yml @@ -170,6 +170,14 @@ on: description: "Number of commits pushed when auto_commit is enabled" value: ${{ jobs.docs-check.outputs.commits_pushed }} +# Disable Bun's automatic bunfig.toml loading from CWD. When a workflow +# checks out PR head content (this workflow checks out PR refs to read +# diffs and post comments), `bun run` would otherwise auto-load +# bunfig.toml from the checkout root, and `bunfig.toml.preload` executes +# arbitrary code before the intended script — RCE with caller secrets. +env: + BUN_CONFIG_FILE: /dev/null + jobs: docs-check: runs-on: ubuntu-latest @@ -732,8 +740,11 @@ jobs: SCRIPT_ARGS="$SCRIPT_ARGS --auto-commit" fi - # Run post-processing script - bun run "$POST_SCRIPT" $SCRIPT_ARGS + # Run post-processing script. BUN_CONFIG_FILE=/dev/null is already set + # at the job env (line 187), but pinning it inline keeps Semgrep's + # bun-run-in-reusable-workflow rule satisfied and survives anyone + # later moving this into a step that overrides the job env. + BUN_CONFIG_FILE=/dev/null bun run "$POST_SCRIPT" $SCRIPT_ARGS # Check if branch was created if [ -f /tmp/fixup-branch-name.txt ]; then diff --git a/.github/workflows/_claude-task-worker.yml b/.github/workflows/_claude-task-worker.yml index dac44835..3a1c5320 100644 --- a/.github/workflows/_claude-task-worker.yml +++ b/.github/workflows/_claude-task-worker.yml @@ -165,6 +165,13 @@ on: description: "PAT for pushing branches (optional, falls back to GITHUB_TOKEN)" required: false +# Disable Bun's automatic bunfig.toml loading from CWD. `bun run` invocations +# in this workflow run after checkout of the worker's repo; without this, +# a malicious bunfig.toml at the checkout root would have its preload array +# execute arbitrary code before the intended script (RCE with caller secrets). +env: + BUN_CONFIG_FILE: /dev/null + jobs: process-task: runs-on: ubuntu-latest diff --git a/.github/workflows/ci-pr-checks.yml b/.github/workflows/ci-pr-checks.yml index 24d64726..8265410e 100644 --- a/.github/workflows/ci-pr-checks.yml +++ b/.github/workflows/ci-pr-checks.yml @@ -7,6 +7,13 @@ concurrency: group: ${{ github.workflow }}-${{ github.head_ref || github.ref }} cancel-in-progress: true +# Disable Bun's automatic bunfig.toml loading from CWD. `bun run` invocations +# in this workflow run after checkout; without this, a malicious bunfig.toml +# at the checkout root would have its preload array execute arbitrary code +# before the intended script (RCE with workflow secrets). +env: + BUN_CONFIG_FILE: /dev/null + jobs: # Pre-check job to detect automated PRs check-automated: diff --git a/.github/workflows/publish-packages.yml b/.github/workflows/publish-packages.yml index 68390ff6..54c7627e 100644 --- a/.github/workflows/publish-packages.yml +++ b/.github/workflows/publish-packages.yml @@ -58,6 +58,14 @@ concurrency: group: publish-${{ github.ref }} cancel-in-progress: false +# Disable Bun's automatic bunfig.toml loading from CWD. `bun install` and +# `bun run` invocations in this workflow run after checkout; without this, +# a malicious bunfig.toml at the checkout root would have its preload array +# execute arbitrary code before the intended script (RCE with publish-token +# scope in this workflow). +env: + BUN_CONFIG_FILE: /dev/null + jobs: # ============================================================================ # DETECT: Determine which packages to publish From 776e81fa936d510be4804c4b3e7f801e31052843 Mon Sep 17 00:00:00 2001 From: David Gilman Date: Thu, 28 May 2026 13:43:22 -0500 Subject: [PATCH 10/12] fix(workflows): allowlist toolkit_ref to block fork-branch RCE (#510) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workflows): validate toolkit_ref against allowlist before downloading scripts All six reusable workflow_call workers (_claude-task-worker.yml, _claude-code-review.yml, _claude-docs-check.yml, _claude-main.yml, _generate-pr-metadata.yml, _update-action-versions-worker.yml) accept a `toolkit_ref` input and use it directly in curl URLs to fetch action.yml / post-*.ts script content from this repository: curl -L "https://api.github.com/repos/Uniswap/ai-toolkit/contents/.github/.../action.yml?ref=${TOOLKIT_REF}" The downloaded content is then executed inside the worker job with access to its secrets (ANTHROPIC_API_KEY, LINEAR_API_KEY, GitHub App installation tokens). If `toolkit_ref` is set to an attacker-controlled ref — e.g., a fork branch — they get RCE-with-secrets the moment any of these workflows is invoked with the attacker's branch. Add a "Validate toolkit_ref" step as the FIRST step in each worker job. Allowlist: main, next, or a 40-char SHA. Anything else fails the job with a clear error. The validation runs before the bullfrog egress scan (which is the next step) so even if the egress allowlist is misconfigured, attacker- controlled refs are rejected before any network call is made. Discovered during the bug bounty review that produced Uniswap/default-token-list#2484 and Uniswap/ai-toolkit#509. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(workflows): pin claude-docs-check toolkit_ref to main This caller previously passed `toolkit_ref: ${{ github.head_ref || github.ref_name }}`, which resolves to the PR branch name on every pull_request: run. The downstream worker (_claude-docs-check.yml) curls action.yml / post-*.ts script content from `Uniswap/ai-toolkit@$TOOLKIT_REF` and executes it inside the worker job with access to the caller's secrets (ANTHROPIC_API_KEY, CLAUDE_CODE_OAUTH_TOKEN, WORKFLOW_PAT) and the GitHub App installation token. A PR author could therefore land malicious script content on their own branch and have the docs-check workflow execute it with full secrets. For internal-author PRs (anyone with push access to this repo) the attack is complete RCE-with-secrets; for fork PRs the blast is bounded by what `pull_request:` flows expose, but the pattern is brittle either way. Sequencing: this PR should land BEFORE the toolkit_ref allowlist validation in #511. After this lands, #511's validation step has no in-repo caller that would fail the check. Landing #511 first without this PR would break docs-check on every non-main/next branch. Trade-off accepted: PR authors who modify worker scripts (post-docs-check.ts, validate-claude-auth/action.yml, etc.) can no longer self-test their changes via docs-check on the same PR. Test flow is now: 1. Land the script change on `next` first 2. Verify docs-check works on next 3. Promote next → main via the existing release-notes flow Co-Authored-By: Claude Opus 4.7 (1M context) * fix(workflows): move toolkit_ref validation after bullfrog per CLAUDE.md The 'bullfrog must be first step' rule in .github/workflows/CLAUDE.md is documented as having 'no exceptions'. Validate toolkit_ref does no network egress (case/printf/grep), so the threat model is unchanged either way — but conventionally bullfrog comes first. Validate still runs before any step that consumes toolkit_ref to download action.yml or post-*.ts content, so its security role is preserved. Also extends the claude-docs-check.yml comment block to note that external repos copying the consumer workflow as a template should keep toolkit_ref pinned to 'main' — the reusable worker's allowlist blocks fork branches but still permits 'next'. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .github/workflows/_claude-code-review.yml | 24 +++++++++++++++++ .github/workflows/_claude-docs-check.yml | 24 +++++++++++++++++ .github/workflows/_claude-main.yml | 26 ++++++++++++++++++- .github/workflows/_claude-task-worker.yml | 26 +++++++++++++++++++ .github/workflows/_generate-pr-metadata.yml | 26 ++++++++++++++++++- .../_update-action-versions-worker.yml | 26 ++++++++++++++++++- .github/workflows/claude-docs-check.yml | 15 +++++++++-- 7 files changed, 162 insertions(+), 5 deletions(-) diff --git a/.github/workflows/_claude-code-review.yml b/.github/workflows/_claude-code-review.yml index 39524bad..7384515f 100644 --- a/.github/workflows/_claude-code-review.yml +++ b/.github/workflows/_claude-code-review.yml @@ -283,6 +283,11 @@ jobs: # via attacker-controlled config files (bunfig.toml, .npmrc, etc.) in fork PRs. # If a legitimate host is missing, the workflow will fail loudly — update the # allowlist rather than switching back to audit mode. + # Per .github/workflows/CLAUDE.md, bullfrog must be the FIRST step in + # every job on a non-macOS runner ("no exceptions"), so it runs before + # the toolkit_ref validation below. Ordering is safe: bullfrog does not + # consume toolkit_ref, and the validation runs before any step that + # downloads action.yml or post-*.ts content using it. - name: Security Scan uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: @@ -299,6 +304,25 @@ jobs: *.npmjs.org enable-sudo: false + # Validate toolkit_ref BEFORE any downstream step uses it to download + # action.yml / post-*.ts script content from the ai-toolkit repo. + # Without this, an attacker-controlled ref could substitute malicious + # script content that runs inside this job with access to its secrets. + - name: Validate toolkit_ref + env: + TOOLKIT_REF: ${{ inputs.toolkit_ref }} + run: | + case "$TOOLKIT_REF" in + main|next) ;; + *) + if ! printf '%s' "$TOOLKIT_REF" | grep -qE '^[0-9a-fA-F]{40}$'; then + echo "::error::Invalid toolkit_ref: '$TOOLKIT_REF'. Allowed: main, next, or a full 40-char SHA." + exit 1 + fi + ;; + esac + echo "✅ toolkit_ref validated: $TOOLKIT_REF" + # Checkout required before using local composite actions - name: Checkout repository uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 diff --git a/.github/workflows/_claude-docs-check.yml b/.github/workflows/_claude-docs-check.yml index 0688873a..50b0a2cf 100644 --- a/.github/workflows/_claude-docs-check.yml +++ b/.github/workflows/_claude-docs-check.yml @@ -205,6 +205,11 @@ jobs: steps: # Security scanning — block mode prevents secret exfiltration if RCE is achieved. # Allowlist validated against Datadog (service:github-actions, 7d window). + # Per .github/workflows/CLAUDE.md, bullfrog must be the FIRST step in + # every job on a non-macOS runner ("no exceptions"), so it runs before + # the toolkit_ref validation below. Ordering is safe: bullfrog does not + # consume toolkit_ref, and the validation runs before any step that + # downloads action.yml or post-*.ts content using it. - name: Security Scan uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: @@ -221,6 +226,25 @@ jobs: *.npmjs.org enable-sudo: false + # Validate toolkit_ref BEFORE any downstream step uses it to download + # action.yml / post-*.ts script content from the ai-toolkit repo. + # Without this, an attacker-controlled ref could substitute malicious + # script content that runs inside this job with access to its secrets. + - name: Validate toolkit_ref + env: + TOOLKIT_REF: ${{ inputs.toolkit_ref }} + run: | + case "$TOOLKIT_REF" in + main|next) ;; + *) + if ! printf '%s' "$TOOLKIT_REF" | grep -qE '^[0-9a-fA-F]{40}$'; then + echo "::error::Invalid toolkit_ref: '$TOOLKIT_REF'. Allowed: main, next, or a full 40-char SHA." + exit 1 + fi + ;; + esac + echo "✅ toolkit_ref validated: $TOOLKIT_REF" + # Initial checkout - name: Checkout repository uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 diff --git a/.github/workflows/_claude-main.yml b/.github/workflows/_claude-main.yml index 818e1ee9..da9a8663 100644 --- a/.github/workflows/_claude-main.yml +++ b/.github/workflows/_claude-main.yml @@ -291,7 +291,12 @@ jobs: steps: # FIXED: Security scanning with bullfrog — block mode prevents exfiltration - # This step cannot be disabled or modified + # This step cannot be disabled or modified. + # Per .github/workflows/CLAUDE.md, bullfrog must be the FIRST step in + # every job on a non-macOS runner ("no exceptions"), so it runs before + # the toolkit_ref validation below. Ordering is safe: bullfrog does not + # consume toolkit_ref, and the validation runs before any step that + # downloads action.yml or post-*.ts content using it. - name: Security scanning uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: @@ -308,6 +313,25 @@ jobs: *.npmjs.org enable-sudo: false + # Validate toolkit_ref BEFORE any downstream step uses it to download + # action.yml / post-*.ts script content from the ai-toolkit repo. + # Without this, an attacker-controlled ref could substitute malicious + # script content that runs inside this job with access to its secrets. + - name: Validate toolkit_ref + env: + TOOLKIT_REF: ${{ inputs.toolkit_ref }} + run: | + case "$TOOLKIT_REF" in + main|next) ;; + *) + if ! printf '%s' "$TOOLKIT_REF" | grep -qE '^[0-9a-fA-F]{40}$'; then + echo "::error::Invalid toolkit_ref: '$TOOLKIT_REF'. Allowed: main, next, or a full 40-char SHA." + exit 1 + fi + ;; + esac + echo "✅ toolkit_ref validated: $TOOLKIT_REF" + # FIXED: Checkout with consistent fetch depth - name: Checkout repository uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 diff --git a/.github/workflows/_claude-task-worker.yml b/.github/workflows/_claude-task-worker.yml index 3a1c5320..62ffd4be 100644 --- a/.github/workflows/_claude-task-worker.yml +++ b/.github/workflows/_claude-task-worker.yml @@ -188,6 +188,12 @@ jobs: steps: # Security scanning — block mode prevents secret exfiltration if RCE is achieved. # + # Per .github/workflows/CLAUDE.md, bullfrog must be the FIRST step in + # every job on a non-macOS runner ("no exceptions"), so it runs before + # the toolkit_ref validation below. Ordering is safe: bullfrog does not + # consume toolkit_ref, and the validation runs before any step that + # downloads action.yml or post-*.ts content using it. + # # Allowlist is workflow-specific. github.com / api.github.com are in # bullfrog's defaultDomains (agent/agent.go:18) and don't need to be # listed. claude.ai / *.claude.ai apply only to the OAuth token path @@ -207,6 +213,26 @@ jobs: registry.npmjs.org enable-sudo: false + # Validate toolkit_ref BEFORE any downstream step uses it to download + # action.yml / post-*.ts script content from the ai-toolkit repo. + # Without this, an attacker-controlled ref (e.g., a fork branch passed + # by a caller) could substitute malicious script content that runs + # inside this job's process with access to its secrets. + - name: Validate toolkit_ref + env: + TOOLKIT_REF: ${{ inputs.toolkit_ref }} + run: | + case "$TOOLKIT_REF" in + main|next) ;; + *) + if ! printf '%s' "$TOOLKIT_REF" | grep -qE '^[0-9a-fA-F]{40}$'; then + echo "::error::Invalid toolkit_ref: '$TOOLKIT_REF'. Allowed: main, next, or a full 40-char SHA." + exit 1 + fi + ;; + esac + echo "✅ toolkit_ref validated: $TOOLKIT_REF" + # Checkout the target branch # Must come before validate-claude-auth (local composite action) - name: Checkout repository diff --git a/.github/workflows/_generate-pr-metadata.yml b/.github/workflows/_generate-pr-metadata.yml index 1a420307..ac82161f 100644 --- a/.github/workflows/_generate-pr-metadata.yml +++ b/.github/workflows/_generate-pr-metadata.yml @@ -172,12 +172,36 @@ jobs: pull-requests: write # Required to update PR title and description steps: - # Security scanning (required for all workflows) + # Security scanning (required for all workflows). + # Per .github/workflows/CLAUDE.md, bullfrog must be the FIRST step in + # every job on a non-macOS runner ("no exceptions"), so it runs before + # the toolkit_ref validation below. Ordering is safe: bullfrog does not + # consume toolkit_ref, and the validation runs before any step that + # downloads action.yml or post-*.ts content using it. - name: Security Scan uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: egress-policy: audit + # Validate toolkit_ref BEFORE any downstream step uses it to download + # action.yml / post-*.ts script content from the ai-toolkit repo. + # Without this, an attacker-controlled ref could substitute malicious + # script content that runs inside this job with access to its secrets. + - name: Validate toolkit_ref + env: + TOOLKIT_REF: ${{ inputs.toolkit_ref }} + run: | + case "$TOOLKIT_REF" in + main|next) ;; + *) + if ! printf '%s' "$TOOLKIT_REF" | grep -qE '^[0-9a-fA-F]{40}$'; then + echo "::error::Invalid toolkit_ref: '$TOOLKIT_REF'. Allowed: main, next, or a full 40-char SHA." + exit 1 + fi + ;; + esac + echo "✅ toolkit_ref validated: $TOOLKIT_REF" + # Checkout code with full history for pattern analysis # Must come before validate-claude-auth (local composite action) - name: Checkout Repository diff --git a/.github/workflows/_update-action-versions-worker.yml b/.github/workflows/_update-action-versions-worker.yml index 12fd67dc..3a4d7d64 100644 --- a/.github/workflows/_update-action-versions-worker.yml +++ b/.github/workflows/_update-action-versions-worker.yml @@ -125,12 +125,36 @@ jobs: actions: read steps: - # Security scanning + # Security scanning. + # Per .github/workflows/CLAUDE.md, bullfrog must be the FIRST step in + # every job on a non-macOS runner ("no exceptions"), so it runs before + # the toolkit_ref validation below. Ordering is safe: bullfrog does not + # consume toolkit_ref, and the validation runs before any step that + # downloads action.yml or post-*.ts content using it. - name: Security scanning uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: egress-policy: audit + # Validate toolkit_ref BEFORE any downstream step uses it to download + # action.yml / post-*.ts script content from the ai-toolkit repo. + # Without this, an attacker-controlled ref could substitute malicious + # script content that runs inside this job with access to its secrets. + - name: Validate toolkit_ref + env: + TOOLKIT_REF: ${{ inputs.toolkit_ref }} + run: | + case "$TOOLKIT_REF" in + main|next) ;; + *) + if ! printf '%s' "$TOOLKIT_REF" | grep -qE '^[0-9a-fA-F]{40}$'; then + echo "::error::Invalid toolkit_ref: '$TOOLKIT_REF'. Allowed: main, next, or a full 40-char SHA." + exit 1 + fi + ;; + esac + echo "✅ toolkit_ref validated: $TOOLKIT_REF" + # Checkout the update branch # Must come before validate-claude-auth (local composite action) - name: Checkout repository diff --git a/.github/workflows/claude-docs-check.yml b/.github/workflows/claude-docs-check.yml index acbf79d3..5886c4f3 100644 --- a/.github/workflows/claude-docs-check.yml +++ b/.github/workflows/claude-docs-check.yml @@ -74,8 +74,19 @@ jobs: fail_on_missing_version: true # Don't fail on missing docs (just warn) fail_on_missing_docs: false - # Use local actions since we're in ai-toolkit - toolkit_ref: ${{ github.head_ref || github.ref_name }} + # Pin to main rather than the trigger ref. ${{ github.head_ref || github.ref_name }} + # would let any PR author point the worker at action.yml / post-*.ts script + # content from their own branch — the worker then downloads and executes + # that content with the workflow's secrets (RCE-with-secrets via supply + # chain). Self-testing changes to worker scripts is now done by landing + # them to `next` first and verifying there, or by invoking the + # workflow_dispatch entrypoint with an explicit SHA in the allowlist. + # + # External repos copying this file as a template: keep this as 'main'. + # The reusable worker's toolkit_ref allowlist now blocks fork-branch + # values, but `next` is permitted and would pick up untested worker + # changes; pinning to 'main' avoids that. + toolkit_ref: 'main' secrets: # Use OAuth token (Pro/Max) instead of API key CLAUDE_CODE_OAUTH_TOKEN: ${{ secrets.CLAUDE_CODE_OAUTH_TOKEN }} From 8356485dfc245903f4b56f247000e3212c7eeb9b Mon Sep 17 00:00:00 2001 From: David Gilman Date: Thu, 28 May 2026 13:50:05 -0500 Subject: [PATCH 11/12] fix(workflows): tighten bullfrog allowlists per workflow (#516) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workflows): tighten bullfrog allowlists per workflow from empirical egress data Three workflows tightened based on bullfrog log analysis. Each workflow now has an allowlist that matches its actual observed egress instead of inheriting the shared 9-entry kitchen-sink list. Method: pulled all available successful run logs per workflow over the last 30 days. Extracted bullfrog audit-mode and block-mode events. Cross-referenced with each workflow's runtime code (Linear MCP yes/no, OAuth yes/no, package manager). Settled on the smallest allowlist that covers observed setup-phase egress plus required runtime hosts that audit mode can't see (DNS-reply coalescing per agent/queue_audit.nft). Cross-cutting drops vs the previous shared 9-entry allowlist: github.com / *.github.com — in bullfrog defaultDomains (agent/agent.go:18); 0 audit lines across all 17 audit-mode runs sampled. claude.ai / *.claude.ai — OAuth path; all three workflows use ANTHROPIC_API_KEY path; 0 audit lines. bun.sh — setup-bun fetches from github releases via *.githubusercontent.com; verified by 15 _generate-pr-metadata setup-bun runs showing 0 bun.sh lines. *.githubusercontent.com — only release-assets.githubusercontent.com is empirically used; pinned to the specific subdomain. *.npmjs.org — only registry.npmjs.org is used; pinned to the specific subdomain. Per-workflow tuning beyond cross-cutting drops: _claude-code-review.yml — block mode (unchanged). Empirical: 2 runs showed only Datadog logs agent blocked. No Linear MCP, dropped api.linear.app. Final allowlist: 3 entries. _claude-docs-check.yml — block mode (unchanged). Empirical: 12 runs showed only Datadog blocked. No Linear MCP. Final: 3 entries. _generate-pr-metadata.yml — FLIPPED audit → block. Empirical: 15 audit runs showed registry.npmjs.org + release-assets.githubusercontent.com. No Linear MCP. Final: 3 entries. NOT TOUCHED (insufficient data): _claude-main.yml — 0 runs in 30d; can't validate empirically. Leaving the existing allowlist as-is until data exists. _update-action-versions-worker.yml — 0 runs in 30d (weekly cron fired outside the window). Wildcards: bullfrog uses Go's path.Match (agent/agent.go:181), so *.foo.com covers any subdomain depth. We pin to exact subdomains empirically observed instead, matching the "based only on logs" review directive. Sister PRs: Uniswap/default-token-list#2485 — dtl Task Worker (per-job tight) Uniswap/ai-toolkit#514 — aitk Task Worker (per-job tight) Co-Authored-By: Claude Opus 4.7 (1M context) * fix(workflows): allow raw.githubusercontent.com for plugin discovery build-plugin-config/action.yml fetches marketplace.json from raw.githubusercontent.com/Uniswap/ai-toolkit/next/.claude-plugin/marketplace.json for plugin discovery. The audit-mode sampling that informed the tight allowlists missed this domain because bullfrog v0.8.4 queues DNS replies (agent/queue_audit.nft) and once a hostname is resolved+cached the kernel resolver short-circuits — no further audit events fire. In block mode the fetch now fails and breaks the "Build plugin configuration" composite step in docs-check, code-review, and generate-pr-metadata. Add raw.githubusercontent.com to all three tightened allowlists and update the explanatory comments so future maintainers understand the audit-mode caveat that led to the gap. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(workflows): allow claude.ai for Claude SDK install + runtime Second iteration of audit-mode-coalescing miss: the audit logs that informed the original tightening showed no claude.ai traffic, so the allowlist dropped it. Block-mode CI then failed at the Claude SDK invocation with: ##[warning] Blocked DNS request to claude.ai from unknown process ReferenceError: Claude Code native binary not found at /home/runner/.local/bin/claude The Claude SDK action installs the native CLI from claude.ai/install.sh and also touches claude.ai at runtime for telemetry/session-token lookups even on ANTHROPIC_API_KEY auth — none of which appeared in audit logs because bullfrog v0.8.4 queues DNS *replies* and cached resolutions emit no further events (agent/queue_audit.nft:13-14). Add claude.ai to all three tightened allowlists (_claude-docs-check.yml, _claude-code-review.yml, _generate-pr-metadata.yml). Update inline comments to record the methodology lesson so future tightenings don't repeat the trap. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(workflows): allow downloads.claude.ai for Claude CLI binary install Third iteration of audit-mode-coalescing miss. After adding claude.ai the Claude SDK install script entry point resolved, but its actual binary download fetches from a different host: curl: (6) Could not resolve host: downloads.claude.ai ReferenceError: Claude Code native binary not found at /home/runner/.local/bin/claude Add downloads.claude.ai to all three tightened allowlists. Lesson recorded in inline comments: bullfrog v0.8.4 audit-mode coalescing hides multiple domains per workflow, so the correct iterative tightening methodology is to flip block-mode in a single test run and harvest from the *block* logs (which DO show every blocked destination), not the audit logs. Co-Authored-By: Claude Opus 4.7 (1M context) * fix(workflows): drop 'Dropped:'/'Pinned:' enumerations from allowlist comments git blame on allowed-domains: already reveals what was removed and why, and runtime block-mode CI failure logs show what's currently blocked. Keep the rationale for each *kept* domain inline; let history speak for the rest. Co-Authored-By: Claude Opus 4.7 (1M context) --------- Co-authored-by: Claude Opus 4.7 (1M context) --- .github/workflows/_claude-code-review.yml | 17 +++++++++------ .github/workflows/_claude-docs-check.yml | 18 ++++++++++------ .github/workflows/_generate-pr-metadata.yml | 23 +++++++++++++++++++-- 3 files changed, 44 insertions(+), 14 deletions(-) diff --git a/.github/workflows/_claude-code-review.yml b/.github/workflows/_claude-code-review.yml index 7384515f..69661013 100644 --- a/.github/workflows/_claude-code-review.yml +++ b/.github/workflows/_claude-code-review.yml @@ -292,16 +292,21 @@ jobs: uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: egress-policy: block + # Per-workflow tightened allowlist. Audit-mode sampling missed + # several destinations because bullfrog v0.8.4 queues DNS *replies* + # (agent/queue_audit.nft) and cached resolutions emit no further + # events. Domains added below as block-mode CI failures revealed: + # - raw.githubusercontent.com: build-plugin-config fetches + # marketplace.json from /Uniswap/ai-toolkit/next/.claude-plugin/. + # - claude.ai: Claude SDK install path (claude.ai/install.sh) + # AND runtime telemetry/session lookup on API-key auth too. allowed-domains: | - github.com - *.github.com - *.githubusercontent.com + release-assets.githubusercontent.com + raw.githubusercontent.com api.anthropic.com claude.ai - *.claude.ai - bun.sh + downloads.claude.ai registry.npmjs.org - *.npmjs.org enable-sudo: false # Validate toolkit_ref BEFORE any downstream step uses it to download diff --git a/.github/workflows/_claude-docs-check.yml b/.github/workflows/_claude-docs-check.yml index 50b0a2cf..8c6298a1 100644 --- a/.github/workflows/_claude-docs-check.yml +++ b/.github/workflows/_claude-docs-check.yml @@ -214,16 +214,22 @@ jobs: uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: egress-policy: block + # Per-workflow tightened allowlist. Audit-mode sampling missed + # several destinations (raw.githubusercontent.com, claude.ai) + # because bullfrog v0.8.4 queues DNS *replies* + # (agent/queue_audit.nft) and cached resolutions emit no further + # events. Add domains here as block-mode CI failures reveal them. + # raw.githubusercontent.com: build-plugin-config fetches + # marketplace.json (Uniswap/ai-toolkit/next/.claude-plugin/). + # claude.ai: Claude SDK install path (https://claude.ai/install.sh) + # AND runtime telemetry/session lookup even on API-key auth. allowed-domains: | - github.com - *.github.com - *.githubusercontent.com + release-assets.githubusercontent.com + raw.githubusercontent.com api.anthropic.com claude.ai - *.claude.ai - bun.sh + downloads.claude.ai registry.npmjs.org - *.npmjs.org enable-sudo: false # Validate toolkit_ref BEFORE any downstream step uses it to download diff --git a/.github/workflows/_generate-pr-metadata.yml b/.github/workflows/_generate-pr-metadata.yml index ac82161f..462907ca 100644 --- a/.github/workflows/_generate-pr-metadata.yml +++ b/.github/workflows/_generate-pr-metadata.yml @@ -172,16 +172,35 @@ jobs: pull-requests: write # Required to update PR title and description steps: - # Security scanning (required for all workflows). + # Security scanning — flipped from audit to block. # Per .github/workflows/CLAUDE.md, bullfrog must be the FIRST step in # every job on a non-macOS runner ("no exceptions"), so it runs before # the toolkit_ref validation below. Ordering is safe: bullfrog does not # consume toolkit_ref, and the validation runs before any step that # downloads action.yml or post-*.ts content using it. + # + # Audit-mode sampling identified registry.npmjs.org (bun install) + # and release-assets.githubusercontent.com (setup-bun) as the only + # first-resolution destinations, BUT bullfrog v0.8.4 audit only + # queues DNS *replies* (agent/queue_audit.nft); cached resolutions + # emit no further events. Block-mode CI then revealed three more: + # api.anthropic.com (runtime API), raw.githubusercontent.com + # (build-plugin-config fetches marketplace.json), and claude.ai + # (Claude SDK install + runtime telemetry, even on API-key auth). + # github.com / api.github.com are in bullfrog defaultDomains. + # Add destinations here as future block-mode CI failures reveal them. - name: Security Scan uses: bullfrogsec/bullfrog@1831f79cce8ad602eef14d2163873f27081ebfb3 # v0.8.4 with: - egress-policy: audit + egress-policy: block + allowed-domains: | + release-assets.githubusercontent.com + raw.githubusercontent.com + api.anthropic.com + claude.ai + downloads.claude.ai + registry.npmjs.org + enable-sudo: false # Validate toolkit_ref BEFORE any downstream step uses it to download # action.yml / post-*.ts script content from the ai-toolkit repo. From 27e7e2de432ac025b23051c5ffab8d88a342b1f9 Mon Sep 17 00:00:00 2001 From: Nick Koutrelakos Date: Fri, 29 May 2026 10:34:45 -0700 Subject: [PATCH 12/12] fix(slack-oauth-backend): use fresh bot token, drop SLACK_BOT_TOKEN (#523) * fix(slack-oauth-backend): use fresh exchange bot token, drop static SLACK_BOT_TOKEN The users.info enrichment and DM delivery both authenticated with the static SLACK_BOT_TOKEN env var. With token rotation enabled, bot tokens are short-lived (xoxe, ~12h) and a static env var goes stale; it also dies on every reinstall. This surfaced as `account_inactive` from users.info in the OAuth callback (observed in Vercel logs). Both consumers run during the callback, when the fresh bot token from the exchange (tokenResponse.access_token) is in hand. Use it directly: - handler.ts: enrich users.info with the fresh token; surface it as OAuthResult.botAccessToken. - routes/oauth.ts: pass botAccessToken to createSlackClient for DM auth. - client.ts: SlackClient takes a per-request botToken instead of reading config; createSlackClient bypasses the singleton when a botToken is given so a per-request token is never cached across installs. - config/index.ts: remove slackBotToken; .env examples + README + DEPLOYMENT docs updated to note there is no static bot token. Health check (/health testAuth) gracefully reports token_rotation mode when no bot token is present, unchanged. Specs updated: bot token now passed explicitly to the constructor in client tests; new tests cover the oauth-only constructor path and the singleton-bypass factory behavior. 74/74 pass, typecheck + lint clean. * test(slack-oauth-backend): distinct bot/user token fixtures prove sourcing Addresses code-review feedback on #523: the handleCallback fixture used the same value for the bot slot (access_token) and user slot (authed_user.access_token), so the botAccessToken assertion passed vacuously and couldn't catch a regression that sourced it from the wrong slot. Give the bot slot a distinct xoxb- value. This also surfaced (and the test now pins) the bot-token fallback: when no user token is present, the selected accessToken correctly falls back to tokenResponse.access_token. --- apps/slack-oauth-backend/.env.example | 12 +- apps/slack-oauth-backend/.env.vercel.example | 3 +- apps/slack-oauth-backend/DEPLOYMENT.md | 3 +- apps/slack-oauth-backend/README.md | 5 - apps/slack-oauth-backend/src/config/index.ts | 14 +- .../src/oauth/handler.spec.ts | 43 +++-- apps/slack-oauth-backend/src/oauth/handler.ts | 17 +- apps/slack-oauth-backend/src/oauth/types.ts | 7 + apps/slack-oauth-backend/src/routes/oauth.ts | 4 +- .../src/slack/client.spec.ts | 177 ++++++++---------- apps/slack-oauth-backend/src/slack/client.ts | 60 +++--- .../tests/integration/oauth-flow.spec.ts | 51 +++-- 12 files changed, 179 insertions(+), 217 deletions(-) diff --git a/apps/slack-oauth-backend/.env.example b/apps/slack-oauth-backend/.env.example index ecc4c314..f3124e75 100644 --- a/apps/slack-oauth-backend/.env.example +++ b/apps/slack-oauth-backend/.env.example @@ -15,14 +15,10 @@ SLACK_CLIENT_SECRET=your_slack_client_secret_here # Example: https://your-domain.com/slack/oauth/callback SLACK_REDIRECT_URI=http://localhost:3000/slack/oauth/callback -# Slack Bot Token (OPTIONAL) -# NOTE: With token rotation enabled, there is no static bot token. -# This is only needed if you want the server to send DMs or look up user info. -# If not set, the server runs in "token rotation mode" - tokens are displayed -# in the browser instead of being sent via DM. -# Get this from https://api.slack.com/apps -> Your App -> OAuth & Permissions -# Should start with xoxb- -# SLACK_BOT_TOKEN=xoxb-your-bot-token-here +# NOTE: There is no SLACK_BOT_TOKEN. With token rotation enabled, bot tokens +# expire (~12h) and a static env var would silently go stale. Post-install +# actions (DMs, user lookup) use the freshly-issued bot token from each OAuth +# exchange instead. No bot-token configuration is required. # Security # Generate a secure random string for session secret diff --git a/apps/slack-oauth-backend/.env.vercel.example b/apps/slack-oauth-backend/.env.vercel.example index 78399d2c..21ff94db 100644 --- a/apps/slack-oauth-backend/.env.vercel.example +++ b/apps/slack-oauth-backend/.env.vercel.example @@ -5,7 +5,8 @@ # Required Slack Configuration SLACK_CLIENT_ID=your_slack_client_id_here SLACK_CLIENT_SECRET=your_slack_client_secret_here -SLACK_BOT_TOKEN=xoxb-your-bot-token-here +# No SLACK_BOT_TOKEN: with token rotation, bot tokens expire (~12h); the server +# uses the freshly-issued bot token from each OAuth exchange instead. # OAuth Redirect URI (will be auto-set after deployment) # Format: https://your-project-name.vercel.app/slack/oauth/callback diff --git a/apps/slack-oauth-backend/DEPLOYMENT.md b/apps/slack-oauth-backend/DEPLOYMENT.md index aac91ac0..9c3954f0 100644 --- a/apps/slack-oauth-backend/DEPLOYMENT.md +++ b/apps/slack-oauth-backend/DEPLOYMENT.md @@ -45,10 +45,11 @@ Add these environment variables in the Vercel dashboard: | --------------------- | -------------------------------------------------------- | --------- | | `SLACK_CLIENT_ID` | Your Slack app client ID | No | | `SLACK_CLIENT_SECRET` | Your Slack app client secret | Yes ✓ | -| `SLACK_BOT_TOKEN` | Bot user OAuth token (xoxb-...) | Yes ✓ | | `SESSION_SECRET` | 32+ character random string | Yes ✓ | | `SLACK_REDIRECT_URI` | `https://[your-project].vercel.app/slack/oauth/callback` | No | +> Note: there is no `SLACK_BOT_TOKEN`. With token rotation enabled, bot tokens expire (~12h), so the server authenticates post-install actions (DM delivery, user lookup) with the freshly-issued bot token from each OAuth exchange rather than a static env var. + #### Optional Variables | Variable | Description | Default | diff --git a/apps/slack-oauth-backend/README.md b/apps/slack-oauth-backend/README.md index 793d1b88..505e9a62 100644 --- a/apps/slack-oauth-backend/README.md +++ b/apps/slack-oauth-backend/README.md @@ -83,7 +83,6 @@ Edit `.env` with your credentials: # Required Slack Configuration SLACK_CLIENT_ID=your_client_id_here SLACK_CLIENT_SECRET=your_client_secret_here -SLACK_BOT_TOKEN=xoxb-your-bot-token SLACK_REDIRECT_URI=http://localhost:3000/slack/oauth/callback # Security (generate with: openssl rand -hex 32) @@ -316,7 +315,6 @@ The service is configured for Vercel deployment from an Nx monorepo. The configu - `SLACK_CLIENT_ID` - `SLACK_CLIENT_SECRET` - - `SLACK_BOT_TOKEN` - `SLACK_REDIRECT_URI` (use your Vercel deployment URL) - `SESSION_SECRET` @@ -355,7 +353,6 @@ The service is configured for Vercel deployment from an Nx monorepo. The configu # Set environment variables heroku config:set SLACK_CLIENT_ID=xxx heroku config:set SLACK_CLIENT_SECRET=xxx - heroku config:set SLACK_BOT_TOKEN=xxx heroku config:set SLACK_REDIRECT_URI=https://your-app.herokuapp.com/slack/oauth/callback # Deploy @@ -381,7 +378,6 @@ The service is configured for Vercel deployment from an Nx monorepo. The configu environment: SLACK_CLIENT_ID: ${env:SLACK_CLIENT_ID} SLACK_CLIENT_SECRET: ${env:SLACK_CLIENT_SECRET} - SLACK_BOT_TOKEN: ${env:SLACK_BOT_TOKEN} SLACK_REDIRECT_URI: ${env:SLACK_REDIRECT_URI} functions: @@ -453,7 +449,6 @@ gcloud run deploy slack-oauth-backend \ | --------------------- | -------- | ----------------------- | ------------------------------------------ | | `SLACK_CLIENT_ID` | ✅ | Slack app client ID | `123456789.987654321` | | `SLACK_CLIENT_SECRET` | ✅ | Slack app client secret | `abcdef123456` | -| `SLACK_BOT_TOKEN` | ✅ | Bot user OAuth token | `xoxb-123456789` | | `SLACK_REDIRECT_URI` | ✅ | OAuth redirect URL | `https://example.com/slack/oauth/callback` | | `SESSION_SECRET` | ✅ | Session encryption key | 32+ character random string | | `PORT` | ❌ | Server port | `3000` (default) | diff --git a/apps/slack-oauth-backend/src/config/index.ts b/apps/slack-oauth-backend/src/config/index.ts index d4de1368..7c8ce1ab 100644 --- a/apps/slack-oauth-backend/src/config/index.ts +++ b/apps/slack-oauth-backend/src/config/index.ts @@ -18,9 +18,6 @@ export interface Config { slackClientSecret: string; slackRedirectUri: string; - // Slack Bot configuration (optional - not available with token rotation) - slackBotToken?: string; - // Security sessionSecret: string; @@ -42,9 +39,7 @@ class ConfigurationError extends Error { function getRequiredEnv(key: string): string { const value = process.env[key]; if (!value) { - throw new ConfigurationError( - `Missing required environment variable: ${key}` - ); + throw new ConfigurationError(`Missing required environment variable: ${key}`); } return value; } @@ -65,9 +60,6 @@ function createConfig(): Config { slackClientSecret: getRequiredEnv('SLACK_CLIENT_SECRET'), slackRedirectUri: getRequiredEnv('SLACK_REDIRECT_URI'), - // Slack Bot configuration (optional - not available with token rotation) - slackBotToken: process.env['SLACK_BOT_TOKEN'], - // Security sessionSecret: getRequiredEnv('SESSION_SECRET'), @@ -84,9 +76,7 @@ function createConfig(): Config { } catch (error) { if (error instanceof ConfigurationError) { console.error(`Configuration Error: ${error.message}`); - console.error( - 'Please ensure all required environment variables are set.' - ); + console.error('Please ensure all required environment variables are set.'); console.error('See .env.example for required variables.'); process.exit(1); } diff --git a/apps/slack-oauth-backend/src/oauth/handler.spec.ts b/apps/slack-oauth-backend/src/oauth/handler.spec.ts index d1191077..118848aa 100644 --- a/apps/slack-oauth-backend/src/oauth/handler.spec.ts +++ b/apps/slack-oauth-backend/src/oauth/handler.spec.ts @@ -11,7 +11,6 @@ jest.mock('../config', () => ({ slackClientId: 'test-client-id', slackClientSecret: 'test-client-secret', slackRedirectUri: 'https://example.com/callback', - slackBotToken: 'xoxb-test-bot-token', }, })); @@ -28,21 +27,19 @@ describe('SlackOAuthHandler', () => { mockUsersInfo = jest.fn(); // Mock WebClient constructor and methods - (WebClient as jest.MockedClass).mockImplementation( - (_token?: string) => { - const client = { - oauth: { - v2: { - access: mockOAuthV2Access, - }, + (WebClient as jest.MockedClass).mockImplementation((_token?: string) => { + const client = { + oauth: { + v2: { + access: mockOAuthV2Access, }, - users: { - info: mockUsersInfo, - }, - } as any; - return client; - } - ); + }, + users: { + info: mockUsersInfo, + }, + } as any; + return client; + }); handler = new SlackOAuthHandler({ clientId: 'test-client-id', @@ -59,9 +56,7 @@ describe('SlackOAuthHandler', () => { expect(authUrl).toContain('https://slack.com/oauth/v2/authorize'); expect(authUrl).toContain('client_id=test-client-id'); - expect(authUrl).toContain( - 'redirect_uri=https%3A%2F%2Fexample.com%2Fcallback' - ); + expect(authUrl).toContain('redirect_uri=https%3A%2F%2Fexample.com%2Fcallback'); expect(authUrl).toContain('state=test-state-123456789'); expect(authUrl).toContain('scope=chat%3Awrite%2Cusers%3Aread'); }); @@ -105,7 +100,8 @@ describe('SlackOAuthHandler', () => { const mockTokenResponse = { ok: true, - access_token: 'xoxp-user-token', + // Distinct from the user slot below so assertions prove bot-slot sourcing. + access_token: 'xoxb-bot-token', token_type: 'user', scope: 'chat:write,users:read', team: { id: 'T123456', name: 'Test Team' }, @@ -144,7 +140,11 @@ describe('SlackOAuthHandler', () => { const result = await handler.handleCallback(validParams); expect(result.success).toBe(true); + // Selected token prefers the user token (authed_user.access_token). expect(result.accessToken).toBe('xoxp-user-token'); + // Bot token is sourced from the bot slot (tokenResponse.access_token), + // distinct from the user token, and surfaced for post-install DM auth. + expect(result.botAccessToken).toBe('xoxb-bot-token'); expect(result.user).toEqual({ id: 'U123456', name: 'testuser', @@ -189,7 +189,10 @@ describe('SlackOAuthHandler', () => { const result = await handler.handleCallback(validParams); expect(result.success).toBe(true); - expect(result.accessToken).toBe('xoxp-user-token'); + // No user token in the response, so the selected token falls back to + // the bot token (tokenResponse.access_token). + expect(result.accessToken).toBe('xoxb-bot-token'); + expect(result.botAccessToken).toBe('xoxb-bot-token'); expect(result.user).toBeUndefined(); expect(mockUsersInfo).not.toHaveBeenCalled(); }); diff --git a/apps/slack-oauth-backend/src/oauth/handler.ts b/apps/slack-oauth-backend/src/oauth/handler.ts index b2e95dc9..6baca619 100644 --- a/apps/slack-oauth-backend/src/oauth/handler.ts +++ b/apps/slack-oauth-backend/src/oauth/handler.ts @@ -124,14 +124,14 @@ export class SlackOAuthHandler implements OAuthHandler { enterpriseId: tokenResponse.enterprise?.id, }); - // Get user information using the bot token (if available) - // With token rotation, bot token may not be available + // Enrich with user info using the bot token freshly issued by THIS + // exchange. A static SLACK_BOT_TOKEN env var is incompatible with token + // rotation (xoxe tokens expire ~12h) and dies on every reinstall, which + // surfaced as `account_inactive` from users.info. The just-minted token + // is always valid and carries users:read. let userInfo: SlackUserInfo | undefined; - if (tokenResponse.authed_user?.id && config.slackBotToken) { - userInfo = await this.getUserInfo( - tokenResponse.authed_user.id, - config.slackBotToken // Use bot token to get user info - ); + if (tokenResponse.authed_user?.id && tokenResponse.access_token) { + userInfo = await this.getUserInfo(tokenResponse.authed_user.id, tokenResponse.access_token); } // Prefer the User OAuth Token (xoxp) if present; otherwise fall back to the Bot token @@ -147,6 +147,9 @@ export class SlackOAuthHandler implements OAuthHandler { success: true, accessToken: selectedToken, refreshToken: selectedRefreshToken, + // Surface the fresh bot token so the route can authenticate post-install + // actions (DM delivery) without a static, rotation-incompatible token. + botAccessToken: tokenResponse.access_token, user: userInfo, details: { team: tokenResponse.team, diff --git a/apps/slack-oauth-backend/src/oauth/types.ts b/apps/slack-oauth-backend/src/oauth/types.ts index a810921d..6b4fd6a9 100644 --- a/apps/slack-oauth-backend/src/oauth/types.ts +++ b/apps/slack-oauth-backend/src/oauth/types.ts @@ -97,6 +97,13 @@ export interface OAuthResult { accessToken?: string; /** Refresh token if successful (when token rotation is enabled) */ refreshToken?: string; + /** + * Bot access token freshly issued by this exchange. Used for post-install + * actions that must authenticate as the bot (e.g. DM delivery). Undefined if + * Slack did not issue a bot token for this install. Never persisted: with + * token rotation these expire (~12h), so callers must use it inline. + */ + botAccessToken?: string; /** User information if successful */ user?: SlackUserInfo; /** Error message if failed */ diff --git a/apps/slack-oauth-backend/src/routes/oauth.ts b/apps/slack-oauth-backend/src/routes/oauth.ts index 332801ad..be222dee 100644 --- a/apps/slack-oauth-backend/src/routes/oauth.ts +++ b/apps/slack-oauth-backend/src/routes/oauth.ts @@ -86,7 +86,9 @@ router.get( let dmSent = false; try { if (result.user?.id && result.accessToken) { - const slackClient = createSlackClient(); + // Authenticate the DM with the bot token freshly issued by this + // exchange (not a static env var, which dies under token rotation). + const slackClient = createSlackClient(undefined, result.botAccessToken); const tokenMessage = formatTokenMessage( result.accessToken, result.user.name, diff --git a/apps/slack-oauth-backend/src/slack/client.spec.ts b/apps/slack-oauth-backend/src/slack/client.spec.ts index 42b90889..3e3a631b 100644 --- a/apps/slack-oauth-backend/src/slack/client.spec.ts +++ b/apps/slack-oauth-backend/src/slack/client.spec.ts @@ -1,10 +1,5 @@ import { WebClient } from '@slack/web-api'; -import { - SlackClient, - SlackApiError, - createSlackClient, - clearSlackCaches, -} from './client'; +import { SlackClient, SlackApiError, createSlackClient, clearSlackCaches } from './client'; // Mock the Slack WebClient jest.mock('@slack/web-api'); @@ -15,10 +10,14 @@ jest.mock('../config', () => ({ slackClientId: 'test-client-id', slackClientSecret: 'test-client-secret', slackRedirectUri: 'https://example.com/callback', - slackBotToken: 'xoxb-test-bot-token', }, })); +// Bot token is supplied per-request by the caller (the fresh token from the +// OAuth exchange), not read from config. Tests pass it explicitly to the +// SlackClient constructor to exercise the bot-client code paths. +const TEST_BOT_TOKEN = 'xoxb-test-bot-token'; + describe('SlackClient', () => { let client: SlackClient; let mockOAuthV2Access: jest.Mock; @@ -41,36 +40,34 @@ describe('SlackClient', () => { mockAuthTest = jest.fn(); // Mock WebClient constructor and methods - (WebClient as jest.MockedClass).mockImplementation( - (_token?: string) => { - const webClient = { - oauth: { - v2: { - access: mockOAuthV2Access, - }, - }, - conversations: { - open: mockConversationsOpen, - }, - chat: { - postMessage: mockChatPostMessage, - }, - users: { - info: mockUsersInfo, - }, - auth: { - test: mockAuthTest, + (WebClient as jest.MockedClass).mockImplementation((_token?: string) => { + const webClient = { + oauth: { + v2: { + access: mockOAuthV2Access, }, - } as any; - return webClient; - } - ); + }, + conversations: { + open: mockConversationsOpen, + }, + chat: { + postMessage: mockChatPostMessage, + }, + users: { + info: mockUsersInfo, + }, + auth: { + test: mockAuthTest, + }, + } as any; + return webClient; + }); - client = new SlackClient(); + client = new SlackClient(undefined, TEST_BOT_TOKEN); }); describe('constructor', () => { - it('should create two WebClient instances with proper configuration', () => { + it('should create two WebClient instances when given a bot token', () => { expect(WebClient).toHaveBeenCalledTimes(2); // First call for oauth client (no token) @@ -87,10 +84,10 @@ describe('SlackClient', () => { }) ); - // Second call for bot client (with bot token) + // Second call for bot client (with the bot token passed to the constructor) expect(WebClient).toHaveBeenNthCalledWith( 2, - 'xoxb-test-bot-token', + TEST_BOT_TOKEN, expect.objectContaining({ retryConfig: { retries: 3, @@ -102,22 +99,23 @@ describe('SlackClient', () => { ); }); - it('should accept an optional token parameter', () => { + it('should create only the oauth client when no bot token is given', () => { + jest.clearAllMocks(); + new SlackClient('xoxp-user-token'); + + // No bot token => only the oauth client is constructed. + expect(WebClient).toHaveBeenCalledTimes(1); + expect(WebClient).toHaveBeenNthCalledWith(1, 'xoxp-user-token', expect.any(Object)); + }); + + it('should accept user and bot token parameters', () => { const customToken = 'xoxp-user-token'; - new SlackClient(customToken); + new SlackClient(customToken, TEST_BOT_TOKEN); // beforeEach creates a client (calls 1-2), then this test creates another (calls 3-4) // Check that calls 3-4 used the custom token and bot token - expect(WebClient).toHaveBeenNthCalledWith( - 3, - customToken, - expect.any(Object) - ); - expect(WebClient).toHaveBeenNthCalledWith( - 4, - 'xoxb-test-bot-token', - expect.any(Object) - ); + expect(WebClient).toHaveBeenNthCalledWith(3, customToken, expect.any(Object)); + expect(WebClient).toHaveBeenNthCalledWith(4, TEST_BOT_TOKEN, expect.any(Object)); }); }); @@ -160,9 +158,7 @@ describe('SlackClient', () => { error: 'invalid_code', }); - await expect(client.exchangeCode('invalid-code')).rejects.toThrow( - SlackApiError - ); + await expect(client.exchangeCode('invalid-code')).rejects.toThrow(SlackApiError); await expect(client.exchangeCode('invalid-code')).rejects.toMatchObject({ message: 'invalid_code', code: 'EXCHANGE_FAILED', @@ -172,9 +168,7 @@ describe('SlackClient', () => { it('should handle network errors during token exchange', async () => { mockOAuthV2Access.mockRejectedValue(new Error('Network error')); - await expect(client.exchangeCode('valid-code')).rejects.toThrow( - SlackApiError - ); + await expect(client.exchangeCode('valid-code')).rejects.toThrow(SlackApiError); await expect(client.exchangeCode('valid-code')).rejects.toMatchObject({ message: 'Failed to exchange authorization code', code: 'EXCHANGE_ERROR', @@ -221,9 +215,7 @@ describe('SlackClient', () => { it('should successfully send a direct message', async () => { const userId = 'U123456'; const message = 'Test message'; - const blocks = [ - { type: 'section', text: { type: 'mrkdwn', text: message } }, - ]; + const blocks = [{ type: 'section', text: { type: 'mrkdwn', text: message } }]; const result = await client.sendDirectMessage(userId, message, blocks); @@ -261,12 +253,8 @@ describe('SlackClient', () => { error: 'user_not_found', }); - await expect(client.sendDirectMessage('U999999', 'Test')).rejects.toThrow( - SlackApiError - ); - await expect( - client.sendDirectMessage('U999999', 'Test') - ).rejects.toMatchObject({ + await expect(client.sendDirectMessage('U999999', 'Test')).rejects.toThrow(SlackApiError); + await expect(client.sendDirectMessage('U999999', 'Test')).rejects.toMatchObject({ message: 'Failed to open direct message channel', code: 'OPEN_DM_FAILED', }); @@ -278,12 +266,8 @@ describe('SlackClient', () => { error: 'channel_not_found', }); - await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toThrow( - SlackApiError - ); - await expect( - client.sendDirectMessage('U123456', 'Test') - ).rejects.toMatchObject({ + await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toThrow(SlackApiError); + await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toMatchObject({ message: 'channel_not_found', code: 'SEND_MESSAGE_FAILED', }); @@ -292,12 +276,8 @@ describe('SlackClient', () => { it('should handle network errors when opening conversation', async () => { mockConversationsOpen.mockRejectedValue(new Error('Network timeout')); - await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toThrow( - SlackApiError - ); - await expect( - client.sendDirectMessage('U123456', 'Test') - ).rejects.toMatchObject({ + await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toThrow(SlackApiError); + await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toMatchObject({ message: 'Failed to send direct message', code: 'DM_ERROR', }); @@ -306,12 +286,8 @@ describe('SlackClient', () => { it('should handle network errors when sending message', async () => { mockChatPostMessage.mockRejectedValue(new Error('Network timeout')); - await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toThrow( - SlackApiError - ); - await expect( - client.sendDirectMessage('U123456', 'Test') - ).rejects.toMatchObject({ + await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toThrow(SlackApiError); + await expect(client.sendDirectMessage('U123456', 'Test')).rejects.toMatchObject({ message: 'Failed to send direct message', code: 'DM_ERROR', }); @@ -382,9 +358,7 @@ describe('SlackClient', () => { error: 'user_not_found', }); - await expect(client.getUserInfo('U999999')).rejects.toThrow( - SlackApiError - ); + await expect(client.getUserInfo('U999999')).rejects.toThrow(SlackApiError); await expect(client.getUserInfo('U999999')).rejects.toMatchObject({ message: 'Failed to get user information', code: 'USER_INFO_FAILED', @@ -394,9 +368,7 @@ describe('SlackClient', () => { it('should handle network errors', async () => { mockUsersInfo.mockRejectedValue(new Error('Network error')); - await expect(client.getUserInfo('U123456')).rejects.toThrow( - SlackApiError - ); + await expect(client.getUserInfo('U123456')).rejects.toThrow(SlackApiError); await expect(client.getUserInfo('U123456')).rejects.toMatchObject({ message: 'Failed to get user information', code: 'USER_INFO_ERROR', @@ -480,23 +452,29 @@ describe('SlackClient', () => { const client = createSlackClient('xoxp-user-token'); expect(client).toBeInstanceOf(SlackClient); - // beforeEach creates a client (calls 1-2), then this test creates another (calls 3-4) - expect(WebClient).toHaveBeenNthCalledWith( - 3, - 'xoxp-user-token', - expect.any(Object) - ); - expect(WebClient).toHaveBeenNthCalledWith( - 4, - 'xoxb-test-bot-token', - expect.any(Object) - ); + // beforeEach creates a client (calls 1-2), then this test creates another. + // Without a bot token, only the oauth client is constructed. + expect(WebClient).toHaveBeenNthCalledWith(3, 'xoxp-user-token', expect.any(Object)); + }); + + it('should return a fresh non-singleton instance when a bot token is given', () => { + const singletonA = createSlackClient(); + const singletonB = createSlackClient(); + // Tokenless calls share the singleton. + expect(singletonA).toBe(singletonB); + + // A per-request bot token must not be cached in the singleton, so each + // botToken call returns a fresh instance. + const withBotA = createSlackClient(undefined, TEST_BOT_TOKEN); + const withBotB = createSlackClient(undefined, TEST_BOT_TOKEN); + expect(withBotA).not.toBe(singletonA); + expect(withBotA).not.toBe(withBotB); }); }); describe('retry and timeout configuration', () => { it('should configure WebClient with retry settings', () => { - new SlackClient(); + new SlackClient(undefined, TEST_BOT_TOKEN); // Verify both WebClient instances were created with retry config const expectedConfig = { @@ -508,10 +486,7 @@ describe('SlackClient', () => { timeout: 10000, }; - expect(WebClient).toHaveBeenCalledWith( - undefined, - expect.objectContaining(expectedConfig) - ); + expect(WebClient).toHaveBeenCalledWith(undefined, expect.objectContaining(expectedConfig)); expect(WebClient).toHaveBeenCalledWith( 'xoxb-test-bot-token', expect.objectContaining(expectedConfig) diff --git a/apps/slack-oauth-backend/src/slack/client.ts b/apps/slack-oauth-backend/src/slack/client.ts index f18f2622..174f366f 100644 --- a/apps/slack-oauth-backend/src/slack/client.ts +++ b/apps/slack-oauth-backend/src/slack/client.ts @@ -21,7 +21,7 @@ export class SlackClient { private readonly botClient?: WebClient; private static instance?: SlackClient; - constructor(token?: string) { + constructor(token?: string, botToken?: string) { // Configure client options with retry, timeout, and connection pooling const clientOptions: WebClientOptions = { retryConfig: { @@ -36,9 +36,13 @@ export class SlackClient { // Client for OAuth operations (no token needed) this.client = new WebClient(token, clientOptions); - // Bot client for sending messages (optional - not available with token rotation) - if (config.slackBotToken) { - this.botClient = new WebClient(config.slackBotToken, clientOptions); + // Bot client for post-install actions (e.g. DMs). The token is supplied + // per-request by the caller (the fresh bot token from the OAuth exchange), + // never a static env var: with token rotation a stored bot token expires + // (~12h) and dies on reinstall. Absent token => no bot client, and bot + // operations skip gracefully (token rotation mode). + if (botToken) { + this.botClient = new WebClient(botToken, clientOptions); } } @@ -87,11 +91,9 @@ export class SlackClient { throw error; } - throw new SlackApiError( - 'Failed to exchange authorization code', - 'EXCHANGE_ERROR', - { originalError: error } - ); + throw new SlackApiError('Failed to exchange authorization code', 'EXCHANGE_ERROR', { + originalError: error, + }); } } @@ -174,7 +176,9 @@ export class SlackClient { async getUserInfo(userId: string): Promise { // Skip if no bot client (token rotation mode) if (!this.botClient) { - logger.info('Skipping user info lookup - bot token not configured (token rotation mode)', { userId }); + logger.info('Skipping user info lookup - bot token not configured (token rotation mode)', { + userId, + }); return null; } @@ -188,11 +192,7 @@ export class SlackClient { }); if (!response.ok || !response.user) { - throw new SlackApiError( - 'Failed to get user information', - 'USER_INFO_FAILED', - response - ); + throw new SlackApiError('Failed to get user information', 'USER_INFO_FAILED', response); } const user = response.user as any; @@ -218,11 +218,9 @@ export class SlackClient { throw error; } - throw new SlackApiError( - 'Failed to get user information', - 'USER_INFO_ERROR', - { originalError: error } - ); + throw new SlackApiError('Failed to get user information', 'USER_INFO_ERROR', { + originalError: error, + }); } }); } @@ -242,11 +240,7 @@ export class SlackClient { const response = await this.botClient.auth.test(); if (!response.ok) { - throw new SlackApiError( - 'Authentication test failed', - 'AUTH_TEST_FAILED', - response - ); + throw new SlackApiError('Authentication test failed', 'AUTH_TEST_FAILED', response); } return { @@ -262,11 +256,9 @@ export class SlackClient { throw error; } - throw new SlackApiError( - 'Failed to test authentication', - 'AUTH_TEST_ERROR', - { originalError: error } - ); + throw new SlackApiError('Failed to test authentication', 'AUTH_TEST_ERROR', { + originalError: error, + }); } } @@ -338,7 +330,13 @@ export interface AuthTestResponse { /** * Create a configured Slack client instance (uses singleton for better connection reuse) */ -export function createSlackClient(token?: string): SlackClient { +export function createSlackClient(token?: string, botToken?: string): SlackClient { + // A per-request bot token (fresh from an OAuth exchange) must never be cached + // in the singleton, or a later request would reuse an earlier install's token. + // Only the tokenless OAuth client is safe to share across requests. + if (botToken) { + return new SlackClient(token, botToken); + } return SlackClient.getInstance(token); } diff --git a/apps/slack-oauth-backend/tests/integration/oauth-flow.spec.ts b/apps/slack-oauth-backend/tests/integration/oauth-flow.spec.ts index da826d9e..77bafa9f 100644 --- a/apps/slack-oauth-backend/tests/integration/oauth-flow.spec.ts +++ b/apps/slack-oauth-backend/tests/integration/oauth-flow.spec.ts @@ -15,7 +15,6 @@ jest.mock('../../src/config', () => ({ slackClientId: 'test-client-id', slackClientSecret: 'test-client-secret', slackRedirectUri: 'https://example.com/callback', - slackBotToken: 'xoxb-test-bot-token', notionDocUrl: 'https://notion.so/setup-docs', environment: 'test', }, @@ -40,9 +39,7 @@ jest.mock('../../src/utils/logger', () => ({ // Mock security middleware to prevent rate limiting in tests // We only mock the rate limiter, keep validation middleware working jest.mock('../../src/middleware/security', () => { - const actual = jest.requireActual( - '../../src/middleware/security' - ) as typeof SecurityMiddleware; + const actual = jest.requireActual('../../src/middleware/security') as typeof SecurityMiddleware; return { ...actual, oauthRateLimiter: (_req: any, _res: any, next: any) => next(), @@ -77,27 +74,25 @@ describe('OAuth Flow Integration Tests', () => { mockUsersInfo = jest.fn(); // Mock WebClient constructor and methods - (WebClient as jest.MockedClass).mockImplementation( - (_token?: string) => { - const client = { - oauth: { - v2: { - access: mockOAuthV2Access, - }, + (WebClient as jest.MockedClass).mockImplementation((_token?: string) => { + const client = { + oauth: { + v2: { + access: mockOAuthV2Access, }, - conversations: { - open: mockConversationsOpen, - }, - chat: { - postMessage: mockChatPostMessage, - }, - users: { - info: mockUsersInfo, - }, - } as any; - return client; - } - ); + }, + conversations: { + open: mockConversationsOpen, + }, + chat: { + postMessage: mockChatPostMessage, + }, + users: { + info: mockUsersInfo, + }, + } as any; + return client; + }); }); describe('GET /slack/oauth/callback - Success Flow', () => { @@ -307,13 +302,9 @@ describe('OAuth Flow Integration Tests', () => { describe('GET /slack/oauth/authorize', () => { it('should generate OAuth URL and redirect', async () => { - const response = await request(app) - .get('/slack/oauth/authorize') - .expect(302); + const response = await request(app).get('/slack/oauth/authorize').expect(302); - expect(response.headers.location).toContain( - 'https://slack.com/oauth/v2/authorize' - ); + expect(response.headers.location).toContain('https://slack.com/oauth/v2/authorize'); expect(response.headers.location).toContain('client_id=test-client-id'); expect(response.headers.location).toContain( 'redirect_uri=https%3A%2F%2Fexample.com%2Fcallback'