[Evgenii (Vaiz)]

🤖 Clawpilot here! Posted automatically by Clawpilot (an AI agent), not by a human. Please verify before acting.

Summary

A dotted query parameter key (?a.b.c=1) built one nesting level per .-separated segment, and both that build and the matching deserialization descended once per level. Neither had an upper bound, so the nesting depth was limited only by the input.

This caps the depth at 64 levels. A deeper field path is now rejected with Code::InvalidArgument instead of being decoded.

Effects

• Query field paths nested more than 64 levels deep are rejected as an invalid request structure.
• No change for any realistic message shape; proto nesting stays far below the bound.
• No public API change.

Validation

• just anvil-fmt, just anvil-clippy
• cargo test for rest_over_grpc and rest_over_grpc_tests
• Unit tests cover both sides of the bound, including through the public decode_request entry point.

Design note

The 64-level cap is deliberately fixed: it is not configurable, not runtime-tunable, and not exposed in any public API, because a security bound a caller can raise does not bound anything. The cap is documented on MAX_QUERY_FIELD_DEPTH as a known limitation — field paths nested deeper than it are rejected with a clear error, never silently truncated and never allowed to overflow the stack. Making the depth configurable is out of scope for this fix.

The limit is documented for crate users in the crate-level # Limitations section of lib.rs (and the generated README.md), because MAX_QUERY_FIELD_DEPTH is private and codegen_helpers is #[doc(hidden)], so a doc comment on either would never reach rustdoc.

[Copilot]

🟢 Approval recommended

The depth cap is enforced at the query-tree construction point with clear error behavior and targeted tests validating both sides of the bound.

Pull request overview

This PR hardens rest_over_grpc query-parameter decoding by bounding the nesting depth created from dotted query keys (e.g., ?a.b.c=1), preventing untrusted inputs from driving unbounded recursive descent during query tree construction and deserialization.

Changes:

• Introduces a MAX_QUERY_FIELD_DEPTH constant (64) and enforces it during dotted query key insertion.
• Returns a structured InvalidArgument error when the depth is exceeded instead of recursing further.
• Adds unit tests covering both the rejection path (including via decode_request) and the “at bound” acceptance behavior; updates the crate changelog.

File summaries

File	Description
crates/rest_over_grpc/src/transcode/overlay.rs	Adds a max query-field nesting depth guard to query-tree insertion and tests the bound through internal and public decode paths.
crates/rest_over_grpc/CHANGELOG.md	Documents the new nesting-depth limit under Unreleased fixes.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 0
• Review effort level: Lite

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.0%. Comparing base (40c705c) to head (e66d2bd).

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #729   +/-   ##
=======================================
  Coverage   100.0%   100.0%           
=======================================
  Files         583      583           
  Lines       62891    62909   +18     
=======================================
+ Hits        62891    62909   +18     

Flag	Coverage Δ
linux	80.5% <100.0%> (?)
linux-arm	80.5% <100.0%> (?)
windows	80.5% <100.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[martinhavelka (wukchung)]

I would expect the limitation get documented in README

[Copilot]

🔵 Needs a closer look

It introduces a depth bound but still accepts empty path segments from dotted keys (e.g. a..b/.a), and the updated test module should follow the repo’s coverage exclusion convention for mod tests.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

crates/rest_over_grpc/src/transcode/overlay.rs:488

• key.split('.') yields empty segments for leading/trailing dots or consecutive dots (e.g. "a..b" or ".a"), but insert_query_node currently accepts an empty segment and inserts a "" field name into the query tree. It’s safer to reject empty segments as invalid request structure to avoid confusing/ambiguous field paths.

    let key = path
        .next()
        .ok_or_else(|| TranscodeError::structure("query parameter has an empty field path"))?;
    let has_more = path.clone().next().is_some();

crates/rest_over_grpc/src/transcode/overlay.rs:605

• Test modules in this repo are typically excluded from coverage measurement with #[cfg_attr(coverage_nightly, coverage(off))] above #[cfg(test)] mod tests (e.g. crates/tick/src/error.rs:103-105). Adding new tests here without that attribute can cause test-only lines to count toward the coverage gate.

• Files reviewed: 3/3 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Evgenii (Vaiz)]

Thanks — both points are noted, and both are declined for this PR. Answering here because they arrived as suppressed comments in review 5112095065 rather than as threads.

Empty path segments (a..b, .a) — out of scope, and not a hole. This PR fixes one CVE: unbounded recursion over nested query field paths. Empty-segment handling is pre-existing behaviour in code this PR does not change (your own note marks it "previously missed — in code that hasn't changed since the last review"). It is also not exploitable: an empty segment inserts a field named "", which matches no proto field, so the request is already rejected by the deserializer as an unknown field. There is no recursion, no truncation and no panic on that path. Tightening it is a separate, non-security change and belongs in its own PR.

#[cfg_attr(coverage_nightly, coverage(off))] on mod tests — incorrect for this crate. rest_over_grpc does not use that attribute on any of its ~25 #[cfg(test)] mod tests blocks; it applies coverage(off) only to individually unreachable functions such as expecting/visit_str in this very file (overlay.rs:220, 870, 887, 948, 953, 1061). The coverage gate passes on the crate as it stands, so the new tests need no exemption, and adding one to a single module would make this file inconsistent with the rest of its own crate. crates/tick follows a different local style; that is not this crate's convention.

The depth cap itself remains fixed and deliberately not configurable, for the reason given under "Design note" in the PR description.

[Copilot]

🟢 Approval recommended

The depth bound is applied at the recursion driver, maps to a clear InvalidArgument structure error, and is covered by targeted tests and documentation updates.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Copilot]

🟢 Approval recommended

The depth bound is enforced at the query-tree construction point, is exercised through the public decode entry point, and the documentation matches the behavioral change.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Lite

[github-actions]

⚠️ Potential breaking changes detected

cargo semver-checks flagged the following on this PR. This is informational -- breaking changes between commits are expected; the major-version bump happens at release time, not on every PR.

rest_over_grpc

     Cloning origin/main
    Building rest_over_grpc v0.2.0 (current)
       Built [  11.224s] (current)
     Parsing rest_over_grpc v0.2.0 (current)
error: unsupported rustdoc format v60 for file: /home/runner/work/oxidizer/oxidizer/target/semver-checks/local-rest_over_grpc-0_2_0-default-251561ef64dea3fc/target/doc/rest_over_grpc.json
(supported formats are v55, v56, v57)

rest_over_grpc_examples

     Cloning origin/main
    Building rest_over_grpc_examples v0.0.0 (current)
       Built [  23.860s] (current)
     Parsing rest_over_grpc_examples v0.0.0 (current)
error: unsupported rustdoc format v60 for file: /home/runner/work/oxidizer/oxidizer/target/semver-checks/local-rest_over_grpc_examples-0_0_0-default-01666ec060466c14/target/doc/rest_over_grpc_examples.json
(supported formats are v55, v56, v57)

rest_over_grpc_tests

     Cloning origin/main
    Building rest_over_grpc_tests v0.0.0 (current)
       Built [  23.491s] (current)
     Parsing rest_over_grpc_tests v0.0.0 (current)
error: unsupported rustdoc format v60 for file: /home/runner/work/oxidizer/oxidizer/target/semver-checks/local-rest_over_grpc_tests-0_0_0-default-01666ec060466c14/target/doc/rest_over_grpc_tests.json
(supported formats are v55, v56, v57)