Skip to content

Switch qhelp-pr-preview.yml to pull_request_target#6971

Closed
aibaars wants to merge 8 commits into
mainfrom
aibaars/pr-qhelp-check
Closed

Switch qhelp-pr-preview.yml to pull_request_target#6971
aibaars wants to merge 8 commits into
mainfrom
aibaars/pr-qhelp-check

Conversation

@aibaars
Copy link
Copy Markdown
Contributor

@aibaars aibaars commented Oct 27, 2021

@JarLob Could you verify it this pull request is safe? The workflow requires permissions to post a comment, and therefore needs to use pull_request_target. It does however checkout the merge commit of the pull request. I think the commands run by the check are safe, and added some additional quoting to protect against command injection.

@JarLob
Copy link
Copy Markdown
Contributor

JarLob commented Oct 27, 2021

It is better to restrict the workflow permissions:
https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/
https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

Plus what we discussed on slack.

@aibaars aibaars force-pushed the aibaars/pr-qhelp-check branch from be26abd to fb15047 Compare October 27, 2021 10:39
@aibaars aibaars force-pushed the aibaars/pr-qhelp-check branch 2 times, most recently from ed800fe to 54e9469 Compare October 27, 2021 10:52
@aibaars
Copy link
Copy Markdown
Contributor Author

aibaars commented Oct 27, 2021

@JarLob I think I addressed your comments. Please let me know if I missed something.

@aibaars aibaars force-pushed the aibaars/pr-qhelp-check branch 5 times, most recently from 9193603 to 50b42b3 Compare October 27, 2021 12:49
@aibaars aibaars force-pushed the aibaars/pr-qhelp-check branch from 50b42b3 to b128c7c Compare October 27, 2021 13:58
@aibaars aibaars force-pushed the aibaars/pr-qhelp-check branch from 943cccf to 2768b3d Compare October 27, 2021 17:01
@aibaars aibaars force-pushed the aibaars/pr-qhelp-check branch 2 times, most recently from 4836ab7 to 5e2cab4 Compare October 27, 2021 17:09
- 'rc/*'
- "rc/*"
paths:
- "ruby/**/*.qhelp"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
- "ruby/**/*.qhelp"
- "**/*.qhelp"

Comment on lines +3 to +4
permissions:
contents: read
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this needed when you have the permission blocks below?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's needed. I just added a very restricted one at the top just in case.

@JarLob
Copy link
Copy Markdown
Contributor

JarLob commented Oct 28, 2021

Hey @aibaars, as we discussed yesterday I think you should make it with workflow_run and don't use pull_request_target at all. Also make the pull request from your personal fork.

@aibaars aibaars closed this Oct 28, 2021
@aibaars aibaars deleted the aibaars/pr-qhelp-check branch October 28, 2021 13:44
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.

3 participants