Repository navigation
Early-path storage, referenced lists, and the first green CI run - #1
Merged
Merged
Conversation
Choosing database storage and evaluating from wp-config.php was a silent fail-open. The bootstrap runs before WordPress, so it had no options to read and handed the library a database backend with no connection; Firewall::create() threw StorageConnectionException, the catch-all swallowed it, and every request sailed through unfiltered. Compiled_Config_Cache now mirrors the connection paths -- [storage] [config][connection] and the like -- into a JSON sidecar beside the compiled file, and the bootstrap injects DB_USER, DB_PASSWORD and DB_HOST into those paths at request time. The sidecar carries path strings only, so the invariant that no credential reaches disk still holds, and a rotated password takes effect on the next request rather than the next rebuild. The bootstrap also publishes what it actually did in $GLOBALS['basic_firewall_early'] -- called, credentials, evaluated, reason -- because BASIC_FIREWALL_EVALUATED is set by the Runner too and so proves nothing about which path ran. Site Health reads that report rather than guessing, and gains checks for the mu-plugin loader and for the wp-config.php snippet: the evaluation-point advice now disappears once the snippet is actually in place, instead of telling a site that has already done the work to do it again. Paths::portable() stops expanding a relative storage path to an absolute one on save, so the field keeps what was typed into it. That also fixes php://stdout being mangled to php:/stdout by path normalisation.
A rule can now point at a remote list instead of carrying its entries. An allow rule for Uptime Robot references https://cdn.uptimerobot.com/api/IPv4andIPv6.txt and follows it; nobody pastes addresses that change. Has_Sources holds the whole pipeline -- fetch, decompress, decode, select, where, template, validate -- shared by Ip_Address and by every condition type, each of which supplies only its own source_template(). A rule takes any number of lists. Sources\Refresher fetches them out of band on a WP-Cron schedule, so a slow or unreachable list costs a cron run and not a request, and records the result for the screen to show. Bugs this turned up, all of them silent: - The library reads snake_case. on_error, buffer and retention_days were being written as onError, buffered and retain_days, so every one of those settings was ignored. A logging handler kept its defaults no matter what the screen said. - schema_check_probability defaults to 0.01, which loses roughly the first hundred events against a freshly created table. Written as 1.0. - Rate limit validation was not idempotent: lines_to_list() stringified an already-parsed path map to "Array", so importing a rate limit rule that had been exported produced a rule with zero paths. path_lines() reverses the parse instead. ValidationIdempotenceTest now asserts validate(validate(x)) == validate(x) across every rule type. - A rate limit key of "client_ip, path" kept only "client_ip" -- the parser split the line on whitespace and took field four rather than the rest of it. - An emptied log handler came back as rotating_file on the next save, because the schema's choices supplied a default for a value the screen had deliberately cleared. - case_sensitive on a regex condition corrupted the pattern: the library lowercases both sides, turning \D into \d, \W into \w, \S into \s and [A-Z] into [a-z]. Regex conditions now store the body between the delimiters and assemble the flags at compile time, so case sensitivity behaves like it does everywhere else. Schema v3 migrates stored patterns. - Rules_Screen fataled on a rate limit rule imported without validation. The Importer now validates what it imports, the way the rule form always did. kanopi/firewall 2.30.0 brings challenge.ttl, which caps how long a solved challenge is honoured -- verified against the library by posting ttl=999999999 and getting exactly 3600 seconds back -- plus checksum (sha256/384/512) and max_size on a referenced list. Checksum and signature together are refused at save, because the library throws on the combination.
…read The rule screen grew an Add List button and a card per referenced list, because the first pass was a wall of fields with no boundary between one list and the next. The repeatable machinery behind it is generic -- data-bfw-repeatable, data-bfw-item, data-bfw-add -- and the logging screen uses the same thing for multiple handlers. Condition rules gain the Name column the Drupal module has: "look at" is the family (header, cookie, POST field, query parameter) and "name" is which one. Without it a query parameter rule was simply unreachable from the UI -- there was nowhere to say ?test=1. Rules can be imported one at a time, not only exported. Every rule type that takes a list can import a file, not just IP addresses. The firewall log filters by rule, level and window, and each entry opens to show why the request was blocked rather than only that it was. The When column was empty because the reader asked for created_at; the column is logged_at. The blocked list screen can add an address directly, and comes populated so the lookup has something to find. Challenge providers all render now, gated on the saved one, so reCAPTCHA's version -- and with it checkbox against invisible, theme, size, minimum score -- is reachable. Before, only the saved provider's section was emitted, which meant the field that chooses the provider's variant could never be seen unless it was already right. The YAML fields on Advanced and Import use core's CodeMirror, via wp_enqueue_code_editor(). Sidebar order is Status, General, Storage, Rules, Logging, Challenge, Presets, Advanced, Log, Blocked, Compiled, Export, Import. The Status page had been registering twice: a self-referential add_submenu_page() with a callback adds a second listener to the same page hook, so the screen rendered itself twice. It registers with an empty callback now, and AdminMenuTest counts callbacks per hook rather than menu entries, which is what actually went wrong. Rebuild folded into the Compiled screen; the rules table gives Type enough width to sit on one line.
…r had CI has failed on every commit since the repository's first, on every job but one. The cause was not any of them: composer.lock was resolved against whatever PHP ran `composer update` -- 8.4 here -- so it pinned Symfony 8.1 (requires >= 8.4.1), doctrine/dbal 4.4 (^8.2) and a set of dev tools needing 8.2 or 8.3. `composer install` then could not run on 8.1, 8.2 or 8.3, and seven of the nine jobs died before reaching a test. Unit tests on 8.4 passed, which is why the matrix looked like it was telling us something. config.platform.php is now 8.1.0, so Composer resolves for the floor the plugin header, readme.txt, the runtime guard in basic-firewall.php and composer.json all declare, rather than for the machine that happened to run the update. The tree drops to Symfony 6.4 LTS and dbal 4.2.5, both of which kanopi/firewall already allows -- its constraint is ~6.4 || ~7.3 || ~8.1 -- and the whole suite passes on them. PHPStan, now analysing at 8.1, immediately found a fatal in code this branch had just added: Has_Sources declared a constant in a trait, which is PHP 8.2. On 8.1 that is a parse error, so the plugin would not have loaded at all on the version it claims to support. It is a static method now. PHP-Scoper cannot live under that pin -- 0.18 needs 8.2 and its own dependencies need 8.3 -- so it moves to its own manifest in build/tools, resolved independently. This is sound because the builder's PHP does not reach the zip: scoping is a text rewrite, and the tree it rewrites is installed from the plugin's own lock. It also takes php-scoper's dependencies out of an ordinary `composer install`, which removes thecodingmachine/safe -- a package that throws on PHP 8.4 and made the release build impossible to run locally. The package job moves to 8.3 for the same reason, and the static job gains `composer check-platform-reqs --no-dev` so that a lock which cannot install on the declared floor fails loudly rather than by exhausting the matrix. vendor-version.php was still reporting v2.26.0; running the build updated it.
Two failures that only became visible once `composer install` started working on the whole matrix. The build tools had the same defect their extraction was meant to fix: I resolved build/tools on PHP 8.4, so the lock pinned symfony/string 8.1 (requires >= 8.4.1) and the package job on 8.3 could not install it. The manifest pins config.platform to 8.3.0 now -- the floor PHP-Scoper's own dependencies impose -- so the lock targets a declared version rather than whichever machine wrote it. Every lock in this repository now does. The integration jobs never reached a test: "Could not locate wp-load.php". CI symlinks the checkout into a throwaway site, and PHP resolves __DIR__ through the symlink back to the checkout, so the bootstrap's walk up four directories lands nowhere near a WordPress. BASIC_FIREWALL_WP_ROOT names the site when walking up cannot find it, and the workflow sets it. Nothing changes for a plugin that really does live inside a site, which is how it runs under ddev.
With `composer install` and the bootstrap fixed, the integration suite ran for the first time and reported one real bug and two tests that were only ever true of this particular site. The bug: Blocked_Clients::check() read the expiry from isBlocked(), which answers whether an address is blocked and not until when. DatabaseStorage::get() hydrates the whole row, so the `expire` column happens to come along; FileStorage::get() returns the stored payload alone and the expiry is not in it. The Blocked screen, the request tester and `wp basic-firewall check` therefore reported every block on file storage as permanent -- the default backend, and the one a fresh install uses. find() reports the expiry on both, alongside expires_at and the offense count, and takes a bare address as an indexed lookup rather than a scan; check() asks it, and falls back to isBlocked() for a backend that does not implement QueryableStorageInterface. This hid for the whole of development because the dev site runs on database storage and CI runs on file, so each was only ever checked against the backend that agreed with it. BlockListWriteTest now runs the block-and-read round trip against both in one run. The two tests: AdminMenuTest called add_menu_page() without loading the admin include that defines it -- warm on a site that has been browsed, absent on a freshly installed one, so the class died on an undefined function rather than on anything about menus. And the challenge ttl test looked for the ceiling in a compiled file with no challenge section, because the section is only emitted when a rule actually challenges; it adds one.
Same dependency as the ttl ceiling: the challenge section is only compiled when a rule actually challenges, so on a site without one the test was looking for `version: v3` in a file that had no challenge section at all. It adds the rule it needs.
sean-e-dietrich
changed the title
Two releases landed while this branch was in flight. 2.32.0 is a one-item
release with nothing to configure -- the challenge pass lifetime now
travels signed in `provider_token` instead of being proposed by the
client, which is 2.30.0's ceiling made unnecessary rather than merely
enforced. Nothing here had to change for it.
2.31.0 is the one with work in it.
## What a block record keeps
Every block record used to hold the visitor's whole cookie jar and
header set, verbatim -- session cookie, Authorization, challenge pass --
persisted for the length of the ban, in the artifact operators paste
into tickets. 2.31.0 made that an allowlist. Confirmed against the
running site rather than the release notes: a blocked request carrying a
session cookie and a bearer token now stores neither.
All four buckets are written out rather than left to the library's
defaults, for the same reason `challenge.ttl` is: a textarea cannot
express "absent, use your default" and a compiled file that disagrees
with the screen is the bug underneath half of this plugin's test suite.
That made the upgrade case the interesting one. A site that saved its
settings before this key existed has nothing stored, and reading that as
"keep nothing" would have stripped the user agent and the query string
out of every record written after the upgrade -- a data loss dressed as
a privacy fix. The compiler distinguishes an absent bucket from an empty
one, and schema 6 writes the document back through the validator so the
storage screen shows what the compiled file contains.
## The bucket WordPress is unusual about
The library keeps the whole query string by default and is right to: for
a scanner -- the commonest reason anybody reads a block record -- the
query string is the attack. Its notes say to narrow it if your URLs
carry reset tokens.
WordPress URLs do. `wp-login.php?action=rp&key=...` is a working
password reset and `wp-activate.php?key=...` is an account, so a blocked
request to either stores a usable credential for the duration of the
ban. Verified: the session cookie and bearer token were gone from the
record and `key=SECRETRESETKEY` was still in it.
Narrowing the bucket is not the fix -- an allowlist cannot say
"everything except this", and enumerating what a scanner might send is
what allowlists are worst at. So the plugin does the thing the library
cannot, because it is the half that knows it is WordPress: a Site Health
check counts how many records currently hold one, and the storage screen
says which URLs do it. Both verified against a real blocked request.
## Off the request path
`DeferredHandler` holds records and flushes them after
`fastcgi_finish_request()`, so a slow log destination is not a slow
page. Exposed per handler.
The half worth pinning is the injection path. This plugin never writes
database credentials into the compiled file -- it injects them at
request time by property path -- and a deferred handler holds the real
one as its own first argument, so the connection moves a level down. A
path that misses does not throw; it injects nowhere and the handler is
built with no connection at all. Checked by resolving both recorded
paths against the parsed configuration and watching the injection land
inside the wrapped handler.
2.31.0 also made a nested `{class, args}` expressible in `logger:` at
all, which is what makes the wrapping possible. That reaches advanced
YAML for free, since it is merged over the compiled configuration --
`FingersCrossedHandler` and friends are configurable now, and the README
says so.
1 task done
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
1 participant
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.
One session of work against the dev site, then the CI failures that turned out to be hiding behind each other. Reviewable commit by commit.
The feature work
a69af80— Database block storage on the wp-config.php path, which failed openChoosing database storage and evaluating from
wp-config.phpwas a silent fail-open: the bootstrap runs before WordPress, had no options to read, handed the library a database backend with no connection, andStorageConnectionExceptionwas swallowed by the catch-all. Every request went through unfiltered.Fixed with a JSON sidecar beside the compiled file carrying the connection paths (
[storage][config][connection]and friends) and runtime injection ofDB_USER/DB_PASSWORD/DB_HOSTinto them. No credential reaches disk, and a rotated password takes effect on the next request rather than the next rebuild.The bootstrap now publishes what it actually did in
$GLOBALS['basic_firewall_early']; Site Health reads that instead of inferring fromBASIC_FIREWALL_EVALUATED, which the Runner sets too. Site Health also checks for the mu-plugin loader and for thewp-config.phpsnippet, and the evaluation-point advice disappears once the snippet is in place.c969958— Referenced lists on every rule type, and kanopi/firewall 2.30.0Any rule can point at any number of remote lists instead of carrying entries, with the full pipeline (fetch → decompress → decode → select → where → template → validate) and out-of-band refresh on WP-Cron.
Silent bugs found while testing it, each one a setting the library never read:
onErroron_errorbuffered,retain_daysbuffer,retention_daysschema_check_probability0.01Plus: rate limit validation was not idempotent, so importing an exported rate limit rule produced zero paths; a key of
client_ip, pathkept onlyclient_ip; an emptied log handler came back asrotating_file;case_sensitiveon a regex corrupted the pattern (\D→\d,[A-Z]→[a-z]) because the library lowercases both sides — regex conditions now store the body between the delimiters, with a schema v3 migration; andRules_Screenfataled on a rule the Importer had never validated.2.30.0 adds
challenge.ttl(verified live: postedttl=999999999, got exactly 3600 seconds back), pluschecksumandmax_sizeon a referenced list.00da404— Admin: the screens the new rule settings needed, and the log you can readAdd List button with a card per list; the same repeatable machinery for multiple log handlers. Condition rules get Drupal's Name column — without it a query parameter rule was unreachable, there was nowhere to say
?test=1. Per-rule import; file import on every list-taking type. Log filters by rule, level and window, with per-entry detail; the When column was empty because the reader asked forcreated_atand the column islogged_at. Challenge providers all render (gated), so reCAPTCHA's checkbox/invisible choice is reachable at all. CodeMirror on the YAML fields viawp_enqueue_code_editor(). Sidebar reordered, and the Status page stopped rendering twice — a self-referentialadd_submenu_page()with a callback adds a second listener to the same page hook.Making CI real
CI had failed on every commit since the repository's first, eight jobs of nine, and none of that was caused by the work above. Each fix exposed the next layer.
c7b5acd— Resolve dependencies for PHP 8.1, which the plugin claims and CI never hadcomposer.lockwas resolved against whatever PHP rancomposer update, so it pinned Symfony 8.1 (requires >= 8.4.1) and doctrine/dbal 4.4 (^8.2).composer installcould not run on 8.1, 8.2 or 8.3 — seven jobs died before reaching a test, and unit tests on 8.4 passed, which is why the matrix looked like it was telling us something.config.platform.phpis8.1.0now, so Composer resolves for the floor the plugin header,readme.txt, the runtime guard andcomposer.jsonall declare. The tree drops to Symfony 6.4 LTS and dbal 4.2.5, both of whichkanopi/firewallalready allows.PHPStan, analysing at 8.1, immediately found a fatal this branch had just added:
Has_Sourcesdeclared a constant in a trait, which is PHP 8.2. On 8.1 that is a parse error — the plugin would not have loaded at all on the version it claims.PHP-Scoper cannot live under that pin (0.18 needs 8.2, its dependencies need 8.3), so it moves to its own manifest in
build/tools. Sound because the builder's PHP never reaches the zip: scoping is a text rewrite, and the tree it rewrites comes from the plugin's own lock. Thestaticjob gainedcomposer check-platform-reqs --no-devso this cannot regress quietly.d570b80— Pin the build tools to their own floor, and tell CI where WordPress isThe tools lock had the same defect its extraction was meant to fix — resolved on 8.4, so
symfony/string8.1 broke the 8.3 package job. Every lock in the repository now declares the floor it targets.The integration jobs never reached a test: "Could not locate wp-load.php". CI symlinks the checkout into a throwaway site and PHP resolves
__DIR__through the symlink back to the checkout, so the bootstrap's walk up four directories landed nowhere near a WordPress.BASIC_FIREWALL_WP_ROOTnames the site when walking up cannot find it.c2cb901— A block's expiry was backend-dependent, which CI found and I could notWith the suite finally running, a real bug:
Blocked_Clients::check()read the expiry fromisBlocked(), which answers whether, not until when.DatabaseStorage::get()hydrates the whole row so theexpirecolumn comes along;FileStorage::get()returns the payload alone and the expiry is not in it. The Blocked screen, the request tester andwp basic-firewall checkreported every block on file storage as permanent — the default backend, and the one a fresh install uses.It hid for the whole of development because the dev site runs on database storage and CI runs on file, so each was only ever checked against the backend that agreed with it.
check()asksfind(), which reports the expiry on both, andBlockListWriteTestnow runs the round trip against both backends in one run.Also two tests that were only ever true of this particular site:
AdminMenuTestcalledadd_menu_page()without loading the admin include that defines it, and the challenge ttl test looked for a ceiling in a compiled file with no challenge section — the section is only emitted when a rule actually challenges.efeb2e5— The reCAPTCHA version test needed a challenge rule tooSame dependency, one test later.
b6b95c7— kanopi/firewall 2.32.0, and the credentials block records were keepingTwo releases landed while this was in flight. 2.32.0 is a one-item release with nothing to configure: the challenge pass lifetime now travels signed in
provider_tokenrather than being proposed by the client, which makes 2.30.0's ceiling unnecessary rather than merely enforced. Nothing here had to change for it.2.31.0 is the one with work in it.
What a block record keeps. Records used to hold the visitor's whole cookie jar and header set, verbatim — session cookie,
Authorization, challenge pass — for the length of the ban, in the artifact operators paste into tickets. 2.31.0 made it an allowlist, exposed on the Storage screen:Verified against the running site, not the notes: a blocked request carrying a session cookie and a bearer token now stores neither.
The bucket WordPress is unusual about. The library keeps the whole query string and is right to, but its notes say to narrow it if your URLs carry reset tokens — and WordPress URLs do.
wp-login.php?action=rp&key=…is a working password reset. Same probe: cookie and bearer gone,key=SECRETRESETKEYstill in the record.Narrowing is not the fix — an allowlist cannot say "everything except this", and enumerating what a scanner might send is what allowlists are worst at. So the plugin does the half the library cannot, because it is the half that knows it is WordPress: a Site Health check counts how many current records hold one, and the Storage screen names the URLs. Both verified against a real blocked request.
The upgrade case. A site that saved settings before this key existed has nothing stored, and reading that as "keep nothing" would strip the user agent and query string out of every record written after the upgrade — data loss dressed as a privacy fix. The compiler distinguishes an absent bucket from an empty one, and schema 6 writes the document back through the validator so the screen matches the compiled file.
Off the request path.
DeferredHandlerflushes afterfastcgi_finish_request(), exposed per handler. The half worth pinning is the injection path: this plugin never writes database credentials into the compiled file, it injects them at request time by property path, and a deferred handler holds the real one as its own first argument — so the connection moves a level down. A path that misses does not throw, it injects nowhere and the handler is built with no connection. Checked by resolving both recorded paths against the parsed config and watching the injection land inside the wrapped handler. 2.31.0 also made nested{class, args}expressible inlogger:at all, which reaches advanced YAML for free.Verification
All nine CI jobs pass. Locally:
phpcsclean,phpstan(level 6) clean, 55 unit tests, 122 integration tests, release zip builds and installs on a clean WordPress with Composer removed fromPATH.One flake seen once and not since: the WP nightly e2e step's
php -Sdev server died mid-run, failing four tests behind it. It passed on re-run and could not be reproduced locally against the same PHP. The built-in server is single-threaded, which makes that harness fragile;PHP_CLI_SERVER_WORKERSwould harden it, and is deliberately not changed here on one observation.On the dev site:
sqlmapblocked,GPTBotblocked from a referenced list,python-requestschallenged, a browser served,/?test=1challenged,/.env403.Ten integration tests skip on CI and none locally — they are guarded on things a throwaway site does not have. Worth a look, but nothing is failing.
Not in this branch
.ddev/nginx/basic-firewall.confand thewp-config.phpsnippet live in thewordpress-ddevrepo2.31.0 leftover:
firewall-doctoris not surfaced in the admin; the one check of it that mattered here was reimplemented natively instead2.30.0 leftovers not yet wired up:
response: tarpit,challenge.revocable,challenge.passes_valid_from,max_entries, StatsD/Prometheus exporters2.27–2.29 leftovers:
Plugins\Reputation,Plugins\EdgeSignal,SharedStorageWorth filing upstream:
Firewall.php:2205callshttp_response_code()with noheaders_sent()guard, which makes/wp-cron.phpblocks return 200Was AI used in this pull request?