Report detections the guard did not block, not only the ones it did - #156
Merged
Conversation
|
Detection reporter reliably logs dry-run events without exposing sensitive data. 🎯 Quality: 100% Elite · 📦 Size: Large — consider splitting if possible 🛡️ Standards: no pre-flight fit check ran for this change — wire 📈 This month: Your 100th PR — above team average · Averaging Excellent |
patchstackdave
force-pushed
the
eng-3586-detection-reporting
branch
from
August 20, 2026 10:29
0ddebc2 to
dd328fd
Compare
A rule that blocks nothing reports nothing. `firewall-log.js` posts enforced blocks in the WordPress-compatible shape and answers "what did we stop"; there was no way to see that a rule carrying `enforcement: dry-run` matched traffic it would otherwise have blocked. Without that, a rule which is quietly wrong and a rule which is protecting look identical from the outside. ## The payload is small on purpose Per detection: the rule id, the request PATH with the query string removed, the parameters the rule reads, the phase, whether it was enforced, the rule-bundle ETag, and a timestamp. It never carries the matched value, the request body, headers, or query-string values. A channel that counts detections is a different thing from a copy of an application's traffic, and the difference is one careless field: once values are collected, every question about retention, access and jurisdiction arrives with them. Anything value-level belongs behind its own explicit opt-in with its own controls, not as a side effect of counting. The route drops the query because `?token=…` is a value, and the guard against regression is a scan of the SERIALIZED payload rather than of the object being built: a field added later (`message`, `value`, `headers`) passes every structural assertion and fails that one. `parameters` is the set the rule READS, from its own definition — not the condition that matched. The engine reports a rule, not which of its conditions fired, and threading that out would mean changing evaluation for the sake of a reporting field. ## Off by default Enabling it adds an outbound POST to every guard configured with a site UUID, which is a change in what an installed app does on the network — that belongs in the shipped docs before it becomes a default rather than after. `reportDetections: true` switches it on. Bounded (500 events, oldest dropped) and fail-open: an unreachable endpoint is silent and never retries into a loop. The drop count is sent WITH the batch, because a consumer computing a rate from these needs to know its denominator is short, and nobody infers that from a gap. 1249 tests, typecheck clean. Four guarantees are mutation-checked: keeping the query string, adding the block message to the payload, removing the queue cap, and deriving `enforced` from the site mode rather than the rule each fail the assertion that names them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
patchstackdave
force-pushed
the
eng-3586-detection-reporting
branch
from
August 20, 2026 10:30
dd328fd to
5e91e0a
Compare
Contributor
Author
|
/review |
daniloradovic
approved these changes
Aug 20, 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.
A rule that blocks nothing reports nothing.
firewall-log.jsposts enforced blocks in the WordPress-compatible shape and answers "what did we stop". There was no way to see that a rule carryingenforcement: dry-runmatched traffic it would otherwise have blocked — so a rule that is quietly wrong and a rule that is protecting look identical from the outside.This adds a second, deliberately narrow channel for that.
The payload
route— request path, query string removedparameters— what the rule readsphase,enforced,rules_etag,detected_atA channel that counts detections is a different thing from a copy of an application's traffic, and the difference is one careless field. Once values are collected, every question about retention, access and jurisdiction arrives with them — so anything value-level belongs behind its own explicit opt-in with its own controls, not as a side effect of counting.
Two details worth review:
The regression guard is a scan of the serialized payload, not of the object being built. A field added later —
message,value,headers— passes every structural assertion and only fails that scan, which is the direction this has to fail in.parametersis the set the rule reads, from its own definition, not the condition that matched. The engine reports a rule, not which of its conditions fired, and threading that out would mean changing evaluation for the sake of a reporting field. A rule with a single parameter — the common case for a narrowly scoped rule — gives the same answer either way.Off by default
Enabling it adds an outbound POST to every guard configured with a site UUID. That is a change in what an installed app does on the network, and it belongs in the shipped docs before it becomes a default rather than after.
reportDetections: trueswitches it on;detectionFlushMstunes the batch window.Wiring it default-on also broke 32 existing tests — the same fact, stated by the suite.
Bounded (500 events, oldest dropped) and fail-open: an unreachable endpoint is silent and never retries into a loop. The drop count is sent with the batch, because a consumer computing a rate from these needs to know its denominator is short, and nobody infers that from a gap.
Verification
1249 tests, typecheck clean. Four guarantees mutation-checked — keeping the query string, adding the block message to the payload, removing the queue cap, and deriving
enforcedfrom the site mode rather than the rule each fail the assertion that names them.