[codex] Harden client edge security headers#147
Merged
MTG-Thomas merged 6 commits intomainfrom Apr 26, 2026
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 6 minutes and 33 seconds. ⌛ 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: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a reusable nginx security-headers snippet, includes it in multiple location blocks and the Docker image, adds unit tests validating header inclusion/absence and duplication across locations, and adds documentation describing the passive header-hardening approach. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/tests/unit/test_nginx_security_headers.py`:
- Around line 46-53: The test test_embed_location_preserves_iframe_framing is
missing an assertion that the Permissions-Policy header is present for the
/embed location; update the test to call _location_block(config, "/embed")
(already used) and add an assertion that the expected Permissions-Policy header
string (matching nginx.conf line 178) appears in block (e.g., ensure 'add_header
Permissions-Policy' with the appropriate directives is in block) so the test
verifies Permissions-Policy alongside nosniff and HSTS.
- Around line 15-26: The helper _location_block currently uses
config.index(marker) which raises ValueError when the marker is missing, causing
inconsistent exceptions; update _location_block (function name) to detect a
missing marker explicitly (e.g., use config.find(marker) and check for -1 or
wrap config.index(marker) in a try/except) and raise the intended
AssertionError(f"Location block not closed: {location}") when the marker isn't
found so callers always see the same AssertionError type.
In `@client/nginx.conf`:
- Around line 42-46: Extract the repeated security header directives (the
add_header lines for X-Content-Type-Options, X-Frame-Options,
Strict-Transport-Security, Permissions-Policy, and
Content-Security-Policy-Report-Only) into a single snippet file and replace the
duplicated blocks with an include of that snippet in each location block that
currently contains those add_header directives; ensure the snippet contains the
exact header lines and that each location retains the same "always" flag so
behavior is unchanged (search for the add_header occurrences in
client/nginx.conf to locate all places to update).
- Around line 19-26: The Content-Security-Policy-Report-Only header currently
allows WebSockets to any origin via the tokens ws: and wss:; update the CSP
string in the add_header line (the one setting
Content-Security-Policy-Report-Only) to restrict connect-src to same-origin
(e.g., include 'self' and remove the ws: and wss: tokens) so WebSocket upgrades
are limited to the app origin (verify behavior against
client/src/services/websocket.ts during smoke tests); ensure the revised header
still includes other required sources like 'self' and any allowed external
endpoints.
🪄 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: 45906b7b-f013-49e3-9ef7-98d6c0a21dd9
📒 Files selected for processing (3)
api/tests/unit/test_nginx_security_headers.pyclient/nginx.confdocs/security/zap-header-hardening.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/tests/unit/test_nginx_security_headers.py`:
- Around line 71-74: Replace the direct Path parents-based resolution in
test_dockerfile_copies_security_header_snippet() with the repository helper
_repo_file() so the test uses the same skip/lookup logic used elsewhere; call
_repo_file("client/Dockerfile") (or equivalent helper usage) to read the
Dockerfile and then assert the COPY line as before, ensuring FileNotFoundErrors
are handled consistently by the helper.
In `@client/Dockerfile`:
- Line 45: The security-headers.conf snippet is being placed into the global
include directory via the COPY instruction (COPY security-headers.conf
/etc/nginx/conf.d/security-headers.conf), causing it to be loaded implicitly and
also explicitly where you include it; instead, change the COPY target to place
the snippet into a non-globbed snippets directory (e.g., COPY
security-headers.conf /etc/nginx/snippets/security-headers.conf) and update all
explicit include directives that reference security-headers.conf to point to the
new snippets location so the headers are only applied when explicitly included.
🪄 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: 0425a165-8446-48ce-b177-76d867ac55a3
📒 Files selected for processing (5)
api/tests/unit/test_nginx_security_headers.pyclient/Dockerfileclient/nginx.confclient/security-headers.confdocs/security/zap-header-hardening.md
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@api/tests/unit/test_nginx_security_headers.py`:
- Around line 20-26: The test's NORMAL_SECURITY_HEADERS constant currently
asserts only a CSP prefix which lets regressions in other directives slip
through; update the NORMAL_SECURITY_HEADERS array (and the duplicate at the
other occurrence) to assert the full Content-Security-Policy (or
Content-Security-Policy-Report-Only) header value string including all
directives (default-src, script-src, style-src, connect-src, frame-ancestors,
object-src, base-uri, etc.) so the unit test verifies the complete policy rather
than just the prefix—locate and replace the entries for 'add_header
Content-Security-Policy-Report-Only "default-src' and its counterpart with the
full expected CSP string.
- Around line 45-58: The test
test_cache_bearing_locations_repeat_normal_security_headers only checks location
blocks via _location_block and misses verifying the server-level include; update
the test to also assert the presence of "include
/etc/nginx/snippets/security-headers.conf;" at the server scope (the
server-level include in client/nginx.conf referenced on Line 22). Use or add a
helper to extract the server block (e.g., _server_block or a top-level block
extractor) or search NGINX_CONF for the include within the server context and
assert it exists so routes relying on the server-level header include (/ws,
/mcp, /register, /authorize, /token, /auth/callback) are covered.
🪄 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: f6977527-7a64-44c2-b87d-abc8c7712d72
📒 Files selected for processing (3)
api/tests/unit/test_nginx_security_headers.pyclient/Dockerfileclient/nginx.conf
|
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.
Summary
/embediframe-compatible by excluding X-Frame-Options DENY and the normal report-only frame-ancestors policyValidation
python -m pytest tests/unit/test_nginx_security_headers.py -qdocker run --rm --add-host api:127.0.0.1 --entrypoint nginx -v "${PWD}\client\nginx.conf:/etc/nginx/conf.d/default.conf:ro" nginx:alpine -tgit diff --checkNotes
This is intentionally conservative for upstream packaging: CSP is report-only, COEP/COOP/CORP are deferred, and
/embedremains frameable.Summary by CodeRabbit
New Features
Tests
Documentation