diff --git a/.github/workflows/_claude-code-review.yml b/.github/workflows/_claude-code-review.yml index 3b3a5a80..69661013 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 @@ -275,22 +283,51 @@ 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: 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 + # 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 04d44609..8c6298a1 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 @@ -197,22 +205,52 @@ 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: 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 + # 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 @@ -507,18 +545,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 @@ -720,8 +770,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-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 7ab4a5dc..62ffd4be 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 @@ -179,11 +186,52 @@ jobs: actions: read steps: - # Security scanning + # 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 + # (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 + + # 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) @@ -535,10 +583,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 }} diff --git a/.github/workflows/_generate-pr-metadata.yml b/.github/workflows/_generate-pr-metadata.yml index 1a420307..462907ca 100644 --- a/.github/workflows/_generate-pr-metadata.yml +++ b/.github/workflows/_generate-pr-metadata.yml @@ -172,11 +172,54 @@ 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. + # 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) 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/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/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 }} 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 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 46e41cc7..6baca619 100644 --- a/apps/slack-oauth-backend/src/oauth/handler.ts +++ b/apps/slack-oauth-backend/src/oauth/handler.ts @@ -106,14 +106,32 @@ export class SlackOAuthHandler implements OAuthHandler { ); } - // Get user information using the bot token (if available) - // With token rotation, bot token may not be available + // 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, + }); + + // 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 @@ -129,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, @@ -230,6 +251,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', @@ -247,20 +270,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, }); 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 0a068326..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, @@ -155,6 +157,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); 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'