danger / danger-js

⚠️ Stop saying "you forgot to …" in code review
http://danger.systems/js/
MIT License
5.24k stars 367 forks source link

Recommend using `pull_request_target` on GitHub Actions #1060

Open orta opened 4 years ago

orta commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

This realistically shouldn't be recommended I think

tlylt commented 1 year ago

@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:

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

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! 🙏

orta commented 1 year ago

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

tlylt commented 1 year ago

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.

H01001000 commented 9 months ago

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