Skip to content

fix(tools): refuse HTTP redirects in capture_witness_receipt and check_mirrors (#173) - #174

Open
drk-ram wants to merge 2 commits into
agentrust-io:mainfrom
drk-ram:reject-unchecked-redirects
Open

drk-ram wants to merge 2 commits into
agentrust-io:mainfrom
drk-ram:reject-unchecked-redirects

Conversation

@drk-ram

@drk-ram drk-ram commented Nov 6, 2026 •

Copy link
Copy Markdown

What and why

capture_witness_receipt.request and check_mirrors._fetch_json validated the initial URL's HTTPS scheme and host, but urllib's default redirect handler could follow a redirect without repeating those checks. Both fetch paths now reject HTTP redirects, keeping each request pinned to its validated destination.

Closes #173.

The follow-up in 9888695 adopts the unittest-discovery correction proposed in dinakarjs/trace-registry@2fa4418. Redirect regressions now use unittest.TestCase and subTest, covering capture GET/POST and mirror GET across five redirect statuses, relative/cross-host targets, and successful requests. No pytest dependency or production-handler change is needed for this follow-up.

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.

N/A — existing registry entries are unchanged.

Checks

  • pytest passes — not run for this follow-up; CI and CONTRIBUTING.md use unittest discovery instead.
  • python -m unittest discover -s tests -v — passed: 420 test methods, including the two redirect methods with 30 redirect subtests and three success controls.
  • Registry and producer files still validate (tools/validate_registry.py,
    tools/validate_producers.py) — python tools/validate_registry.py passed (three day files); python tools/validate_producers.py passed (three producer files).
  • If the anchor format or verification semantics changed, docs/anchor-format.md
    is updated in the same PR, and existing inclusion proofs still verify — N/A; anchor format and inclusion-proof semantics are unchanged.
  • Commits are signed off (DCO).

Additional local checks: ruff check . passed. The two redirect methods also passed with pytest imports explicitly blocked. In-memory replacement of the capture redirect handler produced 16 expected failing subtests; replacing the mirror handler produced 10. Source handlers were not modified by these mutation controls.

Validation was local on Windows/Python 3.11.9; hosted CI and independent re-review remain separate requirements.

Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
@drk-ram
drk-ram requested review from a team, lywinged and rajnisht7 as code owners November 6, 2026 16:30

@dinakarjs dinakarjs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 6b12633c3ec96810400e16b869ba7c7260ef0248. One test-integration correction blocks approval.

tests/test_fetch_redirects.py:8–16 imports pytest and defines parametrized module-level functions, but .github/workflows/ci.yml:122 runs python -m unittest discover -s tests -v, and the CI/witness/runtime dependency locks do not install pytest. In a fresh Python 3.12 environment installed from those locks, discovery of this module fails with ModuleNotFoundError: No module named 'pytest'. In an environment where pytest is available, unittest discovery instead collects zero tests from this module. Installing pytest alone therefore does not make CI execute these security regressions.

Please convert the cases to unittest.TestCase with subTest (the smallest change consistent with CONTRIBUTING.md), or explicitly wire a pytest runner and locked dependency into CI. Verify that the actual CI discovery command executes the redirect cases and that removing either new handler makes the relevant regression fail. Adding witness POST cases alongside GET would also preserve the submission-path coverage; this is a coverage recommendation, not a separately reproduced runtime defect.

The redirect implementation held up in my bounded checks: the 20 committed cases pass under pytest; the existing capture and mirror suites pass 21 and 17 tests respectively. Independent real-loopback controls passed 36 cases across capture GET, capture POST and mirror GET, HTTP 200/301/302/303/307/308, and relative/cross-host destinations. Removing each handler in memory reproduced a second request to the redirect destination. The original HTTPS/host guards passed six direct checks; only the loopback transport tests relaxed the HTTPS guard. Ruff passes. No source was changed.

I did not run the full repository suite or establish live witness/network deployment acceptance. Hosted CI and CodeQL were awaiting authorization at inspection. Requesting changes for CI regression integration, not for a demonstrated bypass of the new redirect handler. Reviewed with OpenAI Codex.

Copy link
Copy Markdown
Contributor

Prepared an adoption patch for the unittest-discovery correction from my review, based on head 6b12633c3ec96810400e16b869ba7c7260ef0248: commit 2fa4418.

Only tests/test_fetch_redirects.py changes. It replaces pytest-only parametrization with two unittest test methods containing 30 redirect subtests and three successful-request controls. Coverage includes capture GET, capture POST and mirror GET; HTTP 301/302/303/307/308; and relative/cross-host redirect destinations. The loopback fixtures relax only the initial HTTPS/host guard, leaving the real opener and redirect handler under test. Production handlers and dependency locks are unchanged.

Local Python 3.12 validation in a fresh environment installed from the hash-locked CI, witness and runtime requirements, with no pytest installed: python -m unittest discover -s tests -v ran 420 tests successfully. Ruff, registry validation and producer validation passed. Replacing the capture redirect-denying handler with the default handler caused 16 failing subtests; doing the same for the mirror handler caused 10. These are failure-detection controls, not additional independent test-method counts.

Your branch is unchanged. The patch is available for cherry-picking or adaptation. This supplies a tested correction but does not withdraw my changes request before adoption and re-review of the resulting head. Hosted CI and independent acceptance remain pending; no live witness or deployment validation is claimed.

Prepared with OpenAI Codex; commit DCO signed.

Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
@drk-ram

drk-ram commented Nov 10, 2026

Copy link
Copy Markdown
Author

@dinakarjs Adopted your unittest patch in 9888695. The CI command python -m unittest discover -s tests -v passes all 420 test methods, including 30 redirect subtests and three success controls covering capture GET/POST and mirror GET. The redirect methods also pass with pytest imports blocked. In-memory handler removal produces 16 expected failing capture subtests and 10 mirror failures. Both registry/producer validators and ruff check . pass. The description now cites unittest discovery rather than the earlier pytest count. Ready for re-review; no hosted-CI completion is claimed.

@dinakarjs dinakarjs left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed exact head 98886954305681eb25e40024e90f6ef8e31b9566. The redirect suite is now discovered by the workflow's unittest command and does not depend on pytest. Both focused methods passed, including all 30 redirect subtests and the three successful request controls. All 420 unittest methods passed after the workflow's editable package install. Ruff, registry validation and producer validation passed. This approval supersedes my test-integration changes request. Hosted CI and CodeQL still report action_required. Prepared with OpenAI Codex.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

capture_witness_receipt and check_mirrors follow redirects off the host allowlist

2 participants