Skip to content

security(ci): acquire exact pull head as data - #16

Merged
stacknil merged 7 commits into
mainfrom
stacknil/pr-ref-acquisition
Sep 12, 2026
Merged

stacknil merged 7 commits into
mainfrom
stacknil/pr-ref-acquisition

Conversation

@stacknil

@stacknil stacknil commented Sep 9, 2026

Copy link
Copy Markdown
Owner

Summary

Add acquire_pull_snapshot(remote, pull_number, expected_head_oid, scratch_root) as the independent pull-head acquisition boundary for Issue #10. The helper creates a fresh verifier-owned bare Git database, fetches exactly the numbered pull-request head ref, requires its tip to equal the expected full OID, and invokes the bounded reader before yielding.

The PR has completed delayed security review. Its own delta against current main is 3 files / 617 additions: the acquisition helper, focused contract tests, and boundary documentation.

Design decision

Keep acquisition verifier-owned and checkout-free. Accept only credential-free HTTPS remotes, plus caller-controlled Path fixtures; initialize from an empty template; fetch one explicit depth-one force refspec; import no tags or submodules; write no FETCH_HEAD; and require the fresh database to contain only refs/repo-sentinel/acquired-head at the expected full lowercase SHA-1 or SHA-256 OID.

Inherited GIT_* state and global/system Git configuration are excluded. Terminal prompting is disabled, GIT_ASKPASS and SSH_ASKPASS are neutralized, and inherited SSH_ASKPASS_REQUIRE is removed. Replacement lookup, redirects, lazy fetch, automatic maintenance, and protocol restrictions remain unchanged. The reader remains authoritative for raw commit, tree, and blob identities, logical path admission, modes, and resource bounds. Acquisition does not rewrite paths or convert downstream refusal into success.

Validation evolution

Before the path-contract merge, the live PR #15 probe acquired the exact expected head and reached the reader, which refused unsupported_path for eight legitimate names outside the former portable ASCII subset. That historical result remains useful evidence that downstream reader refusal propagated without a partial snapshot and that the temporary database was cleaned.

After PR #17 merged at d8e30ba019247a21b9d42e1c1d52900a1f1de623, the same exact-head boundary traversed acquisition, the logical-path reader, and the portable materializer successfully. A controlled invalid-UTF-8 fixture now preserves the acquisition-to-reader refusal and cleanup regression without depending on the obsolete ASCII restriction.

The final remediation adds a real git credential fill regression. With hostile parent GIT_ASKPASS, SSH_ASKPASS, and SSH_ASKPASS_REQUIRE=force, the old behavior invoked the marker helper twice and accepted dummy credentials. The corrected environment invokes it zero times and Git fails non-interactively with terminal prompts disabled. A fetch-level authentication challenge also failed closed with no snapshot, zero marker calls, and complete cleanup.

Main remaining risk

The 64 MiB repository-size check runs after fetch. It fails an over-budget result closed, but it is an admission bound after transport rather than a hard transport or peak-disk quota. The trusted scratch owner must reserve sufficient capacity until the fetch returns or the 60-second acquisition timeout terminates the command. Git and its HTTPS transport remain trusted parsers, and the reader retains its separate timeout.

Compatibility impact

This is an additive, unwired API. Credential-bearing URLs, non-HTTPS network protocols, query or fragment URLs, partial or uppercase OIDs, missing or moved refs, excess repository bytes, and reader refusal fail closed. Ambient askpass helpers cannot convert credential-free acquisition into a private-repository credential flow.

Scope

Workflow activation remains unwired. Scanner invocation remains unwired. Existing events, reports, baseline semantics, caches, permissions, secrets, repository settings, and schemas are unchanged. The rebased PR contains no reader or materializer implementation delta against current main; the only post-review production change is child-Git askpass environment isolation.

Validation

  • Windows Microsoft Windows 10.0.26200, Python 3.14.3, Git 2.55.0.windows.5.
  • python -m unittest tests.test_repo_sentinel_acquire -v: 13 tests passed.
  • python -m unittest discover -s tests -p 'test_*.py': 71 tests passed.
  • WSL2 Linux 6.6.87.2-microsoft-standard-WSL2, Python 3.12.3, Git 2.43.0: the credential-isolation regression passed.
  • python -m pre_commit run --files docs/repo-sentinel-baseline-review.md scripts/repo_sentinel_acquire.py tests/test_repo_sentinel_acquire.py: all applicable hooks passed.
  • python -m ruff check scripts/repo_sentinel_acquire.py tests/test_repo_sentinel_acquire.py: passed.
  • Tag rendering, README snapshot rendering, 157-file Markdown validation, pattern-library validation, placeholder scope check, and git diff --check passed.
  • Read-only HTTPS probe remote: https://github.com/stacknil/sec-writeups-public.git.
  • Pull ref: refs/pull/15/head.
  • Expected and acquired commit: 2f7b7a9bef43715141086b0d79bacbe67a178288.
  • Root tree: 960dc6c6496f1260f6fab74b64f26408024cd5fb.
  • Result: 242 paths, 242 modes, 242 blob-OID fields, and 242 materialized files with exact path identity and file bytes.
  • Total content bytes: 2,260,062.
  • Manifest SHA-256: ad39acf89d0826ce2651ed14d12e143d7f0a8b5cb6b2eb219b46895688f45d0a.
  • The bare database contained only the private acquired-head ref, had no FETCH_HEAD, and was removed on exit. The materialized target was also removed on exit. Repository content was never executed.

Rollback path

Revert this PR or remove the acquisition helper, tests, and documentation. The merged reader and materializer remain independently usable. There is no persisted output, workflow deployment, credential, permission, setting, baseline, or schema migration.

Workflow activation remains a separate reviewed change and is not part of this PR.

@stacknil

stacknil commented Sep 9, 2026

Copy link
Copy Markdown
Owner Author

Post-CI review completed at cc99fa261b549565d6f49661b44d284be8fb7ad4 over base 56fdb32012f287c8fc615df711c02d41a9919052.

  • All seven GitHub checks are successful; no external or inline review comments are present.
  • All three remote file blobs exactly match the locally reviewed blobs.
  • Final scope: 3 files / 542 additions. Behavior plus tests are 494 lines; 48 lines are documentation. The history contains behavior, tests, docs, and the post-live-probe cross-layer refusal regression.
  • Fresh local evidence: 54 repository tests, applicable pre-commit hooks, Ruff, tag rendering, 157-file Markdown validation, git diff --check, and the real three-file changed gate all pass.

Design decision: acquire one fixed numbered PR ref into a fresh bare database, require the sole private ref to equal the expected full OID, then delegate object identity and path/mode admission to the existing reader. No checkout or target execution is introduced.

Main risk: the repository-size check is post-fetch, not a hard transport or peak-disk quota. Git/HTTPS remain trusted parsers and scratch capacity is a caller precondition. The real PR #15 probe also exposed a separate current-repository compatibility blocker: eight paths are outside reader admission, so the end-to-end acquisition correctly refuses rather than yielding a partial snapshot.

Compatibility impact: additive unwired API only. Existing scanner, baseline, reports, workflow events, permissions, caches, secrets, and repository settings are unchanged. No private-repository credential flow is added.

Rollback path: revert the four commits or remove the helper/tests/docs. The reader and materializer remain independently available; there is no persisted format or settings migration.

No blocking defect was found in the acquisition diff itself. Keep draft until delayed unchanged-head review on or after 2026-09-10. Do not activate the advisory workflow until path admission has a separate reviewed decision.

@stacknil
stacknil force-pushed the stacknil/pr-ref-acquisition branch from cc99fa2 to 9668476 Compare September 11, 2026 04:48
@stacknil

Copy link
Copy Markdown
Owner Author

Builder remediation is complete at b4c45e0ed9b99829027de765ce35dd0c94cbac4f.

  • Child Git now receives empty GIT_ASKPASS and SSH_ASKPASS values, while inherited SSH_ASKPASS_REQUIRE is excluded. Existing GIT_*, config, protocol, ref/OID, size, timeout, and cleanup restrictions are unchanged.
  • The real git credential fill regression seeds hostile Git/SSH askpass helpers plus SSH_ASKPASS_REQUIRE=force. The old behavior and focused mutation invoked the marker twice and accepted dummy credentials; the corrected behavior invokes it zero times and exits 128 with terminal prompts disabled.
  • A fetch-level HTTPS authentication challenge returned sanitized fetch_failed, yielded no snapshot, invoked the marker zero times, and cleaned the scratch boundary.
  • Windows validation: 13 focused acquisition tests and 71 full repository tests passed on Python 3.14.3 / Git 2.55.0.windows.5.
  • WSL2 validation: the credential-isolation regression passed on Python 3.12.3 / Git 2.43.0.
  • Pre-commit, Ruff, tag/README rendering, 157-file Markdown validation, pattern validation, placeholder scope validation, and git diff --check passed.
  • The read-only 242-file probe preserved commit 2f7b7a9bef43715141086b0d79bacbe67a178288, tree 960dc6c6496f1260f6fab74b64f26408024cd5fb, 2,260,062 bytes, and manifest ad39acf89d0826ce2651ed14d12e143d7f0a8b5cb6b2eb219b46895688f45d0a; both temporary boundaries were cleaned and repository content was not executed.
  • All seven GitHub checks completed successfully against the exact remediation head.

Design decision: close only the ambient askpass credential channel; do not redesign acquisition.

Main risk: the existing 64 MiB limit remains post-fetch rather than a hard transport/peak-disk cap.

Compatibility impact: none outside child-Git credential isolation; the PR remains an additive, unwired API.

Rollback: revert b4c45e0 to return to the independently reviewed pre-remediation head, or revert the complete PR to remove acquisition without affecting reader/materializer contracts.

PR #16 remains draft for independent unchanged-head review. No workflow was activated.

@stacknil
stacknil marked this pull request as ready for review September 12, 2026 16:39
@stacknil
stacknil merged commit c6adf40 into main Sep 12, 2026
7 checks passed
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.

1 participant