Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Allow customization of the github.event_name check #500

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

diranged
Copy link
Contributor

@diranged diranged commented Oct 10, 2023

The motivation of this PR is to allow the use of automatic merging on pull_request_target triggered builds, if so desired. See #355 (comment) and #497 for context.

Closes #355
Closes #497

Checklist

@diranged
Copy link
Contributor Author

Closes #355
Closes #497

Copy link
Contributor

@Uzlopak Uzlopak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess...

We have a github enterprise with 1000+ devs and dont need pull_request_target. But it doesnt harm, if the user knows what he does.

@simoneb
Copy link
Collaborator

simoneb commented Oct 10, 2023

If we add this, I would like to have some docs showing how to use it. What I do not want to see is that people start using it because it's supported, but then they don't do what's necessary to make this action work properly with that trigger. In fact, if you use that trigger but then you don't check out the right git commit, you end up merging a PR after running CI against a version of the code that's different from what's in the PR.

Copy link
Collaborator

@simoneb simoneb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm requesting changes just to prevent this being merged. As per the comment I added, we need to have docs showing how to use it. Because if you use this trigger but then don't check out the right version of the code, you are in bigger troubles than if you didn't use this trigger in the first place

@dancamma
Copy link

@diranged I tested the PR and it works! 🥳

However, I think we could achieve the same result without creating a new configuration entry. We can simply modify the if statement to accept both values:

if: (github.event_name == 'pull_request' || github.event_name == 'pull_request_target') && (github.actor == 'dependabot[bot]' || inputs.skip-verification == 'true')

What do you think?

@simoneb
Copy link
Collaborator

simoneb commented Oct 11, 2023

Folks the main point here is how we prevent people from misusing this. If you use pull_request_target, you're potentially merging a PR after testing it against the base branch, which would be more unsafe than not supporting that trigger in the first place.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 11, 2023

I think it is actually the best solution to have it as a configuration entry and not in the code, as it makes it possible to explicitly allow pull_request_target.

The documentation part still remains as somebody should be aware, that he is going a potential risky path.

@dancamma
Copy link

dancamma commented Oct 11, 2023

I agree; we have two distinct needs:

  1. To fix the action for the pull_request_target event.
  2. To communicate to the user in the best possible manner that this is not safe.

This PR addresses point 1, but in my opinion, it's not optimal for point 2. This is because pull_request_target events aren't explicitly disabled; instead, they don't work due to a side effect (fetch-metadata doesn't run). For point 2, my proposal is to reopen #502 with an added skip-trigger-validation. In that scenario, the action will throw an informative message if used with pull_request_target, unless the user explicitly sets a skip-verification-trigger config. I would then merge this PR to fix the action in case a user still wants to use the pull_request_target trigger

@diranged
Copy link
Contributor Author

I agree; we have two distinct needs:

  1. To fix the action for the pull_request_target event.
  2. To communicate to the user in the best possible manner that this is not safe.

This PR addresses point 1, but in my opinion, it's not optimal for point 2. This is because pull_request_target events aren't explicitly disabled; instead, they don't work due to a side effect (fetch-metadata doesn't run). For point 2, my proposal is to reopen #502 with an added skip-trigger-validation. In that scenario, the action will throw an informative message if used with pull_request_target, unless the user explicitly sets a skip-verification-trigger config. I would then merge this PR to fix the action in case a user still wants to use the pull_request_target trigger

I do agree with point 2 here... the failure to run as a by-product of the fetch-metadata failing to run was frustrating as an end user to troubleshoot and understand. I would agree that a skip-trigger-validation flag makes sense in addition to the setting I've added here.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 16, 2023

Actually the skip-trigger-validation made the less sense for me.

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 16, 2023

Imho this PR has only one flaw: It is not possibel to use this action in a workflow which listens on pull_request and pull_request_target events.

@diranged
Copy link
Contributor Author

Imho this PR has only one flaw: It is not possibel to use this action in a workflow which listens on pull_request and pull_request_target events.

That is technically true - I can't think of a scenario where that happens though... but you are right. Wouldn't putting in skip-trigger-validation as an option be a workaround in that scenario?

@Uzlopak
Copy link
Contributor

Uzlopak commented Oct 16, 2023

It seems wrong to implement a skip-trigger-validation, which is basically only to skip the validation if it is pull_request_target.

Probably would make sense if it is called allow-pull-request-target.

But still in both RPs is no documentation. That is why @simoneb blocks them, because it is a potential security risk and/or a potential error source. With documentation, if somebody opens an issue, we could than point to the documentation and close the issue.

@simoneb
Copy link
Collaborator

simoneb commented Oct 16, 2023

Yes folks, I am giving this some time because I would like us to converge on a solution which doesn't lend itself to abuse if the user is not careful enough. If that's the compromise we have to accept, I'd much rather not have the feature in the first place rather than allowing users to shoot themselves in the foot.

@diranged
Copy link
Contributor Author

I've updated my PR with some README docs...

@diranged
Copy link
Contributor Author

@simoneb Any update on this one?

@simoneb
Copy link
Collaborator

simoneb commented Jan 8, 2024

Closing this as there is no consensus on the approach.

@simoneb simoneb closed this Jan 8, 2024
@diranged
Copy link
Contributor Author

@simoneb Have you had time to think more about this for an alternative approach? It's really a shame to not be able to use this action in our environment due to this issue.

@simoneb
Copy link
Collaborator

simoneb commented Jan 22, 2024

My personal opinion is that this should not be supported because it leads to misuse and security issues due to that. I don't own this repository so if anybody has a different opinion I'm happy to hear proposals.

In general, the easiest way to see anything landing in any project is to submit a PR. Note though that whatever implementation it contains, it should be one that is secure by default.

@diranged
Copy link
Contributor Author

@simoneb :) This is the PR that I submitted... which just gives a user the option of making this work.

@simoneb simoneb reopened this Jan 22, 2024
@simoneb
Copy link
Collaborator

simoneb commented Jan 22, 2024

Fair enough, let's reopen this then. I didn't notice you had included additional documentation. This makes it look much better now. Approving

@simoneb
Copy link
Collaborator

simoneb commented Jan 23, 2024

I'm going to merge this now, the changes look good to me as they are now because they add the feature by keeping the default behavior secure, and explaining the steps that need to be taken to adopt the alternative behavior while still preserving security.

@simoneb simoneb merged commit 8206d01 into fastify:main Jan 23, 2024
5 checks passed
@simoneb
Copy link
Collaborator

simoneb commented Jan 23, 2024

@diranged this change seems to have broken the action, do you have time to look into it? https://github.com/fastify/github-action-merge-dependabot/actions/runs/7623277429

event-name:
type: string
description:
default: pull_request
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the issue of missing quotes, it should be default: 'pull_request'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@climba03003 @simoneb Can someone re-run that in debug mode (I don't have permissions) - I don't quite think this is a quoting issue, but I'd like to see how the evaluation of ${{ inputs.event-name }} goes.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@diranged I tried a few combinations of with-and-without quotes without success, may I ask some help to fix this? The easiest way to go about it is that you create a branch for the action, where you try your fixes, and push it to a branch on your own remote.

Then you set up a repo to try out the fix where you reference your own version of the action, meaning that assuming that you call the branch "fix-event-name", you'd reference it in the workflow as

uses: diranged/github-action-merge-dependabot@fix-event-name

@diranged diranged deleted the pull_request_target branch January 23, 2024 13:59
@diranged
Copy link
Contributor Author

image

That warning message is from https://github.com/dependabot/fetch-metadata/blob/924483a3d758e3539d35e512c5ca92c75c7a748d/src/dependabot/verified_commits.ts#L18-L24 - which seems to be doing a sane-check that you can't run the dependabot/fetch-metadata action on main, but instead it must run against a PR because that's how it's going to get the PR metadata about the dependencies. (it later fails at https://github.com/dependabot/fetch-metadata/blob/924483a3d758e3539d35e512c5ca92c75c7a748d/src/main.ts#L45-L47).

Looking at the last working build (https://github.com/fastify/github-action-merge-dependabot/actions/runs/7614170037) we can see it fails out with Error: This action must be used in the context of a Pull Request or with a Pull Request number - but doesn't actually fail the build because it never ran the dependabot/fetch-metadata action.
image
image

I need to see how https://github.com/fastify/github-action-merge-dependabot/pull/500/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R58 evaluates on main builds to understand why it failed..

@simoneb
Copy link
Collaborator

simoneb commented Jan 26, 2024

Meanwhile I'm reverting this PR to unbreak the main branch. #563

@simoneb
Copy link
Collaborator

simoneb commented Jan 31, 2024

image That warning message is from https://github.com/dependabot/fetch-metadata/blob/924483a3d758e3539d35e512c5ca92c75c7a748d/src/dependabot/verified_commits.ts#L18-L24 - which seems to be doing a sane-check that you can't run the `dependabot/fetch-metadata` action on _main_, but instead it must run against a PR because that's how it's going to get the PR metadata about the dependencies. (it later fails at https://github.com/dependabot/fetch-metadata/blob/924483a3d758e3539d35e512c5ca92c75c7a748d/src/main.ts#L45-L47).

Looking at the last working build (https://github.com/fastify/github-action-merge-dependabot/actions/runs/7614170037) we can see it fails out with Error: This action must be used in the context of a Pull Request or with a Pull Request number - but doesn't actually fail the build because it never ran the dependabot/fetch-metadata action. image image

I need to see how https://github.com/fastify/github-action-merge-dependabot/pull/500/files#diff-1243c5424efaaa19bd8e813c5e6f6da46316e63761421b3e5f5c8ced9a36e6b6R58 evaluates on main builds to understand why it failed..

Just making sure you noticed the comment I posted here #500 (comment)

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.

Receiving 'Warning: Semver bump '' is invalid!' message Not merging with "pull_request_target"
5 participants