From 3dd87e6506af5e4fbf23c487c89cc7b231ec4ec4 Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:07:08 +0600 Subject: [PATCH 01/19] fix(score): normalise each layer's budget by its own gate count A flat 100-point budget divided among a variable number of gates meant seven maxed gates zeroed a layer whether it had seven gates or thirty. seo is about to grow from 16 gates to 24, which would have cut the fraction of the layer needed to floor it from 44% to 29% -- the v0.2.1 saturation bug returning in slower form. The same constant also put a floor of 70 under the 2-gate security layer. One maxed gate now costs exactly 1/N of its layer and a layer floors only when every one of its gates is maxed. GATE_CAP is unchanged and still does its job. --- scripts/py/omnirank/registry.py | 35 +++++++++ scripts/py/omnirank/report.py | 43 ++++++++++- tests/test_report.py | 129 ++++++++++++++++++++++---------- 3 files changed, 165 insertions(+), 42 deletions(-) diff --git a/scripts/py/omnirank/registry.py b/scripts/py/omnirank/registry.py index cbb94b1..01f5816 100644 --- a/scripts/py/omnirank/registry.py +++ b/scripts/py/omnirank/registry.py @@ -140,6 +140,41 @@ def _r(id_: str, tier: FixTier, severity: str, layer: str, gate: str, MECHANICAL_IDS: frozenset[str] = frozenset( entry.id for entry in _ENTRIES if entry.tier == "mechanical") +# Severities that can move a layer's score. `info` findings are inventory facts +# (see ERROR_COST/WARNING_COST in report.py, where info costs 0), so a gate whose +# every id is `info` can never contribute cost -- counting it in the denominator +# would put a floor under the layer's score that it could never cross. +_SCORING_SEVERITIES: frozenset[str] = frozenset({"error", "warning"}) + +# The scoring SURFACE of each layer: how many distinct gates could move its score. +# `Report.score()` divides the layer's capped cost by GATE_CAP * len(this), so one +# maxed gate always costs 1/N of its layer and a layer floors only when every one +# of its gates is maxed. Before v0.4.0 the budget was a flat 100 per layer, which +# meant seven maxed gates zeroed a layer whether it had seven gates or thirty -- +# so every gate added made saturation cheaper, and v0.4.0 adds eight to `seo`. +# Unreachable gates are excluded: a gate that can never fire must not inflate +# every score by sitting in the denominator. +SCORING_GATES_BY_LAYER: dict[str, frozenset[str]] = { + layer: frozenset( + entry.gate for entry in _ENTRIES + if entry.layer == layer and entry.reachable + and entry.severity in _SCORING_SEVERITIES + ) + for layer in sorted({entry.layer for entry in _ENTRIES}) +} + + +def scoring_gate_count(layer: str) -> int: + """How many gates could move `layer`'s score; 0 for an unknown layer. + + 0 is a legitimate answer, not an error: `offsite` and `smm` are declared in + the Layer enum and ship no gates yet. `Report.score()` floors the surface at + the number of gates it actually saw, so a zero here can never divide by zero + and can never make a real finding free. + """ + return len(SCORING_GATES_BY_LAYER.get(layer, frozenset())) + + # Surfaces where a wrong write de-indexes a business. Hard-capped at `unsafe` # regardless of tier or locator confidence, and never writable -- they can only # ever arrive as a reviewed change. Enumerated in diff --git a/scripts/py/omnirank/report.py b/scripts/py/omnirank/report.py index 2f00c6e..4677b43 100644 --- a/scripts/py/omnirank/report.py +++ b/scripts/py/omnirank/report.py @@ -9,7 +9,7 @@ from . import __version__ from .applicability import Applicability -from .registry import FixTier, tier_for +from .registry import FixTier, scoring_gate_count, tier_for Severity = Literal["error", "warning", "info"] Layer = Literal["seo", "aeo", "geo", "offsite", "smm", "perf"] @@ -134,11 +134,38 @@ def flag_not_evaluated(self, entry: NotEvaluated) -> None: self.not_evaluated.append(entry) def score(self) -> dict[str, int]: - """Per-layer score, with each gate's cost capped before summing (GATE_CAP). + """Per-layer score: capped per gate, then normalised by the layer's surface. + + Two stages, and they answer different questions. + + GATE_CAP (v0.2.1) answers "how much can ONE gate cost?" -- one gate failing + on every page of a site is ONE problem to fix, not fifty. + + The surface divisor (v0.4.0) answers "how much is one gate WORTH?" -- and + the answer has to be 1/N of the layer, not a fixed 15 out of 100. Under the + flat budget, seven maxed gates zeroed a layer whether that layer had seven + gates or thirty, so every gate added made saturation cheaper: at `seo`'s + pre-0.4.0 count of 16 it took 44% of the layer to floor it, and at 24 it + would have taken 29%. The same constant also put an unreachable FLOOR under + small layers -- `security` ships 2 scoring gates, so its worst possible + score under the flat budget was 70. Both symptoms are the same defect: a + constant budget divided among a variable number of gates. + + `surface` is floored at the number of gates actually seen, so a finding on + an unregistered gate (tests, or a gate added to code before the registry) + can never cost more than the layer has room for, and can never divide by + zero. It counts REGISTERED gates rather than gates that ran, which biases + the score upward for an audit where some gates could not run -- the + conservative direction, and `notEvaluated` is where that is reported. Grouped by (layer, gate) rather than just gate: two different layers could in principle share a gate id, and each layer's cap must apply independently to its own cost, not be shared across layers. + + The penalty is integer round-half-up, never float or `round()`: `round()` + is banker's rounding, and float division would make the result depend on + IEEE 754 detail. Both are monotone, so `min`, the sum and this division + together guarantee that adding a finding can never RAISE a score. """ raw_per_gate: dict[tuple[str, str], int] = defaultdict(int) for f in self.findings: @@ -148,10 +175,18 @@ def score(self) -> dict[str, int]: raw_per_gate[(f.layer, f.gate)] += cost costs: dict[str, int] = {layer: 0 for layer in self.layers_run} - for (layer, _gate), raw in raw_per_gate.items(): + seen_gates: dict[str, set[str]] = defaultdict(set) + for (layer, gate), raw in raw_per_gate.items(): costs[layer] = costs.get(layer, 0) + min(GATE_CAP, raw) + seen_gates[layer].add(gate) + + scores: dict[str, int] = {} + for layer, cost in costs.items(): + surface = max(1, scoring_gate_count(layer), len(seen_gates[layer])) + denominator = GATE_CAP * surface + penalty = (100 * cost + denominator // 2) // denominator + scores[layer] = max(0, 100 - penalty) - scores = {layer: max(0, 100 - cost) for layer, cost in costs.items()} overall = sum(scores.values()) // len(scores) if scores else 100 return {**scores, "overall": overall} diff --git a/tests/test_report.py b/tests/test_report.py index d53a80e..e37ab6c 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -4,6 +4,7 @@ import pytest from jsonschema import Draft202012Validator, FormatChecker +from omnirank.registry import SCORING_GATES_BY_LAYER, scoring_gate_count from omnirank.report import Finding, NotEvaluated, Report ROOT = Path(__file__).resolve().parents[1] @@ -23,38 +24,12 @@ def test_empty_report_scores_100_overall(): assert r.score()["overall"] == 100 -def test_error_costs_ten_points(): - r = Report(site="https://x.example", kind="audit") - r.add(f()) - assert r.score()["seo"] == 90 - - -def test_warning_costs_three_points(): - r = Report(site="https://x.example", kind="audit") - r.add(f(severity="warning")) - assert r.score()["seo"] == 97 - - -def test_score_floors_at_zero(): - # v0.2.1: under the old flat-per-finding model, 20 findings from the SAME gate - # were enough to floor a layer (20*10=200). Under the capped model that same - # gate now maxes out at GATE_CAP (15) no matter how many times it fires, so - # flooring requires enough DISTINCT broken gates for their capped costs to sum - # past 100: ceil(100/15) = 7 gates, each firing twice (10*2=20, capped to 15). - r = Report(site="https://x.example", kind="audit") - for gate_n in range(7): - for i in range(2): - r.add(f(url=f"https://x.example/{gate_n}-{i}", - id=f"seo.issue-{gate_n}.broken", gate=f"gate-{gate_n}")) - assert r.score()["seo"] == 0 - - def test_overall_is_mean_of_layers_that_ran(): r = Report(site="https://x.example", kind="audit") - r.add(f()) # seo -> 90 - r.add(f(layer="aeo", id="aeo.faq.missing", gate="faq")) # aeo -> 90 + r.add(f(layer="smm", id="smm.alpha.broken", gate="alpha")) # -> 33 + r.add(f(layer="offsite", id="offsite.beta.broken", gate="beta")) # -> 33 s = r.score() - assert s["overall"] == 90 + assert s["overall"] == 33 assert "geo" not in s @@ -180,7 +155,7 @@ def test_clean_layer_that_ran_scores_100(): r.layers_run.update({"seo", "aeo", "geo"}) r.add(f()) # one seo error s = r.score() - assert s["seo"] == 90 + assert s["seo"] < 100, "a real finding must cost something" assert s["aeo"] == 100, "a layer that ran with no findings scored 100" assert s["geo"] == 100 @@ -212,14 +187,6 @@ def test_layer_that_did_not_run_is_absent(): # systemic issue (one gate firing on every URL of a real site) cannot alone zero # the layer and drown out every other signal. --- -def test_one_gate_firing_fifty_seven_times_does_not_zero_its_layer(): - r = Report(site="https://x.example", kind="audit") - for i in range(57): - r.add(f(url=f"https://x.example/{i}")) # all id=seo.h1.multiple, gate=h1 - assert r.score()["seo"] > 0 - assert r.score()["seo"] == 85, "100 - GATE_CAP(15), regardless of the 57 URLs" - - def test_two_distinct_gates_cost_more_than_one_gate_firing_twice_as_often(): one_gate_twice = Report(site="https://x.example", kind="audit") one_gate_twice.add(f(url="https://x.example/a")) @@ -285,3 +252,89 @@ def test_a_report_without_not_evaluated_key_still_validates(): del d["notEvaluated"] validator = Draft202012Validator(SCHEMA, format_checker=FormatChecker()) assert list(validator.iter_errors(d)) == [] + + +# --- v0.4.0: the layer budget is normalised by the layer's own gate count, so +# adding gates can never make saturation cheaper. `smm` and `offsite` have no +# registered gates, so their surface is exactly the number of gates the test +# itself creates -- an exact, registry-independent denominator that stays +# correct as seo grows from 16 gates to 24. --- + +def test_an_error_costs_ten_raw_points_normalised_by_a_one_gate_surface(): + # One gate seen, none registered -> surface 1, denominator 15*1 = 15. + # penalty = (100*10 + 7) // 15 = 1007 // 15 = 67. + r = Report(site="https://x.example", kind="audit") + r.add(f(layer="smm", id="smm.alpha.broken", gate="alpha")) + assert r.score()["smm"] == 33 + + +def test_a_warning_costs_three_raw_points_normalised_by_a_one_gate_surface(): + # penalty = (100*3 + 7) // 15 = 307 // 15 = 20. + r = Report(site="https://x.example", kind="audit") + r.add(f(layer="smm", id="smm.alpha.broken", gate="alpha", severity="warning")) + assert r.score()["smm"] == 80 + + +def test_info_severity_never_moves_the_score(): + r = Report(site="https://x.example", kind="audit") + r.layers_run.add("smm") + r.add(f(layer="smm", id="smm.alpha.noted", gate="alpha", severity="info")) + assert r.score()["smm"] == 100 + + +def test_one_maxed_gate_costs_exactly_one_gates_worth_of_its_layer(): + # Two gates seen -> surface 2, denominator 30. Gate `alpha` is maxed + # (2 errors = 20 raw, capped to 15); `beta` contributes nothing. + # penalty = (100*15 + 15) // 30 = 1515 // 30 = 50. + r = Report(site="https://x.example", kind="audit") + for i in range(2): + r.add(f(layer="smm", id="smm.alpha.broken", gate="alpha", + url=f"https://x.example/{i}")) + r.add(f(layer="smm", id="smm.beta.noted", gate="beta", severity="info")) + assert r.score()["smm"] == 50 + + +def test_zeroing_a_layer_requires_every_one_of_its_registered_gates(): + """The whole point of v0.4.0's score change, asserted against the live registry. + + Reads the gate list from the registry rather than hardcoding a count, so it + stays true as Tasks 3-10 add gates instead of needing an edit per task. + """ + gates = sorted(SCORING_GATES_BY_LAYER["seo"]) + assert len(gates) >= 16, "sanity: the seo layer has gates to enumerate" + + r = Report(site="https://x.example", kind="audit") + for n, gate in enumerate(gates[:-1]): # every gate but the last, maxed + for i in range(2): # 2 errors = 20 raw -> capped 15 + r.add(f(id=f"seo.{gate}.x", gate=gate, url=f"https://x.example/{n}-{i}")) + assert r.score()["seo"] > 0, ( + "one clean gate must keep the layer off the floor, however many gates exist") + + last = gates[-1] + for i in range(2): + r.add(f(id=f"seo.{last}.x", gate=last, url=f"https://x.example/last-{i}")) + assert r.score()["seo"] == 0, "a layer floors only when every gate is maxed" + + +def test_one_gate_firing_fifty_seven_times_costs_the_same_as_firing_twice(): + # GATE_CAP, preserved verbatim from v0.2.1: repetition of the SAME gate stops + # compounding. Asserted as an equality against the 2-firing case rather than a + # magic number, so it survives the seo surface growing from 16 to 24. + many = Report(site="https://x.example", kind="audit") + for i in range(57): + many.add(f(url=f"https://x.example/{i}")) # all id=seo.h1.multiple, gate=h1 + + twice = Report(site="https://x.example", kind="audit") + for i in range(2): + twice.add(f(url=f"https://x.example/{i}")) + + assert many.score()["seo"] == twice.score()["seo"] + assert many.score()["seo"] > 0 + + +def test_scoring_gate_count_excludes_unreachable_and_info_only_gates(): + assert "crawl-hygiene" not in SCORING_GATES_BY_LAYER["seo"], ( + "crawl-hygiene is reachable=False; a gate that can never fire must not " + "sit in the denominator inflating every score") + assert scoring_gate_count("seo") == len(SCORING_GATES_BY_LAYER["seo"]) + assert scoring_gate_count("not-a-layer") == 0 From 1eca379aeb0ee0a3f5dd106da1c8cc4802cf42e9 Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:08:36 +0600 Subject: [PATCH 02/19] feat(report): add the security layer and two notEvaluated reasons Additive on both axes: an older report contains neither the security layer nor the new reasons and still validates. tests/test_registry.py's LAYERS filter is widened here, before any security.* id exists, so the id scan sees Group A's literals the moment they land instead of reporting them as unemitted. --- schemas/report.schema.json | 6 ++++-- scripts/py/omnirank/report.py | 15 +++++++++++++-- tests/test_registry.py | 2 +- tests/test_report.py | 36 +++++++++++++++++++++++++++++++++++ 4 files changed, 54 insertions(+), 5 deletions(-) diff --git a/schemas/report.schema.json b/schemas/report.schema.json index 93af14a..abf37b9 100644 --- a/schemas/report.schema.json +++ b/schemas/report.schema.json @@ -42,7 +42,7 @@ "properties": { "id": { "type": "string", "pattern": "^[a-z0-9-]+\\.[a-z0-9-]+\\.[a-z0-9-]+$" }, "severity": { "enum": ["error", "warning", "info"] }, - "layer": { "enum": ["seo", "aeo", "geo", "offsite", "smm", "perf"] }, + "layer": { "enum": ["seo", "aeo", "geo", "offsite", "smm", "perf", "security"] }, "url": { "type": "string" }, "gate": { "type": "string" }, "observed": { "type": "string" }, @@ -74,7 +74,9 @@ "properties": { "gate": { "type": "string" }, "reason": { - "enum": ["no-sitemap", "page-unreachable", "not-applicable", "adapter-absent"] + "enum": ["no-sitemap", "page-unreachable", "not-applicable", + "adapter-absent", "matcher-unsupported", "budget-exceeded"], + "description": "Why the gate could not run. Additive: matcher-unsupported and budget-exceeded arrive in 0.4.0, so a report written earlier contains neither and still validates. matcher-unsupported means the check needed a stdlib capability this interpreter lacks (urllib.robotparser is RFC 9309 compliant only on Python 3.14+); OmniRank refuses to judge rather than answer from a matcher it knows is wrong." }, "url": { "type": "string", "description": "Set for a per-page gate that could not run." }, "site": { "type": "string", "description": "Set for a site-level gate that could not run." } diff --git a/scripts/py/omnirank/report.py b/scripts/py/omnirank/report.py index 4677b43..cfeec73 100644 --- a/scripts/py/omnirank/report.py +++ b/scripts/py/omnirank/report.py @@ -12,7 +12,7 @@ from .registry import FixTier, scoring_gate_count, tier_for Severity = Literal["error", "warning", "info"] -Layer = Literal["seo", "aeo", "geo", "offsite", "smm", "perf"] +Layer = Literal["seo", "aeo", "geo", "offsite", "smm", "perf", "security"] # Closed enum, mirrored in schemas/report.schema.json. "no-sitemap" and # "page-unreachable" are populated starting v0.2.1 (see audit.py). "not-applicable" @@ -21,8 +21,19 @@ # (an SMM platform with no adapter yet, a check that only applies to some stacks) # has a reason to report from day one instead of staying silent until someone # remembers to extend the enum. +# +# v0.4.0 adds two, both additive -- a report written before this release contains +# neither value and still validates: +# matcher-unsupported the check needs a capability this interpreter's stdlib +# does not have. urllib.robotparser only became RFC 9309 +# compliant (wildcards, longest-match) in Python 3.14; on +# 3.11-3.13 it silently gives the WRONG answer for such +# rules, so gates/contradictions.py refuses to judge rather +# than fabricate. See omnirank/robots.py. +# budget-exceeded a bounded probe pass hit its cap before reaching this URL. NotEvaluatedReason = Literal[ - "no-sitemap", "page-unreachable", "not-applicable", "adapter-absent" + "no-sitemap", "page-unreachable", "not-applicable", "adapter-absent", + "matcher-unsupported", "budget-exceeded", ] ERROR_COST = 10 diff --git a/tests/test_registry.py b/tests/test_registry.py index edcf40d..4fa53a0 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -19,7 +19,7 @@ ROOT = Path(__file__).resolve().parents[1] SOURCE_DIR = ROOT / "scripts" / "py" / "omnirank" -LAYERS = ("seo", "aeo", "geo", "offsite", "smm", "perf") +LAYERS = ("seo", "aeo", "geo", "offsite", "smm", "perf", "security") ID_LITERAL = re.compile(r'"([a-z0-9-]+\.[a-z0-9-]+\.[a-z0-9-]+)"') # gates/geo.py builds these five ids by f-string from the artifact filename diff --git a/tests/test_report.py b/tests/test_report.py index e37ab6c..7562ae4 100644 --- a/tests/test_report.py +++ b/tests/test_report.py @@ -338,3 +338,39 @@ def test_scoring_gate_count_excludes_unreachable_and_info_only_gates(): "sit in the denominator inflating every score") assert scoring_gate_count("seo") == len(SCORING_GATES_BY_LAYER["seo"]) assert scoring_gate_count("not-a-layer") == 0 + + +# --- v0.4.0: the security layer and the two new notEvaluated reasons --- + +def test_a_security_layer_finding_validates_against_the_report_schema(): + r = Report(site="https://x.example", kind="audit") + r.layers_run.add("security") + r.add(Finding(id="security.mixed-content.subresource", severity="error", + layer="security", url="https://x.example/", gate="mixed-content", + observed="1 http:// subresource", expected="every subresource over https", + fix="Serve it over https.")) + errors = list(Draft202012Validator( + SCHEMA, format_checker=FormatChecker()).iter_errors(r.to_dict())) + assert errors == [], errors + + +def test_the_security_layer_is_scored_only_when_it_ran(): + r = Report(site="https://x.example", kind="audit") + r.layers_run.update({"seo"}) + assert "security" not in r.score(), ( + "a site that was never security-checked must not score 100 on security") + + +def test_the_new_not_evaluated_reasons_validate(): + r = Report(site="https://x.example", kind="audit") + r.flag_not_evaluated(NotEvaluated(gate="robots-sitemap", + url="https://x.example/robots.txt", + reason="matcher-unsupported")) + r.flag_not_evaluated(NotEvaluated(gate="canonical-target", + url="https://x.example/z", + reason="budget-exceeded")) + errors = list(Draft202012Validator( + SCHEMA, format_checker=FormatChecker()).iter_errors(r.to_dict())) + assert errors == [], errors + assert {e.reason for e in r.not_evaluated} == {"matcher-unsupported", + "budget-exceeded"} From ff543a7ea8c02561dba17e82fae9c03706e1a3ff Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:12:54 +0600 Subject: [PATCH 03/19] feat(security): add the security layer's four response-header gates HSTS presence and max-age, X-Content-Type-Options, Content-Security-Policy absence, Referrer-Policy. All info severity, so all cost zero points and none can fail a build: the gap analysis puts header grading out of scope by identity, and the honest position is to report them as inventory facts rather than grade them. CSP absence is reported; CSP contents deliberately are not. HSTS_MIN_MAX_AGE is named in the finding text because it is OmniRank's own floor, not a standard, and no claim is made about any browser preload programme. Also updates the two registry counter tests (48->53 total, infrastructure 6->11) incrementally rather than leaving the suite red until Task 12, per the brief's own fallback for one-commit-at-a-time execution. Reworded the short-max-age fix text to drop the literal substring "preload list": the brief's own test asserted that phrase must be absent while its own verbatim implementation text contained it (as part of an honest disclaimer), so the two were mutually unsatisfiable as written. The disclaimer's substance is preserved without the flagged phrase. --- schemas/omnirank.config.schema.json | 3 +- scripts/py/omnirank/audit.py | 15 ++- scripts/py/omnirank/gates/__init__.py | 6 ++ scripts/py/omnirank/gates/security.py | 130 ++++++++++++++++++++++++++ scripts/py/omnirank/registry.py | 9 ++ tests/test_audit.py | 11 ++- tests/test_config_schema.py | 9 ++ tests/test_gates_security.py | 99 ++++++++++++++++++++ tests/test_registry.py | 9 +- 9 files changed, 281 insertions(+), 10 deletions(-) create mode 100644 scripts/py/omnirank/gates/security.py create mode 100644 tests/test_gates_security.py diff --git a/schemas/omnirank.config.schema.json b/schemas/omnirank.config.schema.json index dc7c815..5766437 100644 --- a/schemas/omnirank.config.schema.json +++ b/schemas/omnirank.config.schema.json @@ -140,7 +140,8 @@ "lastmod-inflation", "schema", "schema-fabrication", "duplicate-title", "duplicate-description", "noindex-in-sitemap", "canonical-cluster", "hreflang-reciprocity", - "response-time", "page-weight", "compression", "render-blocking"] + "response-time", "page-weight", "compression", "render-blocking", + "hsts", "nosniff", "csp", "referrer-policy"] } } } diff --git a/scripts/py/omnirank/audit.py b/scripts/py/omnirank/audit.py index f2e918b..0ba1f44 100644 --- a/scripts/py/omnirank/audit.py +++ b/scripts/py/omnirank/audit.py @@ -5,16 +5,16 @@ from .bands import resolve_band from .config import Config from .fetch import fetch, make_client, read_sitemap -from .gates import aeo, geo, hygiene, jsonld, perf, seo, site +from .gates import aeo, geo, hygiene, jsonld, perf, security, seo, site from .page import PageData from .report import Finding, NotEvaluated, Report # The per-page gate modules whose findings all carry layer="seo" (seo.py AND -# jsonld.py -- schema findings are seo.* too), plus aeo and perf. When a page -# cannot be fetched, none of these ran for it, and each is recorded as its own +# jsonld.py -- schema findings are seo.* too), plus aeo, perf and security. When a +# page cannot be fetched, none of these ran for it, and each is recorded as its own # notEvaluated entry rather than merged into one -- a caller filtering # notEvaluated by gate (e.g. "did aeo run for this URL?") needs them distinct. -PER_PAGE_GATES = ("seo", "aeo", "perf") +PER_PAGE_GATES = ("seo", "aeo", "perf", "security") def default_config(url: str) -> Config: @@ -92,7 +92,11 @@ def audit_site(config: Config, client: httpx.Client | None = None, pages = _collect(client, targets, report) if pages: - report.layers_run.update({"aeo", "perf"}) + # security joins aeo and perf here for the same reason: it is a per-page + # gate set, and a site where zero pages parsed must not be scored 100 on + # a layer that never ran. Task 4 widens this when the site-level + # https-redirect probe lands. + report.layers_run.update({"aeo", "perf", "security"}) if urls is None and sitemap_urls: # hygiene.check_sitemap() distinguishes a redirecting sitemap entry @@ -113,6 +117,7 @@ def audit_site(config: Config, client: httpx.Client | None = None, resolve_band(page.lang, config))) report.extend(jsonld.run(page.html, page.url)) report.extend(perf.run(page)) + report.extend(security.run_page(page)) report.extend(site.run(pages, sitemap_urls)) diff --git a/scripts/py/omnirank/gates/__init__.py b/scripts/py/omnirank/gates/__init__.py index d53429f..8a4b486 100644 --- a/scripts/py/omnirank/gates/__init__.py +++ b/scripts/py/omnirank/gates/__init__.py @@ -26,4 +26,10 @@ `hygiene` has no `run`. It exposes three functions instead, each returning `list[Finding]`: `check_removed(client, urls)`, `check_sitemap(client, site_url, sample)` and `check_lastmod(sitemap_xml, site_url)`. + +`security` exposes `run_page(page: PageData) -> list[Finding]` for the per-response +header and markup gates, plus `check_https_redirect(client: httpx.Client, site_url: +str) -> tuple[list[Finding], list[NotEvaluated]]` for the one site-level probe. It +returns notEvaluated entries alongside findings rather than mutating a Report, +keeping the gate testable without one. """ diff --git a/scripts/py/omnirank/gates/security.py b/scripts/py/omnirank/gates/security.py new file mode 100644 index 0000000..0dc6610 --- /dev/null +++ b/scripts/py/omnirank/gates/security.py @@ -0,0 +1,130 @@ +"""Response-header and markup security signals. + +Scope is deliberately narrow. `docs/research/2026-08-04-competitive-gap-analysis.md` +SS4 draws the line: OmniRank checks security only where insecurity demonstrably +breaks crawling, indexing or rendering -- Mozilla Observatory and testssl.sh grade +headers properly and free, and an SEO tool that scores CSP strength is doing a job +it cannot do well. So the four header gates here are `info`: they cost 0 points +(see ERROR_COST/WARNING_COST in report.py), never fail a build, and are reported as +inventory facts. Only the two gates in this module that DO break something -- +mixed content, which browsers block, and a missing http->https redirect, which +splits canonicalisation -- carry error severity. + +CSP is parsed for exactly one purpose beyond noting its total absence: +`upgrade-insecure-requests`, which suppresses the mixed-content finding. This +module never grades a policy. +""" +from __future__ import annotations + +import re + +from ..page import PageData +from ..report import Finding + +# OmniRank's own floor for Strict-Transport-Security max-age: 180 days in seconds. +# Named here, and named in the finding text, because it is a CHOICE and not a +# vendor requirement -- Chromium's HSTS preload submission form asks for a longer +# max-age than this, and OmniRank deliberately claims nothing whatsoever about the +# preload list, which it cannot observe. +HSTS_MIN_MAX_AGE = 15_552_000 + +_MAX_AGE = re.compile(r"\bmax-age\s*=\s*\"?(\d+)", re.IGNORECASE) + + +def _f(id_: str, gate: str, url: str, severity: str, observed: str, + expected: str, fix: str) -> Finding: + return Finding(id=id_, severity=severity, layer="security", url=url, gate=gate, + observed=observed, expected=expected, fix=fix) + + +def _is_https(url: str) -> bool: + return url.lower().startswith("https://") + + +def csp_value(page: PageData) -> str: + """The page's CSP from the header and/or a meta http-equiv, lowercased. + + Both delivery mechanisms are real and a page may use either; treating only the + header as authoritative would report a meta-delivered policy as absent. + """ + header = page.headers.get("content-security-policy", "") or "" + meta = page.soup().find("meta", attrs={"http-equiv": lambda v: ( + v is not None and v.strip().lower() == "content-security-policy")}) + meta_content = (meta.get("content") or "") if meta else "" + return f"{header} {meta_content}".strip().lower() + + +def _hsts(page: PageData) -> list[Finding]: + if not _is_https(page.url): + # A browser ignores Strict-Transport-Security on a plain-http response + # entirely, so reporting its absence there would be advice with no effect. + return [] + value = page.headers.get("strict-transport-security", "") + if not value.strip(): + return [_f("security.hsts.missing", "hsts", page.url, "info", + "no Strict-Transport-Security header on an https response", + "a Strict-Transport-Security header", + "Send Strict-Transport-Security from the origin or CDN so browsers " + "refuse to downgrade to http. This is reported as a fact, not a " + "defect: OmniRank grades security headers only where their absence " + "breaks crawling, indexing or rendering, and this one does not.")] + match = _MAX_AGE.search(value) + if match is None: + # No max-age directive at all is malformed, not SHORT. Reporting a number + # here would mean inventing one. + return [] + seconds = int(match.group(1)) + if seconds >= HSTS_MIN_MAX_AGE: + return [] + return [_f("security.hsts.short-max-age", "hsts", page.url, "info", + f"Strict-Transport-Security max-age={seconds}", + f"max-age of at least {HSTS_MIN_MAX_AGE} seconds", + f"Raise max-age to at least {HSTS_MIN_MAX_AGE}. That figure is " + "HSTS_MIN_MAX_AGE in gates/security.py -- OmniRank's own floor " + "(180 days), not an industry standard and not a vendor requirement. " + "OmniRank makes no claim about any browser's HSTS preload " + "programme, which it cannot observe.")] + + +def _nosniff(page: PageData) -> list[Finding]: + value = (page.headers.get("x-content-type-options", "") or "").strip().lower() + if value == "nosniff": + return [] + return [_f("security.nosniff.missing", "nosniff", page.url, "info", + f"X-Content-Type-Options is {value!r}" if value + else "no X-Content-Type-Options header", + "X-Content-Type-Options: nosniff", + "Send X-Content-Type-Options: nosniff so browsers honour the declared " + "Content-Type. Reported as a fact: this is not an SEO or AEO signal " + "and OmniRank does not grade it.")] + + +def _csp(page: PageData) -> list[Finding]: + if csp_value(page): + return [] + return [_f("security.csp.absent", "csp", page.url, "info", + "no Content-Security-Policy, by header or meta http-equiv", + "a Content-Security-Policy", + "Consider adding a Content-Security-Policy. OmniRank reports only " + "total absence and deliberately does not assess a policy's contents: " + "whether a given CSP is adequate is a judgement about your threat " + "model, not something an SEO auditor can determine.")] + + +def _referrer_policy(page: PageData) -> list[Finding]: + if (page.headers.get("referrer-policy", "") or "").strip(): + return [] + return [_f("security.referrer-policy.missing", "referrer-policy", page.url, "info", + "no Referrer-Policy header", + "a Referrer-Policy header", + "Consider sending a Referrer-Policy. Reported as a fact: its absence " + "affects neither crawling nor indexing, and OmniRank does not grade " + "which policy value you should choose.")] + + +def run_page(page: PageData) -> list[Finding]: + """Security signals derivable from one already-fetched response.""" + findings: list[Finding] = [] + for check in (_hsts, _nosniff, _csp, _referrer_policy): + findings.extend(check(page)) + return findings diff --git a/scripts/py/omnirank/registry.py b/scripts/py/omnirank/registry.py index 01f5816..e39dbc0 100644 --- a/scripts/py/omnirank/registry.py +++ b/scripts/py/omnirank/registry.py @@ -133,6 +133,15 @@ def _r(id_: str, tier: FixTier, severity: str, layer: str, gate: str, # --- audit.py --------------------------------------------------------- _r("seo.page.unreachable", "advisory", "error", "seo", "sitemap-health"), _r("seo.sitemap.missing", "templated", "error", "seo", "sitemap-health"), + # --- gates/security.py (v0.4.0) --------------------------------------- + # All five are `infrastructure`: the fix is an origin/CDN response-header + # change, and no edit to any file in the user's repo produces it. + _r("security.hsts.missing", "infrastructure", "info", "security", "hsts"), + _r("security.hsts.short-max-age", "infrastructure", "info", "security", "hsts"), + _r("security.nosniff.missing", "infrastructure", "info", "security", "nosniff"), + _r("security.csp.absent", "infrastructure", "info", "security", "csp"), + _r("security.referrer-policy.missing", "infrastructure", "info", "security", + "referrer-policy"), ) REGISTRY: dict[str, RegisteredFinding] = {entry.id: entry for entry in _ENTRIES} diff --git a/tests/test_audit.py b/tests/test_audit.py index 517d29e..ac4b016 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -34,7 +34,14 @@ def mock_site(page_status=200): # by their declared content-encoding and errors on a mismatched body. respx.get(f"{SITE}/").mock(return_value=httpx.Response( page_status, content=gzip.compress(PAGE.encode()), - headers={"content-encoding": "gzip"})) + headers={"content-encoding": "gzip", + # v0.4.0: the clean fixture must be clean on the security layer + # too, or every assertion of `report.findings == []` breaks on + # four info findings. + "strict-transport-security": "max-age=31536000", + "x-content-type-options": "nosniff", + "content-security-policy": "default-src 'self'", + "referrer-policy": "strict-origin-when-cross-origin"})) respx.get(f"{SITE}/llms.txt").mock( return_value=httpx.Response(200, text="# X\n## How to cite us\nCC BY 4.0.")) respx.get(f"{SITE}/llms-full.txt").mock(return_value=httpx.Response(200, text="full")) @@ -140,7 +147,7 @@ def test_unreachable_page_flags_its_per_page_gates_as_not_evaluated(): report = audit_site(default_config(SITE), make_client()) unreachable_url = f"{SITE}/" gates = {e.gate for e in report.not_evaluated if e.url == unreachable_url} - assert gates == {"seo", "aeo", "perf"} + assert gates == {"seo", "aeo", "perf", "security"} assert all(e.reason == "page-unreachable" for e in report.not_evaluated if e.url == unreachable_url) diff --git a/tests/test_config_schema.py b/tests/test_config_schema.py index 077250d..5583b5a 100644 --- a/tests/test_config_schema.py +++ b/tests/test_config_schema.py @@ -136,3 +136,12 @@ def test_the_original_framework_values_still_validate(): cfg = minimal() cfg["stack"] = {"framework": name} assert list(validator().iter_errors(cfg)) == [], name + + +SECURITY_GATES = ["hsts", "nosniff", "csp", "referrer-policy"] + + +def test_security_gates_are_accepted_in_fail_on(): + cfg = minimal() + cfg["audit"] = {"failOn": SECURITY_GATES} + assert list(validator().iter_errors(cfg)) == [] diff --git a/tests/test_gates_security.py b/tests/test_gates_security.py new file mode 100644 index 0000000..54632e9 --- /dev/null +++ b/tests/test_gates_security.py @@ -0,0 +1,99 @@ +from omnirank.gates import security +from omnirank.gates.security import HSTS_MIN_MAX_AGE +from omnirank.page import PageData + +CLEAN_HEADERS = { + "strict-transport-security": "max-age=31536000; includeSubDomains", + "x-content-type-options": "nosniff", + "content-security-policy": "default-src 'self'", + "referrer-policy": "strict-origin-when-cross-origin", +} + +BODY = "T" + + +def page(headers=None, url="https://x.example/", html=BODY) -> PageData: + return PageData(url=url, html=html, status=200, elapsed_ms=5, + headers=dict(CLEAN_HEADERS if headers is None else headers)) + + +def ids(findings) -> set[str]: + return {f.id for f in findings} + + +def test_a_fully_headed_https_page_emits_nothing(): + assert security.run_page(page()) == [] + + +def test_missing_hsts_on_https_is_info(): + found = [f for f in security.run_page(page(headers={})) if f.id == "security.hsts.missing"] + assert len(found) == 1 + assert found[0].severity == "info", ( + "a missing HSTS header breaks no crawl; it is reported as a fact, not graded") + assert found[0].layer == "security" + assert found[0].gate == "hsts" + + +def test_hsts_is_not_evaluated_on_a_plain_http_page(): + # HSTS is meaningless over http -- browsers ignore the header entirely. + found = security.run_page(page(headers={}, url="http://x.example/")) + assert "security.hsts.missing" not in ids(found) + assert "security.hsts.short-max-age" not in ids(found) + + +def test_a_short_hsts_max_age_is_flagged_and_names_the_constant(): + headers = dict(CLEAN_HEADERS, **{"strict-transport-security": "max-age=86400"}) + found = [f for f in security.run_page(page(headers=headers)) + if f.id == "security.hsts.short-max-age"] + assert len(found) == 1 + assert str(HSTS_MIN_MAX_AGE) in found[0].expected + assert "HSTS_MIN_MAX_AGE" in found[0].fix, ( + "the threshold is OmniRank's own choice and the finding must name the " + "constant rather than imply an industry standard") + assert "preload list" not in found[0].fix.lower(), ( + "OmniRank must claim nothing about the Chromium preload list") + + +def test_an_exactly_threshold_max_age_is_not_flagged(): + headers = dict(CLEAN_HEADERS, + **{"strict-transport-security": f"max-age={HSTS_MIN_MAX_AGE}"}) + assert "security.hsts.short-max-age" not in ids(security.run_page(page(headers=headers))) + + +def test_an_unparseable_hsts_max_age_is_not_guessed_at(): + headers = dict(CLEAN_HEADERS, **{"strict-transport-security": "includeSubDomains"}) + found = ids(security.run_page(page(headers=headers))) + assert "security.hsts.short-max-age" not in found, ( + "no max-age directive is not a SHORT max-age; do not invent a number") + assert "security.hsts.missing" not in found, "the header is present" + + +def test_hsts_max_age_is_matched_case_insensitively(): + headers = dict(CLEAN_HEADERS, **{"strict-transport-security": "Max-Age=31536000"}) + assert "security.hsts.short-max-age" not in ids(security.run_page(page(headers=headers))) + + +def test_missing_nosniff_csp_and_referrer_policy_are_each_info(): + found = {f.id: f for f in security.run_page(page(headers={}))} + for finding_id, gate in (("security.nosniff.missing", "nosniff"), + ("security.csp.absent", "csp"), + ("security.referrer-policy.missing", "referrer-policy")): + assert finding_id in found, finding_id + assert found[finding_id].severity == "info", finding_id + assert found[finding_id].gate == gate, finding_id + + +def test_a_csp_delivered_by_meta_tag_counts_as_present(): + html = ("" + "" + "") + headers = {k: v for k, v in CLEAN_HEADERS.items() if k != "content-security-policy"} + assert "security.csp.absent" not in ids(security.run_page(page(headers=headers, + html=html))) + + +def test_csp_absence_is_reported_without_grading_the_policy(): + found = [f for f in security.run_page(page(headers={})) if f.id == "security.csp.absent"] + assert "no Content-Security-Policy" in found[0].observed + assert "weak" not in found[0].fix.lower(), ( + "grading a policy's strength is a judgement OmniRank must not make") diff --git a/tests/test_registry.py b/tests/test_registry.py index 4fa53a0..b27f099 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -58,7 +58,10 @@ def test_every_registered_id_is_emitted_by_some_gate(): def test_registry_covers_all_forty_eight_findings(): - assert len(REGISTRY) == 48 + # v0.4.0 Task 3: +5 for gates/security.py's four response-header gates + # (hsts fires two distinct ids). Task 12 retitles/rebases this once the + # full v0.4.0 gate set has landed. + assert len(REGISTRY) == 53 def test_tier_distribution_matches_the_fixability_classification(): @@ -66,8 +69,10 @@ def test_tier_distribution_matches_the_fixability_classification(): ("mechanical", "templated", "drafted", "advisory", "infrastructure")} for entry in REGISTRY.values(): counts[entry.tier] += 1 + # v0.4.0 Task 3: +5 infrastructure (all five security header gates are + # infrastructure -- the fix is an origin/CDN response-header change). assert counts == {"mechanical": 4, "templated": 15, "drafted": 12, - "advisory": 11, "infrastructure": 6} + "advisory": 11, "infrastructure": 11} def test_mechanical_tier_is_exactly_the_four_documented_ids(): From c5db52c9080fdb678bd5536c80de0c8d540ae542 Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:19:08 +0600 Subject: [PATCH 04/19] feat(security): flag mixed content and a missing http->https redirect The two security gates that carry error severity, because both break something observable: browsers block mixed active content outright, and an http:// origin serving 200 gives every page a live duplicate that splits its canonical signals. Mixed content is suppressed by upgrade-insecure-requests in the CSP -- the one purpose this tool parses CSP for -- and skips rels that are not subresources, so an http canonical is not misreported as a blocked asset. The http probe returns notEvaluated when the origin refuses the connection, because an unreachable origin has not passed this gate. Two brief/test fixes found during TDD: the mixed-content finding's example list was capped at 3 items, so the fixture exercising all four recognised subresource types (script/link/img/iframe) always dropped the last one behind the ellipsis -- raised the cap to 5. Also updated the two registry counter tests again (53->55, templated 15->16, infrastructure 11->12) for this task's two new ids, per the same one-commit-at-a-time fallback used in Task 3. --- schemas/omnirank.config.schema.json | 3 +- scripts/py/omnirank/audit.py | 18 +++-- scripts/py/omnirank/gates/security.py | 105 +++++++++++++++++++++++++- scripts/py/omnirank/registry.py | 8 ++ tests/test_audit.py | 20 +++++ tests/test_config_schema.py | 6 ++ tests/test_gates_security.py | 105 ++++++++++++++++++++++++++ tests/test_registry.py | 13 ++-- 8 files changed, 265 insertions(+), 13 deletions(-) diff --git a/schemas/omnirank.config.schema.json b/schemas/omnirank.config.schema.json index 5766437..5a0837c 100644 --- a/schemas/omnirank.config.schema.json +++ b/schemas/omnirank.config.schema.json @@ -141,7 +141,8 @@ "duplicate-title", "duplicate-description", "noindex-in-sitemap", "canonical-cluster", "hreflang-reciprocity", "response-time", "page-weight", "compression", "render-blocking", - "hsts", "nosniff", "csp", "referrer-policy"] + "hsts", "nosniff", "csp", "referrer-policy", + "mixed-content", "https-redirect"] } } } diff --git a/scripts/py/omnirank/audit.py b/scripts/py/omnirank/audit.py index 0ba1f44..272ea4d 100644 --- a/scripts/py/omnirank/audit.py +++ b/scripts/py/omnirank/audit.py @@ -92,11 +92,7 @@ def audit_site(config: Config, client: httpx.Client | None = None, pages = _collect(client, targets, report) if pages: - # security joins aeo and perf here for the same reason: it is a per-page - # gate set, and a site where zero pages parsed must not be scored 100 on - # a layer that never ran. Task 4 widens this when the site-level - # https-redirect probe lands. - report.layers_run.update({"aeo", "perf", "security"}) + report.layers_run.update({"aeo", "perf"}) if urls is None and sitemap_urls: # hygiene.check_sitemap() distinguishes a redirecting sitemap entry @@ -119,6 +115,18 @@ def audit_site(config: Config, client: httpx.Client | None = None, report.extend(perf.run(page)) report.extend(security.run_page(page)) + https_findings, https_not_evaluated = security.check_https_redirect( + client, config.site_url) + report.extend(https_findings) + for entry in https_not_evaluated: + report.flag_not_evaluated(entry) + if pages or not https_not_evaluated: + # `security` enters layers_run when at least one of its gates actually + # ran: the per-page header gates need a page, the site-level probe does + # not. An empty https_not_evaluated means the probe reached the origin + # and reached a verdict, so the layer ran even on a zero-page audit. + report.layers_run.add("security") + report.extend(site.run(pages, sitemap_urls)) report.urls_checked = len(targets) diff --git a/scripts/py/omnirank/gates/security.py b/scripts/py/omnirank/gates/security.py index 0dc6610..14beeef 100644 --- a/scripts/py/omnirank/gates/security.py +++ b/scripts/py/omnirank/gates/security.py @@ -18,8 +18,12 @@ import re +import httpx + +from ..fetch import fetch +from ..html import has_rel from ..page import PageData -from ..report import Finding +from ..report import Finding, NotEvaluated # OmniRank's own floor for Strict-Transport-Security max-age: 180 days in seconds. # Named here, and named in the finding text, because it is a CHOICE and not a @@ -122,9 +126,106 @@ def _referrer_policy(page: PageData) -> list[Finding]: "which policy value you should choose.")] +# The subresource-bearing (element, attribute) pairs OmniRank reads. `link` is +# filtered further by rel below: a is not +# a subresource -- the browser never fetches it while rendering -- and reporting it +# as mixed content would be a fabricated error on a judgement seo.canonical.* +# already owns. +_SUBRESOURCE_ATTRS: tuple[tuple[str, str], ...] = ( + ("script", "src"), ("link", "href"), ("img", "src"), ("iframe", "src"), +) + +# rel keywords that make a an actual subresource fetch. Matched one token +# at a time through html.has_rel, so rel="shortcut icon" matches "icon". +_SUBRESOURCE_LINK_RELS: frozenset[str] = frozenset({ + "stylesheet", "preload", "modulepreload", "prefetch", "icon", + "apple-touch-icon", "manifest", +}) + + +def _mixed_content(page: PageData) -> list[Finding]: + """http:// subresources declared by an https page. + + An error, unlike the four header gates: browsers BLOCK mixed active content, + so the page is measurably broken as served rather than merely unhardened. + + Suppressed when the page's CSP carries `upgrade-insecure-requests`, which makes + the browser rewrite these to https before requesting them -- reporting them + then would be a false positive, and it is the one thing SS4 of the gap analysis + says to parse CSP for. + + Only literal `http://` values count. A protocol-relative `//host/path` inherits + the page's own scheme and is therefore https here. + """ + if not _is_https(page.url): + return [] + if "upgrade-insecure-requests" in csp_value(page): + return [] + + soup = page.soup() + insecure: list[str] = [] + for tag_name, attr in _SUBRESOURCE_ATTRS: + for tag in soup.find_all(tag_name): + if tag_name == "link" and not any( + has_rel(tag, rel) for rel in _SUBRESOURCE_LINK_RELS): + continue + value = (tag.get(attr) or "").strip() + if value.lower().startswith("http://"): + insecure.append(f"<{tag_name} {attr}={value}>") + + if not insecure: + return [] + # Show up to 5 examples -- enough to cover every subresource TYPE this module + # recognises (script, link, img, iframe) on a page that mixes all of them, so + # the finding text never silently drops a whole category of offender behind + # the ellipsis; a longer real-world list still truncates for readability. + shown = ", ".join(insecure[:5]) + ("…" if len(insecure) > 5 else "") + return [_f("security.mixed-content.subresource", "mixed-content", page.url, + "error", + f"{len(insecure)} http:// subresources on an https page ({shown})", + "every subresource requested over https", + "Serve these over https, or add upgrade-insecure-requests to the " + "page's Content-Security-Policy. Browsers block mixed active content " + "outright, so these resources are not loading for your visitors at " + "all -- this is a rendering failure, not a hardening suggestion.")] + + +def check_https_redirect(client: httpx.Client, + site_url: str) -> tuple[list[Finding], list[NotEvaluated]]: + """Does the http:// form of the site redirect to https? + + One extra request per audit, against the site root only. Returns notEvaluated + entries alongside findings rather than a bare list: an http:// origin that + refuses the connection has not passed this gate, and silence would read as one. + """ + site_url = site_url.rstrip("/") + if not _is_https(site_url): + return [], [NotEvaluated(gate="https-redirect", site=site_url, + reason="not-applicable")] + + http_url = "http://" + site_url[len("https://"):] + "/" + result = fetch(client, http_url) + if result.status == 0: + return [], [NotEvaluated(gate="https-redirect", url=http_url, + reason="page-unreachable")] + + location = (result.headers.get("location", "") or "").strip() + if result.is_redirect and location.lower().startswith("https://"): + return [], [] + + observed = (f"HTTP {result.status} to {location}" if result.is_redirect + else f"HTTP {result.status} served directly over http") + return [_f("security.https-redirect.missing", "https-redirect", http_url, + "error", observed, "a 3xx redirect to the https:// URL", + "Redirect http:// to https:// at the origin or CDN. Serving both " + "schemes gives every page a live duplicate, splitting the canonical " + "and link signals between two URLs that engines must reconcile " + "themselves.")], [] + + def run_page(page: PageData) -> list[Finding]: """Security signals derivable from one already-fetched response.""" findings: list[Finding] = [] - for check in (_hsts, _nosniff, _csp, _referrer_policy): + for check in (_hsts, _nosniff, _csp, _referrer_policy, _mixed_content): findings.extend(check(page)) return findings diff --git a/scripts/py/omnirank/registry.py b/scripts/py/omnirank/registry.py index e39dbc0..632be8d 100644 --- a/scripts/py/omnirank/registry.py +++ b/scripts/py/omnirank/registry.py @@ -142,6 +142,14 @@ def _r(id_: str, tier: FixTier, severity: str, layer: str, gate: str, _r("security.csp.absent", "infrastructure", "info", "security", "csp"), _r("security.referrer-policy.missing", "infrastructure", "info", "security", "referrer-policy"), + # `templated`: the edit is a scheme rewrite in markup the repo owns, which is + # deterministic given repo facts. Whether the https origin actually serves the + # asset is a per-INSTANCE question, and applicability.py is where that demotes. + _r("security.mixed-content.subresource", "templated", "error", "security", + "mixed-content"), + # `infrastructure`: no source edit produces an origin-level redirect. + _r("security.https-redirect.missing", "infrastructure", "error", "security", + "https-redirect"), ) REGISTRY: dict[str, RegisteredFinding] = {entry.id: entry for entry in _ENTRIES} diff --git a/tests/test_audit.py b/tests/test_audit.py index ac4b016..012803f 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -48,6 +48,10 @@ def mock_site(page_status=200): respx.get(f"{SITE}/facts.json").mock(return_value=httpx.Response(200, text='{"a":1}')) respx.get(f"{SITE}/robots.txt").mock( return_value=httpx.Response(200, text="User-agent: *\nAllow: /\n")) + # v0.4.0: audit_site probes the http:// form once per audit. Unmocked, respx + # raises rather than returning, so every @respx.mock audit test needs this. + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) def test_default_config_needs_only_a_url(): @@ -105,6 +109,8 @@ def test_falls_back_to_root_when_sitemap_absent(): respx.get(f"{SITE}/llms-full.txt").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/facts.json").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(200, text="Allow: /")) + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) report = audit_site(default_config(SITE), make_client()) assert report.urls_checked == 1 @@ -119,6 +125,8 @@ def test_missing_sitemap_reports_a_finding_not_a_silent_fallback(): respx.get(f"{SITE}/llms-full.txt").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/facts.json").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(200, text="Allow: /")) + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) report = audit_site(default_config(SITE), make_client()) missing = [f for f in report.findings if f.id == "seo.sitemap.missing"] @@ -190,6 +198,8 @@ def test_a_healthy_sitemap_url_is_not_refetched_by_check_sitemap(): for art in ("llms.txt", "llms-full.txt", "facts.json"): respx.get(f"{SITE}/{art}").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(200, text="Allow: /")) + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) audit_site(default_config(SITE), make_client()) assert route.call_count == 1, ( @@ -230,6 +240,8 @@ def test_findings_are_ordered_by_target_position(): respx.get(f"{SITE}/{art}").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(200, text="Allow: /")) respx.get(f"{SITE}/sitemap.xml").mock(return_value=httpx.Response(404)) + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) report = audit_site(default_config(SITE), make_client(), urls=[f"{SITE}/a", f"{SITE}/b", f"{SITE}/c"]) @@ -258,6 +270,8 @@ def test_layers_that_never_ran_are_absent_when_every_target_404s(): respx.get(f"{SITE}/llms-full.txt").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/facts.json").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(404)) + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) report = audit_site(default_config(SITE), make_client()) score = report.score() @@ -288,6 +302,8 @@ def test_site_pass_sees_duplicates_across_urls(): respx.get(f"{SITE}/{art}").mock(return_value=httpx.Response(200, text=body)) respx.get(f"{SITE}/robots.txt").mock( return_value=httpx.Response(200, text="User-agent: *\nAllow: /\n")) + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) report = audit_site(default_config(SITE), make_client()) found = {f.id for f in report.findings} @@ -353,6 +369,8 @@ def test_duplicate_sitemap_entries_are_fetched_once(): for art in ("llms.txt", "llms-full.txt", "facts.json"): respx.get(f"{SITE}/{art}").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(200, text="Allow: /")) + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) report = audit_site(default_config(SITE), make_client()) assert route.call_count == 1, "a URL listed twice must not be fetched twice" @@ -375,6 +393,8 @@ def test_duplicate_sitemap_entries_do_not_inflate_duplicate_groups(): for art in ("llms.txt", "llms-full.txt", "facts.json"): respx.get(f"{SITE}/{art}").mock(return_value=httpx.Response(404)) respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(200, text="Allow: /")) + respx.get("http://x.example/").mock( + return_value=httpx.Response(301, headers={"location": f"{SITE}/"})) report = audit_site(default_config(SITE), make_client()) dup = [f for f in report.findings if f.id == "seo.duplicate-title.shared"] diff --git a/tests/test_config_schema.py b/tests/test_config_schema.py index 5583b5a..8d1a34a 100644 --- a/tests/test_config_schema.py +++ b/tests/test_config_schema.py @@ -145,3 +145,9 @@ def test_security_gates_are_accepted_in_fail_on(): cfg = minimal() cfg["audit"] = {"failOn": SECURITY_GATES} assert list(validator().iter_errors(cfg)) == [] + + +def test_the_two_scoring_security_gates_are_accepted_in_fail_on(): + cfg = minimal() + cfg["audit"] = {"failOn": ["mixed-content", "https-redirect"]} + assert list(validator().iter_errors(cfg)) == [] diff --git a/tests/test_gates_security.py b/tests/test_gates_security.py index 54632e9..79e5cad 100644 --- a/tests/test_gates_security.py +++ b/tests/test_gates_security.py @@ -1,3 +1,6 @@ +import httpx +import respx +from omnirank.fetch import make_client from omnirank.gates import security from omnirank.gates.security import HSTS_MIN_MAX_AGE from omnirank.page import PageData @@ -97,3 +100,105 @@ def test_csp_absence_is_reported_without_grading_the_policy(): assert "no Content-Security-Policy" in found[0].observed assert "weak" not in found[0].fix.lower(), ( "grading a policy's strength is a judgement OmniRank must not make") + + +MIXED = ("" + "" + "" + "" + "" + "" + "") + + +def test_http_subresources_on_an_https_page_are_an_error(): + found = [f for f in security.run_page(page(html=MIXED)) + if f.id == "security.mixed-content.subresource"] + assert len(found) == 1, "one finding per page, not one per subresource" + assert found[0].severity == "error", "browsers block this; the page is broken" + assert found[0].gate == "mixed-content" + assert "4" in found[0].observed + for fragment in ("script", "link", "img", "iframe"): + assert fragment in found[0].observed, fragment + + +def test_mixed_content_is_not_reported_on_a_plain_http_page(): + found = security.run_page(page(html=MIXED, url="http://x.example/")) + assert "security.mixed-content.subresource" not in ids(found) + + +def test_upgrade_insecure_requests_suppresses_mixed_content(): + headers = dict(CLEAN_HEADERS, + **{"content-security-policy": "upgrade-insecure-requests"}) + found = security.run_page(page(headers=headers, html=MIXED)) + assert "security.mixed-content.subresource" not in ids(found), ( + "the browser rewrites these to https before requesting them") + + +def test_a_non_subresource_http_link_is_not_mixed_content(): + # A canonical or an hreflang alternate is not a subresource: the browser never + # fetches it while rendering, so it is not mixed content. Flagging it would be + # a fabricated error, and seo.canonical.* already owns that judgement. + html = ("" + "" + "" + "x" + "") + assert "security.mixed-content.subresource" not in ids(security.run_page(page(html=html))) + + +def test_protocol_relative_subresources_are_not_mixed_content(): + # //cdn.example/a.js inherits the page's scheme, so on an https page it is https. + html = ("" + "") + assert "security.mixed-content.subresource" not in ids(security.run_page(page(html=html))) + + +@respx.mock +def test_http_that_redirects_to_https_emits_nothing(): + respx.get("http://x.example/").mock(return_value=httpx.Response( + 301, headers={"location": "https://x.example/"})) + findings, not_evaluated = security.check_https_redirect(make_client(), + "https://x.example") + assert findings == [] + assert not_evaluated == [] + + +@respx.mock +def test_http_serving_200_is_an_error(): + respx.get("http://x.example/").mock(return_value=httpx.Response(200, text="hi")) + findings, not_evaluated = security.check_https_redirect(make_client(), + "https://x.example") + assert not_evaluated == [] + assert [f.id for f in findings] == ["security.https-redirect.missing"] + assert findings[0].severity == "error" + assert findings[0].gate == "https-redirect" + assert findings[0].url == "http://x.example/" + + +@respx.mock +def test_http_redirecting_to_another_http_url_is_still_an_error(): + respx.get("http://x.example/").mock(return_value=httpx.Response( + 302, headers={"location": "http://x.example/home"})) + findings, _ = security.check_https_redirect(make_client(), "https://x.example") + assert [f.id for f in findings] == ["security.https-redirect.missing"] + assert "http://x.example/home" in findings[0].observed + + +@respx.mock +def test_an_http_url_that_cannot_be_reached_is_not_evaluated_rather_than_passed(): + respx.get("http://x.example/").mock(side_effect=httpx.ConnectError("refused")) + findings, not_evaluated = security.check_https_redirect(make_client(), + "https://x.example") + assert findings == [], "a gate that could not run must never emit a verdict" + assert [(e.gate, e.reason) for e in not_evaluated] == [ + ("https-redirect", "page-unreachable")] + + +def test_the_probe_is_not_applicable_to_an_http_site(): + findings, not_evaluated = security.check_https_redirect(make_client(), + "http://x.example") + assert findings == [] + assert [(e.gate, e.reason) for e in not_evaluated] == [ + ("https-redirect", "not-applicable")] + assert not_evaluated[0].site == "http://x.example" diff --git a/tests/test_registry.py b/tests/test_registry.py index b27f099..6d17bca 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -59,9 +59,10 @@ def test_every_registered_id_is_emitted_by_some_gate(): def test_registry_covers_all_forty_eight_findings(): # v0.4.0 Task 3: +5 for gates/security.py's four response-header gates - # (hsts fires two distinct ids). Task 12 retitles/rebases this once the - # full v0.4.0 gate set has landed. - assert len(REGISTRY) == 53 + # (hsts fires two distinct ids). Task 4: +2 for mixed-content and + # https-redirect. Task 12 retitles/rebases this once the full v0.4.0 gate + # set has landed. + assert len(REGISTRY) == 55 def test_tier_distribution_matches_the_fixability_classification(): @@ -71,8 +72,10 @@ def test_tier_distribution_matches_the_fixability_classification(): counts[entry.tier] += 1 # v0.4.0 Task 3: +5 infrastructure (all five security header gates are # infrastructure -- the fix is an origin/CDN response-header change). - assert counts == {"mechanical": 4, "templated": 15, "drafted": 12, - "advisory": 11, "infrastructure": 11} + # Task 4: +1 templated (mixed-content, a markup scheme rewrite) and +1 + # infrastructure (https-redirect, an origin-level redirect). + assert counts == {"mechanical": 4, "templated": 16, "drafted": 12, + "advisory": 11, "infrastructure": 12} def test_mechanical_tier_is_exactly_the_four_documented_ids(): From 1801f92c7f5dbaa05afb2427a57499b85a9a141d Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:28:56 +0600 Subject: [PATCH 05/19] feat(robots): probe the stdlib matcher's capability before trusting it urllib.robotparser only became RFC 9309 compliant in Python 3.14. On 3.11-3.13 -- the whole CI matrix -- it ignores path wildcards and matches rules in file order, so 'Disallow: /docs/' followed by 'Allow: /docs/public/' reports /docs/public/x as disallowed. That is a fabricated finding, not merely a missed one, and it would have passed locally on 3.14 exactly like the v0.3.0 read_text(newline=) bug. The stdlib stays the matcher. What is new is a runtime behaviour probe plus a structural scan of the published rules: evaluate when the two are compatible, refuse and let the caller report notEvaluated when they are not. --- scripts/py/omnirank/robots.py | 172 ++++++++++++++++++++++++++++++++++ tests/test_robots.py | 96 +++++++++++++++++++ 2 files changed, 268 insertions(+) create mode 100644 scripts/py/omnirank/robots.py create mode 100644 tests/test_robots.py diff --git a/scripts/py/omnirank/robots.py b/scripts/py/omnirank/robots.py new file mode 100644 index 0000000..bb5f4af --- /dev/null +++ b/scripts/py/omnirank/robots.py @@ -0,0 +1,172 @@ +"""robots.txt evaluation that refuses to answer when it knows it would be wrong. + +`urllib.robotparser` is the matcher here -- no robots grammar is reimplemented -- +but it cannot be trusted unconditionally, because CPython rewrote it for RFC 9309 +in **3.14** and OmniRank's floor is 3.11. On 3.11-3.13 the stdlib parser has no +wildcard support and matches rules in FILE ORDER rather than longest-first: + + robots.txt URL 3.13 3.14 + Disallow: /*.pdf$ /a/b.pdf allow disallow + Disallow: /docs/ then Allow: /docs/pub/ /docs/pub/x disallow allow + +The first is a silent miss. The second is a FABRICATED disallow -- a wrong finding, +which is worse than a missing one and is the exact failure mode this project +defines itself against. Both would ship green on a 3.14 workstation and diverge on +every CI leg. + +So: probe the interpreter's actual behaviour, inspect the rules the site actually +published, and evaluate only when the two are compatible. Otherwise return an +unevaluated verdict and let the caller record `notEvaluated`. The probes are +behaviour tests, not version checks, so a backport or a future rewrite is picked +up automatically. + +Deliberately conservative: `Allow: /` alongside any `Disallow:` counts as +overlapping, because for a first-match parser that pairing genuinely is ambiguous. +That means many real robots.txt files go unevaluated on Python 3.11-3.13 and are +fully evaluated on 3.14+. Reporting less on an older interpreter is the correct +trade; reporting a guess would not be. + +This module emits no Findings -- it returns data. That is load-bearing: +tests/test_registry.py scans gates/*.py and audit.py for finding-id literals, and +an id defined here would be invisible to that scan. +""" +from __future__ import annotations + +from collections.abc import Sequence +from dataclasses import dataclass +from functools import cache +from urllib.robotparser import RobotFileParser + +# The user-agent OmniRank evaluates rules for. Googlebot rather than "*": a group +# addressed to Googlebot is the one that governs whether the page can be indexed, +# and RobotFileParser already falls back to the wildcard group when no +# Googlebot-specific group exists. +DEFAULT_AGENT = "Googlebot" + +UNSUPPORTED_WILDCARDS = "wildcard" +UNSUPPORTED_LONGEST_MATCH = "longest-match" + +_PROBE_HOST = "https://probe.invalid" + + +@dataclass(frozen=True) +class RobotsVerdict: + """Which of `urls` robots.txt disallows -- or why that could not be decided. + + `disallowed` is always empty when `evaluated` is False. Carrying a partial + judgement alongside a refusal is how a "could not check" turns into a silent + pass two call sites later. + """ + + evaluated: bool + reason: str | None + disallowed: tuple[str, ...] + + +def _groups(text: str) -> list[tuple[list[str], list[str]]]: + """(allow_paths, disallow_paths) for each consecutive User-agent group. + + Structural and permissive on purpose: this is NOT a matcher (that is + RobotFileParser's job). It answers exactly one question -- do the rules in this + file need a capability the interpreter's matcher may not have. Consecutive + `User-agent:` lines share one group, per RFC 9309. + """ + groups: list[tuple[list[str], list[str]]] = [] + allows: list[str] = [] + disallows: list[str] = [] + in_agent_run = False + + for raw in text.splitlines(): + line = raw.split("#", 1)[0].strip() + if not line or ":" not in line: + continue + field, _, value = line.partition(":") + field = field.strip().lower() + value = value.strip() + + if field == "user-agent": + if not in_agent_run and (allows or disallows): + groups.append((allows, disallows)) + allows, disallows = [], [] + in_agent_run = True + continue + + in_agent_run = False + if field == "allow" and value: + allows.append(value) + elif field == "disallow" and value: + disallows.append(value) + + if allows or disallows: + groups.append((allows, disallows)) + return groups + + +def rules_need_wildcards(text: str) -> bool: + """True when any rule PATH uses `*` or a terminal `$`. + + The `*` on a `User-agent: *` line is a group selector, not a path pattern, and + `_groups` never collects it -- treating it as one would send every robots.txt + in existence down the unevaluated path. + """ + return any( + "*" in path or path.endswith("$") + for allows, disallows in _groups(text) + for path in (*allows, *disallows) + ) + + +def rules_need_longest_match(text: str) -> bool: + """True when some group has an Allow and a Disallow whose paths overlap. + + Prefix overlap in EITHER direction: `Allow: /` versus `Disallow: /a/` is the + common real-world case and it is genuinely ambiguous under a first-match + parser, which returns whichever line appears first. Disjoint paths give the + same answer under either matcher, so those files stay evaluable everywhere. + """ + for allows, disallows in _groups(text): + for allow in allows: + for disallow in disallows: + if allow.startswith(disallow) or disallow.startswith(allow): + return True + return False + + +@cache +def matcher_supports_wildcards() -> bool: + """Does THIS interpreter's urllib.robotparser honour `*` and `$` in a path?""" + parser = RobotFileParser() + parser.parse(["User-agent: *", "Disallow: /*.pdf$"]) + return not parser.can_fetch("*", f"{_PROBE_HOST}/a/b.pdf") + + +@cache +def matcher_supports_longest_match() -> bool: + """Does THIS interpreter pick the longest matching rule rather than the first? + + The Disallow is written FIRST on purpose: a first-match parser returns it and + answers "disallowed", while RFC 9309 says the longer Allow wins. + """ + parser = RobotFileParser() + parser.parse(["User-agent: *", "Disallow: /a/", "Allow: /a/b"]) + return parser.can_fetch("*", f"{_PROBE_HOST}/a/b") + + +def disallowed_urls(robots_text: str, urls: Sequence[str], + agent: str = DEFAULT_AGENT) -> RobotsVerdict: + """Which of `urls` this robots.txt disallows to `agent`. + + Refuses -- `evaluated=False`, empty `disallowed` -- when the published rules + need a matcher capability this interpreter does not have. + """ + if rules_need_wildcards(robots_text) and not matcher_supports_wildcards(): + return RobotsVerdict(evaluated=False, reason=UNSUPPORTED_WILDCARDS, + disallowed=()) + if rules_need_longest_match(robots_text) and not matcher_supports_longest_match(): + return RobotsVerdict(evaluated=False, reason=UNSUPPORTED_LONGEST_MATCH, + disallowed=()) + + parser = RobotFileParser() + parser.parse(robots_text.splitlines()) + blocked = tuple(url for url in urls if not parser.can_fetch(agent, url)) + return RobotsVerdict(evaluated=True, reason=None, disallowed=blocked) diff --git a/tests/test_robots.py b/tests/test_robots.py new file mode 100644 index 0000000..f1f50a4 --- /dev/null +++ b/tests/test_robots.py @@ -0,0 +1,96 @@ +import sys + +from omnirank import robots + +SIMPLE = "User-agent: *\nDisallow: /private/\n" +WILDCARD = "User-agent: *\nDisallow: /*.pdf$\n" +OVERLAPPING = "User-agent: *\nDisallow: /docs/\nAllow: /docs/public/\n" +DISJOINT = "User-agent: *\nDisallow: /admin/\nAllow: /blog/\n" + + +def test_a_file_with_no_wildcards_or_allows_is_always_evaluated(): + verdict = robots.disallowed_urls(SIMPLE, ["https://x.example/private/a", + "https://x.example/open"]) + assert verdict.evaluated is True + assert verdict.reason is None + assert verdict.disallowed == ("https://x.example/private/a",) + + +def test_rules_need_wildcards_detects_star_and_dollar(): + assert robots.rules_need_wildcards(WILDCARD) is True + assert robots.rules_need_wildcards("User-agent: *\nDisallow: /a/*/b\n") is True + assert robots.rules_need_wildcards(SIMPLE) is False + assert robots.rules_need_wildcards(OVERLAPPING) is False, ( + "the * on the User-agent line is a group selector, not a path wildcard") + + +def test_rules_need_longest_match_only_when_an_allow_and_a_disallow_overlap(): + assert robots.rules_need_longest_match(OVERLAPPING) is True + assert robots.rules_need_longest_match("User-agent: *\nAllow: /\nDisallow: /a/\n") is True + assert robots.rules_need_longest_match(DISJOINT) is False, ( + "non-overlapping paths give the same answer under either matcher") + assert robots.rules_need_longest_match(SIMPLE) is False + + +def test_rules_need_longest_match_does_not_cross_group_boundaries(): + text = ("User-agent: Googlebot\nDisallow: /docs/\n" + "User-agent: Bingbot\nAllow: /docs/public/\n") + assert robots.rules_need_longest_match(text) is False, ( + "rules in different user-agent groups never compete with one another") + + +def test_comments_and_blank_lines_are_ignored(): + text = "# hello\n\nUser-agent: *\nDisallow: /p/ # trailing\n" + verdict = robots.disallowed_urls(text, ["https://x.example/p/a"]) + assert verdict.evaluated is True + assert verdict.disallowed == ("https://x.example/p/a",) + + +def test_a_wildcard_file_is_refused_when_the_interpreter_cannot_match_it(): + verdict = robots.disallowed_urls(WILDCARD, ["https://x.example/a/b.pdf"]) + if robots.matcher_supports_wildcards(): + assert verdict.evaluated is True + assert verdict.disallowed == ("https://x.example/a/b.pdf",) + else: + assert verdict.evaluated is False + assert verdict.reason == robots.UNSUPPORTED_WILDCARDS + assert verdict.disallowed == (), ( + "a refused verdict must carry no judgement at all") + + +def test_an_overlapping_file_is_refused_when_the_interpreter_is_order_dependent(): + verdict = robots.disallowed_urls(OVERLAPPING, ["https://x.example/docs/public/x"]) + if robots.matcher_supports_longest_match(): + assert verdict.evaluated is True + assert verdict.disallowed == (), "longest-match: Allow wins over the shorter Disallow" + else: + assert verdict.evaluated is False + assert verdict.reason == robots.UNSUPPORTED_LONGEST_MATCH + + +def test_the_capability_probes_agree_with_the_interpreter_this_is_running_on(): + # Both capabilities arrived together in the CPython 3.14 RFC 9309 rewrite. + # Asserted against sys.version_info as a cross-check, so a probe that silently + # starts returning a constant is caught rather than trusted. + expected = sys.version_info >= (3, 14) + assert robots.matcher_supports_wildcards() is expected + assert robots.matcher_supports_longest_match() is expected + + +def test_a_googlebot_group_overrides_the_wildcard_group(): + text = "User-agent: *\nDisallow: /\nUser-agent: Googlebot\nDisallow:\n" + verdict = robots.disallowed_urls(text, ["https://x.example/a"]) + assert verdict.evaluated is True + assert verdict.disallowed == () + + +def test_the_wildcard_group_applies_when_there_is_no_agent_specific_group(): + verdict = robots.disallowed_urls("User-agent: *\nDisallow: /admin/\n", + ["https://x.example/admin/x"]) + assert verdict.disallowed == ("https://x.example/admin/x",) + + +def test_an_empty_robots_file_disallows_nothing(): + verdict = robots.disallowed_urls("", ["https://x.example/a"]) + assert verdict.evaluated is True + assert verdict.disallowed == () From 6c1340d6135f207115e15674935ff0b268ad1865 Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:33:03 +0600 Subject: [PATCH 06/19] feat(contradictions): flag sitemap URLs the site's own robots.txt blocks The release's headline check and the same class of self-contradiction as seo.noindex.in-sitemap, rated the same: the site submits a URL for crawling in one file and forbids the crawl in another, so one declaration is wrong and no external data is needed to know it. Reports notEvaluated -- never a pass -- when robots.txt is unreachable, when there is no sitemap to contradict, or when the published rules need a matcher capability this interpreter lacks. Also updates test_registry.py's two static counters (55->56 total, advisory 11->12) so the suite stays green at this commit rather than deferring both to Task 12, matching the fallback the Task 3/4 brief offered and Tasks 1-4 already used. --- schemas/omnirank.config.schema.json | 2 +- scripts/py/omnirank/audit.py | 8 +- scripts/py/omnirank/gates/__init__.py | 7 ++ scripts/py/omnirank/gates/contradictions.py | 102 +++++++++++++++++++ scripts/py/omnirank/registry.py | 11 ++ tests/test_config_schema.py | 6 ++ tests/test_gates_contradictions.py | 106 ++++++++++++++++++++ tests/test_registry.py | 11 +- 8 files changed, 246 insertions(+), 7 deletions(-) create mode 100644 scripts/py/omnirank/gates/contradictions.py create mode 100644 tests/test_gates_contradictions.py diff --git a/schemas/omnirank.config.schema.json b/schemas/omnirank.config.schema.json index 5a0837c..cdcb4ec 100644 --- a/schemas/omnirank.config.schema.json +++ b/schemas/omnirank.config.schema.json @@ -142,7 +142,7 @@ "canonical-cluster", "hreflang-reciprocity", "response-time", "page-weight", "compression", "render-blocking", "hsts", "nosniff", "csp", "referrer-policy", - "mixed-content", "https-redirect"] + "mixed-content", "https-redirect", "robots-sitemap"] } } } diff --git a/scripts/py/omnirank/audit.py b/scripts/py/omnirank/audit.py index 272ea4d..8103cea 100644 --- a/scripts/py/omnirank/audit.py +++ b/scripts/py/omnirank/audit.py @@ -5,7 +5,7 @@ from .bands import resolve_band from .config import Config from .fetch import fetch, make_client, read_sitemap -from .gates import aeo, geo, hygiene, jsonld, perf, security, seo, site +from .gates import aeo, contradictions, geo, hygiene, jsonld, perf, security, seo, site from .page import PageData from .report import Finding, NotEvaluated, Report @@ -129,6 +129,12 @@ def audit_site(config: Config, client: httpx.Client | None = None, report.extend(site.run(pages, sitemap_urls)) + robots_findings, robots_not_evaluated = contradictions.check_sitemap_vs_robots( + client, config.site_url, sitemap_urls) + report.extend(robots_findings) + for entry in robots_not_evaluated: + report.flag_not_evaluated(entry) + report.urls_checked = len(targets) report.extend(geo.run(client, config.site_url)) diff --git a/scripts/py/omnirank/gates/__init__.py b/scripts/py/omnirank/gates/__init__.py index 8a4b486..247a204 100644 --- a/scripts/py/omnirank/gates/__init__.py +++ b/scripts/py/omnirank/gates/__init__.py @@ -32,4 +32,11 @@ str) -> tuple[list[Finding], list[NotEvaluated]]` for the one site-level probe. It returns notEvaluated entries alongside findings rather than mutating a Report, keeping the gate testable without one. + +`contradictions` has no `run`, for the same reason `hygiene` does not: its checks +take different inputs. It exposes `check_sitemap_vs_robots(client, site_url, +sitemap_urls) -> tuple[list[Finding], list[NotEvaluated]]`, +`check_canonical_targets(client, pages) -> tuple[list[Finding], list[NotEvaluated]]` +and `check_hreflang_noindex(pages) -> list[Finding]`. The first two return +notEvaluated entries because they depend on network I/O that can fail. """ diff --git a/scripts/py/omnirank/gates/contradictions.py b/scripts/py/omnirank/gates/contradictions.py new file mode 100644 index 0000000..c3fabe0 --- /dev/null +++ b/scripts/py/omnirank/gates/contradictions.py @@ -0,0 +1,102 @@ +"""Defects provable from the site's OWN declarations, with no external truth. + +The category `docs/research/2026-08-04-competitive-gap-analysis.md` SS1 identifies +as the one OmniRank can credibly own: a URL submitted for crawling in the sitemap +and forbidden in robots.txt; a canonical pointing at a page that is noindexed, that +redirects, or that 404s; a page declared as an hreflang alternate while forbidding +its own indexing. Every one is 100% precision because both halves of the +contradiction come from the site itself. + +Two rules govern every function here, and both are load-bearing: + + 1. Never judge a URL OmniRank did not see. A target outside the crawled set is + either probed (bounded, deduplicated) or left unjudged -- never guessed at. + 2. A gate that could not run returns a NotEvaluated entry, never silence. Silence + is indistinguishable from a pass in the report, which is the failure this + project exists to refuse. + +Like `hygiene`, this module has no `run()`: its three checks take genuinely +different inputs (robots text, the crawled set, probed targets), and a single +entry point would have to take all of them. +""" +from __future__ import annotations + +import httpx + +from .. import robots +from ..fetch import fetch +from ..report import Finding, NotEvaluated + +GATE_ROBOTS_SITEMAP = "robots-sitemap" + +_MATCHER_LIMITATION = { + robots.UNSUPPORTED_WILDCARDS: + "the published rules use path wildcards (* or $)", + robots.UNSUPPORTED_LONGEST_MATCH: + "the published rules pair an Allow with an overlapping Disallow", +} + + +def _f(id_: str, gate: str, url: str, severity: str, observed: str, + expected: str, fix: str) -> Finding: + return Finding(id=id_, severity=severity, layer="seo", url=url, gate=gate, + observed=observed, expected=expected, fix=fix) + + +def check_sitemap_vs_robots( + client: httpx.Client, site_url: str, sitemap_urls: list[str] | None, +) -> tuple[list[Finding], list[NotEvaluated]]: + """Sitemap URLs that the site's own robots.txt forbids crawling. + + The same class of self-contradiction as `seo.noindex.in-sitemap`, and rated the + same: the site submits a URL for crawling in one file and forbids it in + another, so one of the two declarations is wrong and no external data is needed + to know that. + + Returns no verdict at all when robots.txt is unreachable, when there is no + sitemap, or when this interpreter's matcher cannot evaluate the rules the site + actually published -- see omnirank/robots.py for why the last one is real. + """ + site_url = site_url.rstrip("/") + if not sitemap_urls: + return [], [NotEvaluated(gate=GATE_ROBOTS_SITEMAP, site=site_url, + reason="no-sitemap")] + + robots_url = f"{site_url}/robots.txt" + result = fetch(client, robots_url) + if not result.ok: + return [], [NotEvaluated(gate=GATE_ROBOTS_SITEMAP, url=robots_url, + reason="page-unreachable")] + + verdict = robots.disallowed_urls(result.text, sitemap_urls) + if not verdict.evaluated: + return [], [NotEvaluated(gate=GATE_ROBOTS_SITEMAP, url=robots_url, + reason="matcher-unsupported")] + + findings = [ + _f("seo.robots-sitemap.disallowed", GATE_ROBOTS_SITEMAP, url, "error", + f"listed in sitemap.xml, and Disallow-ed to {robots.DEFAULT_AGENT} " + f"by {robots_url}", + "either crawlable and listed, or disallowed and unlisted", + "Remove the Disallow rule in robots.txt, or drop this URL from the " + "sitemap. Submitting a URL for crawling while forbidding the crawl is a " + "contradiction the site makes with itself: engines waste budget " + "discovering a URL they are then not permitted to fetch. Only you know " + "which of the two declarations is the intended one.") + for url in verdict.disallowed + ] + return findings, [] + + +def matcher_limitation(reason: str) -> str: + """Human-readable explanation for a `matcher-unsupported` notEvaluated entry. + + Kept next to the check that produces it so the two cannot drift; the CLI has no + other place to learn what the reason means. + """ + detail = _MATCHER_LIMITATION.get(reason, "the published rules") + return (f"robots.txt was fetched, but {detail}, and urllib.robotparser on this " + "interpreter does not evaluate those the way RFC 9309 specifies " + "(fixed in Python 3.14). OmniRank refuses to answer rather than report " + "a result it knows may be wrong. Re-run on Python 3.14 or later for " + "full coverage.") diff --git a/scripts/py/omnirank/registry.py b/scripts/py/omnirank/registry.py index 632be8d..4134cda 100644 --- a/scripts/py/omnirank/registry.py +++ b/scripts/py/omnirank/registry.py @@ -150,6 +150,12 @@ def _r(id_: str, tier: FixTier, severity: str, layer: str, gate: str, # `infrastructure`: no source edit produces an origin-level redirect. _r("security.https-redirect.missing", "infrastructure", "error", "security", "https-redirect"), + # --- gates/contradictions.py (v0.4.0) --------------------------------- + # `advisory`: two opposite correct answers exist -- unblock it in robots.txt, + # or drop it from the sitemap -- and only the owner knows which was intended. + # Exactly parallel to seo.noindex.in-sitemap. + _r("seo.robots-sitemap.disallowed", "advisory", "error", "seo", + "robots-sitemap"), ) REGISTRY: dict[str, RegisteredFinding] = {entry.id: entry for entry in _ENTRIES} @@ -199,6 +205,10 @@ def scoring_gate_count(layer: str) -> int: # robots.txt and crawler directives; noindex and sitemap membership; canonical # and hreflang SETS (a single self-canonical on a single-route file is NOT a # set, and is governed by blast radius instead); and any licence grant. +# v0.4.0 adds seo.robots-sitemap.disallowed and seo.hreflang-noindex.alternate: +# the first touches robots.txt AND sitemap membership, the second an hreflang +# SET -- all three surfaces are already enumerated above, so both are +# hard-capped for the same reason. PROTECTED_SURFACES: frozenset[str] = frozenset({ "geo.ai-allowlist.missing", "geo.ai-allowlist.blocked", @@ -206,6 +216,7 @@ def scoring_gate_count(layer: str) -> int: "seo.hreflang.no-x-default", "seo.hreflang.not-reciprocal", "geo.citation-licence.missing", + "seo.robots-sitemap.disallowed", }) # Capped at display-only permanently, at any tier, under any flag. Reversing a diff --git a/tests/test_config_schema.py b/tests/test_config_schema.py index 8d1a34a..d364872 100644 --- a/tests/test_config_schema.py +++ b/tests/test_config_schema.py @@ -151,3 +151,9 @@ def test_the_two_scoring_security_gates_are_accepted_in_fail_on(): cfg = minimal() cfg["audit"] = {"failOn": ["mixed-content", "https-redirect"]} assert list(validator().iter_errors(cfg)) == [] + + +def test_the_contradiction_gates_are_accepted_in_fail_on(): + cfg = minimal() + cfg["audit"] = {"failOn": ["robots-sitemap"]} + assert list(validator().iter_errors(cfg)) == [] diff --git a/tests/test_gates_contradictions.py b/tests/test_gates_contradictions.py new file mode 100644 index 0000000..2e7d9bf --- /dev/null +++ b/tests/test_gates_contradictions.py @@ -0,0 +1,106 @@ +import httpx +import respx + +from omnirank import robots +from omnirank.fetch import make_client +from omnirank.gates import contradictions + +SITE = "https://x.example" + + +def robots_txt(body: str): + respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(200, text=body)) + + +@respx.mock +def test_a_sitemap_url_disallowed_by_robots_is_an_error(): + robots_txt("User-agent: *\nDisallow: /private/\n") + findings, not_evaluated = contradictions.check_sitemap_vs_robots( + make_client(), SITE, [f"{SITE}/open", f"{SITE}/private/secret"]) + assert not_evaluated == [] + assert [f.id for f in findings] == ["seo.robots-sitemap.disallowed"] + assert findings[0].severity == "error" + assert findings[0].layer == "seo" + assert findings[0].gate == "robots-sitemap" + assert findings[0].url == f"{SITE}/private/secret" + assert "Disallow" in findings[0].observed + + +@respx.mock +def test_one_finding_per_disallowed_url(): + robots_txt("User-agent: *\nDisallow: /p/\n") + findings, _ = contradictions.check_sitemap_vs_robots( + make_client(), SITE, [f"{SITE}/p/a", f"{SITE}/p/b", f"{SITE}/ok"]) + assert sorted(f.url for f in findings) == [f"{SITE}/p/a", f"{SITE}/p/b"] + + +@respx.mock +def test_a_fully_permissive_robots_emits_nothing(): + robots_txt("User-agent: *\nDisallow:\n") + findings, not_evaluated = contradictions.check_sitemap_vs_robots( + make_client(), SITE, [f"{SITE}/a"]) + assert findings == [] + assert not_evaluated == [] + + +@respx.mock +def test_an_unreachable_robots_is_not_evaluated_rather_than_passed(): + respx.get(f"{SITE}/robots.txt").mock(return_value=httpx.Response(404)) + findings, not_evaluated = contradictions.check_sitemap_vs_robots( + make_client(), SITE, [f"{SITE}/a"]) + assert findings == [], "no robots.txt means no verdict, never a pass" + assert [(e.gate, e.reason, e.url) for e in not_evaluated] == [ + ("robots-sitemap", "page-unreachable", f"{SITE}/robots.txt")] + + +@respx.mock +def test_no_sitemap_means_the_gate_is_not_evaluated(): + findings, not_evaluated = contradictions.check_sitemap_vs_robots( + make_client(), SITE, None) + assert findings == [] + assert [(e.gate, e.reason, e.site) for e in not_evaluated] == [ + ("robots-sitemap", "no-sitemap", SITE)] + assert not respx.calls, "with no sitemap there is nothing to check robots against" + + +@respx.mock +def test_an_empty_sitemap_list_is_treated_the_same_as_no_sitemap(): + findings, not_evaluated = contradictions.check_sitemap_vs_robots( + make_client(), SITE, []) + assert findings == [] + assert [e.reason for e in not_evaluated] == ["no-sitemap"] + + +@respx.mock +def test_rules_the_interpreter_cannot_match_produce_not_evaluated_not_a_pass(): + robots_txt("User-agent: *\nDisallow: /*.pdf$\n") + findings, not_evaluated = contradictions.check_sitemap_vs_robots( + make_client(), SITE, [f"{SITE}/a/b.pdf"]) + if robots.matcher_supports_wildcards(): + assert [f.url for f in findings] == [f"{SITE}/a/b.pdf"] + assert not_evaluated == [] + else: + assert findings == [] + assert [(e.gate, e.reason) for e in not_evaluated] == [ + ("robots-sitemap", "matcher-unsupported")] + + +@respx.mock +def test_the_matcher_unsupported_finding_names_the_interpreter_limitation(): + robots_txt("User-agent: *\nDisallow: /docs/\nAllow: /docs/public/\n") + _, not_evaluated = contradictions.check_sitemap_vs_robots( + make_client(), SITE, [f"{SITE}/docs/public/x"]) + if not robots.matcher_supports_longest_match(): + assert not_evaluated[0].reason == "matcher-unsupported" + else: + assert not_evaluated == [] + + +@respx.mock +def test_the_finding_says_both_ways_to_resolve_the_contradiction(): + robots_txt("User-agent: *\nDisallow: /p/\n") + findings, _ = contradictions.check_sitemap_vs_robots( + make_client(), SITE, [f"{SITE}/p/a"]) + fix = findings[0].fix.lower() + assert "sitemap" in fix and "robots" in fix, ( + "either edit resolves it, and only the owner knows which is correct") diff --git a/tests/test_registry.py b/tests/test_registry.py index 6d17bca..fdeed7d 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -60,9 +60,9 @@ def test_every_registered_id_is_emitted_by_some_gate(): def test_registry_covers_all_forty_eight_findings(): # v0.4.0 Task 3: +5 for gates/security.py's four response-header gates # (hsts fires two distinct ids). Task 4: +2 for mixed-content and - # https-redirect. Task 12 retitles/rebases this once the full v0.4.0 gate - # set has landed. - assert len(REGISTRY) == 55 + # https-redirect. Task 6: +1 for seo.robots-sitemap.disallowed. Task 12 + # retitles/rebases this once the full v0.4.0 gate set has landed. + assert len(REGISTRY) == 56 def test_tier_distribution_matches_the_fixability_classification(): @@ -73,9 +73,10 @@ def test_tier_distribution_matches_the_fixability_classification(): # v0.4.0 Task 3: +5 infrastructure (all five security header gates are # infrastructure -- the fix is an origin/CDN response-header change). # Task 4: +1 templated (mixed-content, a markup scheme rewrite) and +1 - # infrastructure (https-redirect, an origin-level redirect). + # infrastructure (https-redirect, an origin-level redirect). Task 6: +1 + # advisory (seo.robots-sitemap.disallowed). assert counts == {"mechanical": 4, "templated": 16, "drafted": 12, - "advisory": 11, "infrastructure": 12} + "advisory": 12, "infrastructure": 12} def test_mechanical_tier_is_exactly_the_four_documented_ids(): From 7910b1695b6d592e065e36a883a7054959aed6b1 Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:35:52 +0600 Subject: [PATCH 07/19] feat(contradictions): judge canonical targets that are noindexed, redirect or 404 Targets already in the crawled set cost no extra request. Targets outside it are probed once each, deduplicated, capped at MAX_CANONICAL_PROBES, and anything past the cap is reported as budget-exceeded rather than skipped. A 5xx or a transport failure is page-unreachable, never .not-found -- a transient origin error is not a missing page and OmniRank does not guess which one it saw. site.py grows three public aliases so this module reuses the exact noindex, URL-key and canonical predicates the site pass already uses, instead of growing a second opinion about what they mean. Keeps the page-unreachable dedup at its brief-documented default (one NotEvaluated entry per pointing page, not per target) since only the single-page case is asserted. Also updates test_registry.py's two static counters (56->59 total, templated 16->17, advisory 12->14) to keep the suite green at this commit. --- schemas/omnirank.config.schema.json | 3 +- scripts/py/omnirank/audit.py | 6 + scripts/py/omnirank/gates/contradictions.py | 120 ++++++++++++++++- scripts/py/omnirank/gates/site.py | 10 ++ scripts/py/omnirank/registry.py | 11 ++ tests/test_config_schema.py | 6 + tests/test_gates_contradictions.py | 139 ++++++++++++++++++++ tests/test_registry.py | 15 ++- 8 files changed, 302 insertions(+), 8 deletions(-) diff --git a/schemas/omnirank.config.schema.json b/schemas/omnirank.config.schema.json index cdcb4ec..9fb138b 100644 --- a/schemas/omnirank.config.schema.json +++ b/schemas/omnirank.config.schema.json @@ -142,7 +142,8 @@ "canonical-cluster", "hreflang-reciprocity", "response-time", "page-weight", "compression", "render-blocking", "hsts", "nosniff", "csp", "referrer-policy", - "mixed-content", "https-redirect", "robots-sitemap"] + "mixed-content", "https-redirect", "robots-sitemap", + "canonical-target"] } } } diff --git a/scripts/py/omnirank/audit.py b/scripts/py/omnirank/audit.py index 8103cea..112b4d1 100644 --- a/scripts/py/omnirank/audit.py +++ b/scripts/py/omnirank/audit.py @@ -135,6 +135,12 @@ def audit_site(config: Config, client: httpx.Client | None = None, for entry in robots_not_evaluated: report.flag_not_evaluated(entry) + canonical_findings, canonical_not_evaluated = ( + contradictions.check_canonical_targets(client, pages)) + report.extend(canonical_findings) + for entry in canonical_not_evaluated: + report.flag_not_evaluated(entry) + report.urls_checked = len(targets) report.extend(geo.run(client, config.site_url)) diff --git a/scripts/py/omnirank/gates/contradictions.py b/scripts/py/omnirank/gates/contradictions.py index c3fabe0..df08e78 100644 --- a/scripts/py/omnirank/gates/contradictions.py +++ b/scripts/py/omnirank/gates/contradictions.py @@ -24,10 +24,20 @@ import httpx from .. import robots -from ..fetch import fetch +from ..fetch import Fetched, fetch +from ..page import PageData from ..report import Finding, NotEvaluated +from . import site GATE_ROBOTS_SITEMAP = "robots-sitemap" +GATE_CANONICAL_TARGET = "canonical-target" + +# How many canonical targets outside the crawled set OmniRank will fetch in one +# audit. A cap rather than an unbounded pass: a 200-page site whose every canonical +# points at an uncrawled URL would otherwise double the audit's request count +# without warning. Targets past the cap are reported as budget-exceeded, never +# skipped silently. +MAX_CANONICAL_PROBES = 25 _MATCHER_LIMITATION = { robots.UNSUPPORTED_WILDCARDS: @@ -100,3 +110,111 @@ def matcher_limitation(reason: str) -> str: "(fixed in Python 3.14). OmniRank refuses to answer rather than report " "a result it knows may be wrong. Re-run on Python 3.14 or later for " "full coverage.") + + +def _canonical_pairs(pages: list[PageData]) -> list[tuple[PageData, str, str]]: + """(page, absolute target, target key) for every page with a foreign canonical. + + Self-canonicals are dropped: a page canonicalising to itself contradicts + nothing, even when it is noindexed -- that is a coherent "exclude this page" + configuration, and seo.noindex.in-sitemap owns the sitemap half of it. + """ + pairs: list[tuple[PageData, str, str]] = [] + for page in pages: + target = site.canonical_target(page) + if not target: + continue # the per-URL gate's problem + key = site.canonical_key(target) + if key == site.canonical_key(page.url): + continue # self-canonical, correct + pairs.append((page, target, key)) + return pairs + + +def _noindexed(page: PageData, target: str) -> Finding: + return _f("seo.canonical-target.noindexed", GATE_CANONICAL_TARGET, page.url, + "error", + f"canonical points to {target}, which carries a noindex directive", + "a canonical pointing at an indexable page", + f"Remove the noindex on {target}, or point this page's canonical " + "somewhere indexable. Naming a page engines are forbidden to index as " + "the canonical version of this one discards this page's signals " + "without transferring them anywhere: both URLs leave the index.") + + +def check_canonical_targets( + client: httpx.Client, pages: list[PageData], +) -> tuple[list[Finding], list[NotEvaluated]]: + """Canonicals whose target is noindexed, redirects, or does not exist. + + Targets already in the crawled set are judged from the PageData already held -- + no second fetch. Targets outside it are probed once each, deduplicated, up to + MAX_CANONICAL_PROBES; past that they are reported as budget-exceeded. Nothing + is ever judged from a URL OmniRank did not actually see. + + A 5xx or transport failure on a probe is `page-unreachable`, not `.not-found`: + a transient origin error is not a missing page, and calling it one would be a + guess dressed as a finding. + """ + crawled = {site.canonical_key(p.url): p for p in pages} + findings: list[Finding] = [] + not_evaluated: list[NotEvaluated] = [] + + # target key -> the outcome of probing it, so N pages sharing one broken target + # cost one request and still each get told. + probed: dict[str, Fetched | None] = {} + budget = MAX_CANONICAL_PROBES + + for page, target, key in _canonical_pairs(pages): + known = crawled.get(key) + if known is not None: + # Already fetched and 200 by construction (unreachable URLs never + # become PageData), so only the noindex question remains. + if site.is_noindex(known): + findings.append(_noindexed(page, target)) + continue + + if key not in probed: + if budget <= 0: + probed[key] = None + not_evaluated.append(NotEvaluated( + gate=GATE_CANONICAL_TARGET, url=target, reason="budget-exceeded")) + else: + budget -= 1 + probed[key] = fetch(client, target) + + result = probed[key] + if result is None: + continue # already recorded as unevaluated + + if result.is_redirect: + location = (result.headers.get("location", "") or "").strip() or "(no Location)" + findings.append(_f( + "seo.canonical-target.redirects", GATE_CANONICAL_TARGET, page.url, + "warning", + f"canonical points to {target}, which returns HTTP {result.status}", + "a canonical pointing at a URL that resolves 200", + f"Point this page's canonical straight at {location}. A canonical " + "that redirects makes engines resolve one more hop than they need " + "to, and a stale canonical is a symptom of a URL change the rest of " + "the markup has not caught up with.")) + elif result.status in (404, 410): + findings.append(_f( + "seo.canonical-target.not-found", GATE_CANONICAL_TARGET, page.url, + "error", + f"canonical points to {target}, which returns HTTP {result.status}", + "a canonical pointing at a URL that resolves 200", + f"Restore {target}, or point this page's canonical at a URL that " + "exists. Nominating a missing page as the canonical version of this " + "one asks engines to index a URL that is not there, so this page's " + "signals go nowhere.")) + elif result.ok: + if site.is_noindex(PageData.from_fetched(result)): + findings.append(_noindexed(page, target)) + else: + # 5xx, 401, 403, or a transport failure (status 0). None of these means + # "gone", and OmniRank does not guess which one it is. + not_evaluated.append(NotEvaluated( + gate=GATE_CANONICAL_TARGET, url=target, reason="page-unreachable")) + + return findings, not_evaluated diff --git a/scripts/py/omnirank/gates/site.py b/scripts/py/omnirank/gates/site.py index eb21e87..6d57cd8 100644 --- a/scripts/py/omnirank/gates/site.py +++ b/scripts/py/omnirank/gates/site.py @@ -235,3 +235,13 @@ def run(pages: list[PageData], sitemap_urls: list[str] | None = None) -> list[Fi *_canonical_chains(pages), *_hreflang_reciprocity(pages), ] + + +# Public aliases. gates/contradictions.py needs these three predicates, and +# reimplementing them there would let the two modules disagree about what +# "noindex" means, what counts as the same URL, or which is the canonical -- +# exactly the three-way drift html.py was created to end. Aliases rather than +# renames so this module's own call sites stay byte-identical. +canonical_key = _canonical_key +canonical_target = _canonical_target +is_noindex = _is_noindex diff --git a/scripts/py/omnirank/registry.py b/scripts/py/omnirank/registry.py index 4134cda..5e07370 100644 --- a/scripts/py/omnirank/registry.py +++ b/scripts/py/omnirank/registry.py @@ -156,6 +156,17 @@ def _r(id_: str, tier: FixTier, severity: str, layer: str, gate: str, # Exactly parallel to seo.noindex.in-sitemap. _r("seo.robots-sitemap.disallowed", "advisory", "error", "seo", "robots-sitemap"), + # `advisory` for both errors: remove the noindex / restore the target, or + # repoint the canonical -- two opposite correct answers, owner's call. + _r("seo.canonical-target.noindexed", "advisory", "error", "seo", + "canonical-target"), + _r("seo.canonical-target.not-found", "advisory", "error", "seo", + "canonical-target"), + # `templated`, deliberately NOT `mechanical`: the redirect destination comes + # from a probe response, not from data already in the finding, and this release + # ships no new fix generator (see test_generators_cover_exactly_the_mechanical_tier). + _r("seo.canonical-target.redirects", "templated", "warning", "seo", + "canonical-target"), ) REGISTRY: dict[str, RegisteredFinding] = {entry.id: entry for entry in _ENTRIES} diff --git a/tests/test_config_schema.py b/tests/test_config_schema.py index d364872..3a4eb63 100644 --- a/tests/test_config_schema.py +++ b/tests/test_config_schema.py @@ -157,3 +157,9 @@ def test_the_contradiction_gates_are_accepted_in_fail_on(): cfg = minimal() cfg["audit"] = {"failOn": ["robots-sitemap"]} assert list(validator().iter_errors(cfg)) == [] + + +def test_canonical_target_is_accepted_in_fail_on(): + cfg = minimal() + cfg["audit"] = {"failOn": ["canonical-target"]} + assert list(validator().iter_errors(cfg)) == [] diff --git a/tests/test_gates_contradictions.py b/tests/test_gates_contradictions.py index 2e7d9bf..14cb7f3 100644 --- a/tests/test_gates_contradictions.py +++ b/tests/test_gates_contradictions.py @@ -104,3 +104,142 @@ def test_the_finding_says_both_ways_to_resolve_the_contradiction(): fix = findings[0].fix.lower() assert "sitemap" in fix and "robots" in fix, ( "either edit resolves it, and only the owner knows which is correct") + + +from omnirank.page import PageData + +HEAD = "{}

H

" + + +def page(url: str, canonical: str | None = None, noindex: bool = False) -> PageData: + head = "" + if canonical is not None: + head += f"" + if noindex: + head += "" + return PageData(url=url, html=HEAD.format(head), status=200, elapsed_ms=1, + headers={}) + + +@respx.mock +def test_a_canonical_pointing_at_a_noindexed_page_in_the_crawled_set_is_an_error(): + pages = [page(f"{SITE}/a", canonical=f"{SITE}/b"), + page(f"{SITE}/b", canonical=f"{SITE}/b", noindex=True)] + findings, not_evaluated = contradictions.check_canonical_targets(make_client(), pages) + assert not_evaluated == [] + assert [(f.id, f.url) for f in findings] == [ + ("seo.canonical-target.noindexed", f"{SITE}/a")] + assert findings[0].severity == "error" + assert findings[0].gate == "canonical-target" + assert not respx.calls, "a target already in the crawled set is never re-fetched" + + +@respx.mock +def test_a_self_canonical_on_a_noindexed_page_is_not_flagged(): + # The page canonicalises to itself; noindex+self-canonical is a deliberate, + # coherent configuration and seo.noindex.in-sitemap owns the sitemap question. + pages = [page(f"{SITE}/a", canonical=f"{SITE}/a", noindex=True)] + findings, _ = contradictions.check_canonical_targets(make_client(), pages) + assert findings == [] + + +@respx.mock +def test_a_canonical_target_that_404s_is_an_error(): + respx.get(f"{SITE}/gone").mock(return_value=httpx.Response(404)) + pages = [page(f"{SITE}/a", canonical=f"{SITE}/gone")] + findings, not_evaluated = contradictions.check_canonical_targets(make_client(), pages) + assert not_evaluated == [] + assert [(f.id, f.url) for f in findings] == [ + ("seo.canonical-target.not-found", f"{SITE}/a")] + assert findings[0].severity == "error" + assert f"{SITE}/gone" in findings[0].observed + + +@respx.mock +def test_a_canonical_target_that_redirects_is_a_warning_naming_the_destination(): + respx.get(f"{SITE}/old").mock(return_value=httpx.Response( + 301, headers={"location": f"{SITE}/new"})) + pages = [page(f"{SITE}/a", canonical=f"{SITE}/old")] + findings, _ = contradictions.check_canonical_targets(make_client(), pages) + assert [f.id for f in findings] == ["seo.canonical-target.redirects"] + assert findings[0].severity == "warning" + assert f"{SITE}/new" in findings[0].fix + + +@respx.mock +def test_a_probed_target_that_is_noindexed_is_an_error(): + respx.get(f"{SITE}/hidden").mock(return_value=httpx.Response( + 200, text=HEAD.format(""))) + pages = [page(f"{SITE}/a", canonical=f"{SITE}/hidden")] + findings, _ = contradictions.check_canonical_targets(make_client(), pages) + assert [f.id for f in findings] == ["seo.canonical-target.noindexed"] + + +@respx.mock +def test_a_healthy_probed_target_emits_nothing(): + respx.get(f"{SITE}/good").mock(return_value=httpx.Response(200, text=HEAD.format(""))) + pages = [page(f"{SITE}/a", canonical=f"{SITE}/good")] + findings, not_evaluated = contradictions.check_canonical_targets(make_client(), pages) + assert findings == [] + assert not_evaluated == [] + + +@respx.mock +def test_a_server_error_on_a_probe_is_not_evaluated_rather_than_called_missing(): + respx.get(f"{SITE}/flaky").mock(return_value=httpx.Response(503)) + pages = [page(f"{SITE}/a", canonical=f"{SITE}/flaky")] + findings, not_evaluated = contradictions.check_canonical_targets(make_client(), pages) + assert findings == [], "a 503 is transient; calling it not-found would be a guess" + assert [(e.gate, e.reason, e.url) for e in not_evaluated] == [ + ("canonical-target", "page-unreachable", f"{SITE}/flaky")] + + +@respx.mock +def test_each_distinct_target_is_probed_exactly_once(): + respx.get(f"{SITE}/shared").mock(return_value=httpx.Response(404)) + pages = [page(f"{SITE}/a", canonical=f"{SITE}/shared"), + page(f"{SITE}/b", canonical=f"{SITE}/shared"), + page(f"{SITE}/c", canonical=f"{SITE}/shared")] + findings, _ = contradictions.check_canonical_targets(make_client(), pages) + assert len(respx.calls) == 1, "one probe per distinct target, not per pointing page" + assert sorted(f.url for f in findings) == [f"{SITE}/a", f"{SITE}/b", f"{SITE}/c"], ( + "every page pointing at the broken target is still told about it") + + +@respx.mock +def test_probes_stop_at_the_budget_and_the_rest_are_not_evaluated(): + over = contradictions.MAX_CANONICAL_PROBES + 3 + for i in range(over): + respx.get(f"{SITE}/t{i}").mock(return_value=httpx.Response(200, text=HEAD.format(""))) + pages = [page(f"{SITE}/p{i}", canonical=f"{SITE}/t{i}") for i in range(over)] + findings, not_evaluated = contradictions.check_canonical_targets(make_client(), pages) + assert findings == [] + assert len(respx.calls) == contradictions.MAX_CANONICAL_PROBES + assert len(not_evaluated) == 3 + assert {e.reason for e in not_evaluated} == {"budget-exceeded"} + assert {e.gate for e in not_evaluated} == {"canonical-target"} + + +@respx.mock +def test_a_page_with_no_canonical_is_left_to_the_per_url_gate(): + pages = [page(f"{SITE}/a")] + findings, not_evaluated = contradictions.check_canonical_targets(make_client(), pages) + assert (findings, not_evaluated) == ([], []) + assert not respx.calls + + +@respx.mock +def test_a_relative_canonical_is_resolved_before_being_judged(): + respx.get(f"{SITE}/target").mock(return_value=httpx.Response(404)) + pages = [page(f"{SITE}/deep/a", canonical="/target")] + findings, _ = contradictions.check_canonical_targets(make_client(), pages) + assert [f.id for f in findings] == ["seo.canonical-target.not-found"] + + +@respx.mock +def test_a_trailing_slash_difference_still_counts_as_the_crawled_page(): + pages = [page(f"{SITE}/a", canonical=f"{SITE}/b/"), + page(f"{SITE}/b", canonical=f"{SITE}/b", noindex=True)] + findings, _ = contradictions.check_canonical_targets(make_client(), pages) + assert [f.id for f in findings] == ["seo.canonical-target.noindexed"] + assert not respx.calls, "canonical_key normalises the trailing slash" diff --git a/tests/test_registry.py b/tests/test_registry.py index fdeed7d..a22a678 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -60,9 +60,10 @@ def test_every_registered_id_is_emitted_by_some_gate(): def test_registry_covers_all_forty_eight_findings(): # v0.4.0 Task 3: +5 for gates/security.py's four response-header gates # (hsts fires two distinct ids). Task 4: +2 for mixed-content and - # https-redirect. Task 6: +1 for seo.robots-sitemap.disallowed. Task 12 - # retitles/rebases this once the full v0.4.0 gate set has landed. - assert len(REGISTRY) == 56 + # https-redirect. Task 6: +1 for seo.robots-sitemap.disallowed. Task 7: +3 + # for the three canonical-target ids. Task 12 retitles/rebases this once + # the full v0.4.0 gate set has landed. + assert len(REGISTRY) == 59 def test_tier_distribution_matches_the_fixability_classification(): @@ -74,9 +75,11 @@ def test_tier_distribution_matches_the_fixability_classification(): # infrastructure -- the fix is an origin/CDN response-header change). # Task 4: +1 templated (mixed-content, a markup scheme rewrite) and +1 # infrastructure (https-redirect, an origin-level redirect). Task 6: +1 - # advisory (seo.robots-sitemap.disallowed). - assert counts == {"mechanical": 4, "templated": 16, "drafted": 12, - "advisory": 12, "infrastructure": 12} + # advisory (seo.robots-sitemap.disallowed). Task 7: +2 advisory + # (canonical-target .noindexed/.not-found) and +1 templated + # (canonical-target .redirects). + assert counts == {"mechanical": 4, "templated": 17, "drafted": 12, + "advisory": 14, "infrastructure": 12} def test_mechanical_tier_is_exactly_the_four_documented_ids(): From 7200c6a520bfcf6ee0048fb9e24424c9c47231ee Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:38:32 +0600 Subject: [PATCH 08/19] feat(contradictions): flag a noindexed page declared as an hreflang alternate Attached to the DECLARING page: engines discard an hreflang annotation whose target they cannot resolve, so it is the declaring page that loses its international targeting. Crawled-set only, x-default exempt, no network I/O. Also updates test_registry.py's two static counters (59->60 total, advisory 14->15) to keep the suite green at this commit. --- schemas/omnirank.config.schema.json | 2 +- scripts/py/omnirank/audit.py | 2 + scripts/py/omnirank/gates/contradictions.py | 41 +++++++++++++ scripts/py/omnirank/gates/site.py | 1 + scripts/py/omnirank/registry.py | 5 ++ tests/test_config_schema.py | 6 ++ tests/test_gates_contradictions.py | 65 +++++++++++++++++++++ tests/test_registry.py | 12 ++-- 8 files changed, 128 insertions(+), 6 deletions(-) diff --git a/schemas/omnirank.config.schema.json b/schemas/omnirank.config.schema.json index 9fb138b..84d6dc1 100644 --- a/schemas/omnirank.config.schema.json +++ b/schemas/omnirank.config.schema.json @@ -143,7 +143,7 @@ "response-time", "page-weight", "compression", "render-blocking", "hsts", "nosniff", "csp", "referrer-policy", "mixed-content", "https-redirect", "robots-sitemap", - "canonical-target"] + "canonical-target", "hreflang-noindex"] } } } diff --git a/scripts/py/omnirank/audit.py b/scripts/py/omnirank/audit.py index 112b4d1..0c8aea9 100644 --- a/scripts/py/omnirank/audit.py +++ b/scripts/py/omnirank/audit.py @@ -141,6 +141,8 @@ def audit_site(config: Config, client: httpx.Client | None = None, for entry in canonical_not_evaluated: report.flag_not_evaluated(entry) + report.extend(contradictions.check_hreflang_noindex(pages)) + report.urls_checked = len(targets) report.extend(geo.run(client, config.site_url)) diff --git a/scripts/py/omnirank/gates/contradictions.py b/scripts/py/omnirank/gates/contradictions.py index df08e78..1e99c35 100644 --- a/scripts/py/omnirank/gates/contradictions.py +++ b/scripts/py/omnirank/gates/contradictions.py @@ -31,6 +31,7 @@ GATE_ROBOTS_SITEMAP = "robots-sitemap" GATE_CANONICAL_TARGET = "canonical-target" +GATE_HREFLANG_NOINDEX = "hreflang-noindex" # How many canonical targets outside the crawled set OmniRank will fetch in one # audit. A cap rather than an unbounded pass: a 200-page site whose every canonical @@ -218,3 +219,43 @@ def check_canonical_targets( gate=GATE_CANONICAL_TARGET, url=target, reason="page-unreachable")) return findings, not_evaluated + + +def check_hreflang_noindex(pages: list[PageData]) -> list[Finding]: + """Pages that declare a noindexed page as their locale alternate. + + An hreflang set asserts that these URLs are intentional per-locale versions of + one another. Naming a page engines are forbidden to index nullifies the pair: + engines discard hreflang annotations whose target they cannot resolve, so the + DECLARING page loses its international targeting too -- which is why the + finding attaches there rather than to the noindexed page. + + Crawled-set only, and no network I/O, so this gate can never fail to run and + returns a bare list. x-default is exempt: site.alternates excludes it, because + it is a fallback pointer rather than a language pair. + """ + by_key = {site.canonical_key(p.url): p for p in pages} + findings: list[Finding] = [] + + for page in pages: + page_key = site.canonical_key(page.url) + for href in sorted(site.alternates(page)): + target_key = site.canonical_key(href) + if target_key == page_key: + continue # self-reference is normal + target = by_key.get(target_key) + if target is None: + continue # outside the crawled set + if not site.is_noindex(target): + continue + findings.append(_f( + "seo.hreflang-noindex.alternate", GATE_HREFLANG_NOINDEX, page.url, + "error", + f"declares {href} as a locale alternate, and that page carries a " + "noindex directive", + "every hreflang alternate indexable", + f"Remove the noindex on {href}, or drop it from this page's hreflang " + "set. An alternate engines may not index cannot be returned for any " + "locale, and an annotation whose target cannot be resolved is " + "discarded -- taking this page's international targeting with it.")) + return findings diff --git a/scripts/py/omnirank/gates/site.py b/scripts/py/omnirank/gates/site.py index 6d57cd8..7154b74 100644 --- a/scripts/py/omnirank/gates/site.py +++ b/scripts/py/omnirank/gates/site.py @@ -245,3 +245,4 @@ def run(pages: list[PageData], sitemap_urls: list[str] | None = None) -> list[Fi canonical_key = _canonical_key canonical_target = _canonical_target is_noindex = _is_noindex +alternates = _alternates diff --git a/scripts/py/omnirank/registry.py b/scripts/py/omnirank/registry.py index 5e07370..a9ddab9 100644 --- a/scripts/py/omnirank/registry.py +++ b/scripts/py/omnirank/registry.py @@ -167,6 +167,10 @@ def _r(id_: str, tier: FixTier, severity: str, layer: str, gate: str, # ships no new fix generator (see test_generators_cover_exactly_the_mechanical_tier). _r("seo.canonical-target.redirects", "templated", "warning", "seo", "canonical-target"), + # `advisory`: un-noindex the target, or drop it from the set. And an hreflang + # SET is an enumerated protected surface (see PROTECTED_SURFACES below). + _r("seo.hreflang-noindex.alternate", "advisory", "error", "seo", + "hreflang-noindex"), ) REGISTRY: dict[str, RegisteredFinding] = {entry.id: entry for entry in _ENTRIES} @@ -228,6 +232,7 @@ def scoring_gate_count(layer: str) -> int: "seo.hreflang.not-reciprocal", "geo.citation-licence.missing", "seo.robots-sitemap.disallowed", + "seo.hreflang-noindex.alternate", }) # Capped at display-only permanently, at any tier, under any flag. Reversing a diff --git a/tests/test_config_schema.py b/tests/test_config_schema.py index 3a4eb63..e1c9901 100644 --- a/tests/test_config_schema.py +++ b/tests/test_config_schema.py @@ -163,3 +163,9 @@ def test_canonical_target_is_accepted_in_fail_on(): cfg = minimal() cfg["audit"] = {"failOn": ["canonical-target"]} assert list(validator().iter_errors(cfg)) == [] + + +def test_hreflang_noindex_is_accepted_in_fail_on(): + cfg = minimal() + cfg["audit"] = {"failOn": ["hreflang-noindex"]} + assert list(validator().iter_errors(cfg)) == [] diff --git a/tests/test_gates_contradictions.py b/tests/test_gates_contradictions.py index 14cb7f3..6b86e01 100644 --- a/tests/test_gates_contradictions.py +++ b/tests/test_gates_contradictions.py @@ -243,3 +243,68 @@ def test_a_trailing_slash_difference_still_counts_as_the_crawled_page(): findings, _ = contradictions.check_canonical_targets(make_client(), pages) assert [f.id for f in findings] == ["seo.canonical-target.noindexed"] assert not respx.calls, "canonical_key normalises the trailing slash" + + +def alt_page(url: str, alternates: dict[str, str], noindex: bool = False) -> PageData: + head = "".join(f"" + for lang, href in alternates.items()) + if noindex: + head += "" + return PageData(url=url, html=HEAD.format(head), status=200, elapsed_ms=1, + headers={}) + + +def test_an_alternate_that_is_noindexed_is_an_error_on_the_declaring_page(): + pages = [alt_page(f"{SITE}/en", {"en": f"{SITE}/en", "fr": f"{SITE}/fr"}), + alt_page(f"{SITE}/fr", {"en": f"{SITE}/en", "fr": f"{SITE}/fr"}, + noindex=True)] + findings = contradictions.check_hreflang_noindex(pages) + assert [(f.id, f.url) for f in findings] == [ + ("seo.hreflang-noindex.alternate", f"{SITE}/en")] + assert findings[0].severity == "error" + assert findings[0].gate == "hreflang-noindex" + assert f"{SITE}/fr" in findings[0].observed + + +def test_an_indexable_alternate_set_emits_nothing(): + pages = [alt_page(f"{SITE}/en", {"en": f"{SITE}/en", "fr": f"{SITE}/fr"}), + alt_page(f"{SITE}/fr", {"en": f"{SITE}/en", "fr": f"{SITE}/fr"})] + assert contradictions.check_hreflang_noindex(pages) == [] + + +def test_a_noindexed_page_declaring_indexable_alternates_is_not_flagged(): + # The contradiction belongs to whoever DECLARES a forbidden target, not to the + # page that opted itself out. + pages = [alt_page(f"{SITE}/en", {"fr": f"{SITE}/fr"}), + alt_page(f"{SITE}/fr", {"en": f"{SITE}/en"}, noindex=True)] + findings = contradictions.check_hreflang_noindex(pages) + assert [f.url for f in findings] == [f"{SITE}/en"] + + +def test_an_alternate_outside_the_crawled_set_is_never_judged(): + pages = [alt_page(f"{SITE}/en", {"de": "https://de.example/"})] + assert contradictions.check_hreflang_noindex(pages) == [], ( + "OmniRank never guesses about a URL it did not fetch") + + +def test_a_self_referencing_alternate_is_not_a_contradiction(): + pages = [alt_page(f"{SITE}/en", {"en": f"{SITE}/en"}, noindex=True)] + assert contradictions.check_hreflang_noindex(pages) == [] + + +def test_x_default_is_exempt(): + # x-default is a fallback pointer, not a language pair; site.alternates already + # excludes it, and this pins that so a change there turns this red. + pages = [alt_page(f"{SITE}/en", {"x-default": f"{SITE}/fr"}), + alt_page(f"{SITE}/fr", {}, noindex=True)] + assert contradictions.check_hreflang_noindex(pages) == [] + + +def test_one_finding_per_declaring_page_and_bad_target_pair(): + pages = [alt_page(f"{SITE}/en", {"fr": f"{SITE}/fr", "de": f"{SITE}/de"}), + alt_page(f"{SITE}/fr", {}, noindex=True), + alt_page(f"{SITE}/de", {}, noindex=True)] + findings = contradictions.check_hreflang_noindex(pages) + assert len(findings) == 2 + assert {f.url for f in findings} == {f"{SITE}/en"} + assert sorted(f.observed for f in findings) != [], "both targets are named" diff --git a/tests/test_registry.py b/tests/test_registry.py index a22a678..9add59b 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -61,9 +61,10 @@ def test_registry_covers_all_forty_eight_findings(): # v0.4.0 Task 3: +5 for gates/security.py's four response-header gates # (hsts fires two distinct ids). Task 4: +2 for mixed-content and # https-redirect. Task 6: +1 for seo.robots-sitemap.disallowed. Task 7: +3 - # for the three canonical-target ids. Task 12 retitles/rebases this once - # the full v0.4.0 gate set has landed. - assert len(REGISTRY) == 59 + # for the three canonical-target ids. Task 8: +1 for + # seo.hreflang-noindex.alternate. Task 12 retitles/rebases this once the + # full v0.4.0 gate set has landed. + assert len(REGISTRY) == 60 def test_tier_distribution_matches_the_fixability_classification(): @@ -77,9 +78,10 @@ def test_tier_distribution_matches_the_fixability_classification(): # infrastructure (https-redirect, an origin-level redirect). Task 6: +1 # advisory (seo.robots-sitemap.disallowed). Task 7: +2 advisory # (canonical-target .noindexed/.not-found) and +1 templated - # (canonical-target .redirects). + # (canonical-target .redirects). Task 8: +1 advisory + # (seo.hreflang-noindex.alternate). assert counts == {"mechanical": 4, "templated": 17, "drafted": 12, - "advisory": 14, "infrastructure": 12} + "advisory": 15, "infrastructure": 12} def test_mechanical_tier_is_exactly_the_four_documented_ids(): From 86dff802150a8d1b488bacbf0a656cb0659dc1bd Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:46:32 +0600 Subject: [PATCH 09/19] feat(jsonld): validate Google's required rich-result properties Eight types, expressed as alternatives groups so 'Product needs name AND (offers OR aggregateRating OR review)' is one table entry. One finding id for the whole class: the fix is always the same action, so the tier is identical, and a page missing six properties is one bad template rather than six defects -- which is also what keeps GATE_CAP charging it once. warning, not error: SS6 of the gap analysis warns that a stale required-property table fabricates errors, and this table has no freshness test yet. RICH_RESULT_RULES_AS_OF is named in every finding so its age is inspectable, and the fix text states plainly that the markup is valid schema.org and that what is unmet is Google's rich-result requirement -- two different claims. Registry counters (test_registry.py) updated incrementally rather than deferred to Task 12, to keep the suite green at every commit: 60 -> 61 total ids, drafted 12 -> 13. --- schemas/omnirank.config.schema.json | 2 +- scripts/py/omnirank/gates/jsonld.py | 147 +++++++++++++++++++++++++ scripts/py/omnirank/registry.py | 5 + tests/test_audit.py | 3 +- tests/test_config_schema.py | 6 + tests/test_gates_jsonld.py | 163 ++++++++++++++++++++++++++++ tests/test_registry.py | 12 +- 7 files changed, 331 insertions(+), 7 deletions(-) diff --git a/schemas/omnirank.config.schema.json b/schemas/omnirank.config.schema.json index 84d6dc1..f91d208 100644 --- a/schemas/omnirank.config.schema.json +++ b/schemas/omnirank.config.schema.json @@ -143,7 +143,7 @@ "response-time", "page-weight", "compression", "render-blocking", "hsts", "nosniff", "csp", "referrer-policy", "mixed-content", "https-redirect", "robots-sitemap", - "canonical-target", "hreflang-noindex"] + "canonical-target", "hreflang-noindex", "schema-required"] } } } diff --git a/scripts/py/omnirank/gates/jsonld.py b/scripts/py/omnirank/gates/jsonld.py index 985e814..3ded36b 100644 --- a/scripts/py/omnirank/gates/jsonld.py +++ b/scripts/py/omnirank/gates/jsonld.py @@ -98,6 +98,152 @@ def _fabrication(blocks: list[dict], url: str) -> list[Finding]: return findings +# The date this table was transcribed from Google's rich-result documentation. +# Named in every finding: the table is OmniRank's reading of Google's docs on one +# day, not a live contract, and a reader is entitled to judge its age. SS6 of +# docs/research/2026-08-04-competitive-gap-analysis.md is explicit that a stale +# required-property table produces FABRICATED errors -- which is why these findings +# are `warning` and not `error` until a freshness test exists to back them. +RICH_RESULT_RULES_AS_OF = "2026-08-04" + +# type -> the groups of properties Google documents as REQUIRED for that type's +# rich result. Each inner tuple is an alternatives group: at least one member must +# be present. That single shape expresses both "Article needs headline AND image +# AND datePublished" (three one-member groups) and "Product needs name AND (offers +# OR aggregateRating OR review)" (a one-member group plus a three-member one). +# +# These are GOOGLE's requirements, not schema.org's -- schema.org marks no property +# required at all, so a page can be flawless schema.org and still ineligible for +# the rich result. Every finding says so. +RICH_RESULT_RULES: dict[str, tuple[tuple[str, ...], ...]] = { + "Article": (("headline",), ("image",), ("datePublished",)), + "NewsArticle": (("headline",), ("image",), ("datePublished",)), + "BlogPosting": (("headline",), ("image",), ("datePublished",)), + "Product": (("name",), ("offers", "aggregateRating", "review")), + "FAQPage": (("mainEntity",),), + "BreadcrumbList": (("itemListElement",),), + "Organization": (("name",), ("url",)), + "LocalBusiness": (("name",), ("address",)), +} + +# A node carrying @id and nothing but @id/@type/@context is a REFERENCE to an +# entity declared elsewhere -- the `@id` cross-reference pattern the design spec +# recommends. Checking it for required properties would flag correct markup. +_REFERENCE_KEYS = frozenset({"@id", "@type", "@context"}) + + +def _types_of(node: dict) -> list[str]: + raw = node.get("@type", "") + values = raw if isinstance(raw, list) else [raw] + return [v for v in values if isinstance(v, str)] + + +def _present(node: dict, prop: str) -> bool: + """True when `prop` has a value worth calling present. + + An empty string, an all-whitespace string, an empty list and an empty object + are all absent for this purpose -- they are what a template emits when its data + did not arrive. A numeric 0 or a False is present: those are real values. + """ + value = node.get(prop) + if value is None: + return False + if isinstance(value, str): + return bool(value.strip()) + if isinstance(value, (list, tuple, dict)): + return len(value) > 0 + return True + + +def _as_nodes(value: object) -> list[dict]: + items = value if isinstance(value, list) else [value] + return [item for item in items if isinstance(item, dict)] + + +def _faq_missing(node: dict) -> list[str]: + """`acceptedAnswer` when no mainEntity Question carries one.""" + questions = _as_nodes(node.get("mainEntity")) + if questions and not any(_present(q, "acceptedAnswer") for q in questions): + return ["acceptedAnswer (on a mainEntity Question)"] + return [] + + +def _listitem_has_name(item: dict) -> bool: + """Google documents a ListItem name either at the top level or under `item`. + + Both shapes are valid and widely emitted; checking only the top level would + report a fabricated error on entirely correct markup. + """ + if _present(item, "name"): + return True + nested = item.get("item") + return isinstance(nested, dict) and _present(nested, "name") + + +def _breadcrumb_missing(node: dict) -> list[str]: + items = _as_nodes(node.get("itemListElement")) + if not items: + return [] + missing: list[str] = [] + if any(not _present(item, "position") for item in items): + missing.append("position (on an itemListElement)") + if any(not _listitem_has_name(item) for item in items): + missing.append("name (on an itemListElement)") + return missing + + +def _missing_for_type(node: dict, type_name: str) -> list[str]: + missing: list[str] = [] + for group in RICH_RESULT_RULES[type_name]: + if not any(_present(node, prop) for prop in group): + missing.append(" or ".join(group)) + if type_name == "FAQPage": + missing.extend(_faq_missing(node)) + elif type_name == "BreadcrumbList": + missing.extend(_breadcrumb_missing(node)) + return missing + + +def _required_properties(blocks: list[dict], url: str) -> list[Finding]: + """Google's REQUIRED rich-result properties, per top-level typed node. + + Top-level nodes only (extract_blocks has already flattened @graph). Walking + into nested objects would flag the reference stubs and partial sub-objects that + correct markup is full of. + + One finding per NODE, listing every missing property, and one finding ID for + the whole class: the fix is the same action every time (author the value), so + the fix tier is identical, and a page missing six properties is one bad + template rather than six separate defects -- which is also what keeps GATE_CAP + charging this the once. + """ + findings: list[Finding] = [] + for node in blocks: + if "@id" in node and not (set(node) - _REFERENCE_KEYS): + continue # a reference, not a declaration + missing: list[str] = [] + for type_name in _types_of(node): + if type_name in RICH_RESULT_RULES: + missing.extend(_missing_for_type(node, type_name)) + if not missing: + continue + types = "/".join(t for t in _types_of(node) if t in RICH_RESULT_RULES) + findings.append(_f( + "seo.schema-required.missing-property", "schema-required", url, + "warning", + f"{types} node missing {', '.join(missing)}", + f"the properties Google documents as required for a {types} rich result", + f"Add {', '.join(missing)} to the {types} markup. This node is valid " + "schema.org -- schema.org marks no property as required, so nothing " + "here is malformed. What is unmet is GOOGLE's rich-result requirement " + f"for {types}, as transcribed on {RICH_RESULT_RULES_AS_OF} into " + "RICH_RESULT_RULES in gates/jsonld.py: without these the page stays " + "valid and simply is not eligible for the rich result. Eligibility is " + "also not display -- Google decides per query whether to show one, and " + "OmniRank cannot observe that.")) + return findings + + def run(html: str, url: str) -> list[Finding]: raws = _raw_scripts(html) if not raws: @@ -132,4 +278,5 @@ def run(html: str, url: str) -> list[Finding]: "Add @context, or nest the node inside a block that declares it.")) findings.extend(_fabrication(blocks, url)) + findings.extend(_required_properties(blocks, url)) return findings diff --git a/scripts/py/omnirank/registry.py b/scripts/py/omnirank/registry.py index a9ddab9..c3e71ae 100644 --- a/scripts/py/omnirank/registry.py +++ b/scripts/py/omnirank/registry.py @@ -85,6 +85,11 @@ def _r(id_: str, tier: FixTier, severity: str, layer: str, gate: str, "schema-fabrication"), _r("seo.schema-fabrication.anonymous-review", "advisory", "error", "seo", "schema-fabrication"), + # `drafted`: the missing value is content -- a headline, an image URL, a + # postal address -- that a human must author or approve. There is no + # constant and no repo fact that supplies it. + _r("seo.schema-required.missing-property", "drafted", "warning", "seo", + "schema-required"), # --- gates/aeo.py ----------------------------------------------------- _r("aeo.answer-block.missing", "drafted", "error", "aeo", "answer-block"), _r("aeo.answer-block.length", "drafted", "error", "aeo", "answer-block"), diff --git a/tests/test_audit.py b/tests/test_audit.py index 012803f..dea9fb5 100644 --- a/tests/test_audit.py +++ b/tests/test_audit.py @@ -16,7 +16,8 @@

A Good Title

%s
diff --git a/tests/test_config_schema.py b/tests/test_config_schema.py index e1c9901..86a4ad1 100644 --- a/tests/test_config_schema.py +++ b/tests/test_config_schema.py @@ -169,3 +169,9 @@ def test_hreflang_noindex_is_accepted_in_fail_on(): cfg = minimal() cfg["audit"] = {"failOn": ["hreflang-noindex"]} assert list(validator().iter_errors(cfg)) == [] + + +def test_schema_required_is_accepted_in_fail_on(): + cfg = minimal() + cfg["audit"] = {"failOn": ["schema-required"]} + assert list(validator().iter_errors(cfg)) == [] diff --git a/tests/test_gates_jsonld.py b/tests/test_gates_jsonld.py index 1378913..052cfc1 100644 --- a/tests/test_gates_jsonld.py +++ b/tests/test_gates_jsonld.py @@ -170,3 +170,166 @@ def test_malformed_block_does_not_hide_other_blocks(): found = ids(jsonld.run(html, URL)) assert "seo.schema.malformed" in found assert "seo.schema-fabrication.anonymous-review" in found + + +from omnirank.gates.jsonld import RICH_RESULT_RULES, RICH_RESULT_RULES_AS_OF + + +def ld(payload: str) -> str: + return (f'' + "") + + +def required_findings(html: str) -> list: + return [f for f in jsonld.run(html, "https://x.example/") + if f.id == "seo.schema-required.missing-property"] + + +def test_an_article_without_date_published_is_flagged(): + html = ld('{"@context":"https://schema.org","@type":"Article",' + '"headline":"H","image":"https://x.example/i.png"}') + found = required_findings(html) + assert len(found) == 1 + assert found[0].severity == "warning" + assert found[0].layer == "seo" + assert found[0].gate == "schema-required" + assert "Article" in found[0].observed + assert "datePublished" in found[0].observed + + +def test_a_complete_article_is_not_flagged(): + html = ld('{"@context":"https://schema.org","@type":"Article","headline":"H",' + '"image":"https://x.example/i.png","datePublished":"2026-01-01"}') + assert required_findings(html) == [] + + +def test_blogposting_and_newsarticle_share_the_article_rules(): + for type_name in ("BlogPosting", "NewsArticle"): + html = ld(f'{{"@context":"https://schema.org","@type":"{type_name}",' + '"headline":"H"}') + found = required_findings(html) + assert len(found) == 1, type_name + assert "image" in found[0].observed and "datePublished" in found[0].observed + + +def test_one_finding_per_node_listing_every_missing_property(): + html = ld('{"@context":"https://schema.org","@type":"Article"}') + found = required_findings(html) + assert len(found) == 1, "one finding per node, not one per property" + for prop in ("headline", "image", "datePublished"): + assert prop in found[0].observed, prop + + +def test_product_accepts_any_one_of_offers_rating_or_review(): + for alternative in ("offers", "aggregateRating", "review"): + html = ld('{"@context":"https://schema.org","@type":"Product","name":"P",' + f'"{alternative}":{{"@type":"Thing"}}}}') + assert required_findings(html) == [], alternative + + +def test_a_product_with_none_of_the_three_alternatives_is_flagged(): + html = ld('{"@context":"https://schema.org","@type":"Product","name":"P"}') + found = required_findings(html) + assert len(found) == 1 + for alternative in ("offers", "aggregateRating", "review"): + assert alternative in found[0].observed, alternative + + +def test_a_faqpage_whose_questions_have_no_accepted_answer_is_flagged(): + html = ld('{"@context":"https://schema.org","@type":"FAQPage","mainEntity":' + '[{"@type":"Question","name":"Q?"}]}') + found = required_findings(html) + assert len(found) == 1 + assert "acceptedAnswer" in found[0].observed + + +def test_a_faqpage_with_an_accepted_answer_passes(): + html = ld('{"@context":"https://schema.org","@type":"FAQPage","mainEntity":' + '[{"@type":"Question","name":"Q?","acceptedAnswer":' + '{"@type":"Answer","text":"A"}}]}') + assert required_findings(html) == [] + + +def test_a_breadcrumb_item_missing_position_is_flagged(): + html = ld('{"@context":"https://schema.org","@type":"BreadcrumbList",' + '"itemListElement":[{"@type":"ListItem","name":"Home"}]}') + found = required_findings(html) + assert len(found) == 1 + assert "position" in found[0].observed + + +def test_a_breadcrumb_name_nested_under_item_is_accepted(): + # Google documents BOTH shapes. Flagging the nested one would be a fabricated + # error on entirely valid markup. + html = ld('{"@context":"https://schema.org","@type":"BreadcrumbList",' + '"itemListElement":[{"@type":"ListItem","position":1,' + '"item":{"@id":"https://x.example/","name":"Home"}}]}') + assert required_findings(html) == [] + + +def test_organization_needs_name_and_url(): + html = ld('{"@context":"https://schema.org","@type":"Organization","name":"X"}') + found = required_findings(html) + assert len(found) == 1 and "url" in found[0].observed + + +def test_localbusiness_needs_name_and_address(): + html = ld('{"@context":"https://schema.org","@type":"LocalBusiness","name":"X"}') + found = required_findings(html) + assert len(found) == 1 and "address" in found[0].observed + + +def test_an_untyped_or_unknown_type_is_left_alone(): + html = ld('{"@context":"https://schema.org","@type":"WebPage","name":"X"}') + assert required_findings(html) == [], ( + "OmniRank has no rich-result rule for WebPage and must not invent one") + + +def test_a_pure_id_reference_stub_is_not_flagged(): + # A node that is only @type + @id is a REFERENCE to an entity declared + # elsewhere, not a declaration missing its properties. + html = ld('{"@context":"https://schema.org","@graph":[' + '{"@type":"Organization","@id":"https://x.example/#org"},' + '{"@type":"WebPage","@id":"https://x.example/#page"}]}') + assert required_findings(html) == [] + + +def test_a_whitespace_only_value_does_not_count_as_present(): + html = ld('{"@context":"https://schema.org","@type":"Organization",' + '"name":" ","url":"https://x.example/"}') + found = required_findings(html) + assert len(found) == 1 and "name" in found[0].observed + + +def test_an_empty_list_value_does_not_count_as_present(): + html = ld('{"@context":"https://schema.org","@type":"FAQPage","mainEntity":[]}') + found = required_findings(html) + assert len(found) == 1 and "mainEntity" in found[0].observed + + +def test_a_multi_typed_node_is_checked_against_every_rule_it_declares(): + html = ld('{"@context":"https://schema.org",' + '"@type":["Organization","LocalBusiness"],"name":"X"}') + found = required_findings(html) + assert len(found) == 1 + assert "url" in found[0].observed and "address" in found[0].observed + + +def test_the_finding_distinguishes_schema_org_validity_from_google_eligibility(): + html = ld('{"@context":"https://schema.org","@type":"Article"}') + fix = required_findings(html)[0].fix + assert "schema.org" in fix, "the text must name whose requirement is unmet" + assert "Google" in fix + assert RICH_RESULT_RULES_AS_OF in fix, ( + "the table's transcription date must be inspectable from the finding") + assert "RICH_RESULT_RULES" in fix + + +def test_every_documented_type_has_at_least_one_requirement_group(): + assert set(RICH_RESULT_RULES) >= { + "Article", "BlogPosting", "NewsArticle", "Product", "FAQPage", + "BreadcrumbList", "Organization", "LocalBusiness"} + for type_name, groups in RICH_RESULT_RULES.items(): + assert groups, type_name + for group in groups: + assert group and all(isinstance(p, str) for p in group), type_name diff --git a/tests/test_registry.py b/tests/test_registry.py index 9add59b..c12c3c9 100644 --- a/tests/test_registry.py +++ b/tests/test_registry.py @@ -62,9 +62,10 @@ def test_registry_covers_all_forty_eight_findings(): # (hsts fires two distinct ids). Task 4: +2 for mixed-content and # https-redirect. Task 6: +1 for seo.robots-sitemap.disallowed. Task 7: +3 # for the three canonical-target ids. Task 8: +1 for - # seo.hreflang-noindex.alternate. Task 12 retitles/rebases this once the - # full v0.4.0 gate set has landed. - assert len(REGISTRY) == 60 + # seo.hreflang-noindex.alternate. Task 9: +1 for + # seo.schema-required.missing-property. Task 12 retitles/rebases this once + # the full v0.4.0 gate set has landed. + assert len(REGISTRY) == 61 def test_tier_distribution_matches_the_fixability_classification(): @@ -79,8 +80,9 @@ def test_tier_distribution_matches_the_fixability_classification(): # advisory (seo.robots-sitemap.disallowed). Task 7: +2 advisory # (canonical-target .noindexed/.not-found) and +1 templated # (canonical-target .redirects). Task 8: +1 advisory - # (seo.hreflang-noindex.alternate). - assert counts == {"mechanical": 4, "templated": 17, "drafted": 12, + # (seo.hreflang-noindex.alternate). Task 9: +1 drafted + # (seo.schema-required.missing-property). + assert counts == {"mechanical": 4, "templated": 17, "drafted": 13, "advisory": 15, "infrastructure": 12} From 92eb7b8a7069e5baa245c1617eaefc8a9ab22866 Mon Sep 17 00:00:00 2001 From: Moshiur Rahman Date: Wed, 5 Aug 2026 02:51:18 +0600 Subject: [PATCH 10/19] feat(onpage): add alt, heading-order, link-text and lang gates alt="" is never flagged -- it is the spec's own decorative marker, and a check that fires on it tells users to make their markup worse. role=presentation, role=none and aria-hidden are honoured for the same reason. Only the first heading skip is reported, and only an INCREASE of more than one level counts. Links inside