🛡️ Sentinel: [Medium] Fix CORS wildcard credentials vulnerability#108
🛡️ Sentinel: [Medium] Fix CORS wildcard credentials vulnerability#108mattjoyce wants to merge 1 commit into
Conversation
This commit updates the `corsMiddleware` in `internal/api/server.go` to explicitly enforce `Access-Control-Allow-Credentials: false` when a wildcard (`*`) is present in `AllowedOrigins`. It also sets `Access-Control-Allow-Origin: *`. According to the CORS specification, credentialed requests are strictly forbidden with a wildcard origin. The middleware correctly filtered origins, but in the case a wildcard was provided in the allowed origins configuration, it blindly set `Access-Control-Allow-Credentials: true` alongside allowing the origin, leading to a violation of the spec and a potential security risk if clients interpret this misconfigured header pair dynamically. Added `TestCORSMiddlewareWildcard` to `internal/api/server_test.go` to explicitly test this scenario. Co-authored-by: mattjoyce <278869+mattjoyce@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
ductile | aec7cb4 | Commit Preview URL Branch Preview URL |
May 29 2026, 12:09 PM |
📝 WalkthroughWalkthroughThis PR fixes a CORS security vulnerability where wildcard origins combined with credentials violates the spec. The middleware now detects wildcard ChangesCORS Wildcard Credentials Security Fix
🎯 2 (Simple) | ⏱️ ~12 minutes
🚥 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)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.jules/sentinel.md:
- Line 1: The header in .jules/sentinel.md currently violates MD041/MD022 and
uses the wrong date; replace the bare text "## 2024-05-29 - [Wildcard CORS with
Credentials Vulnerability]" with a top-level H1 (e.g., "# Sentinel Timeline")
followed by a blank line, then add the timeline entry as a properly separated
secondary heading or list item and update the date to 2026-05-29 so the file has
a single H1, surrounding blank lines, and the corrected incident/PR date.
In `@internal/api/server_test.go`:
- Around line 127-150: Update TestCORSMiddlewareWildcard in server_test.go to
also exercise an OPTIONS preflight request through corsMiddleware([]string{"*"})
and assert the wildcard preflight response sets Access-Control-Allow-Origin to
"*" and Access-Control-Allow-Credentials to "false"; build the preflight request
using httptest.NewRequest(http.MethodOptions, "/jobs", nil) with an Origin
header and appropriate Access-Control-Request-Method header, call
handler.ServeHTTP on a new recorder, and add assertions similar to the existing
GET checks to guard against regressions in corsMiddleware's OPTIONS handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: b30c1fff-e541-469b-ad7c-067907f8bda4
📒 Files selected for processing (3)
.jules/sentinel.mdinternal/api/server.gointernal/api/server_test.go
| @@ -0,0 +1,6 @@ | |||
| ## 2024-05-29 - [Wildcard CORS with Credentials Vulnerability] | |||
There was a problem hiding this comment.
Fix markdownlint and timeline clarity at the document header.
Line 1 currently triggers MD041/MD022 and uses a date (2024-05-29) that appears inconsistent with this PR’s timeline (2026-05-29). Please add a top-level H1 with proper surrounding blank lines and align the entry date to the actual incident/PR date to avoid audit confusion.
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 1-1: First line in a file should be a top-level heading
(MD041, first-line-heading, first-line-h1)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.jules/sentinel.md at line 1, The header in .jules/sentinel.md currently
violates MD041/MD022 and uses the wrong date; replace the bare text "##
2024-05-29 - [Wildcard CORS with Credentials Vulnerability]" with a top-level H1
(e.g., "# Sentinel Timeline") followed by a blank line, then add the timeline
entry as a properly separated secondary heading or list item and update the date
to 2026-05-29 so the file has a single H1, surrounding blank lines, and the
corrected incident/PR date.
| // TestCORSMiddlewareWildcard verifies that wildcard allows all and disables credentials. | ||
| func TestCORSMiddlewareWildcard(t *testing.T) { | ||
| called := false | ||
| handler := corsMiddleware([]string{"*"})(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | ||
| called = true | ||
| w.WriteHeader(http.StatusOK) | ||
| })) | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/jobs", nil) | ||
| req.Header.Set("Origin", "https://any.example") | ||
| resp := httptest.NewRecorder() | ||
|
|
||
| handler.ServeHTTP(resp, req) | ||
|
|
||
| if !called { | ||
| t.Fatal("handler was not called") | ||
| } | ||
| if got := resp.Header().Get("Access-Control-Allow-Origin"); got != "*" { | ||
| t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) | ||
| } | ||
| if got := resp.Header().Get("Access-Control-Allow-Credentials"); got != "false" { | ||
| t.Fatalf("Access-Control-Allow-Credentials = %q, want false", got) | ||
| } | ||
| } |
There was a problem hiding this comment.
Add wildcard preflight coverage.
This test validates actual requests, but not wildcard OPTIONS preflight. A regression there could silently reintroduce unsafe credential headers. Please add a wildcard preflight assertion for Access-Control-Allow-Origin: * and Access-Control-Allow-Credentials: false.
Proposed test addition
+func TestCORSMiddlewareWildcardPreflight(t *testing.T) {
+ handler := corsMiddleware([]string{"*"})(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+ t.Fatal("preflight should not call next handler")
+ }))
+
+ req := httptest.NewRequest(http.MethodOptions, "/jobs", nil)
+ req.Header.Set("Origin", "https://any.example")
+ req.Header.Set("Access-Control-Request-Method", http.MethodGet)
+ resp := httptest.NewRecorder()
+
+ handler.ServeHTTP(resp, req)
+
+ if resp.Code != http.StatusNoContent {
+ t.Fatalf("status = %d, want %d", resp.Code, http.StatusNoContent)
+ }
+ assertHeader(t, resp, "Access-Control-Allow-Origin", "*")
+ assertHeader(t, resp, "Access-Control-Allow-Credentials", "false")
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // TestCORSMiddlewareWildcard verifies that wildcard allows all and disables credentials. | |
| func TestCORSMiddlewareWildcard(t *testing.T) { | |
| called := false | |
| handler := corsMiddleware([]string{"*"})(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |
| called = true | |
| w.WriteHeader(http.StatusOK) | |
| })) | |
| req := httptest.NewRequest(http.MethodGet, "/jobs", nil) | |
| req.Header.Set("Origin", "https://any.example") | |
| resp := httptest.NewRecorder() | |
| handler.ServeHTTP(resp, req) | |
| if !called { | |
| t.Fatal("handler was not called") | |
| } | |
| if got := resp.Header().Get("Access-Control-Allow-Origin"); got != "*" { | |
| t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) | |
| } | |
| if got := resp.Header().Get("Access-Control-Allow-Credentials"); got != "false" { | |
| t.Fatalf("Access-Control-Allow-Credentials = %q, want false", got) | |
| } | |
| } | |
| func TestCORSMiddlewareWildcardPreflight(t *testing.T) { | |
| handler := corsMiddleware([]string{"*"})(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { | |
| t.Fatal("preflight should not call next handler") | |
| })) | |
| req := httptest.NewRequest(http.MethodOptions, "/jobs", nil) | |
| req.Header.Set("Origin", "https://any.example") | |
| req.Header.Set("Access-Control-Request-Method", http.MethodGet) | |
| resp := httptest.NewRecorder() | |
| handler.ServeHTTP(resp, req) | |
| if resp.Code != http.StatusNoContent { | |
| t.Fatalf("status = %d, want %d", resp.Code, http.StatusNoContent) | |
| } | |
| if got := resp.Header().Get("Access-Control-Allow-Origin"); got != "*" { | |
| t.Fatalf("Access-Control-Allow-Origin = %q, want *", got) | |
| } | |
| if got := resp.Header().Get("Access-Control-Allow-Credentials"); got != "false" { | |
| t.Fatalf("Access-Control-Allow-Credentials = %q, want false", got) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/api/server_test.go` around lines 127 - 150, Update
TestCORSMiddlewareWildcard in server_test.go to also exercise an OPTIONS
preflight request through corsMiddleware([]string{"*"}) and assert the wildcard
preflight response sets Access-Control-Allow-Origin to "*" and
Access-Control-Allow-Credentials to "false"; build the preflight request using
httptest.NewRequest(http.MethodOptions, "/jobs", nil) with an Origin header and
appropriate Access-Control-Request-Method header, call handler.ServeHTTP on a
new recorder, and add assertions similar to the existing GET checks to guard
against regressions in corsMiddleware's OPTIONS handling.
…vault fail-open ductile-admin source diligence found the secret_ref-for-plugins scheme cannot work: PluginConf has no secret_ref (only webhook/relay do); the only plugin secret path is the spawn-time stdin secrets map, needing principal==plugin-name (kebab, no normalization) AND the plugin reading that map. #107 = the dev workstream to make first-party plugins vault-native (the real secret-holder enforce path). #108 = vault compose is fail-OPEN on an unknown principal (silent no-secret) — out of step with privsep fail-closed spine.
…locate, secrets not a config reconcile Relocating plugin code to /opt invalidates attestation → #93 fail-safe downgrades to untrusted until plugin lock re-records fingerprints (witnessed live). Plugin secrets need the #107 vault-native workstream (PluginConf has no secret_ref; principals must be kebab; vault compose fail-open #108). migrate-everything splits 3 ways: keyless-now / secret-holders-after-#107 / unconfinable-admin-#106.
Enforced data-plane gateway live on :8081: vault carried, 5 keyless integrations enforced+attested on default(1001), config/plugin lock, admission re-enabled, all wall-bites pass. #93 downgrade proven live. Remaining carded as dev workstreams: #107 (HEAVY, secret-holders), #106 (admin instance), #108 (vault fail-open), #105 (FHS package). Old --user decommission deferred until #106+#107 land.
Compose already errors on unknown principal, but composePluginSecrets deliberately opted out (silent no-secrets) — fine for keyless plugins, a footgun for one that should receive secrets. New plugins.<name>.requires_vault: true makes an unknown/unregistered principal (or no vault wired) fail the spawn CLOSED + loud (ErrVaultPrincipalRequired). Default false preserves the coexistence opt-out. + schema + 2 tests (unknown-principal and no-composer fail closed).
…on card #110 captures the proven notify model (on-event + on-hook, real Discord posts under enforce), the bucket classification, and the 2 findings (notify_on_complete gating; each notify route needs its own vault_principal'd instance — corrects the trivial-carry assumption). Epic #83 records Phase 3: discord_notify + ap_canary vault-native + fail-closed live, full pipeline model proven, in-binary hardening (#100/#101/#104/#108) redeployed, plugin code @ a1934e5.
This PR fixes a security vulnerability where the API server's CORS middleware would blindly set
Access-Control-Allow-Credentials: trueif a wildcard*was configured in theAllowedOriginslist.🚨 Severity: MEDIUM
💡 Vulnerability: The CORS middleware allowed wildcard origins (
*) along withAccess-Control-Allow-Credentials: true, which is a violation of the W3C CORS specification and exposes the API to CSRF risks.🎯 Impact: If exploited by an overly-permissive client or misconfigured proxy acting on these headers dynamically, this could allow malicious sites to perform cross-origin requests with user credentials (e.g. cookies or Auth headers).
🔧 Fix: Updated
corsMiddlewareto detect if the allowed origins array contains a wildcard. If it does, the middleware explicitly setsAccess-Control-Allow-Origin: *andAccess-Control-Allow-Credentials: false, securing the endpoint for public (non-credentialed) access.✅ Verification: Added a new unit test,
TestCORSMiddlewareWildcard, ensuring wildcard requests set the correct secure CORS headers and disable credentials. Rungo test -v ./internal/api/...to verify.PR created automatically by Jules for task 18343104803289223748 started by @mattjoyce
Summary by CodeRabbit
Bug Fixes
Tests