Fix!: Harden fork pull request handling by ModeSevenIndustrialSolutions · Pull Request #384 · lfreleng-actions/github2gerrit-action

ModeSevenIndustrialSolutions · 2026-08-24T08:36:06Z

Fixes #380
Fixes #381

Makes fork pull request handling reliable and safe. The maintainer-approval
gate (#383) and comment-command authorisation (#382) follow separately, on top
of this.

Why fork PRs currently fail

actions/checkout refuses to check out fork pull request code under a
pull_request_target trigger. Every fork PR dies at the checkout step before
any of the action's own logic runs — see
opendaylight/mdsal PR 29,
which failed three times this way.

The checkout is redundant for pull requests

GITHUB_WORKSPACE is never referenced in src/. Both entry points build the
Orchestrator against a fresh tempfile.TemporaryDirectory(), and
_prepare_workspace_checkout performs its own git init, remote add and
fetch of refs/pull/<N>/head into it. The runner checkout was cloned and
immediately discarded.

Removing it is preferable to setting allow-unsafe-pr-checkout: true, which
would silence the guard while the tool still materialises fork content in the
trusted context.

Push events keep their checkout. Merged-PR reconciliation reads commit
trailers with a local git show (extract_pr_url_from_commit), and commit
enumeration falls back to git log in the working directory. Removing it
outright would have made reconciliation fail silently.

Ordering there is load-bearing: actions/checkout deletes the entire contents
of its target directory when that directory is not already a git repository,
regardless of clean. The reusable workflow therefore seeds the workspace root
with the target repository before placing the action at .g2g-action, and the
composite checkout sets clean: false. Neither measure is sufficient alone —
together they replace what SKIP_CHECKOUT used to guard.

Fork-controlled `.gitreview`

The workspace tree comes from refs/pull/<N>/head, so on a fork PR
.gitreview is authored by someone without write access to the base
repository. _resolve_gerrit_info gave that file unconditional precedence over
the GERRIT_SERVER and GERRIT_PROJECT inputs, and _derive_repo_names took
the project from it. Pinned known_hosts plus StrictHostKeyChecking=yes
confined host redirection, but the project name was unconstrained — a fork
could push a change into a different project on the same Gerrit server.

Why not simply ignore `.gitreview` for forks

Because the fallback would then be the project-name heuristic in
_derive_repo_names, which maps - to /. Against the opendaylight org:

GitHub mirror	Real Gerrit project	Heuristic
`mdsal`	`mdsal`	`mdsal`	ok
`integration-distribution`	`integration/distribution`	`integration/distribution`	ok
`releng-builder`	`releng/builder`	`releng/builder`	ok
`ofextensions-circuitsw`	`ofextensions/circuitsw`	`ofextensions/circuitsw`	ok
`of-config`	`of-config`	`of/config`	wrong

That would silently push of-config fork PRs to a nonexistent project, and
would move every fork PR onto the least-exercised branch of the resolver.

What this does instead

Reads the base repository's .gitreview, pinned to the base ref. It carries
the same authority, sits outside an attacker's reach, and costs no accuracy.

For an untrusted tree the lookup accepts only an authoritative answer: the API
strategy is declined without a base ref (get_contents(ref=None) would read the
default branch), and the master/main raw candidates are dropped. Either
would answer for a branch the PR does not target, and that answer outranks
explicit inputs.

Three resolvers now share the rule — the orchestrator (_read_gitreview),
duplicate detection, and the early config host derivation, which runs before any
PR context exists and takes provenance from PR_HEAD_REPO.

Trust is separate from the fork question

is_fork_pr answers a factual question and returns False when provenance is
unknown. Trust is distinct, and lives in one place:

def head_repo_is_trusted(repository: str, head_repo: str) -> bool:
    if not repository.strip() or not head_repo.strip():
        return False
    return head_repo.strip().lower() == repository.strip().lower()

Unknown provenance is untrusted. _augment_pr_refs_if_needed resolves it via
the API for any event lacking it (issue_comment, workflow_dispatch, direct
URL, and pull_request_review ready for #383), and bulk contexts take
base_ref, head_ref and head_repo from each pull request object.

Trusted heads keep their existing precedence, so same-repository PRs behave
exactly as before — including in the config layer, where dropping the head ref
would otherwise let derivation record one host while the pipeline pushes to
another.

Breaking change

SKIP_CHECKOUT is removed rather than kept as a no-op. Callers still passing it
get a non-fatal unexpected-input warning, which is the only signal that call
sites need updating.

Validation

uv run pytest tests/ — full suite passes
tests/test_fork_pr_hardening.py covers fork detection, the trust predicate,
.gitreview isolation, base-ref pinning, unauthoritative-fallback refusal,
duplicate-detection hints, config-layer provenance, unchanged same-repo and
push behaviour, and provenance plumbing
Checkout tests assert the real invariant (exactly one checkout, push-gated, no
PR ref) and were verified load-bearing by removing the step
Two issue_157 regression tests rewritten: they asserted head-ref-first
ordering, and now assert a fork head ref is ignored and the base ref preferred
prek run --all-files, zizmor --persona auditor (no findings), aislop
(score 100, zero findings, unchanged from base)

Also clears the process-wide Gerrit base-path cache between tests (#385); two
discovery tests leaked r for gerrit.example.org into every later test using
that fixture host.

Follow-up

PR comment commands accept directives from any user #382 — comment commands accept directives from any user
Gate fork PR transfer on maintainer approval #383 — gate fork PR transfer on maintainer approval
Global base-path cache leaks between tests #385 — global base-path cache leaks between tests
Duplicate detection cannot distinguish derived config #386 — duplicate detection cannot distinguish derived config from explicit

Until #383 lands, AUTOMATION_ONLY (default true) remains the only control on
human fork PRs, and it closes them outright.

actions/checkout refuses to check out fork pull request code under a pull_request_target trigger, so every fork PR failed at the checkout step before any of the action's own logic ran. The checkout was redundant. GITHUB_WORKSPACE is never referenced in src/; both entry points build the Orchestrator against a fresh temporary directory, and _prepare_workspace_checkout performs its own git init, remote add and fetch of refs/pull/<N>/head into it. The runner checkout was cloned and then discarded. Remove it rather than setting allow-unsafe-pr-checkout, which would silence the guard while the tool still materialises fork content in the trusted context. Removing the step also drops a fork-controlled .gitreview from the process working directory, where it fed Gerrit target resolution. BREAKING CHANGE: the SKIP_CHECKOUT input is removed. Callers still passing it get a non-fatal unexpected-input warning, which is the only signal that call sites need updating. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

The workspace tree comes from refs/pull/<N>/head, so for a fork pull request .gitreview is authored by someone without write access to the base repository. _resolve_gerrit_info gives that file unconditional precedence over the GERRIT_SERVER and GERRIT_PROJECT inputs, and _derive_repo_names takes the project from it, so a fork could redirect the push to a different Gerrit project. Pinned known_hosts confines host redirection, but the project name was unconstrained. Resolve fork pull requests against the base repository instead of dropping .gitreview support for them. The base copy carries the same authority and is not attacker-controlled, so accuracy is unchanged. Ignoring .gitreview outright would fall back to deriving the project by mapping '-' to '/', which is wrong for repositories such as opendaylight/of-config, and would move every fork PR onto the least-exercised branch of the resolver. Branch names originating in the fork are dropped from base-repository lookups for the same reason: api_ref and the raw URL branch candidates now come from base_ref, and include_env_refs is disabled so GITHUB_HEAD_REF is not consulted. Provenance arrives as PR_HEAD_REPO from the composite action, falling back to the event payload so direct CLI use behaves identically. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

Copilot

Pull request overview

Hardens fork pull request processing by avoiding unsafe checkouts and resolving Gerrit configuration from the base repository.

Changes:

Removes runner-level target repository checkout and retires SKIP_CHECKOUT.
Adds fork provenance detection and trusted .gitreview resolution.
Updates documentation and regression tests.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
`action.yaml`	Removes checkout and exports PR provenance.
`.github/workflows/github2gerrit.yaml`	Updates reusable workflow checkout behavior.
`src/github2gerrit/models.py`	Adds fork detection to GitHub context.
`src/github2gerrit/cli.py`	Populates head-repository provenance.
`src/github2gerrit/core.py`	Isolates fork-controlled `.gitreview` data.
`README.md`	Clarifies fetch-depth behavior.
`docs/cli.md`	Updates CLI option documentation.
`docs/features.md`	Documents fork handling and limitations.
`tests/test_fork_pr_hardening.py`	Adds fork-hardening regression coverage.
`tests/test_action_step_validation.py`	Validates checkout removal.
`tests/test_composite_action_coverage.py`	Updates composite-action expectations.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

1. Checkout was not redundant for push events. Merged-PR reconciliation reads commit trailers with a local git show in the working directory (extract_pr_url_from_commit), and the git log fallback that enumerates commits does the same. Removing the checkout outright made that path fail silently, closing nothing. Restore the checkout for push events only; a push carries no fork content, so the pull_request_target guard never applies. 2. Unknown provenance counted as trusted. Specific-PR workflow_dispatch runs, issue_comment payloads and direct URL invocations carry no pull request refs, and GitHub reports a null head repo for deleted forks, so head_repo stayed empty and the fork tree was read. Resolve provenance through the API whatever the event, and treat what remains unresolved as untrusted. 3. Bulk contexts copied the outer base_ref and head_ref, which are empty under workflow_dispatch, so fork PRs resolved .gitreview against the default branch rather than the branch they target. Build both refs from each pull request object. 4. The restored push checkout would have deleted the action mid-run. actions/checkout wipes its target directory when that directory is not already a git repository, whatever clean says, so the reusable workflow must seed the workspace root with the target repository before placing the action at .g2g-action. Keep that ordering and set clean: false so the later checkout preserves the action. Neither measure is sufficient alone. This is what SKIP_CHECKOUT originally guarded against. 5. Duplicate detection resolves .gitreview before the pipeline does and passed head_ref first, so a fork naming its branch after one in the base repository could steer the project used for the duplicate query. Gate that hint on the same trust predicate, now exposed as GitHubContext.head_is_trusted so both call sites share it. 6. Two checkout assertions passed vacuously: iterating an empty list succeeds, so deleting the required push checkout would not have failed them. Assert exactly one checkout step exists before asserting its properties. 7. Untrusted lookups could still answer for the default branch. api_ref=None makes get_contents read the repository default branch, and fetch_gitreview appends master/main candidates, so a base branch without .gitreview fell back to the default branch's file - which outranks explicit inputs in _resolve_gerrit_info. Decline the API strategy without an authoritative base ref, and pin untrusted lookups to the base ref alone. Same for the duplicate detection lookup. 8. Two checkout tests were named for the opposite of what they assert. Rename them for the push-only invariant, and restore the FETCH_DEPTH documentation that also covers the push checkout and the reconciliation git log window. _read_gitreview grew past the length threshold, so the local parse, branch-candidate collection and remote fetch are now separate helpers. 9. Config-layer host derivation ran before the PR context existed and still consulted GITHUB_HEAD_REF, so a fork branch named after a real base-repository branch could select that branch's host. That lookup now consults the head ref only when PR_HEAD_REPO shows the head lives in the base repository, and prefers GITHUB_BASE_REF otherwise. Keeping the head ref for a trusted head matters: _read_gitreview reads the head tree in that case, so dropping it unconditionally would let derivation record one host while the pipeline pushes to another. Two issue-157 regression tests asserted the old unconditional head-ref-first order; they now assert that a fork head ref is ignored and the base ref preferred, which serves the same original need without the fork-controlled input. The local working-directory file is still read there. Skipping it for untrusted heads was attempted and reverted: this layer cannot tell a pull request the tool is processing from one the workflow merely runs inside, so every discriminator tried also suppressed legitimate local reads. Tracked in issue 386 alongside the wider question of recording where a configuration value came from. 10. Two checkout assertions did not cover the load-bearing parts. Assert clean: false, without which the composite checkout deletes .g2g-action in the reusable push flow, and restore the ordering requirement that the checkout precedes the CLI, without which the step could be moved after it while every checkout test stayed green. Both verified by removing the thing they assert. 11. Two mappings had no coverage at all. The reusable workflow's seed checkout was unprotected because the checkout tests only parse action.yaml, so removing it or moving it below the .g2g-action checkout left the suite green while push runs deleted the action. PR_HEAD_REPO was absent from both env-mapping tests, where a typo would silently make every PR head unknown and push same-repository PRs down the untrusted path. Both verified by removal. 12. The runtime-visible FETCH_DEPTH descriptions still described it as checkout or clone depth. Align the --help text and the reusable workflow input with the corrected tables, so anyone lowering the value sees that it also narrows the reconciliation window. Also clear the process-wide Gerrit base-path cache between tests. That cache leaked 'r' for gerrit.example.org from two base-path discovery tests into every later test using the same fixture host, where it corrupted URL assertions. The leak is order-dependent, so adding or renaming test files can expose it without any change to the code under test. Co-authored-by: Claude <noreply@anthropic.com> Signed-off-by: Matthew Watkins <mwatkins@linuxfoundation.org>

Copilot

Pull request overview

Copilot reviewed 17 out of 17 changed files in this pull request and generated no new comments.

ModeSevenIndustrialSolutions and others added 2 commits August 24, 2026 09:23

ModeSevenIndustrialSolutions requested review from a team and a balanced review from Copilot August 24, 2026 08:36

ModeSevenIndustrialSolutions added the breaking-change Change potentially breaks upgrades label Aug 24, 2026

Copilot started reviewing on behalf of ModeSevenIndustrialSolutions August 24, 2026 08:36 View session

Copilot AI reviewed Aug 24, 2026

View reviewed changes

Comment thread action.yaml

Comment thread src/github2gerrit/core.py Outdated

Comment thread src/github2gerrit/cli.py Outdated

Copilot AI review requested due to automatic review settings August 24, 2026 10:07

Copilot started reviewing on behalf of ModeSevenIndustrialSolutions August 24, 2026 10:07 View session

ModeSevenIndustrialSolutions mentioned this pull request Aug 24, 2026

Global base-path cache leaks between tests #385

Open

3 tasks