fix(security): seal the untrusted-input delimiter against customer text - #52
seonghobae wants to merge 2 commits into
Conversation
NimClient marks caller data as untrusted by wrapping it in <input>...</input>, but json.dumps escapes quotes and backslashes and not angle brackets. Customer free text containing </input> was emitted verbatim, so the message carried two closing tags and the boundary stopped being unambiguous. user_context reaches this block with max_length=4000 and subject_name is re-sent inside report.model_dump() on the editorial-repair round trip, so both are caller-controlled. _sealed_payload escapes < and > as their JSON \uXXXX forms. The document stays valid and every decoded value is identical, while no literal bracket survives in the transmitted prompt. Regression: tests/test_prompt_delimiter.py asserts exactly one open and one close tag under an injection payload, and two companion tests assert the sealed body still decodes to the original values, including ordinary text with 3 < 5 and 7 > 2. RED before the change (1 failed, 2 passed), GREEN after (3 passed). Refs #51. 254 passed, 100% statement and branch coverage. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthrough사용자 페이로드를 JSON으로 직렬화한 뒤 Changes프롬프트 입력 구분자 봉인
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant NimClient
participant sealed_payload
participant MockTransport
NimClient->>sealed_payload: user_payload JSON 직렬화
sealed_payload-->>NimClient: <와 >가 이스케이프된 페이로드 반환
NimClient->>MockTransport: 봉인된 user 메시지 전송
MockTransport-->>NimClient: canned 응답 반환
Merge Risk: ⚪ Minimal · up to The current supported prompt path receives the delimiter-sealing fix, with no concrete merge-blocking issue identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
|
Current stack/owner-boundary review: this security delta is valid, but it cannot be treated as an independent terminal fix while Draft #39 ( Please preserve the valid security semantics rather than merging two incompatible end states or closing one casually:
No source/ref mutation from fleet here; this is the owner-path restack acceptance needed to keep the security fix and the CO-only architecture consistent. |
The previous commit on this branch carried a `ruff format` reflow of `_post` and `_content` alongside the seal. Neither method has anything to do with the untrusted-input delimiter, the repository's CI gate runs `ruff check` and not `ruff format`, and thirty-six files on `main` already drift from the formatter, so the reflow was unrequested. It was also not harmless. Rewriting `_content` put this branch in conflict with `claude/separate-truncated-generation-from-success`, which edits that exact method for #49, and the two could not be merged in either order. `src/four_pillars/nim.py` now differs from `main` by fourteen added lines and one changed line: the `_sealed_payload` helper and its call site. The two branches merge cleanly. Also corrects the helper's docstring, which rendered the JSON escape as ``\\uXXXX`` instead of ``\uXXXX``. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Merge-order note: #39 moves the file this change lives in
I merged both locally to find out what that costs. No pushes. The call site merges cleanly: git follows the rename and the fixed line lands in the new file. The single conflict is about names, not logic, because that branch renames Resolution, whichever order the two land in: keep that branch's exception names and label attribute, keep this change's addition, and drop the The point worth stating plainly is that this must not be resolved by taking one side wholesale. Details and the same note are on #39. |
![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
391f981420a35c6e9e409cbfe18b551531acaf46. - 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/four-pillars/actions/runs/34891101919/job/104250690006)
- CodeQL PR/CodeQL compatibility analysis (python): FAILURE (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34891101919/job/104250689972)
- CodeQL compatibility analysis (actions) check run: failure (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34891101919/job/104250690006)
- CodeQL compatibility analysis (python) check run: failure (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34891101919/job/104250689972)
- Required Noema Review/noema-review: FAILURE (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34891102236/job/104207530269)
- Strix Security Scan/strix: CANCELLED (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34891102113/job/104205681120)
- Strix Security Scan/strix: cancelled (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34891102113/job/104205681120)
- noema-review check run: failure (https://github.com/ContextualWisdomLab/four-pillars/actions/runs/34891102236/job/104207530269)
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Repository file: CHANGELOG.md"]
S1 --> I1["repository behavior"]
I1 --> R1["Review risk: Repository file: CHANGELOG.md"]
R1 --> V1["required checks"]
Evidence --> S2["Python package: nim.py"]
S2 --> I2["Python runtime API"]
I2 --> R2["Review risk: Python package: nim.py"]
R2 --> V2["pytest plus coverage"]
Evidence --> S3["Test: test_prompt_delimiter.py"]
S3 --> I3["regression suite"]
I3 --> R3["Review risk: Test: test_prompt_delimiter.py"]
R3 --> V3["targeted test run"]
OpenCode Review Overview
|
seonghobae
added
bug
ChatGPT Codex Connector
|
Exact-head admission audit: 현재 blocker: 활성 CHANGES_REQUESTED 1건; terminal workflow: CodeQL PR:failure. 유효 commit·diff·review evidence를 보존한 채 Draft/Proposed로 교정합니다. Base 이동이나 queue 대기만을 이유로 Close하지 않으며, Force Push·synthetic status/approval·manual rerun·bypass는 사용하지 않습니다. Blocker 수리 후 새 exact head에서 Checks와 review admission을 다시 받아야 합니다. |
1 participant
Defect
NimClientmarks caller data as untrusted by wrapping it in a delimiter, butjson.dumpsescapes quotes and backslashes and not angle brackets. Customer free text containing</input>was emitted verbatim, so the transmitted message carried two closing tags and the boundary stopped being unambiguous.Reproduced on
main(@8c6a2fa):Both caller-controlled strings reach this block:
user_context(max_length=4000,service.py:62→analysis.py:97) andsubject_name, re-sent insidereport.model_dump()on the editorial-repair round trip atanalysis.py:184.Change
_sealed_payloadescapes<and>as their JSON\uXXXXforms. This is an encoding change only: the document stays valid JSON and every decoded value is identical, while no literal angle bracket survives in the transmitted prompt, so no customer byte can produce the closing tag.Chosen over a nonce delimiter or a separate structured message because it needs no contract change and no decision from the owner of the model-client boundary.
RED to GREEN
tests/test_prompt_delimiter.pycaptures what the client actually transmits throughhttpx.MockTransportand asserts the boundary directly.The two that already passed are the guard against a fix that corrupts data: they decode the sealed body and compare it to the original payload, including ordinary text with
3 < 5and7 > 2. A seal that mangled content would fail them.Severity, stated plainly
This is not cross-tenant. A job's payload holds only that caller's own data, so a successful injection steers the attacker's own report.
quality.pyalso still rejects medical-claim, future-certainty, and false-authority patterns, missing sections, and any 간지 absent from the deterministic calculation. What this closes is the product's own stated boundary being breakable by the customer, plus the induced-schema-failure path that burns provider quota.Verification
254 passed, 1 deselected; 100% production statement and branch coverage;
-W error::ResourceWarningclean. Ruff,compileall,scripts/check_docs.py,scripts/product_gap_audit.py: PASS.Refs #51. Only
src/four_pillars/nim.py(one helper plus its call site), the new test, and CHANGELOG.tests/test_nim.pyandtests/test_nim_errors.pybelong to #39 and are untouched.🤖 Generated with Claude Code
Summary by CodeRabbit
보안 개선
<,>등 특수문자가 포함된 입력도 원래 의미를 유지하면서 안전하게 전달됩니다.테스트
문서