security: reconstruct exact HTTP method strings on integrated policy stack - #161
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
OpenCode Review Overview
Pull request overviewOpenCode reviewed the current-head mergeability evidence and changed-file flow before approval, then found merge conflicts on the affected path. Findings1. HIGH Merge Conflict Guidance - Resolve the PR branch against the latest base branch
gh pr checkout 161 --repo ContextualWisdomLab/EgressWeave
git fetch origin security/reconstruct-policy-integer-values-dd0324
git merge --no-ff origin/security/reconstruct-policy-integer-values-dd0324 # or: git rebase origin/security/reconstruct-policy-integer-values-dd0324
git status --short
# resolve files, then git add <resolved-files>
# merge path: git commit
# rebase path: git rebase --continue
git push origin HEAD:security/reconstruct-method-policy-strings-e80f83
# rebase path only: git push --force-with-lease origin HEAD:security/reconstruct-method-policy-strings-e80f83
Merge Conflict Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Docs: policy-configuration-integrity.md"]
S2 --> I2["operator or user guidance"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["docs review"]
Evidence --> S3["Test: test_policy_method_value_integrity.py"]
S3 --> I3["regression suite"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["targeted test run"]
Changed-File Evidence Mapflowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Docs: policy-configuration-integrity.md"]
S2 --> I2["operator or user guidance"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["docs review"]
Evidence --> S3["Test: test_policy_method_value_integrity.py"]
S3 --> I3["regression suite"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["targeted test run"]
Merge Conflict Guidance
gh pr checkout 161 --repo ContextualWisdomLab/EgressWeave
git fetch origin security/reconstruct-policy-integer-values-dd0324
git merge --no-ff origin/security/reconstruct-policy-integer-values-dd0324 # or: git rebase origin/security/reconstruct-policy-integer-values-dd0324
git status --short
# resolve files, then git add <resolved-files>
# merge path: git commit
# rebase path: git rebase --continue
git push origin HEAD:security/reconstruct-method-policy-strings-e80f83
# rebase path only: git push --force-with-lease origin HEAD:security/reconstruct-method-policy-strings-e80f83 |
![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 reviewed the current-head bounded evidence and found no blocking issues.
Findings
No blocking findings.
Summary
Approval sufficiency: affirmative evidence supports PR #161 intent (seal HTTP method policy values to exact built-in str) - changed-file evidence inspected: src/egressweave/_policy_normalization.py (full hunk), src/egressweave/policy.py (hunk plus trusted receipts at lines 72 and 445), docs/research/policy-configuration-integrity.md, CHANGELOG.md, tests/test_policy_method_value_integrity.py; no confirmed defect after the adversarial phase. Verification posture: Coverage execution evidence reports Result: PASS - supported repository test suites passed (python3 -m coverage run -m pytest tests && python3 -m coverage report --show-missing --fail-under=100); no failed GitHub Checks at head 721f6ce; head-checkout direct reads were sandbox-denied so hunks and CodeGraph verbatim source were used (source limitation recorded in residual_risk). Linter/static: ruff gate configured (python3 -m ruff check .); no failed-check or lint logs in bounded evidence; history commit 7cb22cc 'keep method-string RED lint-clean' corroborates. TDD/regression: new tests/test_policy_method_value_integrity.py (94 lines) exercises subclass rejection through the public EgressPolicy constructors; verbatim current-head test at tests/test_complete_coverage.py:87-96 (test_direct_policy_strings_cover_projection_normalization) covers the comma-separated string form and asserts frozenset({'GET','HEAD'}). Coverage: Coverage execution evidence reports Result: PASS with the 100% coverage gate including the new test file. Docstring coverage: Coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory. DAG: head-flow Mermaid flowchart EgressPolicy constructor -> _require_exact_method_string (outer exact-str gate) -> split(',') -> _normalize_allowed_method (per-entry exact-str gate) -> strip/upper/RFC 9110 token scan/CONNECT denial -> allowed_methods frozenset; reflects current-head flow. PoC/execution: no standalone PoC receipt; the trusted suite-execution result is the Coverage PASS receipt. DDD/domain: policy construction remains the trusted domain boundary storing canonical immutable policy values; mirrors the predecessor integer-sealing pattern (PR #159). CDD/context: docs/research/policy-configuration-integrity.md and the CHANGELOG Security section were updated in the same change set with mutually consistent claims. Similar issues: history commits 654c7c2 ('security: require exact HTTP method policy strings') and c0128bf ('security: seal runtime HTTP method normalization') corroborate the reconstruction. Claim/concept check: RFC 9110 token grammar is backed by the added APA reference; type(value) is str exactness matches the Python data-model contract cited in the docs. Standards search: RFC 9110 (HTTP semantics) cited in docs; external search unavailable in the sandbox (source limitation). Compatibility/convention: the only new identifier is private helper _require_exact_method_string (multi-word snake_case); no public API, DB, config, route, or serialized objects added or renamed; TypeError/ValueError semantics preserved. Breaking-change/backcompat: pre-1.0 tightening is documented for operators; exact int/str and ASCII decimal-string forms remain supported. Performance: unchanged O(n) per-token scan plus one identity check per value. Developer experience: actionable field-specific TypeError/ValueError and an explicit Operator migration section. User experience: non-web; reviewed surface is the policy-construction API with deterministic errors. Visual/DOM: non-web change; no web UI surface, so no Playwright/visual evidence applies. Accessibility/i18n: non-web; unchanged plain-English error strings, no UI surface. Supply-chain/license: no dependency or manifest changes in this PR. Packaging: pyproject.toml untouched; python >=3.10 contract and tests/ layout unchanged. Security/privacy: hardening rejects str subclasses before strip()/upper()/split() dispatch in method normalization; CONNECT denial and RFC 9110 token validation retained; no identifier-exposure or enumeration surface introduced.
Approval sufficiency: bounded evidence supplied affirmative approval evidence for changed files, coverage/docstring posture, risk surfaces, and current-head verification; approval is not based merely on the absence of known blockers.
Verification posture: CodeGraph evidence was initialized and bounded current-head evidence reviewed for changed-file evidence including CHANGELOG.md, docs/research/policy-configuration-integrity.md, src/egressweave/_policy_normalization.py, src/egressweave/policy.py, tests/test_policy_method_value_integrity.py.
Linter/static: workflow/static review evidence is bounded by the current-head GitHub Checks gate and changed-file evidence.
TDD/regression: coverage execution evidence and focused changed hunks were reviewed from bounded-review-evidence.md.
Coverage: coverage execution evidence reports supported repository test suites passed.
Docstring coverage: coverage execution evidence reports configured repository docstring gates passed or docstring coverage was advisory.
DAG: CodeGraph/source-backed behavior map connects CHANGELOG.md to the affected review, runtime, or workflow path and required checks.
PoC/execution: coverage-evidence job executed on the current head and reported PASS.
DDD/domain: workflow and repository-governance invariants were reviewed against changed files in bounded evidence.
CDD/context: CodeGraph evidence, changed-file history, and focused hunks were reviewed from bounded-review-evidence.md.
Similar issues: changed-file history evidence was reviewed for comparable local precedents.
Claim/concept check: bounded evidence, repository source, current-head workflow evidence, and, where numeric, scientific, statistical, or literature-backed claims are affected, original-paper/formula evidence and parameter-recovery expectations were used for claims.
Standards search: standards and external-source claims require trusted bounded source evidence prepared outside the isolated model process; no evidence-backed standards blocker is present in bounded evidence.
Compatibility/convention: changed workflow/script conventions, object naming, and reserved-word safety for schema/API/config/code surfaces were checked in bounded evidence.
Breaking-change/backcompat: deployment evidence and changed-file history were checked for backward-compatibility risk.
Performance: changed surfaces were checked for performance risk in bounded evidence.
Developer experience: changed automation, review, test, setup, and maintenance surfaces were checked for helpful or obstructive DX impact in bounded evidence.
User experience: connected user, operator, API, CLI, documentation, review-comment, status-check, rendering, and workflow-reader behavior was checked for contradictions against code, docs, and tests in bounded evidence.
Visual/DOM: deterministic repair does not infer browser runtime execution; source-backed DOM/UI evidence and trusted workflow receipts were reviewed when present, and non-web surfaces used API/CLI/log/docs/workflow evidence instead.
Accessibility/i18n: accessibility, localization, and human-readable text surfaces were checked where UI, CLI, API message, docs, logs, or review text changed.
Supply-chain/license: dependency, package, model, container, and external-tool changes were checked in bounded evidence.
Packaging: package, build, test, lint, and security contracts were checked in bounded evidence.
Security/privacy: workflow-token, review-gate, and repository-automation security/privacy boundaries were checked in bounded evidence.
Adversarial validation
{"status":"passed","probes":[{"path":"src/egressweave/_policy_normalization.py","line":167,"hypothesis":"A str subclass instance supplied as an allowed HTTP method entry can still reach strip()/upper() and control normalization output via subclass dispatch.","attack_or_counterexample":"Pass an instance of a str subclass overriding upper() (e.g., returning 'CONNECT' or a forged token) as an allowed method value to the public EgressPolicy constructor.","evidence":"Trusted diff trace at src/egressweave/_policy_normalization.py:167: _normalize_allowed_method now evaluates `type(value) is not str` and raises TypeError before any method dispatch on the value, so the subclass instance is rejected before strip()/upper() can run; Coverage execution evidence reports Result: PASS for the supported suites (python3 -m pytest tests with the 100% coverage gate), which executed the subclass-rejection tests at the public API boundary. source-line-sha256=d2afd097cfd54a237dc6e9fe7d73c87701791d267f4fa459a587fcdcf195f59e","outcome":"falsified"},{"path":"docs/research/policy-configuration-integrity.md","line":17,"hypothesis":"The exact-string sealing regresses the documented exact comma-separated allowed_methods public form or weakens RFC 9110 token validation and CONNECT denial.","attack_or_counterexample":"Construct EgressPolicy(allowed_hosts='api.example.com', allowed_ports='443', allowed_methods='get, head') and require canonicalization to authorities {('api.example.com', 443)} and methods {'GET','HEAD'} with CONNECT still rejected.","evidence":"Trusted source trace at docs/research/policy-configuration-integrity.md:17 records the unchanged contract: the exact comma-separated allowed_methods string remains supported and is split only after the outer value is proven to be an exact built-in string, with RFC 9110 token validation, uppercase canonicalization, and CONNECT denial retained; verbatim CodeGraph current-head source at tests/test_complete_coverage.py:87-96 constructs that exact string form through the public constructor and asserts frozenset({'GET','HEAD'}), and Coverage execution evidence reports Result: PASS for the supported suites. source-line-sha256=b28c7992d4dcce20c1cfc294ad0bc6e339820301434721c65014a28a4c89efab","outcome":"falsified"}],"residual_risk":"The focused-hunk inlining was byte-truncated before the tail of the src/egressweave/policy.py diff (the exact call-site ordering of the outer exact-string gate before split()) and before the body of tests/test_policy_method_value_integrity.py; direct reads of the head checkout were sandbox-denied. The split-after-proof ordering is therefore supported indirectly (documented contract, policy.py:72/445 receipts, verbatim passing API-boundary test, Coverage PASS at 100%, corroborating history commits) rather than by direct hunk inspection. If the outer-string branch in policy.py were ever reordered to split before checking exactness, a str subclass could regain split() dispatch; a regression test asserting subclass rejection on the comma-separated form would pin that ordering."}- Result: APPROVE
- Reason: Exact built-in str sealing for HTTP method policy values is implemented at the per-entry and outer-string paths; both adversarial probes were falsified; Coverage execution evidence PASS at the 100% gate, no failed checks, no unresolved threads.
- Head SHA:
721f6ce10035a0e7caded2eb8417cdc6894c7e70 - Workflow run: 31458849982
- Workflow attempt: 1
There was a problem hiding this comment.
Pull request overview
OpenCode reviewed the current-head mergeability evidence and changed-file flow before approval, then found merge conflicts on the affected path.
Findings
1. HIGH Merge Conflict Guidance - Resolve the PR branch against the latest base branch
- Problem: GitHub reports mergeStateStatus
DIRTYfor this pull request. - Root cause: Branch
security/reconstruct-method-policy-strings-e80f83cannot be merged cleanly intosecurity/reconstruct-policy-integer-values-dd0324; the changed-file flow below shows which review/runtime path is blocked by the conflict. - Fix: Merge or rebase the latest
security/reconstruct-policy-integer-values-dd0324intosecurity/reconstruct-method-policy-strings-e80f83, resolve conflict markers in the PR branch, rerun the focused checks, and push the same branch. - Repair commands:
gh pr checkout 161 --repo ContextualWisdomLab/EgressWeave
git fetch origin security/reconstruct-policy-integer-values-dd0324
git merge --no-ff origin/security/reconstruct-policy-integer-values-dd0324 # or: git rebase origin/security/reconstruct-policy-integer-values-dd0324
git status --short
# resolve files, then git add <resolved-files>
# merge path: git commit
# rebase path: git rebase --continue
git push origin HEAD:security/reconstruct-method-policy-strings-e80f83
# rebase path only: git push --force-with-lease origin HEAD:security/reconstruct-method-policy-strings-e80f83- Regression test: Keep OpenCode approval gated on mergeability so model-output failures cannot approve a conflicted PR.
Merge Conflict Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Docs: policy-configuration-integrity.md"]
S2 --> I2["operator or user guidance"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["docs review"]
Evidence --> S3["Test: test_policy_method_value_integrity.py"]
S3 --> I3["regression suite"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["targeted test run"]
- Result: REQUEST_CHANGES
- Reason: mergeStateStatus is
DIRTY; mergeable isCONFLICTING. - Head SHA:
e3dae87ccacb54cedbfcd5b83a6911f39a6cf870 - Workflow run: 31483418051
- Workflow attempt: 1
Changed-File Evidence Map
flowchart LR
PR["PR changed files"] --> Evidence["OpenCode bounded evidence"]
Evidence --> S1["Changed file (3 files)"]
S1 --> I1["repository behavior"]
I1 --> Conflict["Merge conflict blocks this path"]
Conflict --> V1["required checks"]
Evidence --> S2["Docs: policy-configuration-integrity.md"]
S2 --> I2["operator or user guidance"]
I2 --> Conflict["Merge conflict blocks this path"]
Conflict --> V2["docs review"]
Evidence --> S3["Test: test_policy_method_value_integrity.py"]
S3 --> I3["regression suite"]
I3 --> Conflict["Merge conflict blocks this path"]
Conflict --> V3["targeted test run"]
1 participant
Superseded current-stack reconstruction
Closed as historical evidence only. This lane is superseded by Draft PR #184, which reconstructs the same unique exact-HTTP-method-string boundary on the current scalar-integer predecessor #178 instead of the obsolete/conflicted predecessor stack used here.
Historical exact identity retained for audit:
e3dae87ccacb54cedbfcd5b83a6911f39a6cf870;2953538db13d89d821869101b5b1beebbbf22443;CHANGES_REQUESTEDbecause the old stack is conflicting.Current replacement proof at closure:
main:52a5c6a0862e0d035b0698b79a0e8556269e0b00;65485ee196699ecb5a8451621d188afdd8731bf6;365a38820474b4e8a5325b02dbdf3aa1a6f24a37;31519709781: completed successfully, including exact checkout, Python 3.10–3.14 test lanes, coverage gate, Ruff/product guard/compileall, wheel/sdist verification and installed-wheel smoke.A fresh semantic diff audit confirms #184 preserves this PR's unique product contract with a narrower current-stack implementation: exact built-in
strmethod entries before normalization; exact outer comma-separated method strings beforesplit(); unchanged ordinary strings, uppercase/RFC 9110 token semantics and unconditionalCONNECTdenial; runtime hostile-string-subclass rejection; directly affected operator guidance and[Unreleased]parity. The replacement avoids this PR's stale stack/conflict and unrelated historical changelog drift.No checks, reviews, approvals, base identities or mergeability from this PR transfer to #184. #184 remains Draft behind #178 and still requires its own exact-head security/review acceptance, including the actual centrally pinned Dependency Review action rather than wrapper success with that action skipped.
Progress history for #124 only; do not reopen this branch as a competing writer lane.