Repository navigation
Conversation
A plugin config is checked against its config-schema.json at compile time (E1023), but an object schema without `additionalProperties` accepts any key. The roots were closed and several nested objects were not, so a key in the wrong place compiled and was ignored at runtime: `skip_if_empty` under request-transformer's `headers` had no effect and no error. Closed: request-transformer `headers`, `querystring`, `path`, `path.replace`, `body`; response-transformer `headers`, `body`; kafka and nats `ack_response`. Each lists exactly the keys its config struct reads. jwt-auth's `public_key_jwk` is marked open (`true`), since a JWK may carry members beyond those listed (RFC 7517). crates/barbacane-compiler/tests/plugin_schemas.rs requires every object schema with `properties` in every plugin to state `additionalProperties`, checks that misplaced request-transformer keys are rejected and that a JWK with extra members is accepted. Each fails without the schema change. Every config in the fixtures and docs still validates. Signed-off-by: Nicolas Dreno <nicolas.dreno@barbacane.dev>
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 🧰 Additional context used📚 Code guidelines (1)No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: barbacane-dev/barbacane/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughSelected plugin configuration objects now reject undeclared keys during compilation. The JWT-auth ChangesPlugin configuration validation
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to Selected plugin configs now reject unknown nested keys, so misplaced keys fail at compile time. No merge-blocking risk was identified. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Plugin configuration becomes stricter without an identified expansion of access or weakening of authentication. Existing specifications containing previously ignored keys may need correction before recompilation. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 9 functions across 1 files. (6 skipped: 6 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
This branch has not been deployed
1 participant
Summary
Plugin config is checked against
config-schema.jsonat compile time (E1023). An object schema that leavesadditionalPropertiesunset accepts any key, though. The schema roots were closed, but several nested objects weren't, so a key in the wrong place compiled and was then ignored at runtime. #146's reporter hit this:skip_if_emptyplaced underheaderswas accepted and had no effect.Changes
Closed (
additionalProperties: false), each listing exactly the keys its config struct reads:request-transformer:headers,querystring,path,path.replace,bodyresponse-transformer:headers,bodykafkaandnats:ack_responseExplicitly open (
true):jwt-auth'spublic_key_jwk, since a JWK may carry members beyond those listed (RFC 7517:use,x5t, …).Guard:
crates/barbacane-compiler/tests/plugin_schemas.rs.properties, in every plugin'sconfig-schema.json, must stateadditionalProperties(false, ortrueor a schema where the shape is open by design). A new plugin can't reintroduce the gap without the test failing.request-transformerkeys are rejected:skip_if_emptyunderheaders,querystringorbody, an unknownpathkey, and an extrapath.replacekey.Behavior change: a spec with an unknown key in one of these objects now fails to compile with E1023, instead of compiling and silently ignoring the key. The CHANGELOG records it under Changed.
Testing
main's schemas the guard lists exactly the 10 open objects. The misplaced-key test fails onmain'srequest-transformerschema, and the JWK test fails ifpublic_key_jwkis closed.tests/fixtures,docs/rulesets/testsand the YAML blocks ofdocs/validate against the new schemas. The only exception isinvalid-middleware.yaml, which is invalid on purpose.docs/rulesets/generate.mjsproduces no change: the lint validators check only top-level keys.docs/rulesets/tests/run-tests.shpasses.Found while checking, not in this PR
Nine
jwt-authexamples in the docs use top-level keys the schema has never allowed, so they fail E1023 today:requiredandscopes:spec-configuration.md,dispatchers.md,extensions.md.headerandscheme:extensions.md.secretandpublic_key:secrets.md.The schema's top level was already closed, so this PR doesn't cause it. It needs its own docs fix.
Summary by CodeRabbit