protect: accept live rule updates atomically; reject rule_id-less whitelists - #121
Merged
Merged
Conversation
…telists Round-2 review finding: our own validation could make a bad update WORSE than no update. Invalid rules were dropped and the remaining bundle was then written to cache as a success — so a malformed-but-well-formed-envelope response, or an oversized bundle whose important rules fall past the 5000-rule cap, could replace known-good policy with partial or empty policy AND cache that loss. Validation must not worsen the unsigned- bundle threat it was added to reduce. A LIVE update is now all-or-nothing: if any rule or whitelist fails validation, the whole update is rejected, last-known-good is retained (cache → bundled → empty, in that order), every rejection is reported (`onRuleRejected` with `accepted: false`, plus an `onError` summary), and the cache is NOT overwritten. Dropping individual rules is still fine for a bundle we already trust (a cache entry, a bundled fallback). Opt back in to partial acceptance with `acceptPartialBundle: true`. Also: a whitelist with no `rule_id` applies to EVERY rule, so one tripped condition disables the whole firewall for that request. Previously it only warned; it is now rejected unless `allowGlobalWhitelists` is set. Documents that `onSkip` is a trusted server callback whose `detail` must not be forwarded to clients. Not changed: the 5s boot budget. It already bounds what was a 30s startup exposure, and a 1-2s default would push legitimately slow-but-healthy networks onto stale/bundled rules more often — a protection trade-off in the other direction. Hosts can lower it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Implements atomic live updates and rejects ruleless whitelists securely. 🎯 Quality: 100% Elite · 📦 Size: Medium 📈 This month: Your 54th PR — above team average · Averaging Excellent |
Contributor
Author
|
/review |
daniloradovic
approved these changes
Aug 13, 2026
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
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.
An external round-2 review caught that our own validation could make a bad update worse than no update — the sharpest kind of finding, since the hardening was meant to reduce that risk.
The problem
validateBundledropped invalid rules and the remaining bundle was then written to cache as a success. So:could replace known-good policy with partial or empty policy — and cache that loss.
The fix: all-or-nothing for live updates
If any rule or whitelist fails validation, the entire update is rejected:
onRuleRejectedwithaccepted: false, plus anonErrorsummary naming the count and first offenders),Dropping individual bad rules is still correct for a bundle we already trust (a cache entry, the bundled fallback) — the atomic rule applies to fresh remote responses.
acceptPartialBundle: trueopts back into the previous behaviour.Global whitelists
A whitelist with no
rule_idapplies to every rule, so one tripped (attacker-reachable) condition disables the whole firewall for that request. It previously only warned; it is now rejected unlessallowGlobalWhitelistsis set.Tests
tests/protect/atomic-bundle.test.tsproves the guarantee that matters — after a bad update the app is still protected by the previous ruleset and the cache file is byte-identical — plus: rejections are reported and nothing is cached; fallback prefers the bundled ruleset over empty policy; partial acceptance works only when opted in; arule_id-less whitelist is rejected. 759 tests green, typecheck clean.Deliberately not changed
The 5 s boot budget. The review suggested 1–2 s; we're keeping 5 s. It already bounds what was a 30 s startup exposure, and a 1–2 s default would push legitimately slow-but-healthy networks onto stale/bundled rules more often — a protection trade-off in the other direction. Hosts can lower it via
bootTimeoutMs.