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 34 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: Team Run ID: 📒 Files selected for processing (5)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. Important Approval pendingCodeRabbit has no unresolved comments, but it could not review the latest commit because the review limit was reached. Follow the review guidance in this comment to continue. 📝 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: 🟡 Moderate · up to This PR tightens CardDAV path handling, prompt identifiers, and admin authorization, but the current head is not ready to merge while required CI and security checks remain pending or non-passing. Sequence Diagram(s)sequenceDiagram
participant Client
participant OIDCVerifier
participant AuthAuthorization
participant SourcePolicy
Client->>OIDCVerifier: Submit verified OIDC session
OIDCVerifier->>AuthAuthorization: Provide admin role and context
AuthAuthorization->>SourcePolicy: Evaluate role and organization scope
SourcePolicy-->>Client: Allow or deny access
🚥 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 |
|
@opencode-agent Re-run authenticated review for exact current head |
Exact-head Strix dependency triageCurrent head remains The current required Strix run ( Responsibility boundary: this is a central OpenAI-compatible gateway/Strix integration defect, not a Next safe action: keep #1206 unchanged and blocked; after the central compatibility change is merged/published into the Strix sidecar, obtain a fresh exact-head Strix run on |
|
@opencode-agent review Review exact current head |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 4 new potential issues.
⚠️ 1 issue in files not directly in the diff
⚠️ Literal percent escapes corrupt attachment names
For MIME filenames containing percent triplets, _safe_filename decodes literal text already normalized by the email parser. Changed extensions can select the wrong attachment parser.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aba2a03f3c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@coderabbitai review Please review exact current head |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo='ContextualWisdomLab/naruon'
expected='80a56bee03dd04c3605ae58c39c86d8d9f74f935'
echo '== Exact commit =='
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 '== Scope against current develop merge base =='
base="$(git merge-base HEAD develop)"
printf 'merge_base=%s\n' "$base"
git diff --name-status "$base"...HEAD
git diff --stat "$base"...HEAD
echo '== Full changed diff =='
git diff --unified=55 "$base"...HEAD
echo '== CardDAV structure and relevant implementation =='
ast-grep outline backend/services/carddav_discovery.py --items all --match '_txt_context_path|discover|CardDAV' || true
rg -n -C 30 \
'def _txt_context_path\b|unquote|percent|reserved|decoded|category\(|urljoin|urlparse|path' \
backend/services/carddav_discovery.py || true
echo '== CardDAV focused test inventory and contents =='
fd -a '^test_.*carddav.*\.py$' backend/tests -x sh -c '
echo "---- $1"
ast-grep outline "$1" --items all || true
rg -n -C 16 "txt_context_path|percent|reserved|encoded|decode|traversal|control|U\+0085|%25" "$1" || true
' sh {}
echo '== Other changed security controls and regression assertions =='
for file in backend/api/data.py backend/api/emails.py backend/api/prompts.py backend/api/auth.py backend/api/security.py backend/tests/test_emails_api.py backend/tests/test_prompts_api.py backend/tests/test_data_api_document_org_isolation.py; do
if [ -f "$file" ]; then
echo "---- $file"
rg -n -C 8 \
'organization_id|reject_crlf|in_reply_to|references|PromptResponse|prompt_uid|admin|JWKS|source_policy|assert_not_called|status_code\s*==\s*422' \
"$file" || true
fi
done
echo '== Temporary workflow check =='
fd -a '^pr-1206-document-org-filter\.yml$' .github/workflows 2>/dev/null || true
echo '== Pull request state, checks, and reviews =='
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,commit:.commit.oid}]}' \
|| 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: 171243
I found no new blocking source issue. The CardDAV reserved-percent repair is correct in static review.
The current tree also includes the prior email-header regression coverage. The tests inject both CR and LF into
I cannot approve yet. The exact-head required checks are queued or in progress, including backend, CodeQL, scope checks, and image validation. GitHub reports the merge state as You are interacting with an AI system. |
![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 could not approve from deterministic current-head evidence because GitHub Checks have failed.
Findings
1. HIGH Current-head GitHub Checks - Fix failed required checks before approval
- Problem: Failed same-head checks remain for
80a56bee03dd04c3605ae58c39c86d8d9f74f935. - Root cause: The model-unavailable evidence fallback is allowed only when peer GitHub Checks are complete and clean.
- Fix: Read and fix the failed check logs below, then rerun the current-head checks.
- Regression test: Keep the model-unavailable fallback gated on an empty failed-check rollup.
Failed checks:
- CodeQL PR/CodeQL compatibility analysis (actions): FAILURE (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072589095/job/101601340838)
- CodeQL PR/CodeQL compatibility analysis (javascript-typescript): FAILURE (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072589095/job/101601340850)
- CodeQL PR/CodeQL compatibility analysis (python): FAILURE (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072589095/job/101601340879)
- CodeQL compatibility analysis (actions) check run: failure (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072589095/job/101601340838)
- CodeQL compatibility analysis (javascript-typescript) check run: failure (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072589095/job/101601340850)
- CodeQL compatibility analysis (python) check run: failure (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072589095/job/101601340879)
- Required Noema Review/noema-review: FAILURE (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072588268/job/101601215272)
- Strix Security Scan/strix: CANCELLED (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072588302/job/101602472305)
- Strix Security Scan/strix: cancelled (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072588302/job/101602472305)
- noema-review check run: failure (https://github.com/ContextualWisdomLab/naruon/actions/runs/34072588268/job/101601215272)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Backend: auth.py (4 files)"]
S1 --> I1["API and service runtime"]
I1 --> R1["Review risk: Backend: auth.py (4 files)"]
R1 --> V1["backend tests"]
Evidence --> S2["Test: test_auth_oidc_admin_roles.py (7 files)"]
S2 --> I2["regression suite"]
I2 --> R2["Review risk: Test: test_auth_oidc_admin_roles.py (7 files)"]
R2 --> V2["targeted test run"]
Evidence --> S3["Docs: carddav-txt-path-canonicalization.md (2 files)"]
S3 --> I3["operator or user guidance"]
I3 --> R3["Review risk: Docs: carddav-txt-path-canonicalization.md (2 files)"]
R3 --> V3["docs review"]
2 participants
Summary
PromptResponse.idfrom the prompts API so only opaqueprompt_uidis returned;pathhints with one strict UTF-8 decode, reject malformed/nested encodings, and preserve percent-encoded RFC 3986 reserved characters in the executed URI;_source_policygrants non-owner admin delegation, soNone == Nonecannot turn legacy orgless sources into implicit admin scope;Exact current candidate
develop@042b0c70531b229af3acbd0421a2f23098d848b3;80a56bee03dd04c3605ae58c39c86d8d9f74f935;goal/carddav-path-traversal-decode;Prompt API / naming-contract consolidation
This PR is the canonical owner for opaque prompt response identity. PR #1517 was closed as superseded only after exact comparison proved that its executable requirement had been preserved here:
PromptResponseexposesprompt_uidand omits the sequential row identifier;backend/tests/test_prompt_response_naming_contract.pyforbidsidandprompt_record_idwhile retainingprompt_uid;docs/doctoring/prompt-response-semantic-identifiers.mdretains the Prompt Catalog bounded context, persistence boundary, OWASP API1:2023 rationale, and APA 7 naming research;PromptTemplate.idstays private, with no DB migration/index/lock/UPSERT/partition/read-write change.The naming rule is semantic rather than casing-only: meaningful multiword identifiers remain valid in idiomatic snake_case/camelCase/PascalCase; the defect was exporting a sequential database
idwhen the domain already owned opaqueprompt_uid.CardDAV TXT path security and interoperability contract
The parser now separates validation representation from wire representation:
/is the sole structural exception and is canonicalized to the required context-path delimiter;Review → RED → causal repair
The current review finding on
backend/services/carddav_discovery.pywas valid. The former unconditionalunquotetransformed provider-advertised/users/alice%2Fcalendarinto/users/alice/calendarand/collections/a%3Bbinto/collections/a;b.43467ed10fb3ec6c3f2075a38acf9a934342ca02: adds focused regressions requiring those reserved escapes to retain wire identity;882c1dda08276d11bb346774dcc3119f64bc51b9: keeps strict single-decode validation while deriving an execution path that protects reserved escapes;80a56bee03dd04c3605ae58c39c86d8d9f74f935: records the RFC 3986 §2.2/§2.4 distinction and RFC 6764 context-path boundary indocs/doctoring/carddav-txt-path-canonicalization.md.The existing regressions for nested encoded separators/traversal, malformed triplets, invalid UTF-8, Korean path text, literal percent, and Unicode controls remain. The live finding thread was resolved only after the current source order and returned representation were re-read.
References:
Admin authentication and source delegation
HMAC remains a compatibility credential without admin-membership authority. Admin-role claims are accepted only after the existing OIDC path verifies RS256 signature material, configured issuer, configured audience, key id, expiration, and session metadata; the authorization guard rejects admin roles from any other verifier.
A prior review identified the
None == Nonedelegation edge for legacy orgless sources. Its TDD repair remains in ancestry:2389f0b5d8e54e6968b5b9b9f71b35e29e11e254requires an orglesssystem_admin/platform_adminwho is not the owner to receiveownership_denied;5eab848690d942474997a6e62f6ab62aa50dfc00requires a non-null resource organization matching the authenticated organization before admin delegation;Commit
aba2a03f3ca87914fcf1ca1c751b173097852bcaalso removed the redundant duplicate@pytest.mark.asynciodecorator without changing test semantics.Responsibility boundary
backend/api/data.py, its broad fixture file, and workspace-document organization isolation remain outside this branch. #1345 is the canonical owner for that authorization boundary, DAV route normalization/capability truthfulness, and any retained Naruon-local legacy provider boundary while the released contextual-orchestrator contract is adopted. This PR has no buyer-facing UI changes.Fresh verification state
Current exact-head repository-owned runs:
34072589119: success34072589097: success34072589271: in progress34072589117: queued34072589089: queued34072589095: pendingAn explicit
@coderabbitai reviewrequest was posted after the reserved-path repair. There is not yet a qualifying independent formal review on80a56bee…. All live inline review threads are resolved, but thread resolution and predecessor reviews do not satisfy the current-head review gate.Merge boundary
Do not merge until this unchanged exact head has all then-live required checks/workflows terminal-success, zero valid unresolved findings, and a qualifying independent non-author review accepted by live protection. No predecessor evidence transfer, self-approval, administrative bypass, ruleset weakening, force-push, destructive rebase, dummy/no-op requeue commit, or model/bot verdict fabrication is authorized.