[drk-ram]

What and why

Closes #150

• Install an allowlisting HTTPRedirectHandler in tools/check_mirrors.py (_fetch_json) and tools/capture_witness_receipt.py (request) so HTTP 301/302 redirects are re-validated against _check_url_allowed / check_url_allowed, using _build_opener(allowed_hosts).open by default unless an explicit opener is injected.
• Reject non-global IP literals in check_mirrors._allowed_hosts_from_config and check_mirrors._check_url_allowed.
• Reject duplicate JSON object keys and NaN/Infinity/-Infinity constants in both tools and require full 40- or 64-character hexadecimal sha strings (^[0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?$, normalized to lowercase) in check_mirrors._get_head_sha.

Validation

• python -m unittest discover -s tests -q — passed (414 tests)
• python -m pytest tests/test_check_mirrors.py tests/test_capture_witness_receipt.py -q — passed (46 passed, 19 subtests passed)
• python tools/validate_registry.py — passed (OK: 3 day file(s) valid)
• python tools/validate_producers.py — passed (OK: 3 producer key file(s) valid)
• python -m ruff check tools/check_mirrors.py tools/capture_witness_receipt.py tests/test_check_mirrors.py tests/test_capture_witness_receipt.py — passed

Registry impact

• This PR does not modify any existing line under registry/

Existing entries are append-only. If you are changing one, say why here: mirrors
and auditors may already have observed the current commit hashes, and a rewrite
diverges them.

Checks

• pytest passes
• Registry and producer files still validate (tools/validate_registry.py,
  tools/validate_producers.py)
• If the anchor format or verification semantics changed, docs/anchor-format.md
  is updated in the same PR, and existing inclusion proofs still verify
• Commits are signed off (DCO)

[rajnisht7]

The core fixes for redirect allowlisting and strict JSON look correct and match the existing trace_verify pattern. However, the regression coverage is incomplete for a security PR

• capture_witness_receipt.py has no new tests : PR changes redirect handling, _loads_strict, transport selection, receipt_of, registry-entry parsing, and checkpoint parsing but tests/test_capture_witness_receipt.py is untouched.

existing tests patch urllib.request.urlopen, which makes the new code take the bypass branch and never exercise the redirect-protected opener. So we currently have no proof that an allowed HTTPS endpoint redirecting to a disallowed host or to HTTP is actually rejected by request().kindly add tests that exercise the real request() path for:

• HTTPS allowlisted -> disallowed host
• HTTPS allowlisted -> HTTP (same or different host)
• duplicate keys / NaN / Infinity in witness responses, registry lines, and checkpoint

also prefer removing the _ORIG_URLOPEN identity check. Making the transport/opener injectable (or always using the protected opener and mocking that) would be cleaner and would let tests actually hit the security path.

Also worth fixing / clarifying

• Abbreviated SHA false positive - _SHA_RE still accepts 7–39 char values and _check_one treats prefix matches as in_sync. A mirror returning just the first 7 hex chars of the canonical SHA can be reported in-sync. Prefer requiring full object IDs (40 or 64) + case-normalize, or document the abbreviated path explicitly.
• Global allowlist - both tools build one combined host set. A mirror can redirect to the canonical (or another configured host) and still pass the host check. Same issue in capture between witness_base and did_url. Consider tighter per-origin policy.
• Comment vs reality - the code comments say internal/metadata targets are rejected, but any HTTPS host that appears in mirrors.json (including a metadata IP) is added to the allowlist. Current config is safe; the contract is weaker than the comment claims.
• The new check_mirrors test is useful but packs several independent checks into one method and only calls the handler directly. Separate focused tests (and ideally one that goes through _fetch_json) would be better.

[drk-ram]

Thanks @rajnisht7! Addressed in the latest commit: (1) tools/capture_witness_receipt.py: removed module-import-time _ORIG_URLOPEN, added an optional opener parameter to request(), and added unit tests in tests/test_capture_witness_receipt.py exercising request() redirect refusal (HTTPS -> HTTP and HTTPS -> disallowed host) and duplicate keys / NaN / Infinity in receipt_of and checkpoint_from_registry_entry. (2) tools/check_mirrors.py: tightened _SHA_RE to require full 40- or 64-char hex commit SHAs (^[0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?$), normalized .lower(), removed abbreviated prefix matching, scoped _get_head_sha to the entry host, and split the test cases in tests/test_check_mirrors.py.

[dinakarjs]

Reviewed this head; unittest discovery passes 412 tests. The remaining transport concern is still visible in tools/capture_witness_receipt.py: request() selects the guarded opener only when urllib.request.urlopen.module == 'urllib.request'; otherwise it calls the replacement urlopen directly. This replaces the earlier identity heuristic with a module-name heuristic rather than removing the bypass. Please use the explicit opener injection seam for tests and the guarded opener for normal requests. The PR description also still advertises 7–64 mixed-case hex whereas the implementation now requires full lowercase commit IDs; update the final description to match.

[rajnisht7]

reviewed the head 3088898, core redirect allowlisting and strict JSON look good, and the SHA tightening to full 40/64 is the right call.

One thing still needs fixing:
In capture_witness_receipt.request(), the transport selection still falls back to raw urllib.request.urlopen when its module isn’t "urllib.request" which means any monkeypatch or wrapper skips the allowlist redirect handler. Since opener= is already there as an injection point, just always use the guarded opener unless an explicit opener is passed:

if opener is not None:
    open_fn = opener.open if hasattr(opener, "open") else opener
else:
    open_fn = _build_opener(allowed_hosts).open

also:
-PR description still says ^[0-9a-fA-F]{7,64}$, update it to match the new 40/64 requirement.

• the new redirect tests inject opener= (good), but the default path isn’t exercised. After the change above, one test that calls request(...) without opener= and still rejects a bad redirect would lock it in.
• checkpoint_from_registry_entry tests cover NaN and duplicate keys but not Infinity (easy add).
• mirrors.json hosts are still added to the allowlist, so a metadata IP in config would be allowed. Current config is fine, but the “internal targets rejected” comment is stronger than the code. Worth a note or a tighter check.

[drk-ram]

Thanks @rajnisht7 and @dinakarjs! Addressed all points in the latest commit: (1) tools/capture_witness_receipt.py: request() now unconditionally uses _build_opener(allowed_hosts).open whenever opener is None (removing the __module__ == "urllib.request" fallback), capture() threads opener=opener for test injection, and test_request_rejects_redirects_to_http_or_disallowed_hosts now exercises both the injected opener path and the default request(..., opener=None) path alongside Infinity/-Infinity in checkpoint_from_registry_entry; (2) tools/check_mirrors.py: rejected non-global IP literals in both _allowed_hosts_from_config and _check_url_allowed; and (3) updated the PR description to state ^[0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?$.

[dinakarjs]

Re-reviewed a3cf132. The default opener no longer uses the urlopen module heuristic, but the committed CLI test now fails: test_main_accepts_the_entry_file_and_defaults_its_provenance returns a callable _Transport from _build_opener, while request consumes its .open method. The 412-test suite errors on that AttributeError.

I prepared a signed author-adoption companion at dinakarjs#2, final head ef8c565. It fixes the test double without changing production dispatch, and adds independent config/explicit-allowlist tests for the newly introduced non-global-IP guards, with valid public-IP controls. Full repaired suite: 414 passed; final mirror module: 23 passed; Ruff and whitespace pass. Disabling the IP predicate makes the new refusal assertions fail. No live requests or DNS-rebinding assurance claimed.

@drk-ram please adopt with attribution and request independent review. This is COMMENT because I am contributing the correction; I am not independently approving it. Hosted checks remain required. Prepared with OpenAI Codex.

[drk-ram]

Thanks @rajnisht7 and @dinakarjs! Adopted the _build_opener test-double fix in tests/test_capture_witness_receipt.py (test_main_accepts_the_entry_file_and_defaults_its_provenance) and the config/explicit-allowlist non-global-IP guard tests in tests/test_check_mirrors.py, verified Infinity/-Infinity coverage on checkpoint_from_registry_entry, rebased onto origin/main, and updated the PR description to state the exact 40/64-character hex requirement (^[0-9a-fA-F]{40}(?:[0-9a-fA-F]{24})?$). Full unittest discovery (414 tests) and Ruff pass at f10122a.

[dinakarjs]

Follow-up review at exact head f10122a, Linux/Python 3.12.14 with the repository's hash-locked CI/witness/runtime dependencies and the local editable checkout.

Verified adoption of the opener test-double correction and the independent config/explicit-allowlist non-global-IP controls. Full unittest discovery: 414 tests pass; repository Ruff passes. The exact hex-format description is corrected. This closes my earlier test-double and coverage follow-up. I contributed part of the correction, so this is incorporation verification, not independent approval of the combined change. DNS rebinding, live transport guarantees and hosted acceptance remain outside these local tests.

Reviewed with OpenAI Codex.