Use shared CORS header helper in proxy endpoints#27
Conversation
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughBoth Supabase proxy functions refactor header management by extracting CORS logic into shared helpers. The proxy-health function uses Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 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 |
There was a problem hiding this comment.
Pull request overview
This PR centralizes CORS behavior for the proxy-health and proxy-metrics Supabase Edge Functions by switching from local/static header objects to the shared getCorsHeaders(req) helper (origin-validated + request-aware).
Changes:
- Import
getCorsHeadersfromsupabase/functions/_shared/validation.tsinto both proxy endpoints. - Replace static CORS header constants with request-aware header builder helpers (
getJsonCorsHeaders,getPromHeaders). - Update all
Responsepaths (including OPTIONS and error responses) to use the new helpers with the incomingreq.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| supabase/functions/proxy-metrics/index.ts | Replaces static Prometheus response headers with getPromHeaders(req) built on shared CORS helper. |
| supabase/functions/proxy-health/index.ts | Replaces static CORS headers with getJsonCorsHeaders(req) built on shared CORS helper for OPTIONS/error/success responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', | ||
| function getJsonCorsHeaders(req?: Request) { | ||
| return { | ||
| ...getCorsHeaders(req), |
There was a problem hiding this comment.
getCorsHeaders(req) includes Access-Control-Allow-Methods: GET, POST, PUT, PATCH, DELETE, OPTIONS. Because this handler doesn’t enforce GET-only, this change makes cross-origin non-GET requests from allowed origins pass CORS preflight (previously the endpoint didn’t advertise allowed methods). To avoid widening the callable surface, either add an explicit method guard (405 for non-GET/OPTIONS) and/or override Access-Control-Allow-Methods here to GET, OPTIONS to match the endpoint contract.
| ...getCorsHeaders(req), | |
| ...getCorsHeaders(req), | |
| 'Access-Control-Allow-Methods': 'GET, OPTIONS', |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
supabase/functions/proxy-health/index.ts (1)
15-20: Inconsistent helper shape vs.getPromHeaders— embedContent-Typeto actually centralize headers.
getJsonCorsHeadersis named*Json*but does not setContent-Type: application/json, so call sites at lines 198 and 265 still have to manually spread it back in. The sibling helpergetPromHeadersinsupabase/functions/proxy-metrics/index.tsalready embeds itsContent-Type, so aligning this helper removes duplication and makes the OPTIONS / JSON responses use one source of truth.♻️ Proposed refactor
function getJsonCorsHeaders(req?: Request) { return { ...getCorsHeaders(req), + 'Content-Type': 'application/json', 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', } }Then simplify the call sites:
- return new Response(JSON.stringify({ error: error.message }), { - status: 500, headers: { ...getJsonCorsHeaders(req), 'Content-Type': 'application/json' }, - }) + return new Response(JSON.stringify({ error: error.message }), { + status: 500, headers: getJsonCorsHeaders(req), + })- }, null, 2), { - headers: { ...getJsonCorsHeaders(req), 'Content-Type': 'application/json' }, - }) + }, null, 2), { + headers: getJsonCorsHeaders(req), + })OPTIONS responses returning a
Content-Typeheader is benign and matches the pattern used byproxy-metrics.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@supabase/functions/proxy-health/index.ts` around lines 15 - 20, getJsonCorsHeaders currently omits the Content-Type header while getPromHeaders includes it; update getJsonCorsHeaders to add 'Content-Type': 'application/json' so it centralizes JSON response headers (mirror getPromHeaders' shape), then remove the manual spreading of Content-Type at the call sites that currently add it (so they simply use getJsonCorsHeaders for OPTIONS and JSON responses).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@supabase/functions/proxy-health/index.ts`:
- Around line 15-20: getJsonCorsHeaders currently omits the Content-Type header
while getPromHeaders includes it; update getJsonCorsHeaders to add
'Content-Type': 'application/json' so it centralizes JSON response headers
(mirror getPromHeaders' shape), then remove the manual spreading of Content-Type
at the call sites that currently add it (so they simply use getJsonCorsHeaders
for OPTIONS and JSON responses).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 96dee699-bb17-4544-a941-efba0cd695eb
📒 Files selected for processing (2)
supabase/functions/proxy-health/index.tssupabase/functions/proxy-metrics/index.ts
![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: 5e26ec0dd3
ℹ️ 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".
| const shared = getCorsHeaders(req) | ||
| return { | ||
| ...shared, |
There was a problem hiding this comment.
This helper now inherits getCorsHeaders(req), which only echoes a small allowlist of origins and otherwise falls back to a fixed Lovable origin, so browser requests from any other domain will fail CORS even though this endpoint was previously wildcard (*). If teams consume /proxy-health from external dashboards or status pages hosted outside that allowlist, those clients will start getting blocked at the browser despite the API still being reachable over HTTP.
Useful? React with 👍 / 👎.
Motivation
Description
getCorsHeadersfrom../_shared/validation.tsinto bothproxy-healthandproxy-metricsfunctions.corsHeaders/PROM_HEADERSwith small wrapper functionsgetJsonCorsHeaders(req?)andgetPromHeaders(req?)that mergegetCorsHeaders(req)with endpoint-specific headers likeContent-Typeand caching directives.Responseconstructions (OPTIONS, success, method/error, and DB error responses) to call the new header helper with the incomingreq.Testing
Codex Task
Summary by CodeRabbit