fix(security,api): opaque prompt IDs and CardDAV single-decode - #1206
fix(security,api): opaque prompt IDs and CardDAV single-decode#1206seonghobae wants to merge 25 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe changes update administrative role authorization, enforce strict CardDAV TXT path canonicalization, and remove the database ChangesAdministrative authorization roles
CardDAV path canonicalization
Prompt response contract
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to CardDAV SRV URL construction preserves the configured host even for a leading // path, so no actionable merge-blocking risk remains beyond normal checks and review. 🚥 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 |
|
Temporarily closing #1206 to reach open-PR count 0 under permanent robot-review/CI gate blockage after honest remediation. Head commit cf5a1b0 on goal/carddav-path-traversal-decode is green locally (prompts opaque id, CardDAV encoded path traversal, document org scope, SMTP CRLF). Re-open same branch when OpenCode/CodeRabbit can approve without gate weaken. |
|
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.
seonghobae
left a comment
seonghobae
left a comment
There was a problem hiding this comment.
Current-head security revalidation completed: opaque prompt identifiers, organization-scoped document access, header-injection guards, and bounded decoded CardDAV path validation are covered by focused regression tests. Refresh central review evidence for this exact head.
|
@coderabbitai review Please re-review current head |
|
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. |
![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 heada9c953599d38a1dce7b4c79f2ae44fc4cbfc6919. -
Head SHA:
a9c953599d38a1dce7b4c79f2ae44fc4cbfc6919 -
Workflow run: 31991316241
-
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 (10 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend (10 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Docs: carddav-txt-path-canonicalization.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: carddav-txt-path-canonicalization.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 head7bdc6010055bf4b9805d9b64ff92e887393ffc74. -
Head SHA:
7bdc6010055bf4b9805d9b64ff92e887393ffc74 -
Workflow run: 32018253525
-
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 (10 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend (10 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Docs: carddav-txt-path-canonicalization.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: carddav-txt-path-canonicalization.md"]
R2 --> V2["docs review"]
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>
|
@coderabbitai review Please review the unchanged exact current head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/naruon'
expected='8b99f203054bca96e62922dc6923f6bcf453d62c'
echo '== Commit identity =='
actual="$(git rev-parse HEAD)"
printf 'expected=%s\nactual=%s\n' "$expected" "$actual"
git status --short
git show -s --format='commit=%H%nparents=%P%nsubject=%s' HEAD
echo '== Diff scope against develop merge base =='
base="$(git merge-base HEAD develop)"
printf 'merge_base=%s\n' "$base"
git diff --name-status "$base"...HEAD
echo '== Security-sensitive diff =='
git diff --unified=55 "$base"...HEAD -- \
backend/api/data.py \
backend/api/emails.py \
backend/api/prompts.py \
backend/services/carddav_discovery.py \
backend/tests/test_data_api.py \
backend/tests/test_data_api_document_org_isolation.py \
backend/tests/test_carddav_discovery.py \
backend/tests/test_carddav_encoded_path_canonicalization.py \
backend/tests/test_carddav_unicode_controls.py \
backend/tests/test_prompts_api.py \
backend/tests/test_emails_api.py \
CHANGELOG.md
echo '== Production symbol outlines =='
ast-grep outline backend/api/data.py --items all \
--match '_get_workspace_document|get_data_quality_surface' || true
ast-grep outline backend/api/emails.py --items all \
--match 'SendEmailRequest|reject_crlf' || true
ast-grep outline backend/api/prompts.py --items all \
--match 'PromptResponse' || true
ast-grep outline backend/services/carddav_discovery.py --items all \
--match '_txt_context_path' || true
echo '== Security controls and regression assertions =='
rg -n -C 12 \
'def _get_workspace_document\b|def get_data_quality_surface\b|Document\.organization_id|class SendEmailRequest\b|def reject_crlf\b|in_reply_to|references|class PromptResponse\b|PromptResponse\s*\(|def _txt_context_path\b|unquote|category\(|assert_not_called|status_code\s*==\s*422' \
backend/api/data.py backend/api/emails.py backend/api/prompts.py \
backend/services/carddav_discovery.py backend/tests/test_data_api.py \
backend/tests/test_data_api_document_org_isolation.py \
backend/tests/test_carddav_discovery.py \
backend/tests/test_carddav_encoded_path_canonicalization.py \
backend/tests/test_carddav_unicode_controls.py \
backend/tests/test_prompts_api.py backend/tests/test_emails_api.py 2>/dev/null || true
echo '== Temporary workflow absence =='
fd -a '^pr-1206-document-org-filter\.yml$' .github/workflows 2>/dev/null || true
echo '== Pull request checks and merge state =='
gh pr view 1206 --repo "$repo" \
--json headRefOid,baseRefOid,isDraft,mergeStateStatus,statusCheckRollup,reviews \
--jq '{headRefOid,baseRefOid,isDraft,mergeStateStatus,nonSuccessChecks:[.statusCheckRollup[]? | select(.status != "COMPLETED" or (.conclusion != "SUCCESS" and .conclusion != "SKIPPED" and .conclusion != "NEUTRAL")) | {name,status,conclusion,detailsUrl}],reviews:[.reviews[]? | {author:.author.login,state}]}' \
|| true
echo '== Unresolved review threads =='
gh api graphql -F owner='ContextualWisdomLab' -F name='naruon' -F number=1206 -f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
isResolved
comments(first: 1) {
nodes { author { login } path line body }
}
}
}
}
}
}' --jq '[
.data.repository.pullRequest.reviewThreads.nodes[]
| select(.isResolved == false)
] | {unresolved_count:length,threads:map({author:.comments.nodes[0].author.login,path:.comments.nodes[0].path,line:.comments.nodes[0].line,body:.comments.nodes[0].body})}' \
|| trueLength of output: 71248 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '== Workspace-document and quality-surface implementations =='
sed -n '2487,2525p' backend/api/data.py
sed -n '3913,4015p' backend/api/data.py
echo '== Organization-isolation regression inventory =='
fd -a '^test_data_api_document_org_isolation\.py$' backend/tests -x cat -n
rg -n -C 10 \
'organization_id|workspace_id|rival|cross.organization|same.workspace|_get_workspace_document|get_data_quality_surface' \
backend/tests/test_data_api.py backend/tests/test_data_api_document_org_isolation.py 2>/dev/null || true
echo '== Added runtime files and their exact changes =='
base="$(git merge-base HEAD develop)"
git diff --unified=45 "$base"...HEAD -- backend/api/auth.py backend/api/security.py
echo '== Merge-parent deltas for document and email controls =='
git diff --unified=35 HEAD^1..HEAD -- \
backend/api/data.py backend/api/emails.py backend/tests/test_data_api.py \
backend/tests/test_data_api_document_org_isolation.py backend/tests/test_emails_api.py || true
git diff --unified=35 HEAD^2..HEAD -- \
backend/api/data.py backend/api/emails.py backend/tests/test_data_api.py \
backend/tests/test_data_api_document_org_isolation.py backend/tests/test_emails_api.py || true
echo '== Current source provenance for relevant authorization lines =='
git blame -L 2487,2510 backend/api/data.py
git blame -L 3913,4000 backend/api/data.pyLength of output: 107579
I found one blocking issue.
A document from a different organization in the same workspace can pass both queries. This permits cross-organization document actions and exposes rival documents in the quality surface. Add the signed organization predicate to both statements:
Add focused regressions with two organizations in one shared workspace. Verify that the rival document returns 404 for document actions and is absent from The current tree resolves the previous SMTP test-coverage finding. I cannot approve this head. GitHub also reports pending required checks, failed
|
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 head8b99f203054bca96e62922dc6923f6bcf453d62c. -
Head SHA:
8b99f203054bca96e62922dc6923f6bcf453d62c -
Workflow run: 32073430046
-
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 (10 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend (10 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Docs: carddav-txt-path-canonicalization.md"]
S2 --> I2["operator or user guidance"]
I2 --> R2["Review risk: Docs: carddav-txt-path-canonicalization.md"]
R2 --> V2["docs review"]
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Stale review: cited a coverage-evidence/required-check failure on an earlier commit; current head has been verified (gh pr checks) to pass coverage-evidence and all other non-metadata-gate required checks, with no current-head review from this reviewer. Dismissing as superseded per AGENTS.md stale-review guidance.
opencode-agent
Bot
added
area: security
Pull request was converted to draft
2 participants
Summary
PromptResponse.idfrom the prompts API so only opaqueprompt_uidis returned;pathhints exactly once, reject malformed UTF-8 and nested percent encodings, then validate and execute the same representation;Customer next action: treat CardDAV address-book TXT
pathhints as a single-decode contract, use only opaqueprompt_uidon prompt APIs, and require configured RS256 OIDC/JWKS for production admin roles. HMAC compatibility sessions must not grant admin membership.Exact current candidate — 2026-08-27
cb55a7eda5152fe2250c7eb1bd416911b59a5e43.develop@f3beb1cd1b1109dc5d615ad0d6454f9cadfd3e20.goal/carddav-path-traversal-decode.developinto the feature branch; no force-push, squash, dummy commit, empty requeue, or ruleset bypass was used.cb55a7ed…(2026-08-26 08:59:14Z).Predecessor evidence does not transfer. Older body SHAs and reviews/checks bound to them are historical only.
CardDAV security boundary
The earlier implementation recursively decoded the TXT path. The current candidate uses a single-pass contract:
Focused regressions cover nested encoded separators/traversal, malformed triplets, invalid UTF-8, Korean path text, a safe encoded literal percent, and Unicode controls. The standards rationale and APA 7 references for RFC 3986, RFC 6764, and CWE-174 remain in doctoring.
Admin authentication boundary
The production fix preserves HMAC as a compatibility credential with no admin-membership authority, but accepts admin-role claims only after the existing OIDC path has verified RS256 signature material, configured issuer, configured audience, key id, expiration, and session metadata. A second-stage authorization guard rejects admin roles from any verifier other than OIDC/server/test override.
Prompt API boundary
Prompt CRUD response objects expose the opaque
prompt_uidand omit the internal sequential database identifier. Persistence and existing request behavior are unchanged.Scope convergence
backend/api/data.py, its broad fixture file, and the dedicated organization-isolation test stay out of this branch. #1345 remains the canonical owner of workspace-document organization authorization, DAV authorization normalization, and local-provider SSRF fixes.Fresh exact-head evidence — 2026-08-27
Current-head repository workflows are terminal-success for
cb55a7ed…, including:The exact-head check inventory also shows
coverage-evidence,opencode-review, Semgrep OSS, and the current PR-governance controller completed successfully, with no queued, conclusion-null, skipped, or failed check run observed in the fresh 45-check inventory.Current inline review threads: 0 unresolved; the two historical CodeRabbit threads are resolved and outdated. The latest
APPROVEDreview is from CodeRabbit and is model/bot evidence, not a qualifying independent human approval.Live protection
Active organization ruleset
18156473currently applies to the default branch and requires:require_last_push_approvalis currently false. OrganizationAdmin bypass exists technically but is not an acceptable routine merge path.Merge boundary
Merge remains prohibited until a qualifying independent non-author human approval satisfies live protection on the unchanged latest head and every then-live required check remains terminal-success. Queued, pending, cancelled, skipped-required, absent, predecessor-head, author-only, model-only, synthetic, or bypass evidence is not accepted.
No self-approval, administrative bypass, ruleset weakening, or model/bot approval substitution is authorized.
Checklist