31 August 2026¶
34 changes:
- The half of OAuth discovery that was missing
- Three components that were nearly accessible
- The Model Context Protocol, over HTTP, for somebody else's assistant
- Offering an MCP capability without writing a class
- A page head that stops claiming things the page never said
- What the site tells a machine that arrives uninvited
- A keyboard can get past the navigation, and the consent screen stops phoning out
- A token outlived the client that issued it, and kept working
- An error log that was mostly not errors
- A client secret nobody had to present
- The SMTP password was readable to anyone who could read the database
- The webhook signing key and the TOTP seed, likewise
- A tenant scope that held until you asked for page 1
- A grant on one record opened the whole collection
- The session cookie lost its
secureflag behind every TLS-terminating proxy - The legacy CSRF check compared tokens with
=== - A LIMIT built by string concatenation
addslashes()as an escaping fallback, and as a PHP-literal encoder- The README described a framework this is not
- The schema promised tenant isolation nobody implements
- Joins on two columns, and aliases that survive resolution
- Permissions can now be resolved within one organisation
- Roles had two tables, a permissions screen, and no way to make one
- The client secret is hashed, and the realtime key moves out of its way
- A public client can say it is one
- The roles screen had a menu entry and no address
- A guard that read a variable that did not exist (FW-047)
- A pager that could not be told to stop counting (FW-048)
- Two components no theme could style
SchemaBuilder::hasIndex()- Tokens are encrypted at rest, and matched on a digest
The half of OAuth discovery that was missing¶
/.well-known/oauth-protected-resource — OAuth 2.0 Protected Resource Metadata, RFC 9728.
An installation has answered /.well-known/oauth-authorization-server for a long time. That
document says where the authorization server is. This one says where the resource is and
which authorization servers it trusts, and nothing was answering it.
A client holding only the first half has to be told the second out of band — configuration somebody types into a file, gets wrong, and has no way to verify. Both halves exist so that a client can start from either end and find the other.
Which is where an MCP client starts¶
The Model Context Protocol's authorization flow begins here: a client calls a protected endpoint,
is refused with a WWW-Authenticate header naming this document, reads it, finds the
authorization server, and then runs the ordinary OAuth 2.1 authorization-code-with-PKCE exchange
it already knows how to run. Every part of that except this document was already in place.
{
"resource": "https://example.com",
"authorization_servers": ["https://example.com"],
"scopes_supported": ["profile", "email", "phone", "address", "user", "openid", "offline_access"],
"bearer_methods_supported": ["header"],
"resource_documentation": "https://example.com/docs"
}
Three things about that document are deliberate:
scopes_supportedis read fromScopes, not written out. A hardcoded list drifts the first time somebody adds a scope, and the failure is a client asking for something the server refuses — after the person has already been walked through a consent screen.bearer_methods_supportedis["header"]and only that. RFC 6750 also allows a token in a form body and in a query string; the query form puts a credential into every access log, proxy log andRefererbetween the client and here. This framework does not accept it, so it must not advertise it — an unstated capability is one a client tries anyway.- No trailing slash on the identifiers.
resourceis compared as a string when a token's audience is checked, andhttps://example.comandhttps://example.com/are the same address and different strings. The mismatch presents as a token that is valid and rejected, which is the least debuggable failure OAuth has.
init scaffolds the rewrite rule alongside the ones for the other well-known paths, so a
generated project answers it without anybody adding a line.
On the tests¶
The existing discovery tests for this file assert against a copy of the array the method builds, rather than against what it renders — a shape that keeps passing if the method is deleted. The tests added here call the action and read the rendered document.
Three components that were nearly accessible¶
Field, Pagination and Datatable. All three were nearly right, which is why the gaps
lasted: every page link already carried an aria-label, every field already had a real <label>
with a for, every table already had proper headers.
What was missing in each case was a relationship — between a control and the text about it, between a run of links and the page's regions, between a table and the rows that arrive after the request finishes. A relationship is invisible when you can see the layout, because the layout is the relationship.
Form fields say what they mean¶
aria-describedby and aria-invalid did not appear anywhere in src/Pramnos/Html/.
A <small> under an input is not attached to it; it is a sentence that happens to sit nearby.
Somebody using a screen reader heard the label and the control and never the explanation — so the
field that needed the most explanation gave none. The input has carried an id all along, so
only the association was missing.
Field::$error is new and is the rendering end of validation, not a second validator:
Validation\FormRequest and View::$errors still own that. Set it and the field is marked
aria-invalid, and the message is joined to the control:
$field = new Field('email', 'email', 'Email', null, 'Your work address');
$field->error = 'That address is already registered.';
// → aria-describedby="email-error email-description" aria-invalid="true"
Three details that are deliberate. The error id comes first, because aria-describedby is
read in order and a field that is wrong should say so before it explains what it wanted. The
message carries role="alert" — it appears after a failed submit and has to interrupt to be
heard — while the field does not, since an alert there re-announces everything each time
anything changes. And a field with neither text gets no attribute at all: an
aria-describedby pointing at an id that is not on the page is worse than its absence, because
the reader announces the field and then nothing and the silence looks like its own bug.
Selects render down a different path in that class and get the same treatment, which is exactly how one of two paths otherwise ends up accessible and the other does not.
Pagination is a landmark¶
It had aria-label on every link and aria-current="page" on the current one — all of which
help once you are inside it. Getting inside was the part that did not work: with no <nav> it was
an anonymous run of anchors that region-by-region navigation passed straight over. Breadcrumb,
immediately beside it, has been a landmark since it was written. The difference was an
inconsistency, not a decision.
A caller who set containerElement to nav themselves gets the label on their own element rather
than a second region wrapped around it — two nested navigation landmarks announce the region twice
and neither is the answer.
A datatable says what it is, and that it changed¶
Datatable::$caption renders a visually hidden <caption>. Without one a table is announced as
"table, 7 columns" and nothing else, so a reader moving between the several tables an admin screen
carries cannot tell them apart. It is empty by default rather than guessed at: a caption invented
from the table's internal name — dt-emails — is worse than none, because a reader hears it and
believes it was written for them.
The rows are fetched, so the table is also aria-live="polite". Without it, sorting or paging is
silence followed by different data under the same heading. Polite rather than assertive because a
result set is not an emergency — it waits for the reader to finish its sentence.
The visually-hidden recipe already existed for the omnibox label; the caption shares that rule in all three scaffolded themes rather than adding a second copy of the same nine declarations.
The Model Context Protocol, over HTTP, for somebody else's assistant¶
POST /mcp — JSON-RPC in, JSON-RPC out, authenticated with an access token this server issued.
The infrastructure was almost all here already, which is the interesting part of this change.
McpServer::dispatch() has always taken a JSON-RPC message and returned one; run() is only a
loop reading STDIN around it. UnifiedAuthMiddleware has always validated a bearer token, loaded
its scopes from usertokens and resolved the user. What was missing was a transport, a decision
about which tools may be served, and the discovery document the previous entry adds.
Two registries, because the default has to be the safe one¶
The internal server carries nineteen development tools — coverage, the style checker, the framework's own docs — every one written for somebody who already has a shell on the machine.
Filtering a shared list would mean every tool written from then on is public until somebody remembers to exclude it, and the failure is silent and remote: a twentieth tool is added and it answers over HTTP before anybody notices it is reachable. So public exposure is a type, not a flag:
A tool that does not implement it cannot enter PublicRegistry, and no configuration overrides
that. One that implements it and returns an empty scope is refused with an exception rather than
skipped — a tool quietly dropped at boot is absent at run time for a reason nobody can find.
Refusing correctly is half the feature¶
An unauthenticated call gets 401 with
WWW-Authenticate: Bearer resource_metadata="…/.well-known/oauth-protected-resource".
That header is the discovery mechanism: an MCP client calls blind, is refused, reads the document
it is pointed at, finds the authorization server, and comes back with a token. A bare 401 ends
the conversation.
The scope check is structural¶
The server is built per request with only the tools the caller's scopes reach. A tool the caller may not use is not in that server, so naming it directly answers unknown tool rather than forbidden — one decision instead of two that can disagree, nothing gained by guessing, and no confirmation that the guessed tool exists.
A token whose scopes cannot be read reaches nothing. "I do not know what this caller may do" is not a reason to serve everything.
SearchTool¶
The one worth shipping, because it needs no code from an application beyond the
Registry::register() calls it already makes for its own search box.
Search\Registry was built for a box that several kinds of user share: each source declares a
permission, each row is scoped by a filter callable that receives the current user, and both fail
closed — a dropped source leaves no trace rather than an empty group, because an empty group named
"Invoices" tells somebody who may not see invoices that invoices exist. An MCP caller is a signed-in
person with a token, so it is the same question, scoped by the same code.
Its grouped-not-ranked shape suits a language model better than a flat list: "5 users, 3 orders" is a fact it can reason about, where a merged ranking invites it to read meaning into position.
It is not registered for you. Enabling an authenticated endpoint should not also decide what that endpoint exposes, even when the thing exposed is already permission-scoped:
init scaffolds POST /mcp when the authserver feature is on — not with auth, because an
application that authenticates people but issues nobody a token has no caller for this.
Offering an MCP capability without writing a class¶
ScopedMcpTool is five methods. Most of what an application wants to offer is a name, a sentence
and a closure, and a class file for three lines of logic is the reason capabilities do not get
offered at all. So there is a short door:
PublicRegistry::offer(
name: 'station-health',
scope: 'user',
description: 'Report the last successful stream check for a station.',
input: ['station_id' => 'integer', 'verbose' => 'boolean?'],
handler: fn (array $in) => Station::health((int) $in['station_id']),
);
That is a complete, authenticated, scope-gated tool.
The input spec, and why it has no wall¶
['station_id' => 'integer', 'verbose' => 'boolean?'] is the same document as fourteen lines of
nested arrays, and the fourteen lines are where people stop. A trailing ? marks a parameter
optional; everything else is required, because required-by-default is the safer mistake — a model
omitting something the tool needs gets a clear refusal, where a tool quietly running without it
produces a wrong answer nobody questions.
The compact form is not a dialect anybody has to stay inside. A single property can be spelt out
in longhand among the shorthand, and a spec carrying a top-level type passes through
untouched — so enums, patterns and nested objects are written as ordinary JSON Schema and
nothing interferes. PublicRegistry::schema() is public, so a tool written as a class can use the
short form too.
Both doors, one set of rules¶
The short door refuses a tool with no scope exactly as the long one does, with the same exception. Two ways in must not mean two sets of rules — that is how the convenient one becomes the one that skips the check.
Write the class when the tool has state, needs injecting, or is worth unit-testing on its own. Neither door is the real one.
A page head that stops claiming things the page never said¶
Five findings from an SEO/GEO review of what the framework renders, all in the same place.
Empty meta tags were emitted unconditionally — description, og:title, og:url,
og:site_name, og:description. content="" is not the absence of a description. It is a claim
that this page has none, and a crawler records the claim rather than falling back to the page
text. An application that never set the property said nothing of the sort.
It is the rule Seo::jsonLd() has documented as absent is not empty since it was written, and
the rule this renderer broke on every page it produced.
Emptiness is judged after escaping rather than before: escapeHeadValue() already refuses
anything that is not scalar and answers with a blank, and casting first would have turned that
safe answer into a page announcing that its description is Array.
viewport moved onto the document. It lived in the scaffolded themes and nowhere else, so a
theme that omitted it produced a page Google labels not mobile-friendly, with no signal anywhere
that anything was missing. It is a property of an HTML document, not of a decoration around one.
twitter:card is emitted only when there is an image for it. X reads the OpenGraph tags for
everything else; a large-image card promising an image the page does not have renders worse than
no card at all.
xmlns:og and xmlns:fb are gone — RDFa declarations from 2010, parsed by nothing since
Facebook moved to <meta property>, occupying the first hundred bytes of every page this
framework has ever rendered.
And the site now says what it is¶
Html\SiteIdentity puts Organization and WebSite on every page. Until now the only @type
anywhere was BreadcrumbList — the shape of a page's position in a hierarchy that was never
named. Document::addStructuredData() existed the whole time and the framework called it from
nowhere.
It asserts only what is configured, for the same reason the meta tags now do: structured data is a
set of assertions and an empty one is a false one. An unset logo is absent, not "". A
malformed entry in sameAs is dropped rather than published, because it does not fail loudly — it
quietly stops the organisation matching the entity it names. A SearchAction appears only where
Search\Registry actually has sources, since one leading to a page that returns nothing is
offered to a reader, tried once, and teaches them the site is broken.
What the site tells a machine that arrives uninvited¶
/robots.txt and /llms.txt, both generated rather than shipped as files — the one line that
matters in each is derived from the installation's own URL, and a static file in a scaffold is a
static file with somebody else's domain in it.
robots.txt, and why the AI crawlers are named one by one¶
Twelve of them: GPTBot, ClaudeBot, PerplexityBot, Google-Extended, Applebot-Extended,
CCBot and the rest. Every one reads robots.txt and honours it.
Absence is not neutrality. With nothing said, each crawler decides for itself, and they decide
differently — Google-Extended opts a site out of model training while leaving Search untouched,
which is a distinction a site cannot express by staying silent. The default is allow, because a
framework must not choose a site's licensing posture; what it must do is make the choice visible
and settable, which absence never did. One setting flips all twelve.
The paths with side effects are Disallowed rather than left to noindex: noindex keeps a page
out of an index after it has been fetched. On an authentication server every one of those pages
either costs a session, sends mail, or answers differently per visitor.
llms.txt¶
The GEO counterpart of a sitemap: a short markdown document saying what this site is and where the things worth reading are. A crawler follows links; a model arriving cold guesses, and guessing is how a site gets described wrongly and confidently.
It is deliberately short — the format's premise is that it fits in a context window beside the question somebody actually asked.
And it is where the MCP endpoint is announced, which is the one honest connection between these two pieces of work. A model reading it learns the site has tools it can call and where to authenticate for them. Without it the endpoint is a service nobody discovers — machinery complete and unreachable, which is the failure this framework keeps producing. It is announced only when something has actually been offered, because an endpoint serving an empty list is not worth pointing anybody at.
A keyboard can get past the navigation, and the consent screen stops phoning out¶
Two more from the same review, both in the scaffolded themes — which means both were in every site generated from them.
A skip link¶
The first thing any accessibility audit checks, and the <main> landmark it needs has been in
these themes all along. Only the link to it was missing, so reaching the content of a page meant
tabbing through every navigation item, on every page.
Positioned off-screen rather than hidden: display:none takes an element out of the tab order,
which removes the one thing a skip link exists to provide. It appears on focus.
The consent screen sent the person's address to a third party¶
The avatar fell back to Gravatar when an account had none of its own:
$this->user->avatar ?? 'https://www.gravatar.com/avatar/' . md5(strtolower(trim($this->user->email)))
So every render of /oauth/authorize sent md5(email) of the person signing in to another
company — from the one page in the whole flow where they are deciding what to disclose, and to a
party they were never asked about.
An md5 of an address is not anonymous. It is the address, hashed: the set of email addresses that matter is small enough to enumerate, and Gravatar's entire product is the lookup. This was found during an SEO review, which is where an accessibility and privacy pass tends to find things — nobody goes looking for it in the file it is in.
The avatar is now rendered when the account has one, and omitted when it does not.
The test for it strips PHP comments before asserting, because the comment explaining why the fallback is gone names Gravatar — and a test that cannot tell an explanation from the thing it explains is a test that punishes writing the explanation down.
A token outlived the client that issued it, and kept working¶
fk_usertokens_applicationid was created with ON DELETE SET NULL. Delete an OAuth client and
its tokens were not removed — their applicationid became NULL, and a token with a null
applicationid is exactly what a token issued outside OAuth looks like. Each one silently changed
category from issued by this client to not an OAuth token at all, and carried on
authenticating.
Measured on a working installation before the change: 507 of 522 tokens had a null
applicationid, and thirteen of those were still active and unexpired. Thirteen live
credentials belonging to clients that had been deleted.
SET NULL is a reasonable default for a column that annotates a row. applicationid does not
annotate — it answers who may use this token, and on whose behalf. Removing the answer does not
retire the question; it makes it unanswerable while leaving the token valid. And deleting a client
is the one action an operator takes precisely to stop it having access.
CASCADE is what the neighbouring constraints already do for the same reason:
fk_tokenactions_tokenid cascades from usertokens, fk_usertokens_userid cascades from users.
The application is the same kind of parent.
Rows that are already detached are left alone, deliberately. A token whose applicationid is
already NULL cannot be traced back to a client — the old rule destroyed the reference rather than
recording it anywhere. Nothing separates "was issued by a client that is gone" from "was never an
OAuth token", so a sweep would have to guess, and guessing here revokes working credentials. This
only stops more being made.
The constraint is dropped and recreated, because neither PostgreSQL nor MySQL can alter an
existing foreign key's delete rule — there is no ALTER CONSTRAINT for it.
An error log that was mostly not errors¶
MassMessagesController offers two optional filters: account groups and organizations. Both
belong to features an installation can be built without, so both are deliberate feature gates
— a screen that refused to render because one optional filter has nothing to offer would be the
wrong answer, and that part was always right.
What was wrong was how the question was asked. Letting the query fail and catching it means the
database layer logs relation "usergroups" does not exist at error level on every render.
The cost is not the disk. It is that the log stops being usable. On one installation it held 106 errors over thirty days, and most of them were designed-for conditions — so finding a real problem meant investigating every line to discover whether it was one. That is not a log, it is a pile.
hasTable() asks the same question without raising anything. The try blocks stay, because a
table can exist and still be unreadable, but they stop being the normal path.
Seventeen log entries per suite run became zero.
A client secret nobody had to present¶
Application::validateCredentials() added the apisecret condition to its query only
when a secret arrived:
A request that omitted client_secret therefore matched on apikey + status alone,
and every active application authenticated with no secret at all.
Why it was reachable¶
Not through a corner of the API — through the front door. league/oauth2-server 8.5
resolves an absent client_secret to null, and AbstractGrant::validateClient()
hands that value to the client repository without examining it. The repository asked
this method, and this method said yes.
So:
with no secret returned an access token. client_id is a public identifier by design:
it travels in redirect URLs and ships inside every SPA bundle and mobile binary. The
same path is taken by authorization_code, refresh_token and password, which all
route through AbstractGrant::validateClient().
ClientRepository::validateClient() carried a docblock promising the check — "Public
clients (secret=null) are accepted only when the application is configured as
non-confidential" — that no code performed. Application::isConfidential() returns a
hardcoded true, so nothing else was going to catch it either.
Fixed¶
The application row is read once and the secret compared against the stored value with
hash_equals(). A registered secret must be presented and must match; the comparison
is constant-time and no longer travels into a WHERE clause.
A client whose apisecret is empty is left exactly as it was — nothing is registered
for it, so nothing is asked of it — but it accepts only a request that presents no
secret either. Accepting an arbitrary string there would have been looser than the
version this replaces, where a non-empty secret simply failed to match an empty column.
Tests¶
tests/Integration/Auth/OAuth2ClientSecretRequiredTest.php pins the contract against a
real database: registered secret absent, registered secret empty, correct secret, wrong
secret (including a prefix of the real one), no secret registered, disabled application,
unknown client_id, and another client's secret. Verified to fail against the previous
implementation before the fix landed.
One existing assertion had pinned the bug as intended behaviour —
assertTrue($app->validateCredentials('test_key', null)) in
tests/Unit/Pramnos/Auth/ApplicationTest.php — and now asserts the opposite.
Still open¶
isConfidential() returning a hardcoded true means the public column on
applications is not consulted, and a public client is indistinguishable from a
confidential one. That is a separate decision about how public clients should be
registered, not a fix that belongs beside this one.
The SMTP password was readable to anyone who could read the database¶
smtp_pass sat in the settings table as plaintext. Anyone who could read that
table could send mail as the operator: a leaked backup, a dump handed to a
contractor, SQL injection in some unrelated endpoint, a hosting neighbour, a DBA.
Added: Pramnos\Security\Encrypter¶
Authenticated encryption for values that have to be stored and read back. NaCl
secretbox (XSalsa20-Poly1305) via libsodium — the primitive ChannelEncrypter
already uses, rather than a second cipher for the same job — keyed from APP_KEY.
$stored = Encrypter::encrypt($value); // "enc:v1:…"
$value = Encrypter::decrypt($stored);
$value = Encrypter::maybeDecrypt($row); // plaintext passes through unchanged
maybeDecrypt() is the whole migration story. A value without the enc:v1: marker
comes back as it is, so a column can be read through it from the first deploy and
converts itself as rows are rewritten. No migration script, no downtime, no window
where half the rows are unreadable.
Hash what you verify, encrypt what you use¶
The distinction the class exists to hold. A secret the application only ever checks — a password, a 2FA backup code — is hashed, and nothing should be able to recover it. A secret it has to use — an SMTP password, a signing key, a TOTP seed — has to be recoverable, so encryption is the only option available.
Encrypting the first kind would be a downgrade dressed as extra safety: APP_KEY
lives in .env beside the database credentials, so a reversible secret hands back
what a hash never would.
What it defends against¶
Every way a database is read without the filesystem. Not an attacker who owns the
host — they read .env. That distinction belongs in any compliance answer that
uses the phrase "encrypted at rest".
Fixed: smtp_pass¶
Settings now names it in ENCRYPTED_SETTINGS and encrypts on write, decrypts on
read. Every caller is unaffected: getSetting('smtp_pass') returns the plaintext it
always returned. An existing row converts itself the next time the settings screen
is saved, and with no APP_KEY the value is stored as before — a settings screen
that refuses to save is worse than the problem it would be avoiding.
A value that will not decrypt (after an APP_KEY rotation) yields the setting's
default rather than the ciphertext, so the failure reads as "no password configured"
instead of arriving at an SMTP server as a password.
Tests¶
tests/Unit/Security/EncrypterTest.php — round trip including UTF-8, binary and 4 KB
values, nonce freshness, tampering, wrong key, truncation, non-base64 payload, missing
APP_KEY, and the non-base64: key forms. 100% line coverage of the class.
tests/Unit/Application/SettingsEncryptionTest.php for the boundary, and
tests/Integration/Application/SettingsEncryptionAtRestTest.php which asserts on the
column itself — a broken implementation that stored plaintext and returned plaintext
would satisfy every API-level test there is.
Also fixed¶
SettingsPostgreSQLTest::testBulkLoadReadsEverySettingOnPostgreSQL had been failing
on a pristine tree. The bulk read is cached for 300s under the settings category,
keyed on a query that does not change when setUp() drops and recreates the table, so
a previous run's empty result answered for the next five minutes. Only setSetting()
invalidates that category, and the test inserts its rows around it. It now flushes the
category after seeding.
Still open¶
oauth2_webhooks.secret_key and user_twofactor.secret are the same shape of problem.
See the next entry.
The webhook signing key and the TOTP seed, likewise¶
The two remaining plaintext credentials, now encrypted through the same
Encrypter. Both are recoverable secrets — the application needs the actual bytes —
so encryption is the only option available for either.
oauth2_webhook_endpoints.secret_key¶
The HMAC key every delivery is signed with. A copy of it forges a webhook the
receiver accepts as ours, correctly signed: this user's permissions changed,
this token was revoked. It is already write-only through the API — register
returns it once and nothing re-displays it — so encrypting the column changed no
caller.
WebhookService::deliverEvent() reads through maybeDecrypt(). A key that will not
open fails that delivery attempt with a stated reason rather than signing with the
ciphertext, which would have surfaced as the receiver rejecting a forged-looking
request and sent the operator to the wrong side of the connection to debug it.
user_twofactor.secret and twofactor_setup.temp_secret¶
The seed every TOTP code is derived from. A copy is a permanent bypass of the second factor for that account — no expiry, and nothing the user would ever see. The in-progress enrolment seed is the same secret at an earlier moment, so both columns are covered; the dev panel's pending-enrolment view decrypts too, since a developer wants a seed they can type into an authenticator.
Backup codes were already hashed with PasswordHash::make() and stay that way. They
are checked, never used, so a hash is the stronger answer and encrypting them would
have been a downgrade.
A migration, found by the test¶
user_twofactor.secret and twofactor_setup.temp_secret were VARCHAR(64) — ample
for a 32-character base32 seed, and 45 characters short of the encrypted form. MySQL
refuses that write outright:
which is how it was found: the integration test asserting the column holds ciphertext
could not write one. 2026_08_31_000001_widen_totp_secret_columns.php widens both to
255 — room for a later format without another migration. Widening only: no value
changes, nothing truncates, and down() deliberately does nothing, because narrowing
back would truncate a seed and lock an account out of its own second factor.
Its priority is 90, above the 80 and 85 of the tables it alters. At 65 it ran first, found no column, did nothing, and left the old width in place — the migration runner orders by priority, not by filename date.
Tests¶
At-rest assertions on the columns themselves, on both drivers for the 2FA seed
(TwoFactorAuthServiceMySQLTest, TwoFactorAuthServicePostgreSQLTest), because the
part that varies per driver is the widening migration. Each also proves the factor
still works end to end: enrol, complete, getSecret() returns the seed as enrolled,
and a live code verifies.
A seed enrolled before encryption is asserted to still verify afterwards. Getting that wrong locks every existing 2FA user out at once, so it is asserted rather than assumed.
WebhookSubscriptionTest covers the signing key, including the no-APP_KEY case:
registering an endpoint must not fail because a key was never generated.
A tenant scope that held until you asked for page 1¶
OrmModel overrode _getList() to apply the soft-delete filter and the registered
global scopes. It did not override _getPaginated() or _datatablesRecordsTotal() —
and those are exactly what _getApiList() calls the moment a page is requested,
which is what every REST list endpoint, generated CRUD screen and datatable does.
So the same model, with the same scope registered, answered two different questions depending on how it was asked:
Post::addGlobalScope('tenant', fn($f) => …' tenant_id = ' . Auth::tenantId());
$post->_getApiList(['id', 'title']); // scoped
$post->_getApiList(['id', 'title'], '', '', '', '', '', null, null, 1, 20); // not
The second returns every tenant's rows, and reports a total counted over the whole table. That is a leak plus a pager offering pages that come back empty, plus a disclosure of how many records the other tenants hold — from one missing override.
HasScopes documents global scopes with a tenant example, so this is not a hazard
somebody had to go looking for: an application scoping its models the documented way
got the scope on its admin screens and none of it on its API.
The second bug, found on the way¶
mergeSoftDeleteFilter() wraps its input in parentheses, and ApiListQuery hands the
list methods a filter that already begins with where. Together:
A syntax error — returned as an empty result set, with the message buried in the
response envelope's error key. Any soft-deleting model listed with a filter came
back empty and said nothing about why.
Fixed¶
One mergeListConditions() helper on OrmModel — strips a leading where, merges
the soft-delete filter and the scopes, returns null when empty — used by _getList()
and by new overrides of _getPaginated() and _datatablesRecordsTotal(). Nothing
was added to the ApiListSource interface: a new method on a published interface
would break any application class implementing it directly.
The count merges and then calls parent::_getPaginated() rather than the base
_datatablesRecordsTotal(), which would route back through the override and apply
every global scope twice.
Tests¶
tests/Integration/Application/OrmScopesApplyToApiListTest.php — six tests against a
real table. Two of them fail on the previous implementation, which is how the shape
of this was pinned down: the paginated list returned the other tenant's rows, and the
unpaginated list with a filter returned nothing at all.
The rest hold the edges: a caller's filter is combined with the scope rather than
replacing it (otherwise any endpoint accepting a filter is a way to shed the tenant
condition), a model with no scopes still lists everything, and _getList() and
_getApiList() are asserted to agree — the property the whole change is about.
Known limit¶
A local scope queued with applyScope() is consumed by the first query it reaches,
so on a datatables request it applies to the page and not to the total. Global scopes
and soft deletes are re-derived per call and are unaffected. Tenant isolation belongs
in a global scope; the ORM guide now says so.
Documentation¶
The ORM guide's Scopes section described a fluent $query->where(...) API that this
framework does not have — Laravel's, not HasScopes'. Rewritten against the real
filter-string API, with a section on where scopes apply and why that is a security
property rather than a detail.
A grant on one record opened the whole collection¶
object_id is the permission store's only per-record mechanism, and the loop behind
ApiCrudController::authorize() ignored it. Every grant matching (object_type,
action) counted, whatever record it named.
So a grant written as read invoice 42 — the careful, narrow thing an administrator
reaches for when they mean one record — was read by the generated endpoints as read
invoices, and list returned all of them.
It was wrong in the other direction too. A deny on invoice 42 matched the same way, so denying one record denied the list for everybody.
Fixed¶
A grant now applies where it was written to:
| Grant | endpoint question | invoice 42 | invoice 43 |
|---|---|---|---|
object_id NULL or * |
allow | allow | allow |
object_id = 42 |
no rule | allow | no rule |
object_id = 42, deny |
no rule | deny | no rule |
"No rule" rather than a denial, because authorize() is deliberately three-valued: a
project that has granted nothing has to keep working. So this narrows what a grant
opens without locking anybody out of anything they had.
And a way to ask the record question¶
authorize(string $action) is not given an id — it asks about the endpoint, which is
all it can see. permissionForObject($action, $objectId) is the second question, for
a controller that has the id:
// In a generated controller, where the action already has the id.
public function read($id): mixed
{
if (($denied = $this->guard('read')) !== null) {
return \Pramnos\Http\Response::json($denied, $denied['status']);
}
if ($this->permissionForObject('read', (string) $id) === false) {
return \Pramnos\Http\Response::json(
['error' => 'forbidden'], 403
);
}
// …
}
authorize()'s signature is untouched. Adding even an optional parameter to it would
have broken every generated controller that already overrides it — PHP rejects a child
declaring fewer parameters than its parent.
Tests¶
tests/Unit/Application/ApiCrudObjectScopedGrantsTest.php, 14 tests over a resolver
reading canned grants — reached through a new permissionResolver() seam, the same
pattern ClientRepository::makeApplication() already uses, so the matching rules are
tested rather than an authserver schema.
Four of them fail when the object-matching is removed, which is how the old behaviour was pinned: the record grant opening the collection, the record grant answering for a different record, and both directions of the deny.
The session cookie lost its secure flag behind every TLS-terminating proxy¶
Session::isHttps() read $_SERVER['HTTPS'], which describes the connection this
process received. Behind a load balancer or reverse proxy that terminates TLS, that
is the plaintext hop between the proxy and PHP: the browser is on HTTPS and the
variable is empty.
The session cookie's secure flag is set from that answer. So on the most ordinary
production topology there is — anything behind nginx, HAProxy, an ALB, Cloudflare —
the cookie was issued without it, and then travelled on any http:// request to the
domain.
Fixed, without opening a worse hole¶
X-Forwarded-Proto is consulted, but only when the peer is in trusted_proxies —
the same list ClientIpResolver already uses for clientIp(). The header is
client-supplied: believing it unconditionally would let any visitor assert https and
be handed a cookie marked secure over a plaintext connection, which is worse than
the bug, so the untrusted case has a test of its own.
With no proxies declared the answer is $_SERVER['HTTPS'] alone, exactly as before.
The header may also arrive as a list — https, http, two proxies deep — and the first
entry is the one the client spoke.
The legacy CSRF check compared tokens with ===¶
Session::checkTokenValue() compared the submitted token to the session fingerprint
with ===, which returns as soon as two bytes differ.
The token is not weak: it is an HMAC-SHA256 keyed by the session's own 256-bit random token, so it cannot be predicted from the user agent and IP it hashes. An external report described the fingerprint itself as predictable, which it is not. What was wrong is narrower and still worth fixing — a comparison that leaks how far it got has no business in a security check, and the argument for leaving it is only that exploiting it would be difficult.
Now hash_equals(), matching verifyCsrfToken() on the synchronizer path, which was
always constant-time. A non-string is refused rather than coerced: a request that sent
token[]=x has not submitted a token, and hash_equals() would raise on it.
The legacy path is not a deprecated corner — the account controllers, the settings
form and the scaffolded templates all still emit it, so it is what a new project gets.
It had no direct test coverage at all, which is how the === stayed. It has six tests
now, including that the hidden field and the check agree, which nothing asserted
before.
A LIMIT built by string concatenation¶
User::getFeed($limit = 10) put its own parameter into the SQL by concatenation:
$limit is a public parameter. A controller forwarding a request value — the obvious
thing to write for a "load more" endpoint — hands a visitor the end of the statement.
Cast to int now, with a floor of 1. The friend-id IN list beside it is cast too:
those values come from the database and were safe, but they were safe by where they
came from rather than by anything the line does.
An external audit flagged the IN list and missed the LIMIT.
getFeed() is left on prepareQuery() rather than converted to the query builder.
Rule 12 would convert it; rule 2 wants characterization tests over User first, and
getFeed() is the one method those tests deliberately exclude —
UserSocialFeaturesCharacterizationTest says why. Converting behaviour nobody has
pinned down is how it changes by accident.
addslashes() as an escaping fallback, and as a PHP-literal encoder¶
Two unrelated uses, both wrong, both removed.
As a last-resort escape¶
Database::prepareInput() fell back to addslashes() when neither driver's escape
function was available. That is unsafe in two ways at once: addslashes() knows
nothing about the connection's character set — in GBK and other multibyte encodings
that is the well-known %bf%27 bypass — and it fails open, returning a string that
looks escaped and is not.
A missing extension is a broken installation, not a state to degrade into; there is no connection to escape against either. It now throws.
The old third branch was a separate bug: on PostgreSQL without pg_escape_string it
called mysqli_real_escape_string() with a PostgreSQL connection handle, which is a
TypeError, not an escape.
As a PHP-literal encoder¶
Init.php used addslashes() to embed the admin's credentials into a generated PHP
snippet — not into SQL. addslashes() escapes the double quote, and inside a
single-quoted PHP string \" stays a literal backslash followed by a quote:
So an admin password containing " was written to the database with a backslash in
front of it, and the operator could not log in with the password the installer had
just printed to their terminal. Now var_export(), which emits a real PHP literal —
quotes included — and round-trips quotes, backslashes and UTF-8 unchanged.
The README described a framework this is not¶
It asked for PHP 7.4 and ext-pdo. composer.json requires >=8.1, does not
mention PDO, and the Dockerfile is php:8.5-apache; the database layer calls
mysqli_* and pgsql directly and has never used PDO. A reader who took the README
at its word would install the wrong extension and target a version the code does not
run on.
Corrected, along with the contributing guidelines, which still told contributors to
keep src/ PHP 7.4-compatible, and the Docker section, which said 8.4.
The schema promised tenant isolation nobody implements¶
No code changed here. Two comments and a guide did, and this is the finding with the worst consequences of anything in today's list, because it is the one that makes somebody build on a guarantee that is not there.
authserver.user_roles carried the table comment "supports temporary and org-scoped
grants" and a docblock saying assignments "can be scoped to an organisation". There
is no organisation column on that table. The row is (userid, roleid) and nothing
else.
user_organizations opened with "A user must be a member of an organisation before
they can be assigned any organisation-scoped role… This mirrors the GitHub
organisation membership model". Nothing enforces that. No foreign key, no code path.
And underneath both: PermissionResolver has no organisation dimension at all. It
scopes by application (permissions.app_id) and returns every active role a user
holds, whatever organisation that role names. A role scoped to organisation A grants
its permissions everywhere.
The pieces are real — organizations, user_organizations, the org column on
roles — which is what makes the shape convincing. They are storage. Nothing reads
them for an access decision.
What changed¶
Both comments now say what is true, and say where the reader has to look instead.
user_roles' table comment is also corrected, though the migration is hasTable()-
guarded and will not re-run, so an existing installation keeps the old text — which is
why the docblock is where the correction is really aimed.
The authorization guide gains a Multi-tenancy section: what the framework does not
do, and the two places isolation actually belongs — a global scope on the models for
reads, an ABAC condition on the grant for authorization. Plus the warning that follows
from combining this with authorize()'s deliberate "no rule means allowed": a
scaffolded multi-tenant API is open by default, so the scope goes in before the
endpoints are generated, not after.
Not fixed here¶
Giving PermissionResolver an organisation dimension is a design decision, not a bug
fix — it changes what a role means. Recorded rather than quietly done.
Joins on two columns, and aliases that survive resolution¶
Two QueryBuilder limitations, fixed together because they are useless apart.
A join could only say one thing: join($table, $first, $operator, $second). A join on
two columns — a composite key, a membership row matched on both the user and the
organisation — had no expression at all short of joinRaw(), which takes the SQL as
given and resolves no table names.
And the moment a join was given an alias, the qualified name stopped being resolved:
the check that turns authserver.roles into its physical name skipped anything
containing a space. On MySQL that reaches the driver as a reference to a schema that
does not exist. Aliases are exactly what a multi-condition join needs, so either fix
alone would have been half of one.
use Pramnos\Database\JoinClause;
$qb->table('authserver.user_roles ur')
->join('authserver.roles rd', 'rd.roleid', '=', 'ur.roleid')
->leftJoin('authserver.user_organizations uo', function (JoinClause $join) {
$join->on('uo.userid', '=', 'ur.userid')
->on('uo.organization_id', '=', 'rd.organization_id');
});
on() ANDs, orOn() ORs, on('a.x', 'b.x') means equality. Both sides of a
condition are column references and there is deliberately no where() on a
JoinClause: a comparison against a value belongs in the query's WHERE, where it is
bound rather than pasted into the ON. A join builder that quietly accepts user input
is the bug this avoids.
Permissions can now be resolved within one organisation¶
authserver.roles.organization_id has been in the schema from the beginning, NULL
meaning "system-wide". Nothing read it. resolve() returned every active role a user
held whatever organisation it belonged to, so a role defined for organisation 5
decided questions about organisation 3's data — and an application built on the
framework's RBAC had no tenant isolation whatsoever.
resolveForOrganization($userId, $appId, $organizationId) asks the scoped question. A
role counts when it is system-wide, or when it belongs to that organisation and the
user is an active, unexpired member of it — the rule the user_organizations
migration described from the start and nothing enforced.
resolve() is untouched: unscoped, exactly as before, because applications depend on
it and it is the right question when there is one tenant.
Membership is read the way the admin screen writes it¶
OrganizationsController::removemember() sets is_active = 0 rather than deleting,
to keep the audit trail. A join that only asked whether the membership row existed
would therefore have left every former member's access exactly where it was — the
screen would say "removed" and nothing would have been. expires_at is honoured for
the same reason: a temporary membership whose window has closed is not a membership.
Leaving deletes nothing. The role assignment stays and stops counting; rejoining restores the same set. Asserted rather than assumed, because nothing does anything on leaving — which is the point, and exactly what a later "tidy up orphaned roles" change would undo without noticing.
Why one join rather than two queries¶
Measured, on 500 users holding 50 of 400 roles, MySQL in the container:
| ms/op | |
|---|---|
| today, no organisation | 0.105 |
join roles, filter in SQL |
0.138 |
+ join user_organizations |
0.154 |
| memberships already in memory, filter in PHP | 0.141 |
| separate memberships query, filter in PHP | 0.213 |
| membership as one PK lookup, then join | 0.226 |
The membership join costs 16 µs over filtering by organisation alone. Reading the memberships separately costs 59 µs more than the join saves — the round trip dominates, twice over. Filtering in PHP wins only if something has already loaded the memberships for free, and nothing in the request path does; 13 µs does not justify an API for callers to pass them in.
Opt-in, deliberately¶
Nothing calls the scoped method for you. ApiCrudController, Gate and Permissions
are not given an organisation and cannot guess one. The authorization guide's
multi-tenancy section now says which method answers which question, and that for reads
the answer is usually a global scope on the model rather than an authorization call at
all.
Also¶
AuthCharacterizationTest has been failing on its own and passing in a full run,
which is the least useful combination a test can have: it took whatever the previous
test left in the Database singleton. It loads the fixture settings and takes a real
connection now.
Roles had two tables, a permissions screen, and no way to make one¶
authserver.roles and authserver.user_roles ship with the auth feature's
migrations. PermissionsController will grant a permission to role 7. Nothing in the
framework could create role 7, or give it to anybody.
So an installation that enabled the feature got two thirds of an RBAC system: the
storage, and the half of the UI that assumes the other half exists. Which is also why
the membership rule the user_organizations migration described had never been
enforced anywhere — there was no write path to enforce it in.
Added¶
Pramnos\Auth\Role, and a RolesController with the screens the rest of the admin
area has: a list, one role with its permissions and holders, a create/edit form, and
a holders screen that adds and removes people. Wired into the nav beside Permissions
(usertype ≥ 90, authserver feature), into the scaffolded route prefixes, into
project:publish-views, and into the breadcrumb map of all three themes — twelve view
files, because a screen that only exists in one theme is not a feature.
The rule, in the model¶
Role::assignTo() refuses a role belonging to an organisation the user is not an
active, unexpired member of, and says so in words an administrator can act on. It
reads membership exactly as resolveForOrganization() does, so a screen cannot grant
something the resolver would then quietly ignore.
It lives in the model rather than the controller because an API caller reaching the model directly has to get the same answer as somebody clicking the button.
What does not delete¶
Revoking a role deactivates the assignment. Deactivating a role deactivates the role. Both stay readable and the resolver ignores both. Deleting a role does remove its assignments — a row naming a role that no longer exists is not history anybody can read.
And a role's organisation cannot be changed once anybody holds it: the holders who are not members of the new organisation would silently lose it, which is a permission change disguised as an edit. The form says so and the save refuses.
Tests¶
tests/Integration/Auth/RoleAssignmentTest.php — the organisation rule in both
directions, membership that is inactive or expired, idempotence, re-activation,
the audit columns, and the deliberate non-behaviour: leaving an organisation leaves
the assignment exactly where it is.
Both new integration suites build every shared table from its migration. Two
hand-rolled copies that disagreed about whether granted_by exists is exactly the
confusion that produces, and it cost a debugging round here before the rule was
adopted.
Also¶
AuthCharacterizationTest builds the authserver.permissions table it writes to.
Depending on some other suite having created it is what made the class
order-sensitive — passing in a full run, failing on its own.
The client secret is hashed, and the realtime key moves out of its way¶
applications.apisecret sat in the database as the value itself. Anyone who could
read that table could impersonate any OAuth2 client and mint tokens as it.
Hashed now — PasswordHash::make(), the same treatment a password gets — because the
server only ever verifies it. Encryption would have been the weaker choice: APP_KEY
lives in .env beside the database credentials, so a reversible secret hands back
what a hash never does.
The realtime key had to move first¶
broadcast_secret was added on 2026-08-20 precisely so the realtime HMAC key and the
client secret would stop sharing a value — that migration's own words, "the exposure
profiles differ". It was nullable with apisecret as the documented fallback, and
nothing ever wrote it. No controller, no backfill. So in practice every
application's realtime key still was its apisecret, and the separation existed
only as a column.
An HMAC key cannot be hashed: the sender needs the actual bytes. Hashing without splitting first would have broken channel authorization for every application at once, in a way that surfaces as subscribers failing to connect and points nowhere near the cause.
So split_broadcast_secret_from_apisecret copies the current value across, keeping
the key each application already has — nothing to re-issue, no subscriber notices. It
deliberately does not generate a fresh one: a migration is the wrong place to rotate
a credential. AuthServerAppRegistry then reads broadcast_secret only, and the
fallback is gone; the column is encrypted at rest, since unlike the client secret it
has to be read back.
No rotation, no downtime¶
A row still holding a plaintext secret is compared in constant time as before, and re-hashed in place after it verifies. An installation converts itself one successful authentication at a time. A failed guess converts nothing, or a wrong secret would overwrite the row with a hash of itself.
What changes for an operator¶
The client secret can no longer be read off the application's screen, because there is nothing there to read. It is shown once, in the message that follows creating or rotating it, and the screens say so. Rotation used to report only that a new secret had been generated — fine while the value could be looked up afterwards, useless now.
A public client can say it is one¶
Auth\Application::isConfidential() returned a hardcoded true, so a single-page app
and a server-side integration were indistinguishable. Every client had to have a
secret — and since the token endpoint began requiring one, that means an SPA has to
embed one. An embedded secret is not a secret.
is_confidential (default 1) records the difference, and isConfidential() reads it.
Unticking Client Type on the applications form registers a public client: PKCE for
the authorization code, and client_credentials refused, because that grant
authenticates the application with nothing but a secret.
Not the public column¶
An earlier reading of this said to use the existing applications.public. That column
means publicly listed — 0 = private, 1 = shown in a directory. Driving client
authentication from it would mean the first person to tick "list this app" silently
turned off its client authentication. A separate column, and the model now documents
which is which.
The roles screen had a menu entry and no address¶
registerAdminNav() registers admin.roles unconditionally. The administration area resolves
src/Admin/Controllers before the framework's own, so a project with no class there gets a menu
entry that answers 404.
Found by an application's own test — a check that walks every link the navigation offers and
reports the ones that do not open. It said /admin/Roles => 404, which is a sentence this
framework's own suite could never have produced: nothing here routes through a generated project.
init scaffolds the wrapper now, the way it already did for Applications, Tokens, Users and the
rest.
And a check for the general case¶
The specific fix is one file. The mistake is structural: register a nav item, forget the wrapper, and each half looks finished on its own — the link is drawn, the screen exists, and only a request joins them.
So there is a test that reads every $admin('…') target out of registerAdminNav() and asserts
that init scaffolds a controller for it. It is deliberately tolerant of two things it would
otherwise report as faults: the nav names a screen the way a URL does (users) while the file is
Users.php, and Health lives in src/Controllers on purpose, because /health/check is what a
monitor calls and has to answer without a session.
A guard that read a variable that did not exist (FW-047)¶
Logs\Logger::log() sniffs a message for JSON, but only — supposedly — when the caller has not
already said what the entry is:
$content does not exist at that point. It is assigned further down, inside the startoffile
branch, and holds a file's contents as a string. So isset() was false and the guard passed
on every single call.
It lasted because isset() never warns about an undeclared variable. The line looks like a check
and behaves like nothing.
The consequence that mattered¶
Not the wasted json_decode on every log write. It is that a type the caller supplied was
overwritten with 'json' whenever the message happened to parse as JSON — and a bare number
parses, and so do true, null and a quoted string:
So an error logged as '42' was filed as JSON, and a reader filtering the log by type never saw
it. The class of bug this framework spent the day on: a failure whose only symptom is an absence.
Reported by an application migrating onto this framework, which keeps a register of findings against it. Three of its six open items turned out to be already implemented and one was fixed on 29 August — this was the one still standing.
On the test¶
Its first version set the output mode in setUp() and then set it to a different fixed value in
tearDown(), because null — meaning "resolve from the environment each time" — is not a value
setOutputMode() accepts. Three unrelated tests that read their own log files started failing.
It saves and restores the private static by reflection now: restoring exactly what was there is
the only version that does not leak.
A pager that could not be told to stop counting (FW-048)¶
Html\Pagination had displayFirstLast, displayNextPrevious and displayEdgePages — and no
way to suppress the row of page numbers. $out .= $this->numbers() was unconditional.
displayPageNumbers (default true) leaves only previous and next.
Why that is a design case and not a preference¶
In search results the page count is large and moves with the filters. A reader on page 7 of 40 cannot say what page 7 is, and after narrowing the query it is a different page 7. Two links are the entire meaningful interface there, and a row of twelve numbers is noise that changes width as you read it.
It is distinct from displayEdgePages, which only decides whether 1 and the last page are
pinned inside that row. This removes the row.
And nothing to show is nothing to render¶
With the numbers off and neither button pair enabled, every branch is skipped and what was left
was an empty <nav>. An empty landmark is worse than no landmark: it appears in a reader's list of
regions and leads nowhere. Same rule as the single-page case, reached a different way.
On where this came from¶
Reported by an application migrating onto this framework. Its own pager is a 421-line fork, and this one switch was what kept it alive — two call sites set it, and the third already uses this class and works.
The report also said what it did not want: the fork's displayButtons, which renders
<button onclick="window.location.href=…"> instead of an anchor. Not crawlable, does not open in
a new tab, needs JavaScript to work at all. Declining to import a bad feature along with a good
one is the harder half of accepting a request.
Two components no theme could style¶
The convention across the components is that markup carries a neutral pf-* hook and each
scaffolded theme's stylesheet marries it to that theme's look. Twenty-three names work that way
already, with thirty-odd rules per theme — pf-omnibox, pf-skip-link, pf-visually-hidden.
Two components were outside it, in opposite directions.
Breadcrumb emitted class="breadcrumb" — Bootstrap's own name, as a literal. It reads as
neutral and is not: a project on Tailwind got an element carrying a name nothing in its stylesheet
defines, and one on Bootstrap got styling it never asked this component for. It is
$listClass = 'pf-breadcrumb' now, and a caller who wants Bootstrap's name back sets it.
Pagination emitted no class at all. $containerElementClass defaulted to '', so no theme
could reach it, and a project that wanted the pager to look like anything had to set the class at
every call site or write a selector against nav > div > a. It defaults to pf-pagination.
Both hooks now have rules in all three scaffolded themes, and a test asserts that — a hook with no rule anywhere is exactly the failure the convention exists to avoid, and it is invisible: the markup is correct and the page merely looks wrong in a way somebody blames on their own CSS.
A test whose name was the finding¶
HtmlCharacterizationTest::testRenderContainsBootstrapBreadcrumbStructure asserted
class="breadcrumb". A characterization test had recorded a coupling to one CSS framework as
though it were the component's structure, and its name said so plainly for however long it had
been there.
It is testRenderContainsAThemeableBreadcrumbStructure now, still guarding what it was really
for — a landmark, a list, closed properly — plus an assertion that the framework's markup names no
CSS framework of its own accord, and a second test that a caller can still ask for one.
What this did not do¶
There is a second convention in the framework: Form\FieldStyles::for($theme) hands Input and
Select the theme's own classes — form-control, or ten Tailwind utilities. That is not the
same mechanism and it is not wrong. Tailwind's model is utilities in the markup; hiding ten of them
behind a pf-input and re-applying them with @apply fights the framework it is meant to serve.
The rule that separates them: a structural hook, one per element, is pf-*. A utility-dense
set the theme wants visible in the markup is a keyed preset. Datatable::$jui — a boolean choosing
between Bootstrap 3 and jQuery UI, with no path to anything else — is an older generation of the
same idea and is left for its own change.
SchemaBuilder::hasIndex()¶
hasTable() and hasColumn() existed; the index question did not. A migration that
needed to add an index idempotently had to catch the driver's duplicate error or leave
the guard out — two ways of writing "I could not ask", and the token migration below
needed exactly this.
It matches on the index name, not on its columns. Two indexes over the same columns are legal, so a guard asking "is there an index on this column" would skip creating the one a migration needs because an unrelated one covers the same ground. A constraint-backed index counts as existing, since what a caller means is whether creating it would collide.
Per driver, because this has no standard: MySQL keeps one row per index column in
information_schema.statistics, PostgreSQL one row per index in pg_indexes. Tested
against both.
Tokens are encrypted at rest, and matched on a digest¶
usertokens.token held the token itself, and every lookup was
WHERE token = <presented>. Anyone who could read that table held live bearer
credentials — usable until they expired, without needing a client secret or anything
else. The framework's own token screen already refused to display the value, with a
comment saying why; the column it read from was handing it out anyway.
Why not simply hash it¶
Because a hashed token cannot be shown, and showing it is a feature somebody uses: an
administrator reproducing a failing integration copies a token into curl. Taking that
away to gain at-rest protection would be trading one real thing for another.
So the column is split, since the two jobs pull in opposite directions — matching needs a deterministic value, a copy button needs the original back:
| Column | Holds | For |
|---|---|---|
token_lookup |
sha256(token), unique, indexed |
the fifteen authentication lookups |
token |
the value, encrypted | Token::reveal() |
The digest is unkeyed, deliberately¶
A keyed HMAC is the reflex and would be right for a secret somebody could guess. Every
value here is 256 bits from random_bytes() or a signed JWT, so there is no dictionary
to attack and the key buys nothing measurable.
It would cost something: keying the lookup makes APP_KEY load-bearing for
authentication, so rotating it would sign every session out at once. Unkeyed, a rotation
costs the ability to reveal a token and leaves authentication working. That is the
better failure to have.
An index that was never there¶
token is TEXT, which MySQL cannot index without a prefix length — so it never was.
All fifteen authentication lookups were full table scans, on every API request.
token_lookup is fixed-length hex with a unique index, so the auth path is now faster
than before any of this.
Migration¶
Both columns are filled from the plaintext already in the table: no token is invalidated and nobody is signed out. Easier than the client-secret conversion, where the plaintext was gone by the time it mattered.
User::setupDb() also brings an existing table up to date now. It was only ever
CREATE TABLE IF NOT EXISTS, so a column added to those definitions reached a fresh
install and nothing else — which is what every caller that sets up a schema without
migrating had been quietly missing.
Consumers¶
An application reading usertokens.token for display has to go through
Token::reveal(), and one matching on the value has to use token_lookup. That is a
real change for anybody who has written their own token screen rather than using the
framework's.
A form's look, from what the application already declared¶
app/app.php carries scaffold_theme — the UI framework a project was generated against, in the
same vocabulary Form\FieldStyles is keyed by: plain, bootstrap, tailwind. Controller read
it to resolve views and ScaffoldingHelper to find directories. Nothing connected it to the
presets.
So SettingsForm asked the caller to name the theme at every call site, and the default when they
forgot was the literal plain:
A Tailwind project that omitted the argument rendered its forms with inline styles instead of utilities. A wrong look, no error, on a page nobody rechecks because the form works.
It is FieldStyles::configured() now, and the argument is optional — a derived default, overridable
by naming one, which is the same shape as og_title falling back to the page title and
Breadcrumb::$listClass defaulting to a hook.
An unknown value falls back rather than propagating: scaffold_theme names a scaffolding
directory, and that set can grow past the presets here. A test asserts the two lists match, because
if they diverge every project on the missing one silently renders as plain.
And the pf-* rules added earlier today are rewritten in tokens¶
A project's colours live in one file — app/themes/theme.css, propagated by pramnos theme:build
to a stylesheet, a JSON file and ThemeTokens::token(). The neighbouring hook rules are written
against it: var(--text-main, #1e293b), var(--primary-color, #2563eb).
The pf-breadcrumb and pf-pagination rules added earlier were not — they used currentColor and
bare values. So they would have been the one part of a scaffolded theme that did not change when
the palette did, which is the failure the single-file palette exists to prevent. They speak in
tokens now, with the same fallbacks.
Component class names, declared once in app.php¶
'component_classes' => [
'breadcrumb' => 'breadcrumb',
'pagination' => 'pagination',
'pagination.current' => 'active',
],
Six keys, every one listed in Html\ComponentClasses::KEYS with the pf-* hook it replaces. An
unlisted key is reported by unknownKeys() rather than ignored, because a misspelling is otherwise
silent and silence is indistinguishable from a feature that does not work. An empty string means no
class and is honoured — there the caller did speak.
It exists because the objects are not all yours¶
Breadcrumb::$listClass and Pagination::$containerElementClass were already public, so the
override existed. The problem is where the objects are made: a Breadcrumb is constructed in
eight places in a scaffolded project — Document, Application, and an admin_breadcrumb and
account_breadcrumb partial in each of three themes — and two of those are inside this
framework, on the path that renders every page.
So a per-object property covers six of eight, and misses the two that always render. The defaults are read at construction now, so one declaration covers all of them, and the property still wins for a single object: configuration is a statement about the project, a property about one breadcrumb.
And it is still not the first thing to reach for¶
The components emit neutral hooks and each theme's stylesheet dresses them. One CSS rule reaches
all eight sites, needs no PHP, and is the right answer to make this look different — colours and
radii come from app/themes/theme.css and reach the hooks from there.
This is for the other case: markup that must carry a specific name because something other
than a stylesheet is looking for it. A jQuery plugin doing $('.breadcrumb') does not read CSS.
The key is scaffolded into a new project's app.php commented out, with that reasoning beside
it — so somebody finds it by reading their own config rather than this framework's source, and
finds the reason not to use it at the same moment.
A model over a schema-qualified table could not read its own row on MySQL¶
Role declares authserver.roles. The QueryBuilder has resolved that per driver since from()
was taught to — a schema on PostgreSQL, prefix_authserver_roles on MySQL, which has none — but
a Model builds its own SQL, and never asked. So every _load() and _save() sent
authserver.roles to MySQL, which read it as another database, and threw.
Model::getFullTableName() now sends a dotted _dbtable through the same resolver:
Delegated to SchemaBuilder::resolveTable() rather than repeated — the flattening rule and the
prefix guard that keeps pramnos_pramnos_x from happening live there, and two copies would
eventually disagree. #PREFIX# still wins over the dot, because it has already said where the
prefix goes.
Found by executing the roles admin screens. They shipped complete, with a menu entry and a scaffolded wrapper, and could not create a role on a MySQL installation.
A NOT NULL date the model never set was written as timestamp zero¶
The same screen, one line further in. Model::_save() writes every column it finds, and a
NOT NULL column with null in it was coerced to '' — fine for a string, impossible for a
date. authserver.roles.created_at is NOT NULL DEFAULT CURRENT_TIMESTAMP, so a model with no
opinion about it was asking for the column's default and requesting the zero timestamp, which
strict MySQL and PostgreSQL both refuse.
A NOT NULL date, time or timestamp holding null is now omitted from the write: the default
fills it on an insert, the stored value stays on an update. Strings still coerce to '', because
models have relied on that far longer than this has been wrong.
In both loops. The column list is read twice — once cold from SHOW COLUMNS, once from
self::$columnCache — and fixing the cold path alone made the failure depend on whether anything
had saved to that table earlier in the process. Which is the worse bug: it passes in isolation.
One reader for authserver_organization_table¶
Role::membershipTable() read it with a default of ''; the user_organizations migration read
it with a default of 'user_organizations'. An installation holding the setting as an empty
string therefore got authserver.user_organizations from the model and authserver. from the
migration. The migration asks Role now.
Coverage attribution, which is not the same as coverage¶
JoinClauseTest declared #[CoversClass(JoinClause::class)] and nothing else, so twenty-one
lines it genuinely runs — QueryBuilder::resolveJoinTable(), Grammar's multi-condition join —
were recorded as untested. PHPUnit credits only the classes a test names. Both are named now.
Worth stating as a rule: a CoversClass list narrower than what the test exercises does not
weaken the test, it weakens the report — and the report is what tells the next person where to
look.
Two token lookups the encryption change left behind¶
Both fail closed, silently, and both shipped this morning in the same commit that split
usertokens.token into an encrypted value and a token_lookup digest:
Oauth::selectTokenRow()— introspection answered{"active": false}for every token this server had issued. A resource server that trusts introspection refuses every request.OAuth2Middleware::loadTokenFromDatabase()— every Bearer token failed validation, so the whole authenticated API answered as anonymous.
Fifteen lookups were converted and these two were not, because both are written as
where('ut.token', …) — the aliased form, which a grep for where('token' does not see. The
column no longer holds anything a presented value can be compared to, and every caller reads
"no row" as "not a valid token", so the failure has no error and no log line. It looks like a
client presenting a bad token.
selectTokenRow() also inner-joined users and applications to decorate the answer with a
username and a client id. Those are left joins now: the token's own row is the authority on
whether it is active, and an inner join made a token whose client row had been removed introspect
as dead while OAuth2Middleware — which left-joined already — went on accepting it. Two
components disagreeing about a live credential, invisible from either side.
Why the suite was green¶
There are unit tests over introspect() and revoke(). They mock the query builder, and a
mocked builder returns the prepared row whatever the WHERE says — so they cannot answer a
question about a column. They still earn their place (they cover the controller's decisions), and
the column is pinned separately now:
TokenAtRestTest::testNoLookupMatchesOnTheTokenColumn() reads src/ and fails on any surviving
comparison against token, alias or not. Verified by reintroducing the bug: it fails.
Found by another project's integration suite, which routes real requests through the real authentication path. Worth naming as the general lesson — a mock cannot notice a schema change, and fifteen call sites converted by hand want an assertion about the absence of the sixteenth.
The same model fix again, for PostgreSQL — twice¶
The schema-qualified-table fix earlier today was written against MySQL, tested against MySQL, shipped, and the first PostgreSQL request answered 500:
Two defects in one line.
public. should not be there. The dotted-name resolution was placed after the branch that
prepends the connection's schema, so on PostgreSQL it never ran and public was prepended to a
name that already carried a schema. A qualified name is resolved first now: it has said where
it lives, and prepending another schema to it can only be wrong.
() VALUES () should never have been composed. The column-introspection query asked for
table_schema = 'public' AND table_name = 'authserver.roles' — the schema from one place, the
whole dotted name from another. That matches nothing; an empty column list is not an error
anywhere; and _save() built an INSERT with no columns. So the operator saw a syntax error and
the cause — a table name that does not resolve — was two steps back.
There were three copies of that query in Model, and the third one already split the name
correctly. That is the shape of the whole thing: the fix existed in one of three places and
nothing made the other two agree. One reader now, plus a guard: a save that finds no writable
columns throws and says which table it could not read.
The lesson is about the lane, not the code¶
The integration lane runs one engine. An engine-specific mistake cannot fail there, and this one was shipped by a suite that was green on 13,065 tests. So the regression test is a unit test where the engine is a property of a throwaway connection, and it asserts all four backends:
- MySQL — flattened to
prefix_authserver_roles. - MariaDB — the same, asserted separately because
isMySQL()being true on MariaDB is a decision somebody could reverse, and nothing else in the suite runs a MariaDB connection. - PostgreSQL — left alone.
- TimescaleDB — identical to PostgreSQL, plus an assertion that a
typeoftimescaledbnormalises topostgresql. That one matters: this method comparestype == 'postgresql'literally, and a connection left reportingtimescaledbwould fall through to the MySQL path and flatten a schema into a table PostgreSQL does not have.
The connection is a clone, not the mutated singleton: DatabaseCapabilities memoises per
Database instance in a WeakMap, so flipping ->type on the shared object leaves isMySQL()
answering for whichever engine it was asked about first — a test that passes or fails depending on
the order its own methods ran in.
Verified the only way this kind of fix can be: by running a save, a read-back and a delete through the model against a live PostgreSQL/TimescaleDB installation, and by reintroducing each defect to watch the new tests fail.
The new-device auth link had never been executed¶
Auth\NewDeviceAuthLink — 109 statements, none of them run by anything. It is the one
new-device action every account can satisfy, because it needs nothing but the mailbox, so it is
the one an installation actually falls back on.
Nineteen tests now run it against a real store. The four rules that are the method's whole security, each asserted on its own because each is load-bearing when the others are absent:
- Single use — the hash is cleared before an id comes back, so a link cannot sign two sessions in. The mail stays in the inbox afterwards, and a provider's link-preview fetch counts as an open.
- Fifteen minutes, from the constant rather than a number somebody remembers — including the
boundary, which is
<and not<=: a link expiring this very second is still good. - One at a time — issuing again invalidates the link the person is already holding.
- The rate limit — the interval, the count per window, and that sends older than the window stop counting.
Plus the two decisions that surprise people, asserted so nobody quietly reverses them: with no activity-log table a send is allowed (refusing every link when a log is missing would refuse the login itself), and a failed delivery is not recorded as a send (a dead mail server must not tell somebody to watch an inbox nothing is coming to, and spend their rate limit doing it).
One seam, because the happy path was unreachable¶
send() built its notifier inline, so the only reachable parts of it were its refusals — the
fifteen lines that generate the token, store it, mail it and record it could not be run without
a mail server. notifier() is now a protected seam, the same idiom as
Controllers\Me::resolveUser().
That closes the test worth having most: the link in the mail is the link the store accepts. The
two are produced in different places — hashed into userdetails, then formatted into a URL — and
a mismatch would fail only for real users, never for a test that reads the store directly.
Suite: 13,076 → 13,095 tests, wall clock unchanged at 2:23.
The template screens, run on both backends from one set of assertions¶
MailTemplatesController — 131 of 137 statements never executed. Worse than it sounds for this
screen in particular: mailtemplates shipped as a table with no editor, and a screen that has
never been run is indistinguishable from the screen that was missing.
Nineteen tests, asserting what makes the screen worth having rather than decorative: the grouping
that answers is the reset email translated into Greek, the placeholders read from the template
instead of a maintained list that goes stale, the body kept as markup while the labels are
stripped, delete, the gate — and the test send actually rendering, [name] where each
placeholder lands, in the wrapper the template names.
mailer() joins notifier() as a protected seam, for the same reason: the mailer was built
inline, so the only reachable part of test() was its refusals and the two things a test send
exists to prove could not be tested at all.
And then all nineteen again, on PostgreSQL¶
class MailTemplatesScreenPostgreSQLTest extends MailTemplatesScreenTest
{
protected function settingsFixture(): string
{
return ROOT . DS . 'tests' . DS . 'fixtures' . DS . 'app' . DS . 'pg_settings.php';
}
}
Fifteen lines, and the whole file runs against PostgreSQL/TimescaleDB as well — 38 tests in one
second. settingsFixture() is the seam; the assertions are about the screen, and the only thing
that differs is what is underneath it.
This is the pattern worth copying, and it exists because of what happened earlier today: a
Model that addressed its own table correctly on MySQL and not on PostgreSQL, written and tested
on one engine, shipped green past 13,065 tests, and answered 500 on the first request from the
other. A suite that runs one backend cannot fail for the second. Where a test's subject is
engine-independent, saying so in fifteen lines is cheaper than finding out it was not.
Suite: 13,095 → 13,133 tests, wall clock 2:14 — faster than the 2:23 before it, so the cross-backend pass costs nothing measurable.
The inbox crashed on PostgreSQL and apologised on MySQL¶
MessagesController — 102 of 106 statements never executed, on the screen that exists to end a
dead end: MassMessageDispatcher wrote a row per recipient, the count was right, the progress
screen reported every recipient delivered, and nobody could read a word of it.
Sixteen tests now run it, and then run it again on PostgreSQL. The second engine found two defects in the first minute.
A try/catch around a query is only half the handling¶
try {
$result = $qb->…->get();
} catch (\Throwable $e) {
return []; // never reached on PostgreSQL
}
while (($row = $result->fetch()) !== null) { // fatal: fetch() on false
MySQL throws where PostgreSQL answers false. So with an unreadable messages table the listing
reported the problem politely on one backend and fatally crashed on the other, from the same
lines — and the catch block that made it look handled is what hid it. Both listFor() and
loadFor() now guard the result as well as the call.
There are 28 more sites in src/ with the same shape — ->get() followed by ->fetch()
with no falsy guard. They only bite when a query fails, which is exactly when the handling was
supposed to work. The real fix is engine parity in the driver rather than 28 guards, and that is
the next thing worth doing.
messages.attachmenttext has no default¶
TEXT NOT NULL with nothing to fall back on, so an insert omitting it fails — on PostgreSQL
always, on MySQL under strict mode. MassMessageDispatcher already passes '' with a comment
explaining why, which means the trap is known and every future writer has to remember it
independently. Now documented in the Email guide beside the screen.
Not changed here: the column is in a migration deployed since 2020, and altering a long-live table is not something to do inside a coverage pass.
Suite: 13,133 → 13,165 tests, wall clock 2:14 — unchanged.
The test suite now fails where an installation would merely be wrong¶
Yesterday's finding, chased to its cause. Measured on the same builder call against a table that does not exist:
| MySQL | PostgreSQL, lenient | |
|---|---|---|
get() |
throws | false |
first() |
throws | false |
count() |
throws | 0 |
The asymmetry is not the framework's: mysqli has thrown by default since PHP 8.1, while
pg_* answers false. So the shape most of src/ is written in —
try {
$result = $qb->…->get();
} catch (\Throwable $e) {
return []; // never reached on PostgreSQL
}
while (($row = $result->fetch()) !== null) { // fatal: fetch() on false
— is complete on one engine and half the handling on the other, and the catch block is what
makes it look finished.
Database::$throwOnError already closed the gap. What was missing is that nothing turned it
on, so it is now readable from settings:
Read on construction, so it survives the singleton being reset — which a suite does constantly, and which is why setting the property at bootstrap did not work.
What one flag found¶
Both test fixtures set it. That change alone, with nothing else touched, surfaced three things:
- Two second-factor tests passing against a missing accounts table. They assert that a wrong
password is refused; the refusal was happening because the query failed.
PermissionsPostgreSQLTestdropspublic.userson the same connection, so it depended on test order — and while a failed query answeredfalse,User::load()found nothing either way and the difference was invisible. - A characterization test pinning the lenient path, which now accepts the message through either channel. Its real subject — that a failed prepare must not lose the error text — is unchanged and still asserted.
- The inbox crash fixed in the previous commit, which is what started this.
Fourteen new tests pin the parity itself, on both backends, including the asymmetry: if a future PHP or driver release makes mysqli lenient, the suite says so here rather than through a screen crashing.
The runtime default stays false. Turning it on globally would convert every silently-empty
answer in every existing application into an exception — a BC break dressed up as a fix. The
guide says where it belongs instead: in a test environment always, in production deliberately.
Suite: 13,165 → 13,179 tests, wall clock 2:17.
An empty field list compiled to SELECT FROM¶
ApiListSqlBuilder — 149 of 219 statements never executed, on the class that decides what a
caller is allowed to ask for. Forty-two tests now run it, twice: once per backend, because it
branches on the driver in nine places.
The defect they found is one line. Its first branch reads
if ($queryFields === null || $queryFields === '' || $queryFields === '*') {
return $queryFields ?? '*';
}
— the condition says all three spellings of nothing asked for mean the same thing, and the
return disagrees: ?? '*' covers null only, so an empty string came back as an empty string.
Both callers hand the result straight to select(), which compiles it to
a syntax error — a 500 on MySQL, and outside strict mode on PostgreSQL an empty list that reads as this account has no records.
Model::_getApiList() already treats '' as equivalent to null and '*' thirty lines further
down, so the class disagreed with itself and only one of the two places produced SQL.
An existing test pinned the old answer, and reading it was the point: its docblock said "empty
field lists are passed straight through — there is no field list to inspect or extend". True about
inspecting, and not a decision about what to return. It asserts * now, with the reason.
What the forty-two assert¶
The security half, mostly. ?order= is a query string, so the tests include six shapes that must
never reach an ORDER BY — username; DROP TABLE users, username)--, (SELECT 1),
username' OR '1'='1 — each of which must produce the default order and nothing else. Quoting is
not validation; the whitelist is.
And the two edges nobody would guess: = null emits IS NULL, because = NULL matches nothing
and reads as a bug in the caller's data; IN with an empty array is dropped, because IN () is
invalid and IN (NULL) would silently mean nothing matches.
Suite: 13,179 → 13,221 tests. Measured A/B on the same machine in the same minute — without these files 2:34.9, with them 2:33.6 — so they cost nothing detectable. (The earlier 2:14 readings were on a quieter host; the whole spread is contention, and a run with strict mode off took 2:51.)
The screen that mails everybody, and its refusals¶
MassMessagesController — 156 of 283 statements never executed, on the action with the least
recoverable consequence in the framework. Everything else here can be corrected; this one reaches
every person on the list.
Forty-two tests, twice over the two backends, and most of them assert a refusal:
- a GET cannot queue anything, token or not — a GET that mails everybody is one link prefetch away from happening by itself, and a chat client unfurling a URL counts;
- a POST without the anti-CSRF token cannot either;
- criteria matching nobody queue nobody, and say so rather than reporting a send of zero;
- a sent message cannot be edited — it is the record of what people received, and editing it would change the record without changing what anybody got, which makes it a lie rather than out of date;
- a sent message cannot be deleted, for the same reason;
- and below usertype 90 none of the above is even reachable.
The two traps in reading the form¶
Both are documented in the code as traps, which is exactly why they deserved tests: a comment does not fail.
A false boolean is not an empty value. array_filter cannot tell unticked from not
mentioned, so validated_only and active_only are written after it. Dropped, they revert to
their defaults — which excludes accounts the operator chose to include, and makes the count
on the screen disagree with the send.
An empty template is a decision. No wrapper for this campaign looks exactly like an empty
form field, so the form posts __default__ for silence and '' for a choice. Only the choice is
stored, and the audit record omits the options key entirely when nothing was chosen — an empty
object there reads as a decision somebody made.
One assertion that only passed on one backend¶
Mine, not the framework's. request is a JSON column on MySQL and text on PostgreSQL, and
MySQL reformats the document — its own key order, spaces after colons. So
assertStringContainsString('"language":"el"', …) passed on PostgreSQL and failed on MySQL.
Decode and compare the value; the serialisation belongs to the server. Noted in the guide beside
the column.
Suite: 13,221 → 13,263 tests, wall clock 2:34 — unchanged within the spread.
Two backends taking different branches, on purpose¶
timescale:ensure — 115 of 213 statements never executed. The stubbed tests covered what the
command concludes about a table; nothing covered the entry point, and the entry point is where
its one interesting branch lives: what it does on a backend that has no TimescaleDB.
That branch was written for the case the command repairs — a database that gained the extension after the migrations had already run — so it has to behave correctly on a database that never will. Which makes this the one place in the suite where the two lanes run different code on purpose rather than the same code twice:
| lane | what runs |
|---|---|
| MySQL / MariaDB | continuous aggregates, then the documented bow-out |
| PostgreSQL / TimescaleDB | the same, then the whole hypertable plan |
Each lane skips the tests belonging to the other, so the skips in the output are the design rather than a gap.
The interval comparison, which is why a changed declaration reaches the database¶
sameInterval() exists because the command used to compare a policy's presence: a
declaration changed from 30 days to 7 left the old policy in place for ever, silently, and the
only symptom was a disk bill. Now pinned in all three directions:
- equivalent spellings are equal —
@ 30 days,30 DAYS,30 days,30 day; - a real difference is detected, including across units (
24 months≠2 years, because they are different in PostgreSQL and guessing otherwise would invent a policy nobody declared); - an unparsable interval is treated as equal. The safe direction: reading an unfamiliar spelling as drift would rewrite the policy on every single run, which is the failure the comparison exists to avoid.
And the repair path: a blocked table is reported and counted without abandoning the run — the remaining tables are the ones still growing without bound — while a conversion announces its row count and that it holds an exclusive lock before starting, because these are audit tables with millions of rows and somebody running this at 10am deserves to read that sentence first.
Suite: 13,263 → 13,282 tests, wall clock 2:23.
FW-049: the action that outlived its request¶
Reported by an application trying to migrate off its own legacy request class, and correct as
reported. calcParams() clears the controller and not the action:
self::$action is written only when the path has a second segment, and was cleared only by
resetInstance(). So calcParams('module') — a bare module — left the previous call's action in
place, and getAction() answered with it.
The asymmetry inside those six lines was the whole tell: if an action were meant to survive a re-route, the controller beside it would not be cleared either.
One request per process hides it in production. A test suite is one process for thousands of
requests, and that is where it surfaced: a controller reading the action as an identifying hash
terminated with Invalid User because it inherited the action of an unrelated earlier test.
Testing\TestClient already documents the same leak for the controller — "/ served the
previous URL's controller" — and fixes it by resetting the instance. The action was simply
missed.
Cleared where the controller is cleared, and two tests that fail without it.
And the second half of the report: document the decomposition¶
The same report asked, fairly, that if the path decomposition differs from the legacy algorithm it be written down — so we know what we are migrating to. It is now a table in the Routing guide, generated by running the code and pinned by a characterization test. Writing it turned up two anomalies neither side had named:
Leftover path segments become $_GET keys. module/action/x sets _option = x and
$_GET['module'] = 'action' — parts 0 and 1 are never removed before the key/value pairing loop
walks what is left. On /jobposts/view/479, code reading $_GET['jobposts'] gets 'view'.
A slash inside the query string changes the decomposition. $slashes is recounted after the
query string is appended to the path, so an ordinary return-url parameter moves the same path to
a different branch:
r=jobposts/view/479 → _option=479, jobposts=view
r=jobposts/view/479 ?return=/account/settings → _option=479, 479=null
One slash is enough, and _option survives — which is exactly why it goes unnoticed. It is the
leftover keys that move, and code reading one of them by name reads something else. That is a
plausible mechanism for a page that serves differently after a migration.
Neither is fixed here, and that is deliberate: both decide which page is served for URLs already in use, so changing them is a routing change for every installation and belongs in a release somebody chose, not in a coverage pass. What ships is that they are visible, executable, and will fail loudly the day they change.
Suite: 13,284 → 13,294 tests, wall clock 2:14.
Behaviour change: a slash in a query string no longer re-routes the path¶
The two anomalies the last post recorded are fixed. Recorded first, fixed second, deliberately —
both decided what ends up in $_GET for URLs already in use, which makes them routing changes
rather than bug fixes you take for granted.
Leftover path segments became $_GET keys. module/action/x set _option = x and
$_GET['module'] = 'action'. Only $parts[2] was removed before the key/value pairing loop
walked the rest, so the controller and the action were paired into an entry of their own. The two
neighbouring branches already removed them; that one was the outlier — which is what an omission
looks like from the outside.
A slash inside the query string changed the decomposition.
before: r=jobposts/view/479 → _option=479, jobposts=view
before: r=jobposts/view/479 ?return=/account/settings → _option=479, 479=null
now: both → _option=479
$slashes chooses the branch, and it was recounted after the query string had been appended to
$request. The append had no other purpose: the next line threw the query string away again,
$mainString[1] is never read, and $request is not used past that point. So four lines existed
only to corrupt a count.
One slash was enough, and _option survived either way — which is exactly why nobody found it
from the outside. It was the leftover keys that moved.
What made it safe to do¶
The measurement, not the reasoning. The change was applied first and the suite run before anything was rewritten: it failed the four characterization tests written an hour earlier to record the anomalies, and nothing else in 13,294 tests. A routing change with a blast radius that small is a routing change worth making.
If you relied on either: a controller reading $_GET['<module-name>'] on a three-segment URL,
or on a path segment appearing as its own $_GET key, no longer gets it. Read _option and the
named pairs. Nothing else about the decomposition moved — the table in the Routing guide is
generated by running the code, and the rest of it is unchanged.
Suite: 13,294 tests, wall clock 2:51 on a busy host (2:14–2:51 is this evening's spread with identical code; a measured A/B earlier put the new tests at no detectable cost).
A cache flush that broke a user activation¶
Found by a coverage run of this framework's own suite — the timing differs enough under coverage to expose a race the ordinary lane hides:
UnexpectedValueException: RecursiveDirectoryIterator::__construct(…/var/cache/userlist):
Failed to open directory: No such file or directory
FileAdapter.php:610 → Cache.php:728 → Database.php:2673 → User.php:755
← User::activate()
listDirectoryFiles() guarded with is_dir() — and a guard cannot fix a race, because it is a
check followed by a use. The directory goes between the two: another request flushing the same
group, or this adapter's own cleanEmptyDirectories() from a concurrent call.
The last line of that trace is the point. The throw did not break a cache flush; it broke a
user activation. save() flushes the user list, the flush raised, and the operation somebody
asked for failed because of housekeeping that had already succeeded — the directory was gone,
which is exactly the state the flush wanted.
Caught, around the loop rather than the constructor alone since a subdirectory can vanish mid-walk with the same result, and whatever was collected before it went is returned. One catch, six call sites.
The is_dir() guard stays: it is the common case, and an exception per flush of a group nothing
ever wrote to would be a cost paid constantly for a condition that is normal — which the tests
assert, by counting that a missing directory never reaches the iterator at all.
Covering a race honestly¶
A race cannot be reproduced by arranging files, so directoryIterator() is now a protected seam
and the test makes it throw. That is the only honest way to cover a catch for something that
happens between two statements — and it is the same idiom as notifier() and mailer() earlier
today.
The Adminer route, and the two things a mistake there would not announce¶
167 of 235 statements never executed on /adminer, a route that serves a database browser. The
gate is documented at length; what nothing ran was the rewriting, and two parts of it are
security rather than presentation.
Which files it will send. The path comes from a query parameter and the directory sits in
vendor/. Eight traversal shapes are now asserted refused — ../../../../etc/passwd,
static/../../../../etc/passwd, static/./../../composer.json, static/ + twelve ../ — and
so are five extensions. static/x.php is the one worth naming: nothing here would execute it,
but it would be read out verbatim, which is how a config file leaks. The whitelist runs before
any filesystem call, and a positive case is asserted too, so the refusals cannot be passing
because everything is refused.
What its links carry. Adminer puts the server, the user and the database in its own URLs.
Left in, every link on a page this route served would name this installation's database host and
account — in the address bar, in the browser history, and in a Referer on the way to
adminer.org. All seven driver keys are asserted stripped, not only server; absolute URLs are
asserted untouched, because those are Adminer's own outbound links and rewriting them would
corrupt somebody else's URL to no purpose.
Suite: 13,294 → 13,313 tests, wall clock 2:15. Total coverage 90.03% → 90.49%.
The password-reset token, and a fixture that proved nothing¶
Seven methods of Account — store, resolve, clear, the link, the mail and the two lookups — none
of them executed by anything, on the credential path with the widest audience on any installation:
the forgot-password form can be submitted by anybody, from any page, for any address.
Twenty-eight tests, on both backends, because storeResetToken() uses upsert() — which compiles
to ON DUPLICATE KEY UPDATE on MySQL and ON CONFLICT … DO UPDATE on PostgreSQL. One live token
per account was a claim about two different statements, and only one had ever run.
Two things I had backwards, and both are now written down¶
Resolving a token does not spend it. I asserted single use on consumeResetToken() and it
failed — correctly. The flow clears the token after updatePassword(), and that is the right
design: burning it when the form is merely opened would lose the only link of anybody who
mistypes their confirmation. The single-use property is real and lives one level up, so the test
now pins the order — cleared after the write, never before, since the reverse leaves an
account with neither a password change nor a live link.
A language catalogue defines $lang; it does not return an array.
$lang['Password reset'] = 'Αλλαγή κωδικού'; // right
return ['Password reset' => '…']; // loads, defines nothing, load() answers false
My fixture used the second form, so Language::using('el', …) switched nothing and the test
passed on an assertion about currentlang() that was itself wrong — using() swaps the
strings and only restores the language name in its finally, so currentlang() reports the
previous one while the callback runs. Two wrong things agreeing. The test now asserts what a
reader of the mail would notice: the translated words.
Worth the note in the guide, because the failure mode of a return-style catalogue is that
nothing raises: every string renders as itself, which looks like a site written in English
rather than a catalogue that was never loaded.
And one decision worth having a test¶
The mail goes out in the recipient's language, not the request's. That form can be submitted
by anybody — a Greek visitor asking for an English speaker's reset — and the person who reads the
mail is the account holder. composeAndSendResetEmail() is protected now so the decision is
observable at all; private, it was a documented intention with nothing checking it.
Suite: 13,313 → 13,341 tests, wall clock 2:21.
An export that aborted whole, and a test I should not have written¶
buildExportData() reads eleven tables, several belonging to features an installation may not
have enabled — and composed as a single array literal, one unreadable table aborted the whole
export. Somebody exercising a data-subject request got an error, and the ten sections that were
perfectly readable went with it. For a document an installation is legally obliged to produce,
partial is worth more than nothing.
Each section is resolved on its own now. Two details of the recovery are deliberate: the section stays present and empty, because a missing key reads as this framework has no such concept while an empty one reads as you have none of it — and only the second is something an installation is entitled to say; and the failure is logged, because in the file those two are indistinguishable, so they have to be distinguishable to the operator, who is the only person who can act.
Found while writing tests under the strict mode the fixtures now set, which is the point of
having turned it on: in lenient mode each helper's if ($result) quietly produced an empty
section and nothing aborted, so the defect only exists where failures are loud — and only shows
up in production as an export that errors for some installations and not others.
And the part worth admitting¶
I wrote sixteen tests for that export before noticing AccountExportTest already existed,
covering the same four properties — sections populated, secrets excluded, the extensibility hook,
a missing table degrading. Pure duplication, deleted. The remaining file covers what nothing did:
revokesession(), which is POST-only with the anti-CSRF token and scoped to the caller's own
account — a session id is a string in a form, and an unscoped update would let anybody sign
anybody out.
The duplication also produced a genuine finding, so it was not wasted:
Two tests, one framework table, two shapes¶
AccountExportTest builds sessions with its own columns. UsersControllerTest builds
tokenactions and usertokens with theirs. Both use CREATE TABLE IF NOT EXISTS, so whichever
runs first decides the shape, and the loser fails on an insert naming a column the winner never
created. In isolation both pass; together one fails, and which one depends on the filter you
happened to use.
Now in the Testing guide as three rules, in order of preference: do not create a table you do not
read (my own fix — the revocation test needs sessions and nothing else); drop before you create
when you do need one; and build from the real migration rather than a hand-written CREATE TABLE,
because two hand-rolled shapes drift from each other as surely as each drifts from production.
Two flakes that are not this¶
Chasing the above turned up two pre-existing timing flakes, one run in three:
UsersControllerTest::testTheTokenActionPanelFindsARealAction and
PageCacheTest::testAnAbandonedLockExpires. Neither is caused by this work — the second is a
lock-expiry test asserting on wall-clock behaviour — and both are named here rather than guessed
at, as the next thing worth fixing.
Suite: 13,341 → 13,353 tests, wall clock 2:23.
FW-051: a property declared, cleared, and never assigned¶
Request::$originalRequestNoChange has existed since the class was written. resetInstance()
clears it. Nothing ever assigned it, so every read answered ''.
Its own docblock states the contract it was not keeping — the original $_GET request that
should never change — as against $originalRequest, which calcParams() is free to rewrite.
Assigned now where r is read, before calcParams() can touch anything, because "as it arrived"
is precisely what must not reflect a re-route. Three tests: the value is kept, it survives a
re-route that rewrites $originalRequest, and resetInstance() clears it so one request does not
answer for the next. All three fail without the assignment.
Why an empty string was the worst possible wrong answer¶
Because it is a plausible one. An application building a page's canonical URL from it gets sURL
alone, compares that with the real address, finds them different, and redirects the page to
itself:
if (sURL . Request::$originalRequestNoChange != $model->getFriendlyUrl()) {
$this->redirect($model->getFriendlyUrl(), true, 301);
}
An infinite loop that curl -L abandons at ten hops. No exception, no log line, and the code
doing it reads correctly. The reporter's own account of finding it is the part worth keeping:
"two wrong assumptions before the right one, both because I was measuring the effect instead of
the cause." The effect was a redirect loop, and it was attributed first to the $_GET leftovers
fixed earlier today — reasonably, since those were real and had just been documented as a
plausible mechanism. Only a probe that ran both implementations over the same input, showed the
decomposition now agreeing, and then backtraced the redirect found the line above.
Which is the argument for the characterization table added with FW-049: once the decomposition was written down and pinned, it could be ruled out, and the search moved on.
Suite: 13,353 → 13,356 tests, wall clock 2:18.
The gate on the database browser, and a property read before its guard¶
/adminer serves a database browser with the connection supplied from configuration, so
reaching the page is reaching the database. There is no second credential to stop a mistake in
the gate — and 167 of the route's 235 statements had never been executed.
Nine tests drive the real thing rather than a stub: the signed-in account through
RequestIdentity and $_SESSION, because getCurrentUser() and staticIsLogged() are
independent questions and mayOpen() needs yes to each; the environment through APP_DEBUG; the
feature through FeatureRegistry. Every clause asserted on its own, including the one that reads
as belt-and-braces and is not — in development, below root, the floor only applies if the
devpanel feature is actually enabled, because a floor configured for a panel the installation
does not have is a number with nothing behind it.
Two findings came out of writing them.
A property read before the guard that protects it¶
$user = User::getCurrentUser();
$usertype = (int) ($user->usertype ?? 0); // false->usertype, before the check below
if ($user === null || !Session::staticIsLogged()) {
return false;
}
getCurrentUser() answers false for an anonymous visitor, not null. So the usertype line
raised attempt to read property on bool before the guard ran — harmless in the result, since it
lands on 0 and the visitor is refused, and a warning in the log of the one route where a log entry
is the only visible trace of somebody trying the door. Guard first, and !is_object() rather than
=== null, since false was never covered by that comparison either.
FeatureRegistry::loadFromConfig() only adds¶
The test asserting "refused without the DevPanel" reported the gate opening. The gate was
right; the test was wrong, and so was my assumption: loadFromConfig([]) is a no-op, not "no
features". It enables what you pass and never disables anything — invisible in production, where
it runs once at boot, and in a suite it means whatever an earlier test enabled is still enabled.
reset() exists for exactly this and says so in its docblock. Now in the Testing guide, with the
half that is easy to forget: restore what the suite runs with afterwards, because a test that
leaves the registry empty makes the next test's isEnabled('auth') answer false, and that test
fails for a reason that has nothing to do with it.
Suite: 13,356 → 13,365 tests, wall clock 2:15. Total coverage 90.49% → 90.63%.
Security: the log endpoint would read any file on the disk¶
Auth\Controllers\ApiAdmin — the SPA administration endpoints — had 48 statements and none of
them had ever been executed. Writing tests for it found three defects in one method, and the
first is a vulnerability:
GET /admin/logs?file=../../../../etc/passwd
→ 200 {"lines":["root:x:0:0:root:/root:/bin/bash", …],"total":18}
The class's own docblock says why that must not happen — "The file is chosen from the viewer's whitelist, never from the raw parameter: a log endpoint that accepts a path is a file-disclosure endpoint" — and it did it anyway. The reason is one condition:
An empty whitelist skipped the check entirely. "Nothing is configured" was read as "everything
is allowed" — the fail-open shape. Every caller in the framework passes a whitelist except
ApiAdmin::logs(), which constructs new LogViewer(), so that endpoint was the one place the
short-circuit was reachable. Authenticated and permission-checked, so this is privilege escalation
rather than an open door: a grant of may read the log file was a grant of may read any file this
process can.
Two fixes, and the first is independent of the second on purpose:
- A filename is a name, never a path — refused if it contains a separator or
.., before the whitelist and even when a caller passes$checkWhitelist = false, because that argument asks to skip a list, not to be handed a path. - No whitelist means the log directory's contents, plus the two names that map outside it. A viewer constructed with no arguments is asking to read this installation's logs, not any file on the disk.
A test was protecting it¶
LogViewerTest::testSetFileAcceptsAnyFilenameWithEmptyWhitelist asserted the fail-open behaviour,
and its own docblock says what it was: "This covers the !empty($this->whitelist)
short-circuit". A test written to execute a line, which ended up defending what the line did.
Nothing in it claims an empty whitelist should allow every file; it only recorded that it did.
Worth stating as a rule, because this loop has now hit it twice: a characterization test earns its place by describing behaviour, and the description is not an argument. When one blocks a fix, read what it says it covers — if the answer is "this branch", the branch is the thing under discussion, not the test.
And the two ordinary bugs beside it¶
The default file name was wrong. 'pramnosframework' without the extension, against a
whitelist of 'pramnosframework.log' and a path builder that appends the name unchanged — so the
endpoint threw on its own default parameter.
A log that has never been written was a 500. The try covered setFile() — the "may I read
this?" question — while processFile() throws for a file that does not exist, which is the normal
state of a fresh installation. Uncaught, the panel answered 500 for the endpoint somebody opens
precisely because they are trying to find out what went wrong. An empty page now, in the shape the
reader already handles.
Twenty tests, on both backends: 401 and 403 distinctly from every action, each action authorised
under its own name (search is not users — an omnibox reaches whatever the application
registered), the omnibox limit capped here rather than taken from the request, four traversal
shapes refused, and a missing dashboard table counted as null rather than 0, because "no
sessions" and "no sessions table" are different things and only one deserves a number.
Suite: 13,365 → 13,386 tests. Total coverage 90.63% → measured next round.