fix(dav): stop advertising unimplemented production methods - #1296
fix(dav): stop advertising unimplemented production methods#1296seonghobae wants to merge 15 commits into
Conversation
|
Important Review available on request
Reviews should be triggered manually for repositories with fewer than 10 stars. Select Trigger review above or comment ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📝 WalkthroughWalkthroughChangesDAV capability scope
nanoid security enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
📝 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 |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
PR governance metadata gate is not ready for
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/services/webdav_service.py (1)
20-35: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftEnforce workspace isolation across DAV persistence paths. The DAV service queries do not consistently constrain records to the active workspace. This can disclose or select resources from another workspace in the same organization.
backend/services/webdav_service.py#L20-L35: require a workspace scope or explicitly constrain intentional global accounts withworkspace_id IS NULL.backend/services/webdav_service.py#L52-L68: acceptworkspace_idand filterProjectFolderrecords by it.backend/services/webdav_service.py#L107-L119: applyworkspace_idto the task and provenance query.backend/api/dav.py#L201-L205: passauth_context.workspace_idto the project-folder lookup.🤖 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 `@backend/services/webdav_service.py` around lines 20 - 35, Enforce workspace isolation across all listed DAV persistence paths: in backend/services/webdav_service.py lines 20-35, require a workspace scope or explicitly filter intentional global accounts with workspace_id IS NULL; in lines 52-68, update the project-folder lookup to accept workspace_id and constrain ProjectFolder records; in lines 107-119, apply workspace_id to the task and provenance query; and in backend/api/dav.py lines 201-205, pass auth_context.workspace_id to the project-folder lookup. Update the relevant service methods and callers while preserving existing organization and user scoping.Source: Coding guidelines
🤖 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/tests/test_release_governance.py`:
- Around line 327-332: Update the source-publishing check around the
governed_workflows loop to parse each workflow and evaluate effective workflow-
and job-level contents permissions, including permissions: write-all and
job-level contents: write. Detect publication through shell commands, GitHub Git
refs API calls, and supported actions that create or update refs, while
continuing to allow package-only publishing; append the relative workflow path
only when these semantic checks identify source publication.
In `@docs/doctoring/nanoid-cve-2026-67214.md`:
- Line 53: Update the documented backend verification command to run with
PYTHONWARNINGS=error and DISABLE_BACKGROUND_WORKERS=1, then rerun the full suite
in the existing frozen offline mode. Replace the reported result only after all
tests pass with no Timeout, Fatal, Warn, or Denied output; do not report
warnings.
In `@frontend/src/lib/dependency-security.test.ts`:
- Around line 28-30: Update the lockfile assertions in the dependency security
test to parse both package entries and snapshot keys for every locked nanoid
version. Assert that each discovered version is outside the documented affected
range, rejecting all affected versions rather than only the literal nanoid@5.1.6
key, while preserving the existing expected 5.1.16 checks.
---
Outside diff comments:
In `@backend/services/webdav_service.py`:
- Around line 20-35: Enforce workspace isolation across all listed DAV
persistence paths: in backend/services/webdav_service.py lines 20-35, require a
workspace scope or explicitly filter intentional global accounts with
workspace_id IS NULL; in lines 52-68, update the project-folder lookup to accept
workspace_id and constrain ProjectFolder records; in lines 107-119, apply
workspace_id to the task and provenance query; and in backend/api/dav.py lines
201-205, pass auth_context.workspace_id to the project-folder lookup. Update the
relevant service methods and callers while preserving existing organization and
user scoping.
🪄 Autofix
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 Plus
Run ID: 0b7b400a-fdce-4478-9f62-f209c837e47e
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
CHANGELOG.mdbackend/api/dav.pybackend/docs/dav-production-capabilities.mdbackend/services/webdav_service.pybackend/tests/test_dav_api.pybackend/tests/test_dav_sync.pybackend/tests/test_release_governance.pydocs/doctoring/nanoid-cve-2026-67214.mdfrontend/package.jsonfrontend/pnpm-workspace.yamlfrontend/src/lib/dependency-security.test.ts
Keep the fail-closed DAV method set and the patched nanoid 5.1.16 pin when merging current develop. Align the incoming 3.x-only Nano ID lock test with that same patched override. Co-authored-by: Seongho Bae <seonghobae@users.noreply.github.com>
|
Merged current Resolution on exact head
Focused verification in progress: PYTHONWARNINGS=error DISABLE_BACKGROUND_WORKERS=1 python -m pytest \
backend/tests/test_dav_api.py \
backend/tests/test_dav_sync.py \
backend/tests/test_frontend_nanoid_security.py \
backend/tests/test_release_governance.py \
backend/tests/test_workflow_source_publication_governance.py -q |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review — 254ba721 (do not merge this head)
The DAV honesty contract is the right product change: OPTIONS/PROPFIND only, DAV: 1, unsupported verbs unregistered so clients get framework 405 instead of a success-shaped or 501 handler. Owner-scope, ownerless-path reject, XML escaping, and repr() log encoding stay in place. The nanoid@5.1.16 pin is consistent across package, workspace, and both lock sections.
Do not merge this head. The develop-merge commit added test_cryptography_runtime_pins_are_bleichenbacher_oracle_fixed, which calls tomllib.loads with no import. App CI will NameError as soon as that test is collected/run.
Landing vehicle: #1380 (45652bbc) adds the missing top-of-module import tomllib on top of this branch.
flowchart LR
dav["DAV OPTIONS/PROPFIND"] --> honest["405 for other verbs"]
merge["254ba721 merge"] --> bug["NameError tomllib"]
bug --> ci["backend pytest fails"]
fix["#1380 import tomllib"] --> ciFix["pin contract can run"]
Prior CodeRabbit CHANGES_REQUESTED on fa0ab43c
| Finding | Still valid on 254ba721? |
|---|---|
Semantic source-publish detector (write-all, job-level contents: write, refs API, package-only allow) |
Addressed by backend/tests/test_workflow_source_publication_governance.py. The leftover substring check in test_release_governance.py is residual only. |
Doctoring command must use PYTHONWARNINGS=error and DISABLE_BACKGROUND_WORKERS=1 |
Addressed in docs/doctoring/nanoid-cve-2026-67214.md. |
Reject every affected locked nanoid version, not only 5.1.6 |
Addressed in frontend/src/lib/dependency-security.test.ts. |
ProjectFolder / task queries must filter workspace_id |
Not a one-line fix on this PR. ProjectFolder and TicketTask have no workspace_id column. WebdavAccount already filters when a workspace is passed. |
Next action
Close or leave #1296 open as historical, and land #1380 after its current-head checks are green. Do not cherry-pick this head without the tomllib import.
Sent by Cursor Automation: Fix Issues
| """Require every governed Python surface to use the first oracle-safe release.""" | ||
| backend_requirements = read_repo_text("backend/requirements.txt") | ||
| backend_project_text = read_repo_text("backend/pyproject.toml") | ||
| backend_project = tomllib.loads(backend_project_text) |
There was a problem hiding this comment.
P1 — this new pin test calls tomllib.loads but the module never imports tomllib. App CI raises NameError: name 'tomllib' is not defined before any cryptography assertion runs.
Add a top-of-module stdlib import (do not import inside the test body):
import tomllibLanding fix is already on 45652bbc in #1380. Verify with:
PYTHONWARNINGS=error DISABLE_BACKGROUND_WORKERS=1 python -m pytest \\
backend/tests/test_release_governance.py::test_cryptography_runtime_pins_are_bleichenbacher_oracle_fixed -q
|
Closing as technically superseded by #1380, not because the DAV work is unwanted. Fresh exact-head comparison proves |
|
Hosted Exact head |
|
Hosted Exact head |
3 participants
Summary
OPTIONSandPROPFIND);501 Not ImplementedDAV/demo handlers so unsupported verbs receive framework-level405 Method Not Allowed;nanoidremediation through the authoritative pnpm workspace and frozen lock;Product contract
An unsupported capability is no longer advertised as if it were part of the product. Provider-backed mutation remains available only through signed writeback-intent APIs until DAV mutation semantics, source capability checks, and ETag/If-Match enforcement are implemented. The durable DAV boundary is recorded in
backend/docs/dav-production-capabilities.md.RCA and remedy
The prior branch attempted to materialize the changelog and lock through two push-triggered Actions jobs with
contents: writeandgit push. Those temporary publishers were the first security boundary failure and also left the lock unchanged because the real override authority isfrontend/pnpm-workspace.yaml, not onlypackage.json.Exact head
00bb2d6bd80faee9e0ff5fec6c558003207f7bf3removes both workflows, recordsnanoid: 5.1.6in the workspace and lock, and addstest_github_workflows_never_push_repository_source_branches. The two GHAS token-permission findings are therefore addressed at source rather than weakened.Verification
Validated locally against the published source tree:
git diff --check: passed;postcss 8.5.24 -> nanoid 5.1.6;Current exact-head GitHub CI/security/review evidence remains required before merge.
Summary by CodeRabbit
nanoid5.1.16 and added safeguards to keep all installations on a secure version.OPTIONSandPROPFINDcapabilities.405 Method Not Allowed.