Enforce CORS for custom protocols#275
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
WalkthroughReplace CORS bypass mechanism with strict CORS enforcement for custom protocols. New module defines an allowlist of target protocols (flow:, flow-internal:) and validates request origin/protocol policies via Electron webRequest hooks, rejecting disallowed requests and applying conditional CORS response headers. Integration updates intercept rules initialization. ChangesCustom Protocol CORS Enforcement
Sequence Diagram(s)sequenceDiagram
participant Client as Client/Page
participant BeforeSend as onBeforeSendHeaders
participant Policy as Policy Check
participant HeadersRcv as onHeadersReceived
participant Response as Response
participant Cleanup as Cleanup
Client->>BeforeSend: Custom protocol request
BeforeSend->>Policy: Extract origin & protocol
Policy-->>BeforeSend: Policy decision (allow/deny)
alt Policy denies
BeforeSend-->>Client: Cancel request
else Policy allows
BeforeSend->>HeadersRcv: Store request metadata
HeadersRcv->>Response: Strip existing CORS headers
Response->>Response: Apply Access-Control-Allow-*
Response->>Response: Merge Vary header
Response-->>Client: Modified response
Response->>Cleanup: Request completion
end
Cleanup->>Cleanup: Remove request state
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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 |
Greptile SummaryThis PR replaces the old wildcard
Confidence Score: 4/5The core security improvement is sound — wildcard CORS is gone, origins are validated against a strict allowlist, and disallowed requests are cancelled before they reach the protocol handler. The change is safe to merge for typical flows between the custom protocols. The origin-validation and request-lifecycle logic are correct and well-structured. Two headers (Access-Control-Allow-Credentials and Access-Control-Expose-Headers) are stripped from every custom-protocol response but never restored, which would silently break any credentialed cross-origin fetch or header-exposure scenario from an allowed origin. A minor Vary deduplication issue with case-sensitive Set comparisons could produce duplicate tokens. Neither issue affects the existing protocol handlers today, but they are behaviour regressions worth fixing before the allowlist is widened. src/main/controllers/sessions-controller/intercept-rules/custom-protocol-cors.ts — specifically the onHeadersReceived injection block and the appendVaryHeader helper. Important Files Changed
Sequence DiagramsequenceDiagram
participant Browser
participant onBeforeSendHeaders
participant corsRequests as corsRequests Map
participant onHeadersReceived
participant ProtocolHandler as Protocol Handler (flow://)
Browser->>onBeforeSendHeaders: request with Origin header
onBeforeSendHeaders->>onBeforeSendHeaders: check target protocol in CUSTOM_PROTOCOLS
alt origin not allowed
onBeforeSendHeaders-->>Browser: cancel: true
else origin allowed
onBeforeSendHeaders->>corsRequests: "set(id, {origin, requestedMethod, requestedHeaders})"
onBeforeSendHeaders-->>Browser: proceed
Browser->>ProtocolHandler: send request
ProtocolHandler-->>onHeadersReceived: response headers
onHeadersReceived->>onHeadersReceived: stripCorsResponseHeaders()
onHeadersReceived->>corsRequests: get(id)
corsRequests-->>onHeadersReceived: metadata
onHeadersReceived->>onHeadersReceived: "set Access-Control-Allow-Origin = exact origin"
onHeadersReceived->>onHeadersReceived: set ACAM / ACAH (if preflight)
onHeadersReceived->>onHeadersReceived: appendVaryHeader()
onHeadersReceived-->>Browser: modified response headers
Browser->>corsRequests: onCompleted / onErrorOccurred / onBeforeRedirect delete(id)
end
Reviews (1): Last reviewed commit: "Enforce CORS for custom protocols" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function appendVaryHeader(responseHeaders: Record<string, string[]>, values: string[]) { | ||
| const existingHeader = Object.keys(responseHeaders).find((header) => header.toLowerCase() === "vary"); | ||
| const existingValues = existingHeader ? responseHeaders[existingHeader] : []; | ||
| const mergedValues = new Set<string>(); | ||
|
|
||
| for (const value of existingValues) { | ||
| value | ||
| .split(",") | ||
| .map((item) => item.trim()) | ||
| .filter(Boolean) | ||
| .forEach((item) => mergedValues.add(item)); | ||
| } | ||
|
|
||
| values.forEach((value) => mergedValues.add(value)); | ||
| responseHeaders[existingHeader ?? "Vary"] = [Array.from(mergedValues).join(", ")]; | ||
| } |
There was a problem hiding this comment.
appendVaryHeader
The mergedValues Set uses string equality for deduplication, so it is case-sensitive. If the existing Vary header already contains a value like "origin" (lowercase) and this function appends "Origin" (title-case), both tokens survive as distinct Set entries and the resulting header becomes "origin, Origin, ...". HTTP/1.1 header field names are case-insensitive, so proxies and cache layers will see duplicate effective tokens. Normalize all values to lowercase (or a single canonical casing) before adding them to mergedValues.
| if (corsRequest && isAllowedCustomProtocolCorsRequest(details.url, corsRequest.origin)) { | ||
| responseHeaders["Access-Control-Allow-Origin"] = [corsRequest.origin]; | ||
| if (corsRequest.requestedMethod) { | ||
| responseHeaders["Access-Control-Allow-Methods"] = [corsRequest.requestedMethod]; | ||
| } | ||
| if (corsRequest.requestedHeaders) { | ||
| responseHeaders["Access-Control-Allow-Headers"] = [corsRequest.requestedHeaders]; | ||
| } | ||
| appendVaryHeader(responseHeaders, ["Origin", "Access-Control-Request-Method", "Access-Control-Request-Headers"]); | ||
| } |
There was a problem hiding this comment.
Access-Control-Allow-Credentials stripped but never restored
stripCorsResponseHeaders removes the access-control-allow-credentials header from every custom-protocol response, and the injection block never adds it back. A credentialed fetch (e.g. fetch(url, { credentials: 'include' })) from an allowed origin will receive a response with Access-Control-Allow-Origin set to the exact origin, but without Access-Control-Allow-Credentials: true. The browser will block the response even though the origin is allowed. Access-Control-Expose-Headers has the same problem — it is stripped and never re-applied, so custom response headers are silently hidden from JavaScript.
| if (corsRequest && isAllowedCustomProtocolCorsRequest(details.url, corsRequest.origin)) { | |
| responseHeaders["Access-Control-Allow-Origin"] = [corsRequest.origin]; | |
| if (corsRequest.requestedMethod) { | |
| responseHeaders["Access-Control-Allow-Methods"] = [corsRequest.requestedMethod]; | |
| } | |
| if (corsRequest.requestedHeaders) { | |
| responseHeaders["Access-Control-Allow-Headers"] = [corsRequest.requestedHeaders]; | |
| } | |
| appendVaryHeader(responseHeaders, ["Origin", "Access-Control-Request-Method", "Access-Control-Request-Headers"]); | |
| } | |
| if (corsRequest && isAllowedCustomProtocolCorsRequest(details.url, corsRequest.origin)) { | |
| responseHeaders["Access-Control-Allow-Origin"] = [corsRequest.origin]; | |
| responseHeaders["Access-Control-Allow-Credentials"] = ["true"]; | |
| if (corsRequest.requestedMethod) { | |
| responseHeaders["Access-Control-Allow-Methods"] = [corsRequest.requestedMethod]; | |
| } | |
| if (corsRequest.requestedHeaders) { | |
| responseHeaders["Access-Control-Allow-Headers"] = [corsRequest.requestedHeaders]; | |
| } | |
| appendVaryHeader(responseHeaders, ["Origin", "Access-Control-Request-Method", "Access-Control-Request-Headers"]); | |
| } |
1 participant
Summary
Fixes #166
Validation
Summary by CodeRabbit