Centralize CORS headers and response header builders for proxy endpoints#29
Centralize CORS headers and response header builders for proxy endpoints#29adm01-debug wants to merge 1 commit into
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 (2)
✨ 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 refactors the proxy-health and proxy-metrics Supabase Edge Functions to centralize CORS/security headers by using _shared/validation.ts#getCorsHeaders(req) and to standardize response headers via small per-endpoint header builder helpers.
Changes:
- Replace inline CORS header literals with
getCorsHeaders(req)in both endpoints. - Introduce
buildJsonHeaders(req)/buildPromHeaders(req)to merge CORS/security headers with endpoint-specificContent-Type(and cache directives). - Apply the new header builders consistently across
OPTIONSand non-OPTIONSresponses.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| supabase/functions/proxy-metrics/index.ts | Replaces PROM_HEADERS with buildPromHeaders(req) built on getCorsHeaders(req) and reuses it across responses. |
| supabase/functions/proxy-health/index.ts | Replaces inline corsHeaders with buildJsonHeaders(req) built on getCorsHeaders(req) and reuses it across responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Deno.serve(async (req) => { | ||
| const headers = buildJsonHeaders(req) | ||
|
|
||
| if (req.method === 'OPTIONS') { | ||
| return new Response('ok', { headers: corsHeaders }) | ||
| return new Response('ok', { headers }) |
There was a problem hiding this comment.
buildJsonHeaders(req) always sets Content-Type: application/json, but the OPTIONS preflight response body is the plain string 'ok'. This makes the OPTIONS response content-type inaccurate and may confuse clients/tools. Consider using handleCors(req) (which returns an empty body) or returning null/empty body (e.g., 204) and/or omitting Content-Type for OPTIONS responses.
| import { createClient } from 'https://esm.sh/@supabase/supabase-js@2.49.1' | ||
| import { getCorsHeaders } from '../_shared/validation.ts' | ||
|
|
||
| const corsHeaders = { | ||
| 'Access-Control-Allow-Origin': '*', | ||
| 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', | ||
| function buildJsonHeaders(req: Request): Record<string, string> { | ||
| return { | ||
| ...getCorsHeaders(req), | ||
| 'Content-Type': 'application/json', |
There was a problem hiding this comment.
This change switches from Access-Control-Allow-Origin: * (previous inline corsHeaders) to origin-validated getCorsHeaders(req) (allowlist + fallback origin) and also adds default Cache-Control: no-store/security headers. That’s a behavioral change for CORS/caching, so the PR description’s claim that functional behavior is “otherwise unchanged” is no longer accurate; please update the description/release notes (or explicitly confirm the new CORS policy is intended for these endpoints).
| import { createClient } from 'https://esm.sh/@supabase/supabase-js@2.45.0' | ||
| import { getCorsHeaders } from '../_shared/validation.ts' | ||
|
|
||
| const SUPABASE_URL = Deno.env.get('SUPABASE_URL')! | ||
| const SERVICE_ROLE_KEY = Deno.env.get('SUPABASE_SERVICE_ROLE_KEY')! | ||
| const SCRAPE_TOKEN = Deno.env.get('PROXY_METRICS_TOKEN') ?? '' | ||
|
|
||
| const PROM_HEADERS = { | ||
| 'Content-Type': 'text/plain; version=0.0.4; charset=utf-8', | ||
| 'Cache-Control': 'no-store', | ||
| 'Access-Control-Allow-Origin': '*', | ||
| 'Access-Control-Allow-Headers': 'authorization, content-type', | ||
| 'Access-Control-Allow-Methods': 'GET, OPTIONS', | ||
| function buildPromHeaders(req: Request): Record<string, string> { | ||
| return { | ||
| ...getCorsHeaders(req), | ||
| 'Content-Type': 'text/plain; version=0.0.4; charset=utf-8', | ||
| 'Cache-Control': 'no-store', | ||
| } |
There was a problem hiding this comment.
This change replaces Access-Control-Allow-Origin: * with origin-validated getCorsHeaders(req) (allowlist + fallback origin) and adds default Cache-Control: no-store/security headers from _shared/validation.ts. That’s a functional change in CORS/caching behavior for proxy-metrics, so please reflect it in the PR description (or explicitly confirm this endpoint is intended to follow the stricter CORS allowlist now).
|
Folded into umbrella PR #32. Same target as the already-merged #27 — kept HEAD's centralized helper ( Generated by Claude Code |
Motivation
Content-Typeand cache headers while keeping CORS logic centralized in../_shared/validation.ts.Description
getCorsHeadersfrom../_shared/validation.tsinproxy-healthandproxy-metrics.corsHeaders/PROM_HEADERSobjects withbuildJsonHeaders(req)andbuildPromHeaders(req)functions that mergegetCorsHeaders(req)with endpoint-specific headers likeContent-TypeandCache-Control.OPTIONSresponses and all other responses in bothsupabase/functions/proxy-health/index.tsandsupabase/functions/proxy-metrics/index.ts.Testing
deno checkto validate TypeScript types and imports, and it completed successfully.deno test(existing test suite) and all tests passed.OPTIONS/GETresponses in CI to verify presence of CORS andContent-Type/cache headers, which succeeded.Codex Task