"Unit tests" CI workflow fails for PRs from forks

[inga-lovinde]

Describe the bug
PRs from forks (including all PRs from external contributors, per https://github.com/n8n-io/n8n/blob/n8n%401.11.0/CONTRIBUTING.md ) cannot be merged, because "Unit tests" workflow is failing on "Run actions/checkout" step.

For example, it is failing for #7377

To Reproduce
Steps to reproduce the behavior:

1. Fork this repository;
2. Create a branch with a random name, make any changes in it;
3. Submit a PR;
4. Observe that "Unit tests" workflow is failing.

Expected behavior
"Unit tests" workflow is completing successfully.

Environment (please complete the following information):

• OS: irrelevant;
• n8n Version: latest main branch; present since 1.8.1;
• Node.js Version: irrelevant;
• Database system: irrelevant;
• Operation mode: irrelevant.

Security notice

This issue has a minor security impact: a malicious actor can create a branch in their fork with the same name as the known good feature branch in upstream repository, make seemingly benign but actually breaking changes, submit a PR, and then "Unit tests" workflow will presumably pass for that PR (by running tests against the existing feature branch in upstream repository), even though tests would actually fail for their changes.

Additional context

• The workflow fails because it tries to check out the source branch by name from the upstream repo, where of course it doesn't exist. All other recent PRs for which the workflow succeeded, were submitted by n8n employees, from branches in upstream repository (not from forks), so of course checking out the branch by name works. Checking out only fails when the source branch is not in upstream but in fork; that is, only for external contributors such as me (because you seem to all work on upstream repository, not on your own forks).
• Other workflows, such as "Lint changes", succeed for such PRs because they check out a special merge branch for this PR, refs/remotes/pull/PR_ID/merge. One consequence of that is that even for internal contributors, linting and testing in CI is done for different versions of code; while linting is done on a merge of source and target branches, testing is only done on a source branch (which might not include the latest changes from the target branch).
• It seems that in August, test workflows were running against PR merge branch rather than against source branch. It seems that this changed in ci: Add reusable unit test workflow (no-changelog) #7159
• In ci: Add reusable unit test workflow (no-changelog) #7159, instead of calling checkout action directly in ci-pull-requests.yml, testing workflow code was unified between ci-pull-requests.yml and ci-master.yml, slightly changing the behavior:
  • Before ci: Add reusable unit test workflow (no-changelog) #7159, ci-pull-requests.yml called checkout with ref: ${{ inputs.branch }} (which apparently was empty, judging by workflow logs from that time, e.g. https://github.com/n8n-io/n8n/actions/runs/6157004828/job/16706916335)
    • For empty ref values, checkout@v3 action falls back to github.context.ref, which in turn is defined as "For workflows triggered by pull_request, this is the pull request merge branch"
  • In ci: Add reusable unit test workflow (no-changelog) #7159, this was changed to computing ref values manually in units-tests-dispatch.yml, based on inputs.ref and inputs.prNumber, both being optional parameters... with ci-pull-requests.yml only passing ref: ${{ github.event.pull_request.head.ref }}. So even though the intent of units-tests-dispatch.yml apparently was to support running for merge branches, it needed prNumber for that, which ci-pull-requests.yml didn't pass.
  • What's more, it seems that the code in units-tests-dispatch.yml is not even actually used, with both ci-pull-requests.yml and ci-master.yml calling units-tests-reusable.yml, and with no trace of the "prepare" step in workflow logs. But I'm not an expert in github workflows, so maybe I'm missing how this dispatch code will run.

Unfortunately, there is no public description or comments or links in #7159, so it is not clear what was the intent of the behavior changes (PR title makes it seem as if it was just code reorganization which is not supposed to change the behavior). For pull requests, there are three options basically:

1. Run workflow against the source branch (the bug then is that the wrong repository is getting checked out; another bug is that cacheKey in tests workflow refers to merge commit, which means that changing the target branch will likely reset the cache, even though the same code should be tested);
2. Run workflow against the head readonly PR branch, which is generally the same code as in the source branch (and the bug then is that it doesn't seem to be implemented anywhere; another bug, just like in option 1, is that wrong cacheKey is used);
3. Run workflow against the merge readonly PR branch, just like it worked before ci: Add reusable unit test workflow (no-changelog) #7159, and just like linting and building workflows are doing now. In this case, one way to fix it would be to remove units-tests-dispatch.yml entirely, to remove ref parameter value from ci-pull-requests.yml (and rely on checkout action inferring it instead), to remove default ref value from units-tests-reusable.yml, and probably update ci-master.yml to pass main branch name explicitly (or maybe to pass nothing and rely on checkout action in this workflow too) instead of ${{ inputs.branch }} (I'm not even sure if ${{ inputs.branch }} means anything for workflows triggered automatically by pushes?)

But being an outside observer only, I don't know which of the three options is right for you.

[inga-lovinde]

Additionally, after submitting this issue, I received a notification that "Check issue template" workflow is failing because repository it relies on doesn't exist: https://github.com/n8n-io/n8n/actions/runs/6497229064

[Joffcom]

@inga-lovinde Thanks for the report, I had reported issues with merging community PRs the other day, I have just raised it again. The issue with the issue template will be resolved soon when I replace it with something else 😃

[Joffcom]

Hey @inga-lovinde,

The issue with the PRs has been resolved and the CI tests are now working as we expect them to. The other issue with the notifications when opening issues may be resolved later this week or next week. For now I am going to mark this one as closed, Let me know if you have any questions about this.