Repository navigation
Conversation
Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
dinakarjs
left a comment
There was a problem hiding this comment.
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.
|
Prepared an adoption patch for the unittest-discovery correction from my review, based on head Only Local Python 3.12 validation in a fresh environment installed from the hash-locked CI, witness and runtime requirements, with no pytest installed: 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>
|
@dinakarjs Adopted your unittest patch in 9888695. The CI command |
dinakarjs
left a comment
There was a problem hiding this comment.
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.
What and why
capture_witness_receipt.requestandcheck_mirrors._fetch_jsonvalidated 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
9888695adopts the unittest-discovery correction proposed indinakarjs/trace-registry@2fa4418. Redirect regressions now useunittest.TestCaseandsubTest, 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
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
pytestpasses — 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.tools/validate_registry.py,tools/validate_producers.py) —python tools/validate_registry.pypassed (three day files);python tools/validate_producers.pypassed (three producer files).docs/anchor-format.mdis updated in the same PR, and existing inclusion proofs still verify — N/A; anchor format and inclusion-proof semantics are unchanged.
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.