fix(dav): reject ambiguous nested authorization encodings - #1345
fix(dav): reject ambiguous nested authorization encodings#1345seonghobae wants to merge 31 commits into
Conversation
|
Warning Review limit reachedNext included review available in 58 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThe PR adds bounded DAV authorization-path normalization, restricts allowlisted local provider hostnames to approved private networks, and adds organization scoping to document queries. Tests and boundary documentation cover the updated behavior. ChangesDAV authorization path validation
Local provider network boundaries
Document organization authorization
Estimated code review effort: 3 (Moderate) | ~30 minutes Merge Risk: 🟡 Moderate · up to This PR tightens DAV authorization decoding, local-provider network restrictions, and organization-scoped document access. It is not merge-ready until qualifying fresh independent review and every live required check succeed on the unchanged current head; the remaining branch-coverage request is a non-blocking follow-up. Sequence Diagram(s)sequenceDiagram
participant DAVClient
participant DAVHandler
participant PathNormalizer
participant DAVAuthorization
participant PROPFINDHandler
DAVClient->>DAVHandler: send DAV request with authorization path
DAVHandler->>PathNormalizer: validate and normalize path
PathNormalizer-->>DAVHandler: normalized path or HTTP error
DAVHandler->>DAVAuthorization: authorize normalized path
DAVAuthorization-->>DAVHandler: authorization result
DAVHandler->>PROPFINDHandler: process normalized path
PROPFINDHandler-->>DAVClient: WebDAV response
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
|
PR governance metadata gate is not ready for
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Current-head review disposition for the remaining DAV test comment: not a valid code/test defect on @coderabbitai review |
|
I have withdrawn that finding. I will run a fresh automated review of the current head. 🐇 ✏️ Learnings added
|
|
@coderabbitai review |
|
|
@coderabbitai review |
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Superseded by later exact-head fixes. The DAV normalized-path finding is resolved and its review thread records it as addressed. This dismissal clears stale predecessor-head review state only; a fresh qualifying current-head independent approval remains required.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
Bring Ready PR #1206 onto protected develop@dd8d15191338b841f9e6f3a06507c6a5643b95d0 with a normal merge commit. Preserve opaque prompt_uid, CardDAV single-pass decode, and OIDC-only admin roles. Do not pull #1345 workspace-document isolation. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
Bring protected develop@dd8d15191338b841f9e6f3a06507c6a5643b95d0 (#1367 calendar conflicts and later develop history) into fix/dav-single-decode-authorization without changing DAV single-decode authorization, capability advertisement, local-provider SSRF, or workspace-document organization isolation semantics. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
@coderabbitai review Please review the unchanged exact current head |
|
|
![opencode-agent[bot]](https://avatars.githubusercontent.com/in/1549082?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current headff6e47554a76992f8166d28aca5ffc12bd5e2088. -
Head SHA:
ff6e47554a76992f8166d28aca5ffc12bd5e2088 -
Workflow run: 32116669104
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Backend (7 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend (7 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Docs: dav-and-local-provider-network-boundaries.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: dav-and-local-provider-network-boundaries.md"]
R2 --> V2["docs review"]
OpenCode Review Overview
Pull request overviewOpenCode cannot approve yet because required coverage evidence did not pass. Review outcome1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
Coverage evidenceCoverage evidence job did not run or did not publish coverage evidence. Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Backend (7 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend (7 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Docs: dav-and-local-provider-network-boundaries.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: dav-and-local-provider-network-boundaries.md"]
R2 --> V2["docs review"]
|
There was a problem hiding this comment.
Pull request overview
OpenCode cannot approve yet because required coverage evidence did not pass.
Review outcome
1. HIGH .github/workflows/opencode-review.yml:1 - Coverage evidence did not prove required test/docstring evidence
-
Problem: The required coverage-evidence job result was
failure, so OpenCode cannot establish approval sufficiency for this head. -
Root cause: Automated approval is only valid when the same-head coverage-evidence job proves supported repository test suites passed and configured docstring gates passed or were advisory, or reports not applicable because no supported source files or package manifests exist. Missing, failed, skipped, unavailable, or unsupported-tooling test evidence is a blocker.
-
Fix: Install or configure the repository test/docstring evidence tooling when source files or package manifests exist, rerun the current-head coverage-evidence job, and approve only after it reports
successwith required evidence or explicit no-source not-applicable evidence. -
Regression test: Keep the approval branch checking
needs.coverage-evidence.result == successbefore posting APPROVE, and publish REQUEST_CHANGES when coverage-evidence blocker states such as cancelled, skipped, failed, unsupported-tooling, or below-100 evidence are present. -
Result: REQUEST_CHANGES
-
Reason: coverage-evidence result was
failure, so required test/docstring evidence was not proven for current head89d885084fec5510c53c8cab992bf1f41c6abd55. -
Head SHA:
89d885084fec5510c53c8cab992bf1f41c6abd55 -
Workflow run: 32420608708
-
Workflow attempt: 1
Coverage evidence
Coverage evidence job did not run or did not publish coverage evidence.
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Backend (7 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend (7 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Docs: dav-and-local-provider-network-boundaries.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: dav-and-local-provider-network-boundaries.md"]
R2 --> V2["docs review"]
Verified: coverage-evidence and other non-metadata-gate required checks on the current head now report pass (gh pr checks). The coverage-evidence failure this review cited has since cleared on rerun. Dismissing as superseded.
opencode-agent
Bot
added
area: auth
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| def _has_ambiguous_percent_encoding(path: str) -> bool: | ||
| """Detect residual encodings that another decode would make structural.""" | ||
|
|
||
| for index, character in enumerate(path): | ||
| if character != "%": | ||
| continue | ||
| octet = _residual_percent_octet(path, index) | ||
| if octet in _DAV_STRUCTURAL_OCTETS: | ||
| return True | ||
| return False |
There was a problem hiding this comment.
📝 Info: Double-encoded harmless data now kept literal
_residual_percent_octet follows %25 chains and preserves double-encoded non-structural values (e.g. %2541) as literal text instead of decoding them to A, unlike the previous recursive unquote loop. Downstream folder_uid/segment matching sees the literal percent text. This is the stated single-decode design, noted for callers that relied on full decoding.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed as intended — this is the stated single-decode contract of this PR. _residual_percent_octet (backend/api/dav.py:23-35) exists only to find octets that another decode would turn structural (_DAV_STRUCTURAL_OCTETS checked at backend/api/dav.py:44-46); harmless double-encoded text such as %2541 deliberately stays literal so downstream folder_uid/segment matching compares the exact client-sent bytes rather than a second-decoded value an attacker could smuggle. The contract is documented in docs/architecture/dav-and-local-provider-network-boundaries.md and covered by tests/test_dav_normalized_route.py (77 passed: uv run pytest tests/test_dav_api.py tests/test_dav_normalized_route.py tests/test_llm_provider_urls.py -q).
| def _is_local_provider_network_address( | ||
| address: ipaddress.IPv4Address | ipaddress.IPv6Address, | ||
| ) -> bool: | ||
| return any( | ||
| address.version == network.version and address in network | ||
| for network in _LOCAL_PROVIDER_NETWORKS | ||
| ) |
There was a problem hiding this comment.
📝 Info: Allowlisted-host private range narrowed
_is_local_provider_network_address admits only 10/8, 172.16/12, 192.168/16, and fc00::/7 for allowlisted local providers, where the prior code accepted any is_private result. Container networks outside these ranges that previously worked will now be rejected.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Confirmed as intended hardening. _is_local_provider_network_address (backend/services/llm_provider_urls.py:83-90) now admits only the explicit local ranges in _LOCAL_PROVIDER_NETWORKS (10/8, 172.16/12, 192.168/16, fc00::/7) for already-allowlisted local provider hosts. The previous acceptance of any is_private address also admitted non-global surprises (e.g. 100.64/10 CGNAT, 198.18/15 benchmark, ::1/unique-local variants outside fc00::/7); narrowing to enumerated CIDRs makes the SSRF boundary deterministic and is covered by the expanded tests/test_llm_provider_urls.py cases in this PR.
|
Strix finding disposition (check-run 97109339574, model MEDIUM "Authentication Bypass via Session Verifier Override in AuthContext" — REBUTTED (false positive on unchanged trusted context). The report itself is conditional ("If there exists a code path where..."); no such path exists:
|
2 participants
Summary
This current-base security/interoperability lane contains five bounded fixes on one develop-base owner:
OPTIONSnow advertises only WebDAV class 1 plus the actually implementedOPTIONS/PROPFINDmethods. Unsupported writeback/richer DAV verbs are not registered and therefore fail with framework405 Method Not Allowedrather than being falsely advertised and routed to501handlers.ALLOW_LOCAL_LLM_PROVIDERSis enabled; exact allowlisted single-label provider names may resolve only to RFC 1918 IPv4 or RFC 4193 IPv6 unique-local space. Link-local, metadata, multicast, unspecified, reserved, broadcast, and other non-global classes remain denied.127.0.0.1/::1is accepted only when the original validated hostname is itself an explicit local-development identity.organization_id IS NULLdocuments.This PR does not include #1206 (CardDAV TXT path single-decode / opaque prompt IDs). Those remain on #1206.
Reconciliation onto current protected develop
bde6998fe53c652a4fa1b8f05485aa0f679454d6ff6e47554a76992f8166d28aca5ffc12bd5e2088dd8d15191338b841f9e6f3a06507c6a5643b95d0(includes feat(calendar): prevent status-weighted double booking #1367)origin/developintofix/dav-single-decode-authorization(no squash, no force-push, no dummy commit)PYTHONWARNINGS=error DISABLE_BACKGROUND_WORKERS=1): 82 passed / 0 failed in 0.40s, with noTimeout/Fatal/Warn/Deniedoutput.tests/test_dav_api.py33 passed +tests/test_dav_normalized_route.py1 passed = 34tests/test_llm_provider_urls.py43 passedtests/test_data_document_authorization.py5 passedTest-first lineage
DAV authorization
The original DAV RED head
3dc8420d2829b8b2aa772ad969fde3967a3f4d0bchanged onlybackend/tests/test_dav_api.py. The predecessor implementation repeatedly calledurllib.parse.unquote()and violated the single-decode authorization contract. Subsequent commits implemented the bounded residual-encoding validator, propagated the normalized path through owner checks/logging/route handling, corrected route-level expectations, and added the backslash route regression.DAV capability truthfulness
Stale PR #1296 contained a useful capability-truthfulness slice mixed with already-shipped and unrelated work. It was reconstructed on this current develop-base owner rather than merging the stale branch wholesale. RED commit
76b5d16dc681c95422e98dec2cf211c2f1afb5a0changes the route tests to require only implemented protocol methods and405for unregistered verbs. Production commita094523deb2f83f8020ccfc8badefc48789de613narrows the router andOPTIONSresponse while preserving the single-decode/tenant boundary. Temporary Draft #1372 was closed after its head and this head resolved to identical tree587d9abdc050c4dede21bafd19f9530cba327957, so one owner remains.Local-provider address classes
Strix on predecessor head
f7abb8282333b452fb9b9f67b8f12d9b953cb977identified the hostname-scoped local-provider SSRF boundary. RED headb66af0d4aa1a9695dd24116d5301cf4759b1484aadded regressions proving that allowlisted local providers must reject metadata/link-local, multicast, unspecified, and reserved/broadcast classes. Production restricted the hostname-scoped exception to RFC 1918 IPv4 and RFC 4193 IPv6 unique-local space.DNS-rebinding loopback closure
A later exact-evidence Strix failure exposed a second-order defect:
_validate_global_address()treated loopback as allowed wheneverALLOW_LOCAL_LLM_PROVIDERSwas true, regardless of the original hostname. RED head95f23c25ced6e850aba0de9b4ad1c6f4ff6c24eeadded realistic DNS-resolution regressions for an allowlisted remote hostname and an allowlisted container hostname rebinding to127.0.0.1; the focused backend run failed exactly those two tests. Production headd73e9e4e1d117ee005ace91dfe289afe31ae7132bound loopback admission to explicit local-development hostname identity while preserving private container-network access. Follow-up test-contract head7052b66b14f54a3c3115a11c429963708501e8a7makes the helper-level identity requirement explicit and preserves the localhost success path.Workspace-document tenant isolation
Strix on predecessor head
df2bf10b2f724fa83bb10f3742c5c73796e19161reported a CWE-639 cross-organization IDOR:_get_workspace_documentfiltereddocument_idandworkspace_idbut notorganization_id. The same root cause existed in the document slice of the data-quality repository surface. RED headc05855f90250f2f7271af86888a4579b39375c53added focused authorization regressions; production adds the organization predicate to both query sites.Current exact candidate
develop@dd8d15191338b841f9e6f3a06507c6a5643b95d0.ff6e47554a76992f8166d28aca5ffc12bd5e2088.mergeStateStatusremainsBLOCKEDuntil independent non-author current-head APPROVE and exact-head product CI succeed.bde6998f/a094523ddo not transfer toff6e47554a76992f8166d28aca5ffc12bd5e2088.Standards and claim boundary
The DAV authorization design follows RFC 3986 §2.4 single-decoding semantics and treats CWE-174-style double decoding as an authorization hazard. Capability discovery now limits protocol claims to behavior actually implemented by this endpoint; provider-backed DAV mutation remains explicitly out of scope until a separately reviewed writeback implementation provides the required source/capability/concurrency controls. The network boundary distinguishes explicit local-development identity, intentionally private container networking, and special-purpose/non-unicast address classes rather than treating all non-global addresses as interchangeable. Workspace organization ownership remains the tenant boundary already carried by
AuthContextandDocument.organization_id.Merge boundary
Do not merge until this unchanged exact head has every live required workflow/check terminal-success, zero actionable review threads, a qualifying independent non-author current-head APPROVE, and normal protected-branch/ruleset acceptance without bypass. Pending, queued, skipped-required, neutral-required, failed, stale, predecessor-head, rate-limited, status-only, author-only, or model-only evidence is non-passing. Exact-head product CI must pass independently on
ff6e47554a76992f8166d28aca5ffc12bd5e2088.This update does not APPROVE, merge, squash, force-push develop, dummy-commit, empty-requeue, or force-cancel.
Customer next action
Keep this PR Ready. Wait for exact-head product CI and required security/governance checks to finish on
ff6e47554a76992f8166d28aca5ffc12bd5e2088. An independent non-author reviewer must APPROVE that exact head. Do not treat predecessor reviews or the previous behind-base mergeability as transferable. Do not merge until those current-head gates pass.Refs #1344.
Type of change
Checklist: