🛡️ Sentinel: [HIGH] Fix overly permissive CORS configuration#112
Open
mattjoyce wants to merge 1 commit into
Open
🛡️ Sentinel: [HIGH] Fix overly permissive CORS configuration#112mattjoyce wants to merge 1 commit into
mattjoyce wants to merge 1 commit into
Conversation
The CORS middleware previously allowed credentialed requests when configured with a wildcard origin (`*`), which could inadvertently expose sensitive information to arbitrary origins. This commit explicitly checks for the wildcard `*` in the `AllowedOrigins` configuration. If present, it securely handles requests by emitting `Access-Control-Allow-Origin: *` and avoiding emission of `Access-Control-Allow-Credentials`, aligning with browser specs and security best practices. Testing has been added to ensure compliance. Co-authored-by: mattjoyce <278869+mattjoyce@users.noreply.github.com>
Contributor
|
👋 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 | 1e54201 | Commit Preview URL Branch Preview URL |
Jun 02 2026, 12:07 PM |
📝 WalkthroughWalkthroughThis pull request fixes a CORS security vulnerability where the middleware allowed credential sharing with wildcard origins. The middleware now detects wildcard configuration and sets permissive headers without credentials; specific origins receive both the origin header and credentials header. ChangesCORS Wildcard Credentials Security Fix
Estimated code review effort🎯 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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:
- Around line 1-4: Change the top-level heading "## 2024-05-18 - [Fix CORS
Wildcard Vulnerability]" to a proper H1 (replace "##" with "#") and add a single
blank line after that heading so the file starts with an H1 followed by an empty
line, which will silence MD041/MD022; locate the exact heading text to update.
In `@plan.md`:
- Around line 1-8: The file plan.md is missing a top-level heading which
triggers markdownlint MD041; add a H1 heading at the very top of plan.md (e.g.,
"Plan" or a concise descriptive title) so the document no longer begins with a
list, ensuring the lint warning is resolved and the rest of the content (the
bullets describing corsMiddleware, tests, pre-commit, PR) remains unchanged.
In `@run_tests.bash`:
- Line 1: The current Makefile target test and the script ./scripts/test-fast
only run ./scripts/test-fast which calls go test ./..., omitting the full
quality gate; update the Makefile test target or ./scripts/test-fast to run the
full set of checks: run gofmt (and fail on diffs), goimports check, go vet,
golangci-lint, gosec with -severity medium, and run go test with the required
flags (-race -count=1 -shuffle=on -vet=all) so the merge-grade gate cannot be
bypassed; modify the test: rule and/or ./scripts/test-fast script to execute
those commands in sequence (failing the script on any failure) and include clear
command names in logs to help trace failures.
🪄 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: fa9a6105-f53a-4b3d-a2e4-a2ddb072c9f1
📒 Files selected for processing (5)
.jules/sentinel.mdinternal/api/server.gointernal/api/server_test.goplan.mdrun_tests.bash
Comment on lines
+1
to
+4
| ## 2024-05-18 - [Fix CORS Wildcard Vulnerability] | ||
| **Vulnerability:** The CORS middleware allowed cross-origin credentials (`Access-Control-Allow-Credentials: true`) to be sent even when the origin was matched via a wildcard (`*`). | ||
| **Learning:** Returning credentials alongside a reflected origin when a wildcard is conceptually intended bypasses browser restrictions, making the application vulnerable to exposing sensitive data across all origins. | ||
| **Prevention:** Always verify if a wildcard is present in `AllowedOrigins`. If it is, explicitly return `Access-Control-Allow-Origin: *` and omit the `Access-Control-Allow-Credentials` header to comply securely with the CORS specification. |
There was a problem hiding this comment.
Normalize the heading so markdownlint stays clean.
This new entry triggers the reported MD041/MD022 warnings because the file starts with an H2 and has no blank line after it.
Proposed fix
-## 2024-05-18 - [Fix CORS Wildcard Vulnerability]
+# 2024-05-18 - Fix CORS Wildcard Vulnerability
+
**Vulnerability:** The CORS middleware allowed cross-origin credentials (`Access-Control-Allow-Credentials: true`) to be sent even when the origin was matched via a wildcard (`*`).
**Learning:** Returning credentials alongside a reflected origin when a wildcard is conceptually intended bypasses browser restrictions, making the application vulnerable to exposing sensitive data across all origins.
**Prevention:** Always verify if a wildcard is present in `AllowedOrigins`. If it is, explicitly return `Access-Control-Allow-Origin: *` and omit the `Access-Control-Allow-Credentials` header to comply securely with the CORS specification.📝 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.
Suggested change
| ## 2024-05-18 - [Fix CORS Wildcard Vulnerability] | |
| **Vulnerability:** The CORS middleware allowed cross-origin credentials (`Access-Control-Allow-Credentials: true`) to be sent even when the origin was matched via a wildcard (`*`). | |
| **Learning:** Returning credentials alongside a reflected origin when a wildcard is conceptually intended bypasses browser restrictions, making the application vulnerable to exposing sensitive data across all origins. | |
| **Prevention:** Always verify if a wildcard is present in `AllowedOrigins`. If it is, explicitly return `Access-Control-Allow-Origin: *` and omit the `Access-Control-Allow-Credentials` header to comply securely with the CORS specification. | |
| # 2024-05-18 - Fix CORS Wildcard Vulnerability | |
| **Vulnerability:** The CORS middleware allowed cross-origin credentials (`Access-Control-Allow-Credentials: true`) to be sent even when the origin was matched via a wildcard (`*`). | |
| **Learning:** Returning credentials alongside a reflected origin when a wildcard is conceptually intended bypasses browser restrictions, making the application vulnerable to exposing sensitive data across all origins. | |
| **Prevention:** Always verify if a wildcard is present in `AllowedOrigins`. If it is, explicitly return `Access-Control-Allow-Origin: *` and omit the `Access-Control-Allow-Credentials` header to comply securely with the CORS specification. |
🧰 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 around lines 1 - 4, Change the top-level heading "##
2024-05-18 - [Fix CORS Wildcard Vulnerability]" to a proper H1 (replace "##"
with "#") and add a single blank line after that heading so the file starts with
an H1 followed by an empty line, which will silence MD041/MD022; locate the
exact heading text to update.
Comment on lines
+1
to
+8
| 1. **Fix CORS Vulnerability**: In `internal/api/server.go`, update `corsMiddleware` to securely handle the wildcard `*` origin. | ||
| - Detect if `*` is present in `allowedOrigins`. | ||
| - If `*` is present, accept any `Origin` header. | ||
| - Set `Access-Control-Allow-Origin: *` to properly handle the wildcard. | ||
| - Most importantly, do not set `Access-Control-Allow-Credentials: "true"` when `*` is configured, preventing credential sharing across all origins. | ||
| 2. **Add Tests**: Update `internal/api/server_test.go` to test wildcard origin functionality and ensure `Access-Control-Allow-Credentials` is not present when wildcard is used. | ||
| 3. **Pre-commit Checks**: Run pre-commit instructions to ensure testing and formatting are correct. | ||
| 4. **Submit**: Create PR with title "🛡️ Sentinel: [HIGH] Fix overly permissive CORS configuration". |
There was a problem hiding this comment.
Add a top-level heading to satisfy markdownlint.
The new file starts with a list, so it will keep emitting the reported MD041 warning.
Proposed fix
+# Plan
+
1. **Fix CORS Vulnerability**: In `internal/api/server.go`, update `corsMiddleware` to securely handle the wildcard `*` origin.
- Detect if `*` is present in `allowedOrigins`.
- If `*` is present, accept any `Origin` header.
- Set `Access-Control-Allow-Origin: *` to properly handle the wildcard.
- Most importantly, do not set `Access-Control-Allow-Credentials: "true"` when `*` is configured, preventing credential sharing across all origins.
2. **Add Tests**: Update `internal/api/server_test.go` to test wildcard origin functionality and ensure `Access-Control-Allow-Credentials` is not present when wildcard is used.
3. **Pre-commit Checks**: Run pre-commit instructions to ensure testing and formatting are correct.
4. **Submit**: Create PR with title "🛡️ Sentinel: [HIGH] Fix overly permissive CORS configuration".📝 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.
Suggested change
| 1. **Fix CORS Vulnerability**: In `internal/api/server.go`, update `corsMiddleware` to securely handle the wildcard `*` origin. | |
| - Detect if `*` is present in `allowedOrigins`. | |
| - If `*` is present, accept any `Origin` header. | |
| - Set `Access-Control-Allow-Origin: *` to properly handle the wildcard. | |
| - Most importantly, do not set `Access-Control-Allow-Credentials: "true"` when `*` is configured, preventing credential sharing across all origins. | |
| 2. **Add Tests**: Update `internal/api/server_test.go` to test wildcard origin functionality and ensure `Access-Control-Allow-Credentials` is not present when wildcard is used. | |
| 3. **Pre-commit Checks**: Run pre-commit instructions to ensure testing and formatting are correct. | |
| 4. **Submit**: Create PR with title "🛡️ Sentinel: [HIGH] Fix overly permissive CORS configuration". | |
| # Plan | |
| 1. **Fix CORS Vulnerability**: In `internal/api/server.go`, update `corsMiddleware` to securely handle the wildcard `*` origin. | |
| - Detect if `*` is present in `allowedOrigins`. | |
| - If `*` is present, accept any `Origin` header. | |
| - Set `Access-Control-Allow-Origin: *` to properly handle the wildcard. | |
| - Most importantly, do not set `Access-Control-Allow-Credentials: "true"` when `*` is configured, preventing credential sharing across all origins. | |
| 2. **Add Tests**: Update `internal/api/server_test.go` to test wildcard origin functionality and ensure `Access-Control-Allow-Credentials` is not present when wildcard is used. | |
| 3. **Pre-commit Checks**: Run pre-commit instructions to ensure testing and formatting are correct. | |
| 4. **Submit**: Create PR with title "🛡️ Sentinel: [HIGH] Fix overly permissive CORS configuration". |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[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 `@plan.md` around lines 1 - 8, The file plan.md is missing a top-level heading
which triggers markdownlint MD041; add a H1 heading at the very top of plan.md
(e.g., "Plan" or a concise descriptive title) so the document no longer begins
with a list, ensuring the lint warning is resolved and the rest of the content
(the bullets describing corsMiddleware, tests, pre-commit, PR) remains
unchanged.
| @@ -0,0 +1 @@ | |||
| make test | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -HI '^(Makefile|makefile|GNUmakefile)$' | while read -r mf; do
echo "== ${mf} =="
echo "-- test target --"
awk '
BEGIN { printing = 0 }
/^[[:alnum:]_.-]+:/ {
if ($0 ~ /^test:/) {
printing = 1
} else if (printing) {
exit
}
}
printing { print }
' "$mf"
echo "-- quality-gate references --"
rg -n 'gofmt|goimports|go vet|golangci-lint|gosec|go test .*-(race|vet=all)|shuffle=on' "$mf" || true
echo
doneRepository: mattjoyce/ductile
Length of output: 153
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and print the relevant test script(s)
fd -HI 'test-fast' -t f . | head -n 20 | cat
# Show the primary script contents (small/targeted read)
path="$(fd -HI 'test-fast' -t f . | head -n 1)"
echo "== Using: $path =="
nl -ba "$path" | sed -n '1,200p'
# If the script shells out to other scripts/targets, trace them quickly
echo "== Calls from $path (shell/exec targets) =="
rg -n '(make[[:space:]]+|./scripts/|bash[[:space:]]|sh[[:space:]]|-c[[:space:]]|golangci-lint|gosec|go vet|go test|goimports|gofmt)' "$path" || true
# Inspect any referenced ./scripts/* from the call sites
rg -o './scripts/[A-Za-z0-9._-]+' "$path" | sort -u | while read -r sp; do
if [ -f "${sp}" ]; then
echo
echo "== Script: $sp =="
nl -ba "$sp" | sed -n '1,200p'
else
echo "== Missing path referenced: $sp =="
fi
done
# Also check scripts/test-fast for any go test flags that match the merge gate
rg -n 'go test .* -race|shuffle=on|vet=all|count=1' "$path" || true
# Finally, check whether any of the scripts run golangci-lint / gosec with severity medium
rg -n 'golangci-lint|gosec .*severity[[:space:]]+medium|gosec|go vet|goimports|gofmt' "$path" || trueRepository: mattjoyce/ductile
Length of output: 176
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find exact path(s) to test-fast
echo "== test-fast candidates =="
fd -HI 'test-fast' -t f . || true
# Prefer scripts/test-fast if it exists
if [ -f "./scripts/test-fast" ]; then
path="./scripts/test-fast"
else
path="$(fd -HI 'test-fast' -t f . | head -n 1)"
fi
echo
echo "== Using: $path =="
echo
echo "---- contents (first 250 lines) ----"
cat -n "$path" | sed -n '1,250p'
echo
echo "---- references to other checks/scripts ----"
rg -n 'golangci-lint|gosec|goimports|gofmt|go vet|go test|shuffle=on|vet=all|-race|severity[[:space:]]+medium|count=1' "$path" || true
echo
echo "---- shelling out / invoking other scripts ----"
rg -n '(make[[:space:]]+|./scripts/|bash[[:space:]]+|sh[[:space:]]+|-c[[:space:]]|exec[[:space:]]+)'. "$path" 2>/dev/null || true
# Print any referenced ./scripts/* files so we can inspect them next
echo
echo "== referenced ./scripts/* =="
rg -o './scripts/[A-Za-z0-9._-]+' "$path" | sort -u | while read -r sp; do
if [ -f "$sp" ]; then
echo "$sp"
else
echo "$sp (missing)"
fi
doneRepository: mattjoyce/ductile
Length of output: 627
make test doesn’t run the full merge-grade Go gate
Makefiletest:runs./scripts/test-fast, and./scripts/test-fastonly runsgo test ./...- This omits required checks:
gofmt,goimports,go vet,golangci-lint,gosec -severity medium, and the requiredgo test -race -count=1 -shuffle=on -vet=allinvocation
Update make test / test-fast to invoke the full quality gate so the merge-grade bar can’t be bypassed.
🤖 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 `@run_tests.bash` at line 1, The current Makefile target test and the script
./scripts/test-fast only run ./scripts/test-fast which calls go test ./...,
omitting the full quality gate; update the Makefile test target or
./scripts/test-fast to run the full set of checks: run gofmt (and fail on
diffs), goimports check, go vet, golangci-lint, gosec with -severity medium, and
run go test with the required flags (-race -count=1 -shuffle=on -vet=all) so the
merge-grade gate cannot be bypassed; modify the test: rule and/or
./scripts/test-fast script to execute those commands in sequence (failing the
script on any failure) and include clear command names in logs to help trace
failures.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🚨 Severity: HIGH
💡 Vulnerability: The CORS middleware allowed cross-origin credentials (
Access-Control-Allow-Credentials: true) to be sent even when the origin was matched via a wildcard (*).🎯 Impact: Exposes authenticated endpoints and sensitive data to unauthorized external sites because the wildcard configuration combined with credentials bypassed standard restrictions.
🔧 Fix: Modified
corsMiddlewareininternal/api/server.goto detect if*is present inAllowedOrigins. If it is, the server explicitly returnsAccess-Control-Allow-Origin: *and omits theAccess-Control-Allow-Credentialsheader.✅ Verification: Unit tests added in
internal/api/server_test.goand verified by runningmake test.PR created automatically by Jules for task 1051497440380116851 started by @mattjoyce
Summary by CodeRabbit
Bug Fixes
Tests
Documentation