feat: prefer cimd for worker oauth - #734
Conversation
|
Warning Review limit reached
Next review available in: 17 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughWorker OAuth now enables Client ID Metadata Documents with Dynamic Client Registration fallback. Tests cover discovery metadata, the complete CIMD authorization flow, MCP access, and rejection of invalid metadata without persisted OAuth records. The preview workflow also creates or updates deployment comments on pull requests. ChangesCIMD OAuth support
Worker preview comments
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OAuthClient
participant WorkerOAuth
participant ClientMetadataURL
participant OAuthKV
participant MCP
OAuthClient->>WorkerOAuth: Start authorize request
WorkerOAuth->>ClientMetadataURL: Fetch client metadata
ClientMetadataURL-->>WorkerOAuth: Return client metadata
OAuthClient->>WorkerOAuth: Approve and exchange code with PKCE
WorkerOAuth-->>OAuthClient: Return tokens
OAuthClient->>MCP: Send authenticated request
MCP-->>OAuthClient: Return MCP response
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
MCP tool token costMeasured with
Change from baseline
Per-tool changes
Per-tool breakdown
Per-tool counts encode each complete tool object independently. The total encodes the complete |
Bundle ReportBundle size has no change ✅ |
PR Summary by QodoPrefer CIMD for Worker OAuth with DCR fallback
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #734 +/- ##
==========================================
+ Coverage 85.46% 85.50% +0.03%
==========================================
Files 52 52
Lines 2573 2573
Branches 725 725
==========================================
+ Hits 2199 2200 +1
+ Misses 203 202 -1
Partials 171 171 ☔ View full report in Codecov by Harness. |
Unit Test Results 1 files 53 suites 4s ⏱️ Results for commit a935e4c. ♻️ This comment has been updated with latest results. |
|
Tick the box to add this pull request to the merge queue (same as
|
Code Review by Qodo
Context used✅ Compliance rules (platform):
74 rules 1. Uncaught client lookup errors
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| authorizeEndpoint: AUTHORIZE_PATH, | ||
| tokenEndpoint: TOKEN_PATH, | ||
| clientRegistrationEndpoint: REGISTER_PATH, | ||
| clientIdMetadataDocumentEnabled: true, |
There was a problem hiding this comment.
1. Uncaught client lookup errors 🐞 Bug ☼ Reliability
With CIMD enabled, client resolution during /authorize now involves outbound metadata fetches, but handleAuthorizeGet/handleAuthorizePost call helpers.lookupClient() without any error handling. If lookupClient rejects (e.g., metadata fetch/KV failure), the exception propagates into createWorkerFetchHandler which rethrows, resulting in an uncontrolled Worker error instead of a deterministic OAuth failure response.
Agent Prompt
## Issue description
Enabling CIMD (`clientIdMetadataDocumentEnabled: true`) makes client resolution depend on outbound fetches during authorization. `handleAuthorizeGet` and `handleAuthorizePost` currently `await helpers.lookupClient(...)` without a `try/catch`, so any rejection can escape the handler and become an uncaught Worker error.
## Issue Context
- `createWorkerFetchHandler` logs and then rethrows caught errors, so uncaught exceptions from the OAuth layer are not converted into a safe HTTP response.
- CIMD tests stub `globalThis.fetch` and assert it is called during `/authorize`, demonstrating that authorization now depends on outbound fetch.
## Fix Focus Areas
- packages/worker/src/worker-oauth.ts[254-308]
- packages/worker/src/worker.ts[463-466]
- packages/worker/src/worker-oauth.test.ts[817-929]
## Implementation notes
- Wrap `helpers.lookupClient(authRequest.clientId)` in both `handleAuthorizeGet` and `handleAuthorizePost` with `try/catch`.
- On failure:
- For GET: return a deterministic error page (ideally a 502-style message via `authorizeErrorResponse(...)` rather than a generic crash).
- For POST: re-render the consent page with a generic error (or return `authorizeErrorResponse(..., 502)`), consistent with other transient failures.
- Optionally log a sanitized diagnostic (similar to the existing `oauth-complete-authorization` logging) so operational failures are observable without leaking sensitive details.
- Add/extend a test that stubs `fetch` to reject during CIMD authorization and asserts the response is controlled (not an unhandled exception).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Cloudflare Worker preview
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.changeset/sixty-hats-raise.md (1)
1-3: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd release metadata for the runtime OAuth feature.
This changeset is empty, but CIMD support is a user-facing runtime change. Replace it with the affected package name and an appropriate
patch,minor, ormajorbump.As per coding guidelines: “user-facing runtime changes require a patch/minor/major changeset.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.changeset/sixty-hats-raise.md around lines 1 - 3, Replace the empty changeset front matter in sixty-hats-raise.md with release metadata for the package affected by the runtime OAuth/CIMD feature, selecting the appropriate patch, minor, or major bump and adding a concise user-facing summary.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/deploy-worker.yml:
- Around line 27-29: Update the workflow’s preview job permissions to retain
only contents read access, then move the PR comment step into a separate job
that depends on preview via needs and grants only pull-requests write
permission. Preserve the existing deployment steps in preview and ensure the new
job receives the outputs or context required by the comment step.
---
Outside diff comments:
In @.changeset/sixty-hats-raise.md:
- Around line 1-3: Replace the empty changeset front matter in
sixty-hats-raise.md with release metadata for the package affected by the
runtime OAuth/CIMD feature, selecting the appropriate patch, minor, or major
bump and adding a concise user-facing summary.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 24b08352-a1d6-4cde-9bd0-336f6aa50de4
📒 Files selected for processing (2)
.changeset/sixty-hats-raise.md.github/workflows/deploy-worker.yml
| permissions: | ||
| contents: read | ||
| pull-requests: write |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repository files matching deploy-worker:\n'
fd -a 'deploy-worker\.yml|deploy-worker\.yaml' . || true
printf '\nWorkflow file outline/contents length:\n'
if [ -f .github/workflows/deploy-worker.yml ]; then
wc -l .github/workflows/deploy-worker.yml
sed -n '1,260p' .github/workflows/deploy-worker.yml | cat -n
fi
printf '\nChangeset references:\n'
git ls-files .changeset 2>/dev/null || true
fd -a .changeset .github . 2>/dev/null | sed -n '1,100p'
if [ -d .changeset ]; then
git ls-files .changeset | sed -n '1,100p'
for f in .changeset/*; do
[ -f "$f" ] && { echo "--- $f"; cat "$f"; echo; }
done
fi
printf '\nRelated workflow permissions usages:\n'
rg -n "pull-requests:\s*(write|read)|deploy-worker|on:\s*$" .github/workflows || trueRepository: chrisdoc/hevy-mcp
Length of output: 16012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path(".github/workflows/deploy-worker.yml")
if not p.exists():
print("missing .github/workflows/deploy-worker.yml")
raise SystemExit
text = p.read_text()
lines = text.splitlines()
# Light YAML-like extraction for top-level jobs and permissions inside first job context.
in_jobs = False
current_job = None
for i, line in enumerate(lines, 1):
if line.startswith("jobs:"):
in_jobs = True
continue
if in_jobs and re.match(r"^\S.*:\s*$", line):
current_job = line.rstrip(":").strip()
print(f"job {current_job} starts line {i}")
if line.startswith("permissions:"):
print(f"permissions block at line {i} in job {current_job}")
# read next non-comment and non-empty indented blocks until next top job/section.
start = i
block_lines = []
for j in range(i, min(len(lines), i+20)):
if re.match(r"^\w+\s*:", lines[j]) or (j > i and re.search(r"\bpermissions:\s*$", lines[j-1]) is None and re.match(r"^[a-zA-Z_][\w-]*[:!?-]?\\w*:?$", lines[j])):
break
block_lines.append(lines[j])
print("\n".join(f"{k}: {block_lines[k-i]}" for k in range(start, min(len(lines), start+20))) if block_lines else "")
print("\nRelevant lines around permissions:")
for i in range(max(1,20), min(len(lines), 45)+1):
if "pull-requests" in lines[i-1] or "permissions" in lines[i-1] or "pull_request" in lines[i-1]:
print(f"{i}: {lines[i-1]}")
print("\nSequential dependency/comment deployment order:")
for i, line in enumerate(lines):
if any(tok in line for tok in ["needs:", "uses:", "run:", "pull-requests:", "pull_request_comment", "issue_comment", "actions/github-script", "azure-cloud-sdk", "azure-functions", "`@cloudflare/actions-deploy`"]):
print(f"{i+1}: {line}")
PYRepository: chrisdoc/hevy-mcp
Length of output: 346
🌐 Web query:
GitHub Actions permissions pull-requests write PR-based workflow job-scoped permissions security actions using pull_request checkout npm ci
💡 Result:
To secure GitHub Actions workflows involving pull requests, you must apply the principle of least privilege, especially when using triggers that grant elevated access [1][2][3]. Security Best Practices 1. Default to read-only: Set all repository and job-level GITHUB_TOKEN permissions to read-only [1][2][4]. Only explicitly grant 'write' permissions to individual jobs that require them [1][3]. 2. Use pull_request, not pull_request_target: Prefer the pull_request trigger, which is inherently safer because it runs with restricted permissions (no secrets access, read-only token) for forks [2][5]. 3. Secure pull_request_target: Only use pull_request_target when you require access to repository secrets or a write-enabled token [5]. Because this trigger runs in the context of the base branch, it is highly sensitive [6][7]. Never check out and execute code from an untrusted PR head while using this trigger unless you have carefully mitigated the risks [6][8][9]. 4. Protect against pwn requests: The actions/checkout action (v7+) now blocks common pwn request patterns—such as checking out untrusted fork code during a pull_request_target or workflow_run—by default [8][10][9]. Avoid using the allow-unsafe-pr-checkout: true input unless absolutely necessary [6][10]. Permissions Configuration Example For jobs requiring interaction with PRs or npm, scope permissions narrowly at the job level [3][11]: jobs: job-name: permissions: contents: read pull-requests: write # Only if needed to comment on PRs steps: - uses: actions/checkout@v7 - uses: actions/setup-node@v7 - run: npm ci When using npm ci, the setup-node action generally only requires contents: read to check out code and install dependencies [12]. If you are performing operations like publishing or updating dependencies that require writing to the repository, grant write access only to the specific job performing those tasks [3][11]. Always prefer OIDC for sensitive authentication (e.g., to npm registries) rather than long-lived secrets [4][11].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/secure-use
- 2: https://github.blog/security/supply-chain-security/four-tips-to-keep-your-github-actions-workflows-secure/
- 3: https://docs.github.com/actions/reference/authentication-in-a-workflow
- 4: https://secure-pipelines.com/ci-cd-security/github-actions-security-cheat-sheet/
- 5: https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/
- 6: https://docs.github.com/en/actions/reference/security/securely-using-pull_request_target
- 7: https://securitylab.github.com/resources/github-actions-new-patterns-and-mitigations/
- 8: https://socket.dev/blog/github-actions-checkout-blocks-pull-request-target-checkouts
- 9: https://github.blog/changelog/2026-06-18-safer-pull_request_target-defaults-for-github-actions-checkout/
- 10: https://github.com/actions/checkout
- 11: https://github.com/lurx/codegloss/blob/e58acbbec1a655dee6b8e13451c964f0a90d3954/.github/workflows/publish.yml
- 12: https://github.com/actions/setup-node
Isolate PR-write access from the deployment job.
pull-requests: write is set on the preview job, so every earlier step in that job—including actions/checkout, npm ci, and the Cloudflare deployment steps—runs with a write-enabled PR token. Move the PR comment step into a separate needs job that owns only pull-requests: write, and keep this job read-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/deploy-worker.yml around lines 27 - 29, Update the
workflow’s preview job permissions to retain only contents read access, then
move the PR comment step into a separate job that depends on preview via needs
and grants only pull-requests write permission. Preserve the existing deployment
steps in preview and ensure the new job receives the outputs or context required
by the comment step.
Source: MCP tools
1 participant
Summary
Verification
npx vitest run packages/worker/src/worker-oauth.test.tsnpm run test:unitnpm run test:worker-httpnpm run check:typesnpm run buildnpm run worker:dry-runnpm run checknpm run check:changesetnonetoken auth, and S256 PKCESummary by CodeRabbit