[Feature/Bug] Forked PR support

[nbrugger-tgm]

When using this action on a fork-pr it breaks and fails the step.

Log

with:
    filters: sensitive:
    - 'somefile'
  
    event: pull_request_target
    base: 2bc51e424f0f82b59a5e751971cd33fd39930ef2
    head: 00d1b3fef04e6191889f4dcf17173c7f1c46ff28

/usr/bin/git fetch --prune --unshallow
From https://github.com/nbrugger-tgm/java-spring-template
 * [new branch]      feat/new-endpoint -> origin/feat/new-endpoint
 * [new branch]      main              -> origin/main
 * [new branch]      staging           -> origin/staging
##[debug]baseSha: 2bc51e424f0f82b59a5e751971cd33fd39930ef2
##[debug]headSha: 00d1b3fef04e6191889f4dcf17173c7f1c46ff28
/usr/bin/git merge-base 2bc51e424f0f82b59a5e751971cd33fd39930ef2 00d1b3fef04e6191889f4dcf17173c7f1c46ff28
fatal: Not a valid commit name 00d1b3fef04e6191889f4dcf17173c7f1c46ff28
Error: The process '/usr/bin/git' failed with exit code 128

head is the fork origin (00d1b3fef04e6191889f4dcf17173c7f1c46ff28)

If it is ok i would pick up on this issue and contribute if you can confirm that it is a bug and not just misuse. Also testing this would be done by me by adding a new pull_request (without target) workflow that just logs the result

[tony84727]

Hello, thank you for bringing this up.

Is the 'fork-pr' a PR opened from a fork to merge a fork branch into the upstream repository?

I'm not sure how to reproduce this.

Is it possible to post the fork-pr here? So we can look into the scenario and the workflow file.

[nbrugger-tgm]

Is the 'fork-pr' a PR opened from a fork to merge a fork branch into the upstream repository?

Yes

[nbrugger-tgm]

Reference:
Log: https://github.com/nbrugger-tgm/java-spring-template/runs/4180564172?check_suite_focus=true#step:6:1
PR: https://github.com/nbrugger-tgm/java-spring-template/pull/26
Workflow: Process PR / Formal Check (pull_request_target) https://github.com/nbrugger-tgm/java-spring-template/runs/4180564172?check_suite_focus=true

As the workflow file is quite big here a line link: https://github.com/nbrugger-tgm/java-spring-template/actions/runs/1449676603/workflow#L68

[tony84727]

Thank you for the details!

The culprit seems to be the pull_request_target event and perhaps this issue

Because pull_request_target workflows have access to secrets and read/write GITHUB_TOKEN.
It seems that the actions/checkout action intentionally not to checkout the merge commit to prevent adding external potentially malicious code from forks.

And there seems to be a workaround for the issue, if the user really want to accept the risk.

I've tested the workaround, adding a ref option to actions/checkout, can fix this issue.

Is there any other change you would like to add? Or maybe we can just add a document for the workaround of this case?

[nbrugger-tgm]

Since your action does not uses any file from the checked out source there should not be a security risk involved (except you check out the head source and leave it like this).

I dont know exactly how you build the diff but i think you could fall back to github.base_ref and github.target_ref (or the commit sha) when it comes to the pull_request_target event. That way you dont need to check out the PR code (which is good for performance and security) and can reliable build a git diff

Is this approach feasable?

[nbrugger-tgm]

Or maybe we can just add a document for the workaround of this case?

I would not do this as it encourages the user to introduce security vulnerabilities

[tony84727]

Since your action does not uses any file from the checked out source there should not be a security risk involved (except you check out the head source and leave it like this).

Yes, the action only "unshallow" the repository and won't modify the worktree.

(Unshallow might be inefficient for large repositroies...but I haven't figured out a way to just fetch a few commits we need to calculate the diff)

I dont know exactly how you build the diff but i think you could fall back to github.base_ref and github.target_ref (or the commit sha) when it comes to the pull_request_target event.

For github.target_ref, do you mean github.head_ref?

I think this approach is doable but I'm not sure if simply fallback to github.head_ref alone will work.

For the fork-pr scenario, "unshallow" alone won't get all the commits required to calculate the diff because the "head" or "target" commit is in another repository, the fork repository.

We will need to fetch the head commit by the github.head_ref, which should point to the appropriate commit in the remote repository.

Also, we need to be careful not to checkout the github.head_ref.

I would not do this as it encourages the user to introduce security vulnerabilities

Indeed, for figuring out the diff, we don't need to checkout the external commit. If the above approach is possible, then we don't need to compromise.

[nbrugger-tgm]

I think I know a possible solution. If you use commit sha for the git diff you could use git parse-ref ${{ github.head_ref}} to get the latest commit sha. For the base brach you could to the same or you use the merge base as you do now. Maybe you would need to fetch the fork origin using git fetch. I think doing this should solche the problem