fix(security): harden OAuth, auth, and API route security#30
Conversation
- Fix OAuth authorization code race condition with atomic UPDATE...WHERE used_at IS NULL to prevent double-exchange attacks - Add PKCE code_verifier length validation (43-128 chars per RFC 7636) and constant-time comparison for challenge verification - Add per-IP rate limiting on /token endpoint to prevent brute-force - Tighten non-prod JWT issuer allowlist (prefer explicit CLERK_ISSUER) - Validate Yahoo OAuth redirect Location header against *.yahoo.com - Add encodeURIComponent() on dynamic route params to prevent injection - Fail early with 401 when bearer token unavailable (defense-in-depth) - Tighten SSRF allowlist to Flaim-specific worker names only - Sanitize error messages in platform clients (log details server-side) - Remove JWT payload claim logging from auto-pull route - Add platform/sport/seasonYear enum validation on default league route https://claude.ai/code/session_01YGWmBeoSA3FT4xrQvYitx3
Removes DB round-trips on /token and ?raw=true requests by using Cloudflare Workers native rate_limits bindings. Fixes TOCTOU race, X-Forwarded-For spoofability, and grant_type scoping issues by deleting the Supabase-based rate limiting code entirely. - TOKEN_RATE_LIMITER: 10 req/60s per IP (brute-force PKCE protection) - CREDENTIALS_RATE_LIMITER: 15 req/60s per user (runaway loop protection) - Removed checkRateLimit, incrementRateLimit, RateLimitResult from OAuthStorage - Removed RATE_LIMIT_PER_DAY constant and X-RateLimit-* response headers - Updated test mocks with mock rate limiter bindings Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add Yahoo redirect catch block logging - Fix SSRF hostname bypass in debug route - Deduplicate VALID_SPORTS constants in default league route - Update sleeper test assertion for error message change Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Code Review: fix(security): harden OAuth, auth, and API route securityOverall this is a well-structured security hardening PR. The fixes address real vulnerabilities. A few items worth discussing before merge. Bugs / Correctness Issues1. Placeholder namespace_id values in wrangler.jsonc — likely a deploy-stopper The rate_limits bindings use namespace_id "1001" and "1002". Cloudflare Workers rate_limits bindings require a real namespace ID provisioned via 2. Yahoo redirect validation missing HTTPS protocol check File: web/app/api/connect/yahoo/authorize/route.ts The hostname is validated but the protocol is not. A redirect to http://api.login.yahoo.com (plain HTTP) would pass the current guard. Since Yahoo OAuth only uses HTTPS, add 3. Misleading comment in SSRF logic File: web/app/api/debug/test-mcp/route.ts The comment says "prevent bypass via fantasy-mcp.evil.workers.dev", but that hostname would actually pass the check because parts[0] is still "fantasy-mcp". The code correctly blocks the suffix bypass evil.fantasy-mcp.workers.dev. Update the comment to match. Practical risk is low since Cloudflare controls workers.dev, but the mismatch will confuse future reviewers. Security Observations (positive)
Regression Concern4. X-RateLimit- and Retry-After headers dropped from 429 responses* The old Supabase-based code emitted X-RateLimit-Limit, X-RateLimit-Remaining, X-RateLimit-Reset, and Retry-After on 429 responses. These are gone because the CF native rate limiter does not expose that metadata. If any client relied on those headers for backoff, it will now receive a bare 429 with no guidance. Not a blocker if clients handle 429 generically, but worth confirming. Minor Notes
Test CoverageThe eval-api-key.test.ts mock stubs both rate limiter bindings correctly. To better cover the new security paths, consider adding:
Summary: The core fixes (atomic code exchange, PKCE hardening, SSRF tightening, consistent early auth failures) are sound and well-implemented. The main item to resolve before merging is the placeholder namespace_id values in wrangler.jsonc — those will likely cause a production deploy failure. The HTTPS protocol check on Yahoo redirects is a quick one-liner worth adding. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the application's security posture and improves performance across several critical areas. Key updates include fortifying OAuth authorization flows against race conditions and timing attacks, migrating to more efficient native Cloudflare Workers rate limiting, and strengthening API route protections through stricter SSRF allowlists and JWT issuer validation. Additionally, it introduces comprehensive input validation for core league parameters and refines error messaging for external API interactions, contributing to a more robust and secure system. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 98132e49e6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
web/app/api/debug/test-mcp/route.ts
Outdated
| return parts.length >= 3 | ||
| && parts[parts.length - 1] === 'dev' | ||
| && parts[parts.length - 2] === 'workers' | ||
| && ALLOWED_WORKER_PREFIXES.some(prefix => parts[0] === prefix); |
There was a problem hiding this comment.
The new workers.dev check still allows arbitrary accounts as long as the first label matches an allowed prefix, so URLs like https://fantasy-mcp.evil.workers.dev/... pass validation. That means this endpoint can still be used to fetch attacker-controlled workers despite the hardening goal, because the account segment is never validated against a trusted value.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant security hardening measures across the authentication and API layers, including fixing an OAuth race condition with an atomic database update, strengthening PKCE validation, replacing database-based rate limiting with a more efficient native solution, tightening SSRF protection, and sanitizing error messages. While these enhancements are substantial and generally well-implemented, specific issues were identified in the debug route's SSRF protection logic, notably the inclusion of localhost in the allowlist and a bypassable check for Cloudflare Workers subdomains. Additionally, a minor suggestion was noted to improve readability in one of the new validation checks.
web/app/api/debug/test-mcp/route.ts
Outdated
| // Localhost for development | ||
| 'localhost', | ||
| '127.0.0.1', |
There was a problem hiding this comment.
The SSRF allowlist includes localhost and 127.0.0.1. While the comment suggests this is for development, these entries should be conditionally included based on the environment (e.g., process.env.NODE_ENV === 'development'). If this route is accessible in a production or staging environment, an attacker could use it to probe internal services running on the same host or within the local network.
| if (url.hostname.endsWith('.workers.dev')) { | ||
| const parts = url.hostname.split('.'); | ||
| return parts.length >= 3 | ||
| && parts[parts.length - 1] === 'dev' | ||
| && parts[parts.length - 2] === 'workers' | ||
| && ALLOWED_WORKER_PREFIXES.some(prefix => parts[0] === prefix); | ||
| } |
There was a problem hiding this comment.
The validation logic for workers.dev subdomains only checks the first segment of the hostname against ALLOWED_WORKER_PREFIXES. Since Cloudflare Workers subdomains are structured as worker-name.subdomain.workers.dev, an attacker can create a worker with one of the allowed names (e.g., fantasy-mcp) on their own Cloudflare account (e.g., fantasy-mcp.attacker.workers.dev) and bypass this check. This allows the debug route to fetch from attacker-controlled workers, which could lead to SSRF or leakage of sensitive headers if they are forwarded in the fetch call.
| if (redirectUrl.hostname !== 'api.login.yahoo.com' && !redirectUrl.hostname.endsWith('.yahoo.com')) { | ||
| console.error('Yahoo authorize: unexpected redirect target:', redirectUrl.hostname); | ||
| return NextResponse.json({ error: 'Invalid redirect target' }, { status: 502 }); | ||
| } |
There was a problem hiding this comment.
For improved readability, you could simplify the domain validation logic by first checking for valid domains and then handling the invalid case. This avoids a complex negative condition and makes the intent clearer.
const isYahooDomain = redirectUrl.hostname === 'api.login.yahoo.com' || redirectUrl.hostname.endsWith('.yahoo.com');
if (!isYahooDomain) {
console.error('Yahoo authorize: unexpected redirect target:', redirectUrl.hostname);
return NextResponse.json({ error: 'Invalid redirect target' }, { status: 502 });
}…lowlist Address PR review feedback: - Pin workers.dev check to gerrygugger account subdomain and enforce exactly 4 hostname segments (blocks fantasy-mcp.evil.workers.dev) - Gate localhost/127.0.0.1 behind NODE_ENV=development so they're excluded in production Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code Review — PR #30:
|
| Area | Status |
|---|---|
| OAuth race condition fix | ✅ Solid |
| PKCE RFC 7636 compliance | ✅ Solid |
| CF native rate limiting | ✅ Solid (pending namespace_id confirmation) |
| Auth guard hardening | ✅ Solid |
| SSRF tightening | ✅ Solid |
| Error message sanitization | ✅ Solid |
Retry-After on 429 |
🟡 Regression |
| Rate limiter namespace IDs | 🟡 Needs verification |
| Open redirect allowlist breadth | 🟡 Minor |
The PR is in good shape overall. The namespace_id question and the missing Retry-After are the items most worth resolving before merging to production.
🤖 Generated with Claude Code
2 participants
Summary
Test plan
auth-workertype-check and all 154 tests passsleeper-client49 tests passespn-client61 tests passyahoo-client50 tests passfantasy-mcp8 tests passReplaces #29 (stale review bot state).
🤖 Generated with Claude Code