danger-js icon indicating copy to clipboard operation
danger-js copied to clipboard

Recommend using `pull_request_target` on GitHub Actions

Open orta opened this issue 4 years ago • 6 comments

Describe the bug Fixes the issues with secrets: https://github.blog/2020-08-03-github-actions-improvements-for-fork-and-pull-request-workflows/

Mainly need to update docs, and two maybe do a check if the action is pull_request and the PR is from a forked repo and tell folks.

orta avatar Aug 05 '20 15:08 orta

This could work, but it is complex.

Danger JS relies on the API to get all its info, like the diff etc. Which means it can handle the different head on your CI, but it would be very surprising to fs.readFileSync and get the old version of the file.

orta avatar Aug 14 '20 21:08 orta

This realistically shouldn't be recommended I think

orta avatar Sep 01 '20 15:09 orta

@orta is there no other way to use dangerjs for projects that follow a forking workflow, via GitHub Actions? I tried browsing the docs and the relevant issues and the only approaches are:

  • use pull_request_target: potentially risky and some functionalities won't work because the files checked out would not be from fork`
  • use a GitHub token (directly exposing its value) in the action: basically giving away the token, just that the token only has public repo rights
    • image

Both approaches don't seem to be very secure...

I think according to the advisor https://securitylab.github.com/research/github-actions-preventing-pwn-requests/ we could potentially explore a way to split the workflow into two portions:

  1. (Read only) test and identify the issues or comments, do not post the comments yet. Save them in artifact
  2. (With write access) Retrieve the artifacts and post the comments accordingly.

So something like this:

name: Danger Checks Fork PR

# read-only
# no access to secrets
on:
  pull_request:
    branches:
      - main

jobs:
  build:
       name: Danger Checks Fork PR
       run : yarn danger ci --check
       // save stuff into artifacts
name: Danger Write Comments for Fork PR

on:
  workflow_run:
    workflows: ["Danger Checks Fork PR"]
    types: [completed]

jobs:
  on-success:
    if: ${{ github.event.workflow_run.conclusion == 'success' }}
    // read stuff from artifacts
    name: Danger Write
    run: yarn danger ci --write
    secrets:
      token: ${{ secrets.GITHUB_TOKEN}}

This will require Danger to have

  • a way to only do the check and export the action items
  • a way to only read action items and process them

Similar things have been done for another common task facing the same challenge: creating a PR preview of the updated websites from a fork.

Do you know if the above is already available or if the whole thing is achievable with Danger?

Thanks a lot! 🙏

tlylt avatar Apr 08 '23 02:04 tlylt

Yep this should be possible.

You'd probably have to write some code to this repo to handle the separation of results and processing, but it should be possible - Danger already splits those responsibilities between separate CLI commands (so that danger swift/rust/etc can run) so it could be possible with some digging right now too

orta avatar Apr 08 '23 13:04 orta

Yep this should be possible.

You'd probably have to write some code to this repo to handle the separation of results and processing, but it should be possible - Danger already splits those responsibilities between separate CLI commands (so that danger swift/rust/etc can run) so it could be possible with some digging right now too

hmm, thank you! Will take a stab at it when I find the time.

tlylt avatar Apr 09 '23 01:04 tlylt

Yeah, I used this approach after asking at #1373.

For reference, tho I didn't use danger at last, but I hope danger can support it natively sort of like this. like danger check action and danger comment action Benchmark (check) file: https://github.com/discordeno/discordeno/blob/main/.github/workflows/benchmark.yml Comment file: https://github.com/discordeno/discordeno/blob/main/.github/workflows/commentBenchResult.yml

H01001000 avatar Dec 08 '23 00:12 H01001000