Skip to content

security(request): snapshot untrusted extension mappings before timeout access - #210

Draft
seonghobae wants to merge 4 commits into
mainfrom
security/request-extension-mapping-fail-closed-main-7faf
Draft

security(request): snapshot untrusted extension mappings before timeout access#210
seonghobae wants to merge 4 commits into
mainfrom
security/request-extension-mapping-fail-closed-main-7faf

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Purpose

Repair a request-boundary fail-closed gap proven against exact protected main 7faf7a3b8a47980113982914000e724ab6a6cda5.

Before this PR, _bind_bounded_request_timeouts(...) called extensions.get("timeout") directly on the caller-provided Mapping before detaching it, then independently materialized the same mapping later. A custom mapping could therefore execute its own get() implementation and leak an arbitrary exception across the generic denial boundary; a stateful mapping could also present two different snapshots during one decision.

RCA and test-first proof

  • Immutable test-only RED head: 10569788099d0eb869befa65003b2d47f7d8c9c9.
  • Hosted RED CI: run 31643103797, Python 3.12 job 94270211704. Exact checkout, dependency installation and Ruff passed; coverage run -m pytest -q failed because the hostile outer mapping's overridden get() raised RuntimeError("secret extensions get failure") instead of remaining inside the generic request boundary.
  • Narrow production repair head: c758f11764456d9584175d5f2e806ffd6ab4c1dc.
  • Compatibility-spec correction head: 38b6e4c561d4e5fbf2a339254e5363d9f681d22c. A valid custom Mapping is accepted from its detached built-in snapshot; production code must not dynamically dispatch its hostile get() method. Rejecting all mapping subclasses was rejected as a wrong-layer compatibility regression.
  • The first repaired CI exposed an exact owned-production coverage gap on the new copy-failure branch while pytest itself was green.
  • Current coverage-remediation head: b7021ef272170664abd81c22b38a2a5e39bfe497, adding a hostile outer-copy regression that requires the stable generic denial with no cause/context.

Root-cause remedy

Detach the outer request-extension mapping exactly once through the existing exception-masking copy seam, then perform timeout lookup and replacement only against that built-in detached dictionary. This removes attacker-controlled Mapping.get() dispatch and makes one authorization decision observe one extension snapshot. The change does not widen allowed extension keys, timeout semantics, SNI/authority, DNS/TLS/proxy behavior, request/response limits, credentials, redirects, Unix sockets, or release authority.

Exact-head validation

On unchanged exact current head b7021ef272170664abd81c22b38a2a5e39bfe497 over live protected base 7faf7a3b8a47980113982914000e724ab6a6cda5:

  • repository CI run 31643563698: success; Python 3.10–3.14 tests and coverage reports pass, including Python 3.14; package wheel/sdist build and installed-wheel smoke verification pass;
  • SAST Semgrep run 31643563689: success;
  • Security Scan run 31643563652: aggregate/wrapper success; OSV, Trivy and Scorecard pass, but the actual immutable-pinned Dependency review action is skipped, so supply-chain evidence remains non-passing;
  • GHAS CodeQL and Semgrep OSS report no new alerts in changed code;
  • central coverage-source-tree and coverage-evidence checks pass;
  • OpenCode and Noema required-workflow transport is green, but no formal independent PR review exists and model/check/status evidence is not promoted to approval;
  • required Strix exact-head check 94271801977 completed wrapper success but emitted the Strix backend unavailable warning and explicitly produced no vulnerability report. The workflow treated that outcome as a neutral skip; under EgressWeave fail-closed evidence semantics this is non-passing semantic security evidence;
  • formal PR reviews: none;
  • unresolved review threads: none;
  • CodeRabbit did not produce a substantive current-head review because provider review capacity was rate-limited; rate-limit/status text is not review evidence.

Remaining acceptance / ownership

The product fix itself has exact-head CI, package, Python 3.14, exact coverage, SAST and GHAS evidence. The remaining blockers are evidence/control-plane boundaries rather than a defect in these two changed files:

  1. Dependency Review — require the actual immutable-pinned action to execute and succeed. The root fail-closed availability repair is organization-owned and read-only to this writer (ContextualWisdomLab/.github#897 or a freshly verified protected successor). Do not add an EgressWeave-local substitute or weaken the gate.
  2. Strix — require a real exact-head semantic report. Backend-unavailable/no-report neutral success is non-passing and is tracked in the organization-owned fail-closed control plane (ContextualWisdomLab/.github#891 or a freshly verified protected successor). Do not churn this clean source head merely to retrigger provider capacity.
  3. Current semantic/source review — validate any future human/automated finding against this unchanged exact head; resolve only addressed threads. Do not synthesize approval or infer it from wrappers, models, statuses, reactions, authorship, dismissed or predecessor evidence.

Keep this PR Draft. Immediately before any integration, refetch the exact head, independently resolved live base, ancestry, live ruleset, every required workflow and actual semantic step, reviews/findings/threads, and writer evidence. Merge only an unchanged accepted tree under normal governance; no predecessor, synthetic-merge, skipped, neutral, queued, rate-limited, wrapper-only, model-only, status-only or stale evidence transfers.

@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Warning

Review limit reached

@seonghobae, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 57 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b3265d37-8689-478f-a398-c86c45eb4336

📥 Commits

Reviewing files that changed from the base of the PR and between 7faf7a3 and b7021ef.

📒 Files selected for processing (2)
  • src/egressweave/request_safety.py
  • tests/test_request_timeout_untrusted_objects.py

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@seonghobae
seonghobae marked this pull request as ready for review August 12, 2026 23:11
@seonghobae
seonghobae marked this pull request as draft August 13, 2026 00:05

Copy link
Copy Markdown
Contributor Author

@coderabbitai review
@opencode-agent review
@cwl-noema-review review

Review exact current head b7021ef272170664abd81c22b38a2a5e39bfe497 only against live protected main 7faf7a3b8a47980113982914000e724ab6a6cda5. Validate one-time detachment of the outer untrusted extension mapping before timeout access, acceptance of valid custom Mapping inputs through a built-in snapshot, masking of outer and nested copy failures, unknown key and numeric-protocol rejection, finite per-phase capping, preservation of SNI/extension allowlisting in the subsequent boundary, and the bounded two-file scope. Submit a real current-head source-review verdict; do not mutate, merge, or infer approval from wrapper/status/model evidence.

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown

Rate Limit Exceeded

@seonghobae have exceeded the limit for the number of chat messages per hour. Please wait 11 minutes and 29 seconds before sending another message.

@seonghobae seonghobae left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current-head source review for b7021ef272170664abd81c22b38a2a5e39bfe497 found no blocking defect in the two-file request-extension snapshot scope. _bind_bounded_request_timeouts now detaches the outer Mapping once through the existing exception-masking copy seam, performs timeout lookup only on the built-in dictionary snapshot, separately snapshots nested timeout mappings, and retains generic non-leaking denial for copy and value failures. This removes hostile get() dispatch and split-snapshot behavior without changing allowed extension keys or finite timeout policy. This is author-side review evidence only, not independent approval. Keep Draft until corrected Dependency Review and authoritative Strix evidence execute on the unchanged head and an independent current-head review is present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant