docs(security): correct two lockout ceilings the code does not implement (BACKLOG #1131) - #361
Open
wshallwshall wants to merge 7 commits into
Open
docs(security): correct two lockout ceilings the code does not implement (BACKLOG #1131)#361wshallwshall wants to merge 7 commits into
wshallwshall wants to merge 7 commits into
Conversation
…s unbounded (BACKLOG #1131)
Two sentences in docs/SECURITY.md asserted a ceiling the shipped code does not implement, and
one test's name asserted the same thing while its body checked something weaker.
WHAT WAS FALSE
:1544 (anti-automation table, row 1) said the lockout is "auto-expiring, so an attacker cannot
maliciously lock an account indefinitely". Each individual lock does auto-expire, but
_register_failure (auth/service.py:749-769) restarts the counter on a lapsed window (:757) and
re-locks on the next run to the threshold (:761-765). The account row persists a failure count
and an expiry, never a count of locks, so nothing accumulates across cycles and the number of
cycles has no ceiling. The claim was false in the direction that matters: a reader acting on it
would conclude no operator action is ever needed.
:59 said "a single-admin deployment can't be locked out". In context that sentence is about
AUTO-RETIREMENT, but its words denied the failed-attempt lockout, which is a different mechanism
and does reach a claimed sole administrator. It is also the sentence a reader greps. Two sites,
one false claim, one axis.
WHAT REPLACES THEM
A bounded account rather than a bare denial: the auto-expiry, the shipped thresholds, the serial
re-lock, the signal the holder gets, the two recovery routes and their cost to the victim, and
the sole-administrator residual. Deleting the false clause would have left the section quieter
and no more informative.
Scope-checked rather than asserted: only local accounts can be locked (the counter has exactly
two feeders, auth/service.py:672 and :2092, both local-gated); control 6 narrows who can reach
the account but is inert behind an undeclared proxy or NAT, per its own honest-limit note at
:906-908, so it is not described as preventing the campaign; and the routes that clear a live
lock both issue a new password rather than merely lifting the lock.
MEANING CHANGE ON ANCHORED LINES -- READ THIS BEFORE TRUSTING A GREEN GATE
docs/SECURITY.md:1544 and :59 carry ASVS evidence anchors whose expect text is matched as a
SUBSTRING. A meaning change on such a line leaves the anchor resolving and every gate in this
repo silent. This commit changes the MEANING of both lines, not just their position. The
covering cells must be re-read by hand; that is the assessor lane's call, and the cell ids are
deliberately not named here because this repo is public and the scorecard is not.
The retired anchor text "an attacker cannot maliciously lock an account indefinitely" is deleted
by this commit. It was certifying a gap, so it should be retired rather than re-pointed at the
corrected sentence -- re-pointing would make the record cite the remediation as evidence of the
defect.
THE TEST
test_malicious_lockout_clause_is_discharged_in_the_doc asserted in its NAME and docstring that
"an attacker cannot lock an account out indefinitely", while its body only required the section
to contain "auto-expiring" OR "lapsed window restarts". The second token is the phrase a truthful
correction KEEPS, so the test would have stayed green through this fix while its name went on
asserting what the fix disproves. It is renamed to
test_lockout_auto_expires_but_re_locking_is_unbounded and now derives what it checks:
- Store.record_login_failure's keyword-only parameters, by signature, so the absence of a lock
counter is checked structurally. Deliberately not a grep for a SQL literal: that write spells
the column as a bind parameter and clears the lock when passed None, so a spelling-based guard
would be blind to an unlock built on it -- the same instrument failure this item is about.
- _register_failure's reads off the user row, by AST, so a cross-cycle ceiling landing later reds
the test and forces the prose to be re-derived.
- The two call sites that feed the counter, by AST, which is what makes the local-only scope
falsifiable.
- Positively, in the row's own "Threshold / window" cell, that both halves are stated; and the
thresholds as rendered forms with units rather than bare integers, since "15 min" contains both
"5" and "15" and a bare-substring check on those is close to a tautology.
Proved red-capable rather than assumed: reverting the retired clause, weakening the unbounded
claim, and removing the local scope each turn the test red, with the unmodified tree green.
NOT IN THIS COMMIT
The ledger reference correction for #1131 (the item cites docs/SECURITY.md:1474 for a sentence
that lives at :1544) is a docs/BACKLOG.md edit and the ledger is single-writer.
auth/service.py:579-581's docstring carries the same over-strong claim this commit removes from
:59 ("this can't lock out a legitimate single-admin deployment"). It is the source the doc
sentence was written from, so the defect can regenerate from it. Left alone because the scope of
this change was set deliberately at two documentation sites; flagged for follow-up.
The sole-administrator dead end is an availability defect in a shipped control, not a
documentation gap, and is being filed separately so that fixing the prose does not close it.
wshallwshall
enabled auto-merge (squash)
August 13, 2026 02:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A truth repair, not a verdict move. BACKLOG #1131 stays OPEN and the ASVS cell stays
partial-- this does not close it.Two sentences in
docs/SECURITY.mdasserted a ceiling the code does not implement::1544-- the per-account lockout is "auto-expiring, so an attacker cannot maliciously lock an account indefinitely":59-- "a single-admin deployment can't be locked out"Each lock does auto-expire; repetition is unbounded.
:59is the one that matters most for a reader: it flatly denies the thing #1131 is about and sits 1,485 lines before:1544, so it is the sentence a grep lands on first.+139 / -7 across
docs/SECURITY.mdandtests/test_security_doc_rate_limits.py.Verification, stated as what was scanned rather than as a bare count
One gap, stated rather than hidden: a full pytest run was still executing when a usage warning fired, and no full-suite baseline was taken before editing. If CI comes back red, do not attribute it to this commit without re-running at
03fe5d99first. The two modules that actually read the changed document are green and baseline-matched.The thing no gate in this repo can see
Both changed lines carry ASVS evidence anchors whose
expecttext is matched as a SUBSTRING. This commit changes their meaning, not their position -- so the anchors still resolve, every gate stays green, and the reasoning that cites them goes stale. The covering cells need a human re-read.Those cell ids were sent to the ASVS dispatcher and assessor lane by session message and deliberately not written here: this repo is public, the scorecard is not, and a path-to-cell mapping published one commit at a time is the leak that assembles by diligence. This PR names the lines and the fact that a meaning change landed; that split is the standing rule for both lanes.
Two cells must move together rather than in sequence. The second is a PASS anchored to the section heading, which this commit preserves verbatim, and its re-score trigger is worded for a row gained, lost or re-thresholded -- which a row rewrite is not. Sequencing them leaves a window where a PASS certifies effectiveness against a table that now says the control can be weaponised.
Provenance
Authored by ASVS Lane A (Builder 1), routed here for push and PR. Verified before pushing: tip
4dc6d4c6as stated,merge-treerc=0 read with no pipe, and a leak scan of all 139 added lines clean.The #1131 claim is still held by the authoring lane and releases when this is accepted, not before.