fix(dns): login_surface judges the host it fetches, under an approved-login-host policy (#42, #43) - #54
Conversation
|
Important Review skippedBot user detected. To trigger a single review, invoke the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
3bd0ffe to
52f6117
Compare
…-login-host policy Closes #43. Closes #42. #43: login_surface prefix-tested the PROBE host and then fetched the FINAL URL of the redirect chain, so the guard and its consumer asked about different values. In one direction a credential surface reached by redirect was never looked at; in the other a project homepage was body-checked for login markers. The host judged is now the host of the URL about to be fetched, read once by host_of_url() through the same parser the walk used to build the --resolve pin. The probe host survives only to describe a finding, never to decide one. #42: with no policy, login_surface reported every login form it found, so the first legitimate cPanel service in this estate (33 reseller accounts, so: when, not if) would have turned the daily check permanently red — and a gate that is red every day for a known-good reason is a gate nobody reads. A login form on an approved host is the service working; on an unapproved host it is the finding. approved-login-hosts.txt lists EXACT hostnames, refuses a wildcard or pattern with exit 2 rather than guessing at it, ignores a `suffix` line loudly so the list can only come out stricter than intended, and ships empty — nothing answers on a cPanel name today, and approving an unverified name would pre-excuse the credential surface this check exists to find. An approved host serving no login form is not a finding either: decided, not defaulted, and recorded in the file. The body fetch additionally refuses a pin that belongs to a different host (case-insensitively: a Location header may capitalise it). Verification: the suite goes 90 -> 113 controls, all green. Both directions of #43 have a control; the policy's three cases have a control each, with the approved/unapproved pair differing in nothing but the list. Two reversions of the fix ship in the suite and their kills are asserted by name with both verdicts printed — mutant A (the decision alone) dies on the direction-one control, mutant B (the pre-fix code verbatim) on direction two — and it is asserted that mutant A is masked on direction two by the pin assertion, so that comment cannot quietly become false. Ten further mutants were run by hand; nine died at exactly the control written for them, and the tenth (removing host_of_url's https check) is recorded as a non-kill, because url_host_port already refuses those URLs — the line is labelled a contract assertion rather than a defence. Estate equivalence: the real 138-hostname sweep was replayed hermetically through the shipped script (scripted curl and dig) against the pre-change script from git — identical hostname rows, same verdicts, `hostnames: 138`, `approved login hosts: 0`. Unaffected rows are byte-identical; the only changes are the new header line, the finding row's explanation, and the summary text. The live answer is the `edge` job, which runs on merge and daily. The workflow now also re-runs when approved-login-hosts.txt changes. #41 is untouched and remains deferred: its replacement entries are data (the Pages project hostnames and the account's workers.dev subdomain) that this environment cannot obtain, and shipping it without them would report legitimate Pages/Workers targets every day — the permanent-red failure #42 exists to prevent. See docs/plan-2026-09-25-detector-issues.adoc. Co-authored-by: arena-agent <297053741+arena-agent@users.noreply.github.com>
1119880 to
f305598
Compare
CI is dark on this branch, and it is not this changeEvery workflow on this head is What was measured, in order:
So the blackout is not caused by the diff: with the workflow file byte-identical to Consequences for review:
|
Closes #43. Closes #42. Leaves #41 deferred, with the reason and the missing input written down.
What was wrong
login_surface()prefix-tested the probe host and then fetched the final URL of the redirect chain — a guard asking a different question than its consumer. One direction never looked at a credential surface reached by redirect; the other body-checked a project homepage for login markers.And with no policy, every login form it found was a finding — so the first legitimate cPanel service in this estate (33 reseller accounts, so: when, not if) would have turned the daily check permanently red. A gate that is red every day for a known-good reason is a gate nobody reads, which is how the September incident survived six months.
What changed
host_of_url()that goes through the sameurl_host_portparser the walk used to build the--resolvepin. The probe host now only describes a finding; it never decides one.approved-login-hosts.txt(besideallowed-origins.txt): a login form on an approved host is the service working; on an unapproved host it is the finding. Exact hostnames only — a wildcard or pattern is refused with exit 2 rather than interpreted, asuffixline is ignored loudly so the list can only come out stricter than intended, and an absent file approves nothing.Why it should be believed
113 controls, was 90, all green in CI on this head. Both directions of #43 have a control; the policy has one per case, with the approved/unapproved pair differing in nothing but the list, so neither can pass for the wrong reason.
Two reversions of the fix ship in the suite, and the kills are asserted by name with both verdicts printed:
$probeinstead of the fetched host)rc=0 state=finding calls=3rc=1 state=none calls=2rc=1 state=none calls=2rc=0 state=finding calls=3Mutant A is invisible to control 35, because the pin assertion refuses the fetch before the wrong host is reached — a second guard holding the same invariant, not a hole. That is asserted (control 48) rather than explained away in a comment that can quietly become false. The mutant files are asserted to differ from the shipped code and to parse, because a mutation that matched nothing would make every kill vacuous.
Ten further mutants run by hand; nine died at exactly the control written for them (suffix-approval, absence-as-finding, approvals ignored, pattern accepted, missing pin, pin/host mismatch, loader validation removed, absent-file-approves-everything, emptied prefix list). The tenth is recorded as a non-kill: removing
host_of_url's https check kills nothing, becauseurl_host_portalready refuses those URLs. It is documented as a contract assertion rather than a defence, so no reader is told it guards something it does not.Estate equivalence, replayed hermetically: the real 138-hostname sweep was run through the shipped script with a scripted
curlanddig, against the pre-change script taken fromgit— identical hostname rows, identical verdicts,hostnames: 138,approved login hosts: 0. The only differences are the new header line, the finding row's explanation, and the summary text. The intended changes appear exactly where they should: a chainwww. -> webmail.reportsat webmail.…, reached by redirectwhere the old code saidok, and an approved host reportsok (approved login host …)where the old code reported a finding.Not measured here: the live sweep. This environment has no network and no Cloudflare credential, so equivalence is argued from the hermetic replay plus the fact that the change is inert unless a login-prefixed hostname answers or a chain lands on one. The live answer is the
edgejob, which runs on merge and daily — the job whose colour changes if that reasoning is wrong.#41 is not in this PR, deliberately
Its acceptance criterion needs data, not code: the exact
<project>.pages.devhostnames and the account'sworkers.devsubdomain.github.ionarrows to an org-namespaced suffix andworkers.devto an account-scoped one, butpages.devproject names carry no ownership marker, so without the inventory the audit would report legitimate Pages and Workers targets as findings every day — the permanent-red failure this PR's policy half exists to prevent. The plan, the interim design and the two missing data items are indocs/plan-2026-09-25-detector-issues.adoc.🤖 Generated with Claude Code