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

Fixed Danger CI for forks #2638

Merged
merged 1 commit into from Dec 22, 2020

Conversation

RunDevelopment
Copy link
Member

I think this fixes #2627.

The basic problem with Danger and forks is that Github doesn't give Danger access to big-boy stuff like making comments. This is for security reasons. Without this limitation, malicious parties could make a fork of any repo with GH actions and then have full access to the original repo by making a PR with changed workflows. They recently added this way around the limitation.

pull_request_target makes it possible to write comments even for PRs from forks but is less secure. The basic idea is that the problem with PRs from forks is that we run untrusted code (that's why it only has read-only access). So, if we never checkout the PR code and instead run the workflow on the base branch, then we will only run trusted code. This means that we can't execute malicious code (at least not without downloading it ourselves).

Problem solved?
Not quite. Since we don't actually have the code of the PR, it's hard to implement anything CI-like (like linters, etc.). To work around this, I make a local branch of the PR but never check it out. The PR will only exist in git's storage. All interactions with the PR files will be through git. This means that we can get the file contents of the PR files but we can't execute them. This should make it secure to use (the workflow literally has full access to our repo, so it better be secure).

I also want to point out that pull_request_target has another problem. Since we never execute any files from PRs, it will hard to change the workflow and dangerfile in future PRs because we can only test them after merging them into master. Not ideal. We will have to review changes to those files especially carefully.
(This is also the reason why there's no message for this PR.)

You can this approach work here.

One more thing. Existing PRs won't get a message either. The trigger seems to ignore them.

Copy link
Member

@mAAdhaTTah mAAdhaTTah left a comment

Choose a reason for hiding this comment

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

This seems reasonable if it works. I'm ok with the limitations on things like ESLint. I think that's slightly disappointing only cuz we couldn't run the perf test (which would be helpful for discussing #2597), but I know you're somewhat opposed to that anyway so it's not really an issue.

@RunDevelopment
Copy link
Member Author

slightly disappointing only cuz we couldn't run the perf test

We could. #2153 supports Prism forks but the fork's code is executed locally (with full access to all NodeJS stuff). If I changed #2153 to run all scripts (= languages + core) in a sandbox, then the benchmarking would be safe even with untrusted code. I don't know if that will skew the benchmarking results thou.

@RunDevelopment RunDevelopment merged commit 7f23ef3 into PrismJS:master Dec 22, 2020
@RunDevelopment RunDevelopment deleted the danger-ci-fix branch December 22, 2020 09:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Danger doesn't work with pull requests
2 participants