Fix OIDC JWKS URL allowlisting - #312
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds HTTPS+allowlisted OIDC host validation and startup enforcement, requires per-path DAV ownership checks for WebDAV/CalDAV handlers, and tightens Strix CI/workflow and gate scripts to default an approved Vertex model and fail closed on provider-signaled infrastructure issues; tests and docs updated accordingly. ChangesSecurity Hardening: OIDC, DAV, and Strix CI
🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
PR governance metadata gate is not ready for
|
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@backend/api/dav.py`:
- Around line 21-28: The _ensure_dav_owner_scope function currently allows
ownerless DAV paths; change its logic to "fail closed" by rejecting when
_dav_path_owner_user_id(path) returns None (raise HTTPException 403) and only
allow when owner_user_id == auth_context.user_id; update the function so the
only early return is when the owner matches the auth_context, and otherwise
raise 403. Ensure you locate and update the _ensure_dav_owner_scope function and
its use of _dav_path_owner_user_id and AuthContext so ownerless paths are
rejected before any OPTIONS/PROPFIND/capability responses are generated (and
keep path values separate from any log/response display per guidelines).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ba25d6d0-c0d1-4959-830c-363b2621a1e7
📒 Files selected for processing (9)
AGENTS.mdbackend/api/dav.pybackend/core/config.pybackend/core/url_validation.pybackend/scripts/start_backend.pybackend/tests/test_auth_real.pybackend/tests/test_config.pybackend/tests/test_dav_api.pybackend/tests/test_start_backend.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/ci/strix_quick_gate.sh (1)
2319-2321: 💤 Low valueMinor: redundant case variants with
-iflag.The
-iflag makes matching case-insensitive, soFatal|FATAL,Denied|DENIED,Warn|WARN|Warning|WARNINGcan be simplified toFatal|Denied|Warn|Warning. Not a bug, just slightly verbose.Simplification (optional)
- if grep -Eiq '(^|[^[:alpha:]])(Fatal|FATAL|Denied|DENIED|Warn|WARN|Warning|WARNING)([^[:alpha:]]|$)' "$STRIX_LOG"; then + if grep -Eiq '(^|[^[:alpha:]])(Fatal|Denied|Warn|Warning)([^[:alpha:]]|$)' "$STRIX_LOG"; then🤖 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 `@scripts/ci/strix_quick_gate.sh` around lines 2319 - 2321, The grep call using -i is case-insensitive, so simplify the alternation pattern in the grep invocation (the line that checks "$STRIX_LOG") by removing duplicate uppercase/lowercase variants; update the pattern from 'Fatal|FATAL|Denied|DENIED|Warn|WARN|Warning|WARNING' to just 'Fatal|Denied|Warn|Warning' while keeping the surrounding anchors '(^|[^[:alpha:]])' and '([^[:alpha:]]|$)' and the -Eiq flags unchanged.
🤖 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 `@scripts/ci/test_strix_quick_gate.sh`:
- Line 269: The variable fail_on_provider_signal is currently defaulting to 0
which makes tests permissive; change its default to fail-closed by setting local
fail_on_provider_signal="${29-1}" (instead of "${29-0}") so callers that omit
arg 29 will enforce provider signals; keep the existing export of
fail_on_provider_signal intact so downstream code still sees the value.
---
Nitpick comments:
In `@scripts/ci/strix_quick_gate.sh`:
- Around line 2319-2321: The grep call using -i is case-insensitive, so simplify
the alternation pattern in the grep invocation (the line that checks
"$STRIX_LOG") by removing duplicate uppercase/lowercase variants; update the
pattern from 'Fatal|FATAL|Denied|DENIED|Warn|WARN|Warning|WARNING' to just
'Fatal|Denied|Warn|Warning' while keeping the surrounding anchors
'(^|[^[:alpha:]])' and '([^[:alpha:]]|$)' and the -Eiq flags unchanged.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 50eb3090-1540-4e3a-8451-ea54754ee541
📒 Files selected for processing (9)
.github/workflows/strix.ymlAGENTS.mdREADME.mdbackend/api/dav.pybackend/tests/test_dav_api.pybackend/tests/test_release_governance.pydocs/plans/2026-05-29-strix-full-scan-operational-model.mdscripts/ci/strix_quick_gate.shscripts/ci/test_strix_quick_gate.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/api/dav.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/ci/test_strix_quick_gate.sh (1)
5040-5128: ⚡ Quick winCover the disabled provider-signal branch too.
These new cases only prove
STRIX_FAIL_ON_PROVIDER_SIGNAL=1. None of therun_gate_case_allow_provider_signalcallers hit the success-path detector (Fatal/Warning/Deniedon exit0), so the flag could be ignored or hardcoded and this file would still pass. Please add onerun_gate_case_allow_provider_signalcase for one of these scenarios so the new toggle is verified in both directions.🤖 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 `@scripts/ci/test_strix_quick_gate.sh` around lines 5040 - 5128, Add a complementary test that exercises the "allow provider signal" path by adding a run_gate_case_allow_provider_signal invocation for one of the existing scenarios (e.g., duplicate the parameters used in the "provider-fatal-success-signal" run_gate_case but call run_gate_case_allow_provider_signal instead), so the opposite branch of STRIX_FAIL_ON_PROVIDER_SIGNAL is exercised; ensure the new call uses the same identifying scenario name (like "provider-fatal-success-signal") and identical arguments except for using run_gate_case_allow_provider_signal to verify the disabled provider-signal behavior.
🤖 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.
Nitpick comments:
In `@scripts/ci/test_strix_quick_gate.sh`:
- Around line 5040-5128: Add a complementary test that exercises the "allow
provider signal" path by adding a run_gate_case_allow_provider_signal invocation
for one of the existing scenarios (e.g., duplicate the parameters used in the
"provider-fatal-success-signal" run_gate_case but call
run_gate_case_allow_provider_signal instead), so the opposite branch of
STRIX_FAIL_ON_PROVIDER_SIGNAL is exercised; ensure the new call uses the same
identifying scenario name (like "provider-fatal-success-signal") and identical
arguments except for using run_gate_case_allow_provider_signal to verify the
disabled provider-signal behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76267a47-a6d2-4270-8537-7f798b7c3647
📒 Files selected for processing (2)
scripts/ci/strix_quick_gate.shscripts/ci/test_strix_quick_gate.sh
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/strix.yml (1)
256-257:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRestore the Vertex fallback path until the new provider contract is proven.
Clearing
STRIX_VERTEX_FALLBACK_MODELShere removes the same-provider fallback and turns preview-model blips into hard workflow failures. I don't see PR evidence that the default preview model now has the exact clean contract required to run without that fallback in governed CI, so this conflicts with the repo workflow policy.Suggested patch
- STRIX_VERTEX_FALLBACK_MODELS: "" + STRIX_VERTEX_FALLBACK_MODELS: "vertex_ai/gemini-2.5-flash"As per coding guidelines: "Do not disable silent Vertex fallback models in the workflow unless a future PR proves a new exact fallback contract with no Timeout/Fatal/Warn/Denied output"
🤖 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 @.github/workflows/strix.yml around lines 256 - 257, The workflow currently clears STRIX_VERTEX_FALLBACK_MODELS which disables the same-provider Vertex fallback and can turn preview-model blips into hard failures; restore the fallback by removing the empty assignment or resetting STRIX_VERTEX_FALLBACK_MODELS back to its prior/default fallback value (i.e., preserve the preview-model fallback token/list) so the same-provider fallback remains enabled until a future PR proves the new provider contract; keep STRIX_FAIL_ON_PROVIDER_SIGNAL unchanged.
🤖 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.
Outside diff comments:
In @.github/workflows/strix.yml:
- Around line 256-257: The workflow currently clears
STRIX_VERTEX_FALLBACK_MODELS which disables the same-provider Vertex fallback
and can turn preview-model blips into hard failures; restore the fallback by
removing the empty assignment or resetting STRIX_VERTEX_FALLBACK_MODELS back to
its prior/default fallback value (i.e., preserve the preview-model fallback
token/list) so the same-provider fallback remains enabled until a future PR
proves the new provider contract; keep STRIX_FAIL_ON_PROVIDER_SIGNAL unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93f2deee-cc1f-4be6-9f1c-44121a93ad3d
📒 Files selected for processing (2)
.github/workflows/strix.ymlscripts/ci/test_strix_quick_gate.sh
Contributor
Author
|
seonghobae
temporarily deployed
to
fix/oidc-jwks-url-allowlist-20260529 - naruon PR #312
May 30, 2026 16:08 — with 
Render
Destroyed
Render
Destroyed
This was referenced Jun 1, 2026
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
Verification
Summary by CodeRabbit
Security Enhancements
New Features
Tests