chore(api): drop redundant matcher entry from CORS proxy#4492
chore(api): drop redundant matcher entry from CORS proxy#4492saddlepaddle wants to merge 1 commit into
Conversation
|
Capy auto-review is paused for this organization because the monthly auto-review limit has been reached. Increase the limit or turn it off in billing settings to resume automatic reviews. |
Greptile SummaryThis PR narrows the Next.js middleware matcher in
Confidence Score: 4/5Safe to merge — the CORS logic itself is unchanged and the excluded endpoints are verified server-to-server callers that never send Origin headers. The matcher change correctly excludes the four server-to-server endpoint groups. The two minor concerns are the lack of word-boundary guards on apps/api/src/proxy.ts — the new exclusion regex is worth a second look for prefix-matching breadth.
|
| Filename | Overview |
|---|---|
| apps/api/src/proxy.ts | Matcher narrowed to exclude four server-to-server endpoint groups from CORS middleware; two excluded prefixes lack word-boundary guards, and removing the second old matcher changes static-extension coverage for API paths. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Incoming /api/* request] --> B{Matches new middleware matcher?}
B -- "Starts with _next / ingest / monitoring OR static file extension" --> C[Middleware skipped - no CORS headers]
B -- "Starts with api/auth/jwks, api/auth/token, api/github/webhook, api/integrations/*/webhook" --> D[Server-to-server: Middleware skipped - no CORS headers]
B -- "All other paths e.g. tRPC, /api/auth/get-session, durable streams" --> E[CORS middleware runs]
E --> F{OPTIONS preflight?}
F -- Yes --> G[204 + CORS headers]
F -- No --> H[NextResponse.next + CORS headers set]
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
apps/api/src/proxy.ts:77
The excluded prefixes `api/auth/token` and `api/github/webhook` use plain prefix matching with no path separator after them, so the lookahead also silently excludes any future path that merely starts with these strings. For example, a hypothetical browser-facing `api/auth/token-refresh` or `api/github/webhooks` endpoint would be matched by the exclusion and would never receive CORS headers — failing silently for cross-origin browser calls. Adding a trailing `/` (or `$`-anchoring where the path is a leaf) tightens the scope to exactly the intended endpoints.
```suggestion
"/((?!_next|ingest|monitoring|api/auth/jwks|api/auth/token$|api/auth/token/|api/github/webhook$|api/github/webhook/|api/integrations/[^/]+/webhook|[^?]*\.(?:html?|css|js(?!on)|jpe?g|webp|png|gif|svg|ttf|woff2?|ico|csv|docx?|xlsx?|zip|webmanifest))).*)",
```
### Issue 2 of 2
apps/api/src/proxy.ts:77
Dropping the second old matcher `/(api|trpc)(.*)` changes CORS coverage for any API/tRPC path whose URL contains a static-file extension (e.g. `/api/bundle.js` or `/trpc/subscriptions.ndjson`). The old union would have applied CORS headers via the second rule; the new single matcher's extension exclusion wins, so those paths would receive no CORS headers. In practice this is theoretical since Next.js API routes and tRPC routers don't expose extension-suffixed paths, but it is a subtle semantic difference worth being aware of if new endpoint patterns are added.
Reviews (1): Last reviewed commit: "fix(api/cost): skip CORS middleware on s..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| matcher: [ | ||
| "/((?!_next|ingest|monitoring|[^?]*\\.(?:html?|css|js(?!on)|jpe?g|webp|png|gif|svg|ttf|woff2?|ico|csv|docx?|xlsx?|zip|webmanifest)).*)", | ||
| "/(api|trpc)(.*)", | ||
| "/((?!_next|ingest|monitoring|api/auth/jwks|api/auth/token|api/github/webhook|api/integrations/[^/]+/webhook|[^?]*\\.(?:html?|css|js(?!on)|jpe?g|webp|png|gif|svg|ttf|woff2?|ico|csv|docx?|xlsx?|zip|webmanifest)).*)", |
There was a problem hiding this comment.
api/auth/token and api/github/webhook use plain prefix matching with no path separator after them, so the lookahead also silently excludes any future path that merely starts with these strings. For example, a hypothetical browser-facing api/auth/token-refresh or api/github/webhooks endpoint would be matched by the exclusion and would never receive CORS headers — failing silently for cross-origin browser calls. Adding a trailing / (or $-anchoring where the path is a leaf) tightens the scope to exactly the intended endpoints.
| "/((?!_next|ingest|monitoring|api/auth/jwks|api/auth/token|api/github/webhook|api/integrations/[^/]+/webhook|[^?]*\\.(?:html?|css|js(?!on)|jpe?g|webp|png|gif|svg|ttf|woff2?|ico|csv|docx?|xlsx?|zip|webmanifest)).*)", | |
| "/((?!_next|ingest|monitoring|api/auth/jwks|api/auth/token$|api/auth/token/|api/github/webhook$|api/github/webhook/|api/integrations/[^/]+/webhook|[^?]*\.(?:html?|css|js(?!on)|jpe?g|webp|png|gif|svg|ttf|woff2?|ico|csv|docx?|xlsx?|zip|webmanifest))).*)", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/api/src/proxy.ts
Line: 77
Comment:
The excluded prefixes `api/auth/token` and `api/github/webhook` use plain prefix matching with no path separator after them, so the lookahead also silently excludes any future path that merely starts with these strings. For example, a hypothetical browser-facing `api/auth/token-refresh` or `api/github/webhooks` endpoint would be matched by the exclusion and would never receive CORS headers — failing silently for cross-origin browser calls. Adding a trailing `/` (or `$`-anchoring where the path is a leaf) tightens the scope to exactly the intended endpoints.
```suggestion
"/((?!_next|ingest|monitoring|api/auth/jwks|api/auth/token$|api/auth/token/|api/github/webhook$|api/github/webhook/|api/integrations/[^/]+/webhook|[^?]*\.(?:html?|css|js(?!on)|jpe?g|webp|png|gif|svg|ttf|woff2?|ico|csv|docx?|xlsx?|zip|webmanifest))).*)",
```
How can I resolve this? If you propose a fix, please make it concise.| matcher: [ | ||
| "/((?!_next|ingest|monitoring|[^?]*\\.(?:html?|css|js(?!on)|jpe?g|webp|png|gif|svg|ttf|woff2?|ico|csv|docx?|xlsx?|zip|webmanifest)).*)", | ||
| "/(api|trpc)(.*)", | ||
| "/((?!_next|ingest|monitoring|api/auth/jwks|api/auth/token|api/github/webhook|api/integrations/[^/]+/webhook|[^?]*\\.(?:html?|css|js(?!on)|jpe?g|webp|png|gif|svg|ttf|woff2?|ico|csv|docx?|xlsx?|zip|webmanifest)).*)", |
There was a problem hiding this comment.
/(api|trpc)(.*) changes CORS coverage for any API/tRPC path whose URL contains a static-file extension (e.g. /api/bundle.js or /trpc/subscriptions.ndjson). The old union would have applied CORS headers via the second rule; the new single matcher's extension exclusion wins, so those paths would receive no CORS headers. In practice this is theoretical since Next.js API routes and tRPC routers don't expose extension-suffixed paths, but it is a subtle semantic difference worth being aware of if new endpoint patterns are added.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/api/src/proxy.ts
Line: 77
Comment:
Dropping the second old matcher `/(api|trpc)(.*)` changes CORS coverage for any API/tRPC path whose URL contains a static-file extension (e.g. `/api/bundle.js` or `/trpc/subscriptions.ndjson`). The old union would have applied CORS headers via the second rule; the new single matcher's extension exclusion wins, so those paths would receive no CORS headers. In practice this is theoretical since Next.js API routes and tRPC routers don't expose extension-suffixed paths, but it is a subtle semantic difference worth being aware of if new endpoint patterns are added.
How can I resolve this? If you propose a fix, please make it concise.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe PR broadens the middleware ChangesCORS / proxy matcher
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
🚀 Preview Deployment🔗 Preview Links
Preview updates automatically with new commits |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
The proxy.ts middleware had two matcher entries: "/((?!_next|ingest|monitoring|<asset-exts>).*)" "/(api|trpc)(.*)" The second is fully covered by the first — Next's matcher dedupes overlapping patterns so behaviour is unchanged, it's just dead config. Dropping it leaves a single, easier-to-read pattern. No CORS or routing changes; chat-attachments stays local-only per Avi's follow-up decision, so the matcher doesn't need finer-grained scoping.
saddlepaddle
force-pushed
the
saddlepaddle/api-proxy-skip-server-to-server
branch
from
e4e6c40 to
2a2e15b
Compare
saddlepaddle
changed the title
|
Misread the ask — going to land the endpoint removal in a fresh PR instead. Closing this. |
1 participant
Summary
apps/api/src/proxy.tshad two matcher entries:```ts
matcher: [
"/((?!_next|ingest|monitoring|).)",
"/(api|trpc)(.)", // ← fully covered by the line above
],
```
The second is redundant — the first wildcard already matches everything not in its exclusion list, including
/api/*and/trpc/*. Next dedupes overlapping matchers so behaviour is identical, it's just dead config.Dropping it leaves a single readable pattern.
Scope
Test plan
app.superset.sh,admin.superset.sh, desktop renderer (CORS unchanged)Summary by CodeRabbit
Bug Fixes
Performance