fix(interop)!: reject double-dot interop segments (LAB-5906) - #98
Conversation
The interop segment pattern admits `..` inside a segment (`a..b`), but the CachekitIO server rejects `..` anywhere in a key (protocol spec/cache-key-format.md#server-side-requirements, Traversal row), so such a key failed with 400 on every request while working on other backends. validate_segment now returns CachekitError::InvalidKey for a namespace or operation containing `..`, after the grammar check, and the macro's compile-time mirror (parse_segment) makes it a compile error for either `namespace` or `interop`. A lone `.` stays valid. The README states the rule. Re-vendors test-vectors/interop-mode.json 1.2.0 byte-for-byte from cachekit-io/protocol#94 (sha256 702613766d1b92bc3a337627a96b9aedc89abfeb4d9208c2bb00c9539a0a1f40; 35 key / 13 error vectors) and updates the sha and count pins. BREAKING CHANGE: an interop namespace or operation containing `..` now fails: interop_key returns InvalidKey, and #[cachekit] does not compile. Rename the segment; its keys become a full cache miss.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cachekit-io/cachekit-rs/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (5)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe runtime and macro validators now reject ChangesInterop key validation
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to Interop keys containing Security Architecture ReviewSecurity architecture risk: ⚪ Minimal · up to The change consistently narrows accepted inputs without adding a route around validation or increasing access to cached data. Callers using double-dot segments will now receive an error or fail compilation; lone dots remain valid. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 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 |
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Providing Context (Files & MCPs)Add these hints in your PR description (or a comment) to unlock deeper checks:
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
![kodus-27b[bot]](https://avatars.githubusercontent.com/u/223823690?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
1 participant
Summary
Interop/v1 key segments (namespace and operation) containing
..are now rejected, both at runtime and at compile time. The segment grammar^[a-z0-9][a-z0-9._-]{0,63}$allows.., but the CachekitIO server rejects..anywhere in a key. Before this change, such keys were accepted locally and then failed on every CachekitIO request. A single.remains valid in any position, including trailing (for exampleapp.orget.).Modified public APIs
cachekit::interop::interop_key: returnsCachekitError::InvalidKeywhen eithernamespaceoroperationcontains... The# Errorsrustdoc now lists this case.#[cachekit(...)]attribute macro: anamespaceorinteropliteral containing..is a compile error, spanned to the offending literal. The attribute docs describe the rule.Implementation notes
validate_segment(crates/cachekit/src/interop.rs), checks run in this order: grammar, then.., then reserved namespace (ns/nsapi). A segment that fails the grammar reports the grammar error, not the..error...ban applies to both segment kinds.must not contain "..": the CachekitIO server rejects ".." anywhere in a key. The tests assert on this text.:delimiters separate segments, and the args hash is hex, so any..in a full key must lie inside a single segment.parse_segment) is kept in sync, and the cross-reference comments in both crates now mention the..ban.Tests and fixtures
interop::tests::segment_rejects_double_dot: rejectsa..b,ab..,a...b(namespace) andx..y,x..(operation), and acceptsapp./users.fetch.by_idandapp.v1/get..cachekit-macrosdouble_dot_segment_is_a_compile_error: equivalent coverage at the macro level.tests/interop_vector_tests.rs: the header now references protocolinterop-mode.json1.2.0 (protocol PR chore(deps): update rust-dev-deps #94, sha2567026137…a1f40). The count pins change from 34 to 35 key vectors and from 11 to 13 error vectors.README.md(interop section) documents the rule.Breaking change
Any existing interop namespace or operation containing
..now fails:interop_keyreturnsInvalidKey, and#[cachekit]does not compile. Affected segments must be renamed, and their keys become a full cache miss. These keys already failed against CachekitIO, but other backends previously accepted them.Summary
The interop/v1 segment pattern admits
..inside a segment (a..b), but the CachekitIO server rejects..anywhere in a key (cache-key-format.md → Server-Side Requirements, the Traversal row). So this SDK accepted such a segment and minted a key that fails with400on every CachekitIO request, while the same key works on other backends. cachekit-io/protocol#94 forbids..in either segment; this PR implements it.Changes
validate_segment(crates/cachekit/src/interop.rs) returnsCachekitError::InvalidKeyfor a namespace or operation containing.., after the grammar check. The macro's compile-time mirror (parse_segmentincrates/cachekit-macros/src/lib.rs) makes it a compile error for eithernamespaceorinterop. A lone.stays valid, including at the end of a segment.interop-mode.json1.2.0 byte-for-byte from cachekit-io/protocol#94 (sha256702613766d1b92bc3a337627a96b9aedc89abfeb4d9208c2bb00c9539a0a1f40; 35 key / 13 error vectors) and updates the sha and count pins. The new vectors arereject_double_dot_namespace(a..b),reject_double_dot_operation(x..) andlone_dots_stay_valid(app./users.fetch.by_id).README.md(interop section), theinteropmodule docs and the#[cachekit]attribute docs state the rule.interop::tests::segment_rejects_double_dot(rejectsa..b,x..y,x..,ab..,a...b; acceptsapp.andget.),cachekit-macrosdouble_dot_segment_is_a_compile_error, and the vector suite's count pins (tests/interop_vector_tests.rs; the sha is pinned in its header).Merge order
Merge cachekit-io/protocol#94 first. The vendored fixture must stay byte-identical to
test-vectors/interop-mode.jsonat its merge commit (sha256 above).Breaking
An interop namespace or operation containing
..now fails:interop_keyreturnsInvalidKey, and#[cachekit]does not compile. Rename the segment; its keys become a full cache miss. Before this change, such a key already failed on CachekitIO.Release notes read the breaking note from this block:
BEGIN_COMMIT_OVERRIDE
fix(interop)!: reject double-dot interop segments (LAB-5906) (#98)
BREAKING CHANGE: an interop namespace or operation containing
..now fails:interop_keyreturnsInvalidKey, and#[cachekit]does not compile. Rename the segment; its keys become a full cache miss.END_COMMIT_OVERRIDE
Test plan
cargo fmt --all -- --checkcargo clippy --all-targets --features cachekitio,redis,encryption,l1,macros,memcached,file,tracing -- -D warningscargo test --features cachekitio,redis,encryption,l1,macros,memcached,file,tracing: all suites pass, including doc-testssegment_rejects_double_dotfails; with the macro check removed,double_dot_segment_is_a_compile_errorfails