advanced-security / maven-dependency-submission-action

GitHub Action for submitting Maven dependencies
MIT License
48 stars 24 forks source link

Need to specify ref and sha when running a pull request from a fork with pull_request_target #51

Open jmservera opened 1 year ago

jmservera commented 1 year ago

I'm using this action in combination with the dependency review action, to check for new dependencies, including transient ones, when someone creates a PR. As it needs write access, the only way to obtain it when running from a fork is to use the pull_request_target and then change the head during checkout. But, if I run the action with pull_request_target it is automatically sent to the main instead of the PR head.

I see that with the command-line it is possible to specify the head, is it possible to add this field to the action?

This is my workflow:

name: 50-Dependency-Review
on:
# dependency scan works comparing pull requests to the base branch
  pull_request_target:
    # These types are all required for CRDA to scan pull requests correctly and securely.
    types: [ opened, synchronize, reopened, labeled, edited ]
    paths-ignore:
      - '**/*.md'
      - '**/*.txt'

permissions:
  contents: write

jobs:
  dependency-review:
    runs-on: ubuntu-latest
    steps:
      - name: Get User Permission
        id: checkAccess
        uses: actions-cool/check-user-permission@v2
        with:
          require: write
          username: ${{ github.triggering_actor }}
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
      - name: Check User Permission
        if: steps.checkAccess.outputs.require-result == 'false'
        run: |
          echo "${{ github.triggering_actor }} does not have permissions on this repo."
          echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
          echo "Job originally triggered by ${{ github.actor }}"
          exit 1
      - name: 'Checkout Repository'
        uses: actions/checkout@v4
        with:
          ref: ${{  github.event.pull_request.head.sha }} # This is dangerous without the first access check
      - run: echo "${{ github.event_name }}"
      # Use the dependency snapshot to detect transient dependencies
      # https://github.com/actions/dependency-review-action/issues/595#event-10791333872
      - name: Submit Dependency Snapshot
        uses: advanced-security/maven-dependency-submission-action@v3
        with:
          directory: ${{ github.workspace }}/todo
      - name: 'Dependency Review from PR'
        uses: actions/dependency-review-action@v3
jmservera commented 1 year ago

I've reviewed the library being used and it specifically disallows pull_request_target, so, is there any other way to achieve what I'm trying to do? Just setting the permissions with pull_request type does not work in the case of a fork.

jmservera commented 1 year ago

I finally created a modified version of the action to make it work with forks:

name: 50-Dependency-Review
on:
# dependency scan works comparing pull requests to the base branch
  pull_request_target:
  #pull_request:
    # These types are all required for CRDA to scan pull requests correctly and securely.
    types: [ opened, synchronize, reopened, labeled, edited ]
    paths-ignore:
      - '**/*.md'
      - '**/*.txt'

permissions:
  contents: write

jobs:
  dependency-review:
    runs-on: ubuntu-latest
    permissions:
      contents: write # for actions/checkout to fetch code
      security-events: write # for github/codeql-action/upload-sarif to upload SARIF results
      actions: read # only required for a private repository by github/codeql-action/upload-sarif to get the Action run status    
    steps:
      - name: Get User Permission
        id: checkAccess
        uses: actions-cool/check-user-permission@v2
        with:
          require: write
          username: ${{ github.triggering_actor }}
        env:
          GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
      - name: Show the two contexts
        run: |
          echo "ctxt: ${{ github.sha }} pr: ${{ github.event.pull_request.head.sha }}"
          echo "ref: /ref/pull/${{ github.event.pull_request.number }}"
      - name: Check User Permission
        if: steps.checkAccess.outputs.require-result == 'false'
        run: |
          echo "${{ github.triggering_actor }} does not have permissions on this repo."
          echo "Current permission level is ${{ steps.checkAccess.outputs.user-permission }}"
          echo "Job originally triggered by ${{ github.actor }}"
          exit 1
      - name: 'Checkout Repository'
        uses: actions/checkout@v4
        with:
          ref: ${{  github.event.pull_request.head.sha }} # This is dangerous without the first access check
      - run: echo "${{ github.event_name }}"
      # Use the dependency snapshot to detect transient dependencies
      # https://github.com/actions/dependency-review-action/issues/595#event-10791333872
      - name: Submit Dependency Snapshot
        #uses: advanced-security/maven-dependency-submission-action@v3
        uses: jmservera/maven-dependency-submission-action@extend-with-sha
        with:
          directory: ${{ github.workspace }}/todo
          sha: ${{ github.event.pull_request.head.sha }}
          ref: "refs/pull/${{ github.event.pull_request.number }}/merge"

      - name: 'Dependency Review from PR'
        uses: actions/dependency-review-action@v3

Any thoughts?

peter-murray commented 1 year ago

I have provided you a branch that has the ability to set a ref and sha here, please verify that this meets your requirements https://github.com/advanced-security/maven-dependency-submission-action/tree/submit-context

jmservera commented 1 year ago

I have provided you a branch that has the ability to set a ref and sha here, please verify that this meets your requirements https://github.com/advanced-security/maven-dependency-submission-action/tree/submit-context

I tried to fix it myself in a completely different way, but your solution looks awesome and cleaner. I'll test it right now. Thanks!

jmservera commented 1 year ago

@peter-murray I've identified a small issue with the variable names, but it looks like it will do the trick. Thanks

peter-murray commented 1 year ago

I have merged the changes to main and am now working on a v4 release that will provide this functionality