Add configurable CORS handling to proxy-metrics and proxy-health functions#28
Add configurable CORS handling to proxy-metrics and proxy-health functions#28adm01-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 |
![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: 26c8be86f7
ℹ️ 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".
|
|
||
| function getCorsHeaders(req?: Request) { | ||
| const requestOrigin = req?.headers.get('origin') ?? '' | ||
| const allowOrigin = ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN |
There was a problem hiding this comment.
This fallback makes the allowlist non-authoritative: even if PROXY_HEALTH_ALLOWED_ORIGINS is explicitly set without https://pronto-talk-suite.lovable.app, requests from that origin still receive Access-Control-Allow-Origin equal to their origin because non-matches are forced to DEFAULT_ALLOWED_ORIGIN. In practice, operators cannot fully restrict CORS via env vars, and the same bypass pattern exists in proxy-metrics (getPromHeaders). For strict deployments, this unintentionally permits cross-origin reads from the hardcoded domain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
Adds configurable, origin-validated CORS behavior to the proxy-metrics and proxy-health Supabase Edge Functions, replacing the previous wildcard (*) CORS policy and adding Vary: Origin to preserve correct caching semantics.
Changes:
- Introduces per-function origin allowlists (from env) with a default origin + local dev origins.
- Generates response headers dynamically based on the request
Originand uses those headers for OPTIONS and main responses. - Adds
Vary: Originto avoid incorrect shared-cache behavior across different origins.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
supabase/functions/proxy-metrics/index.ts |
Replaces static PROM_HEADERS (with *) with dynamic headers derived from Origin + env-configured allowlist. |
supabase/functions/proxy-health/index.ts |
Replaces static CORS headers (with *) with a helper that selects an allowed origin and adds Vary: Origin. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const allowOrigin = ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN | ||
| return { | ||
| 'Access-Control-Allow-Origin': allowOrigin, | ||
| 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', | ||
| Vary: 'Origin', | ||
| } |
There was a problem hiding this comment.
The same fallback logic as proxy-metrics (ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN) makes the default origin effectively always allowed, even if PROXY_HEALTH_ALLOWED_ORIGINS is configured to exclude it. If the goal is a configurable allowlist, consider making the default origin configurable too, or omitting Access-Control-Allow-Origin for disallowed origins.
| const allowOrigin = ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN | |
| return { | |
| 'Access-Control-Allow-Origin': allowOrigin, | |
| 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', | |
| Vary: 'Origin', | |
| } | |
| const headers: Record<string, string> = { | |
| 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', | |
| Vary: 'Origin', | |
| } | |
| if (requestOrigin && ALLOWED_ORIGINS.has(requestOrigin)) { | |
| headers['Access-Control-Allow-Origin'] = requestOrigin | |
| } | |
| return headers |
| const allowOrigin = ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN | ||
| return { | ||
| 'Access-Control-Allow-Origin': allowOrigin, | ||
| 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', |
There was a problem hiding this comment.
getCorsHeaders() omits Access-Control-Allow-Methods. For preflight requests, browsers validate Access-Control-Request-Method against Access-Control-Allow-Methods, and omitting it can cause CORS failures (even for GET requests when using Authorization/custom headers). Add an explicit allow-methods value (e.g., GET, OPTIONS) to this header set.
| 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', | |
| 'Access-Control-Allow-Headers': 'authorization, x-client-info, apikey, content-type', | |
| 'Access-Control-Allow-Methods': 'GET, OPTIONS', |
| const DEFAULT_ALLOWED_ORIGIN = 'https://pronto-talk-suite.lovable.app' | ||
| const ALLOWED_ORIGINS = new Set( | ||
| (Deno.env.get('PROXY_HEALTH_ALLOWED_ORIGINS') | ||
| ?? `${DEFAULT_ALLOWED_ORIGIN},http://localhost:5173,http://127.0.0.1:5173`) | ||
| .split(',') |
There was a problem hiding this comment.
This function introduces its own allowed-origin parsing and CORS header construction, but the codebase already provides getCorsHeaders(req) / handleCors(req) in supabase/functions/_shared/validation.ts (with origin validation, localhost/preview handling, and security headers). Consider reusing/extending the shared helper to avoid CORS behavior diverging across edge functions.
| const allowOrigin = ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN | ||
| return { | ||
| 'Content-Type': 'text/plain; version=0.0.4; charset=utf-8', | ||
| 'Cache-Control': 'no-store', | ||
| 'Access-Control-Allow-Origin': allowOrigin, | ||
| 'Access-Control-Allow-Headers': 'authorization, content-type', | ||
| 'Access-Control-Allow-Methods': 'GET, OPTIONS', | ||
| Vary: 'Origin', | ||
| } |
There was a problem hiding this comment.
The fallback logic ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN means the default origin will still be CORS-allowed even if PROXY_METRICS_ALLOWED_ORIGINS is configured to exclude it (requests from the default origin will hit the fallback and still get Access-Control-Allow-Origin set). If the intent is a fully configurable allowlist, consider making the default origin configurable via env too, or omitting Access-Control-Allow-Origin when the request origin is not allowed.
| const allowOrigin = ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN | |
| return { | |
| 'Content-Type': 'text/plain; version=0.0.4; charset=utf-8', | |
| 'Cache-Control': 'no-store', | |
| 'Access-Control-Allow-Origin': allowOrigin, | |
| 'Access-Control-Allow-Headers': 'authorization, content-type', | |
| 'Access-Control-Allow-Methods': 'GET, OPTIONS', | |
| Vary: 'Origin', | |
| } | |
| const headers: Record<string, string> = { | |
| 'Content-Type': 'text/plain; version=0.0.4; charset=utf-8', | |
| 'Cache-Control': 'no-store', | |
| 'Access-Control-Allow-Headers': 'authorization, content-type', | |
| 'Access-Control-Allow-Methods': 'GET, OPTIONS', | |
| Vary: 'Origin', | |
| } | |
| if (requestOrigin && ALLOWED_ORIGINS.has(requestOrigin)) { | |
| headers['Access-Control-Allow-Origin'] = requestOrigin | |
| } | |
| return headers |
| function getPromHeaders(req?: Request) { | ||
| const requestOrigin = req?.headers.get('origin') ?? '' | ||
| const allowOrigin = ALLOWED_ORIGINS.has(requestOrigin) ? requestOrigin : DEFAULT_ALLOWED_ORIGIN | ||
| return { | ||
| 'Content-Type': 'text/plain; version=0.0.4; charset=utf-8', |
There was a problem hiding this comment.
This adds a local CORS/origin allowlist implementation, but the repo already has a shared, origin-validated getCorsHeaders(req) helper in supabase/functions/_shared/validation.ts (and notes not to use ad-hoc CORS in new code). Duplicating CORS logic in each function increases drift risk (e.g., preview origins / localhost port handling / security headers); consider reusing/extending the shared helper and layering Prometheus-specific headers (Content-Type, etc.) on top.
|
Folded into umbrella PR #32. Note: PR #27 (already on Generated by Claude Code |
Motivation
*forAccess-Control-Allow-Origin.Vary: Originheader.Description
DEFAULT_ALLOWED_ORIGINandALLOWED_ORIGINS(fromPROXY_METRICS_ALLOWED_ORIGINS/PROXY_HEALTH_ALLOWED_ORIGINS) and defaults to the app plus common localhost dev origins.getPromHeadersandgetCorsHeadershelper functions that pick theAccess-Control-Allow-Originbased on the requestOriginheader and includeVary: Origin.'*') with the dynamic headers in bothsupabase/functions/proxy-metrics/index.tsandsupabase/functions/proxy-health/index.tsand ensures OPTIONS and other responses use the computed headers.Testing
OPTIONSandGETrequests with allowed and disallowedOriginheaders; responses contained the expectedAccess-Control-Allow-OriginandVaryheaders.Codex Task