proposal: body-aware policies#6
Merged
Merged
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✨ Finishing Touches🧪 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
kanywst
added a commit
that referenced
this pull request
May 9, 2026
Three medium-priority concerns from the review needed reconciliation
between the design doc and the v1 implementation:
1. Replacement contract: doc claimed structured replacement (status /
body / headers from the rule's value). v1 actually returns bool
from the evaluator and does a fixed 503 on deny. The structured
replacement requires the evaluator to surface json.Value plus a
discriminator and is deferred to a follow-up PR. Doc now splits
'v1' (fixed 503) from 'deferred' (structured).
2. 503 vs 403 asymmetry: doc didn't say which deny code response-side
uses, request-side already returns 403. v1 uses 503 on the
response side (upstream being replaced) vs 403 on the request
side (request rejected before upstream). Doc now states this
explicitly as intentional.
3. Backward compat for nested input shape: doc showed
'{request, response}' nested input which would have broken
existing input.method / input.path refs. v1 keeps the request-side
input flat (unchanged) and adds 'input.response.*' ONLY for the
allow_response target rule. input.request.* is reserved for the
v2 post-snapshot picture. Doc now shows v1 vs v2 input shapes
separately and clarifies 'allow' policies are untouched.
Per-context state section also corrected: v1 does not snapshot
request fields. The snapshot lives behind body-aware-policies.md
(PR #6) and shows up here only when that lands.
kanywst
added a commit
that referenced
this pull request
May 9, 2026
Three medium-priority concerns from the review needed reconciliation
between the design doc and the v1 implementation:
1. Replacement contract: doc claimed structured replacement (status /
body / headers from the rule's value). v1 actually returns bool
from the evaluator and does a fixed 503 on deny. The structured
replacement requires the evaluator to surface json.Value plus a
discriminator and is deferred to a follow-up PR. Doc now splits
'v1' (fixed 503) from 'deferred' (structured).
2. 503 vs 403 asymmetry: doc didn't say which deny code response-side
uses, request-side already returns 403. v1 uses 503 on the
response side (upstream being replaced) vs 403 on the request
side (request rejected before upstream). Doc now states this
explicitly as intentional.
3. Backward compat for nested input shape: doc showed
'{request, response}' nested input which would have broken
existing input.method / input.path refs. v1 keeps the request-side
input flat (unchanged) and adds 'input.response.*' ONLY for the
allow_response target rule. input.request.* is reserved for the
v2 post-snapshot picture. Doc now shows v1 vs v2 input shapes
separately and clarifies 'allow' policies are untouched.
Per-context state section also corrected: v1 does not snapshot
request fields. The snapshot lives behind body-aware-policies.md
(PR #6) and shows up here only when that lands.
eval.zig:
- New evaluateWithTarget(arena, input, ast, target_rule). evaluate()
becomes a thin wrapper targeting 'allow'.
main.zig:
- New evaluate_target wasm export with explicit target_rule pointer
/ length pair, mirroring the request-target ABI.
proxy_wasm.zig:
- proxy_on_request_body waits for end_of_stream then reads up to
max_body_bytes (64 KiB) via proxy_get_buffer_bytes.
- Builds {body: <parsed-or-null>, body_raw: <string>}: tries JSON
parse first; on failure 'body' is null and 'body_raw' carries the
raw bytes so policies can still match e.g. with the upcoming
'contains' builtin.
- Evaluates 'allow_body' target rule. Deny -> 403 + Action.Pause.
- Header-side path stays on 'allow', unchanged.
Note on header context: hosts (Envoy/wamr in particular) clear
:method / :path from the header map by the time the body callback
fires, so a body rule that needs request method must depend on a
snapshot taken in proxy_on_request_headers. Per-context snapshot
plumbing is intentionally out of scope for v1; v1 surfaces only the
body itself. Tracked in the design doc.
Tests:
- src/eval.zig: 3 unit cases (amount-over-limit denies, missing
target denies, body_raw fallback).
- test/run.mjs and test/run_wasmtime.py: 4 cases each driving the
new evaluate_target export end-to-end through the wasm boundary.
Release build grows from 50K to 54K. ci.yml gains test-unit job in
line with the other implementation branches.
Three medium-priority concerns from the review needed reconciliation between the design doc and the v1 implementation: 1. Pre-eval optimization with require_body_eval: doc described a flag but didn't say the header-side allow rule should still run first to short-circuit deny on header-only decisions. v1 doesn't ship the flag at all -- body callback always evaluates allow_body when it fires. Reframed as 'v1 vs v2': v2 lands the flag plus the header-phase short-circuit (saves CPU and buffering on rejected requests). Static-AST flag inference noted via the streaming evaluation proposal. 2. RequestContext lifecycle in host_allocator: doc proposed the snapshot map without saying anything about the deep-free / leak risk for the inner slices and json.Value tree. v1 doesn't ship per-context state; the snapshot is v2. Doc now recommends a per-context arena (lazy on first header callback, reset on proxy_on_done) so v2 starts from a shape that doesn't need manual deep-frees. 3. 'body: undefined' on cap exceedance: doc was wrong, undefined isn't valid JSON. v1's actual behaviour: body is set to JSON null when the body fails to parse, is empty, or was truncated by the host's max_body_bytes cap. body_raw always carries the raw (capped) bytes. Doc now states this explicitly and shows how Rego-style policies can branch on body_raw == '' vs body == null to distinguish 'no body' from 'non-JSON body'. Input shape section now splits v1 (body-only) from v2 (body plus request snapshot) so existing 'allow' rules are visibly unaffected.
kanywst
force-pushed
the
feat/body-aware-policies
branch
from
f9415f9 to
a46c101
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Design doc only. Tracks the body-aware policies item from ROADMAP near term.
See
docs/proposals/body-aware-policies.md.Summary:
:method/:path/:authorityplus configurable headers inproxy_on_request_headersso they survive intoproxy_on_request_body.proxy_on_request_bodyto buffer up tomax_body_bytes, parse JSON, evaluate the policy withinput.bodyandinput.body_rawavailable.require_body_evalplugin config flag so hosts that don't need it don't pay buffering cost.Status: design only, no implementation. Discussion welcome before writing code.