Dependabot PRs have restricted permissions for testing

[nwiltsie]

Per this link:

By default, GitHub Actions workflows triggered by Dependabot get a GITHUB_TOKEN with read-only permissions. You can use the permissions key in your workflow to increase the access for the token

Additionally, dependabot has a separate set of available secrets:

Dependabot secrets also include secrets that are used by GitHub Actions workflows triggered by Dependabot pull requests. Dependabot itself may not use these secrets, but the workflows require them.

That means that this construct to use secrets.UCLAHS_CDS_REPO_READ_TOKEN...

https://github.com/uclahs-cds/tool-Nextflow-action/blob/da23bc9d87fda1d5b6751d3b671057d0b4d7e297/.github/workflows/nextflow-tests.yml#L245-L255

... does not work on dependabot PRs.

[nwiltsie]

Per https://securitylab.github.com/research/github-actions-preventing-pwn-requests/, the "official" solution to this is to have a two-workflow approach - one low-permissions workflow that is triggered by dependabot, and then one high-permissions workflow triggered on the successful conclusion of the first with workflow_run.

[nwiltsie]

So here are the current workflows (two workflows in each repository, and then the reusable workflow here):

flowchart

    subgraph workflow [Reusable Workflow]
        direction TB
        reused[[Discovery]]
        reuset1[[Test #1]]
        reuset2[[Test #2]]
        reuset3[[Test #3]]
        reuses[[Summary]]

        reused --> reuset1 --> reuses
        reused --> reuset2 --> reuses
        reused --> reuset3 --> reuses
    end

    subgraph workflow2 [Repo Workflow 1]
        issue_comment[/"issue_comment event\n(text starts with '/fix-tests')"/]
    end

    subgraph workflow1 [Repo Workflow 2]
        pull_request[/pull_request event/]
        push[/push event/]
    end

    issue_comment --> workflow
    pull_request --> workflow
    push --> workflow

Here are the jobs referenced in the reusable workflow. The #ff8888 nodes are "dangerous" - running arbitrary Nextflow code in the tests and committing changes. The #ffbb00 nodes indicate non-default permissions.

flowchart
    subgraph JOB: Discovery
        direction TB
        d_isissue{{issue_comment?}}
        d_existing{{Existing review\nwith changes\nrequested?}}

        d_review[Extract latest\nbot review]:::medium

        d_isissue -->|Yes| d_review --> d_existing
        d_isissue -->|No| d1

        d_existing -->|Yes| react[React to comment]:::medium --> d1
        d_existing -->|No| d_complain[Complain\nvia comment]:::medium --> d_fail([Fail])

        d1[Extract PR number] -->
        d3[Find all tests] -->
        dexit([Succeed])
    end

    subgraph JOB: Summary
        direction TB
        dl[Download\nresults]-->
        status{{Any failures?}}

        success([Succeed])
        fail([Fail])

        dismiss[Dismiss latest\nbot review]:::medium
        mergeable{{Mergeable?}}
        autofix{{Autofix?}}
        review_exists{{Existing\nreview?}}

        merge_repeat{{Repeated\nreview?}}
        fix_repeat{{Repeated\nreview?}}

        apology[Review with\nmerge instructions]:::medium
        instructions[Review with\ndiff notes and\nfix instructions]:::medium

        doit[Commit\nfixes]:::danger

        status -->|No| review_exists
        status -->|Yes| mergeable

        review_exists -->|Yes| dismiss --> success
        review_exists -->|No| success

        mergeable -->|Yes| autofix
        mergeable -->|No| merge_repeat

        merge_repeat -->|No| apology --> fail
        merge_repeat -->|Yes| fail

        autofix -->|No| fix_repeat
        autofix -->|Yes| doit --> fail

        fix_repeat -->|No| instructions --> fail
        fix_repeat -->|Yes| fail
    end

    subgraph JOB: Test
        direction TB
        test[Run test]:::danger -->
        test_status{{Success?}}

        upload[Upload artifact]:::medium

        test_status -->
        |No| add[Add diff note\nto artifact] -->       
        change[Change artifact name] -->
        upload

        test_status -->
        |Yes| upload

        upload --> testexit([Exit])

    end

    classDef danger fill:#f88;
    classDef medium fill:#fb0;

[nwiltsie]

That also shows the problem from #30 - JOB: Discovery looks for the PR number, which a generic push event doesn't have.

[nwiltsie]

Okay, here is my latest scheme. Each repo now has two workflows:

• A combined workflow that triggers on:
  • pushs to main (can only happen if the user has privileges)
  • issue_comments that start with /fix-tests or /run-tests
  • pull_request_targets to main
• Another that triggers on workflow_run, requiring success of the (named) prior workflow and calls the reusable

The first workflow will fail for issue_comments and pull_request_targets that do not come from repository collaborators and will post a comment about that. A new trigger is if a collaborator comments /run-tests, which enables a trusted collaborator to run the tests on behalf of a sneaky outsider.

flowchart

    subgraph workflow [Reusable Workflow]
        direction TB
        reused[[Discovery]]
        reuset1[[Test #1]]
        reuset2[[Test #2]]
        reuset3[[Test #3]]
        reuses[[Summary]]

        reused --> reuset1 --> reuses
        reused --> reuset2 --> reuses
        reused --> reuset3 --> reuses
    end

    subgraph workflow3 [Repo Workflow 2]
        direction TB
        workflow_run[/"workflow_run event\n(workflow #1 was successful)"/]
    end

    subgraph workflow1 [Repo Workflow 1]
        direction TB
        pull_request[/"pull_request_target event\n(to main)"/]
        push[/"push event (to main)"/]
        issue_comment[/"issue_comment event (on PR,\ntext starts with '/fix-tests' or '/run-tests')"/]

        wf1_user_check{{"User is\ncollaborator\n(or dependabot)?"}}

        issue_comment --> wf1_user_check
        pull_request --> wf1_user_check

        trust[Post comment\nabout collaborators\nand /run-tests]:::medium

        wf1_user_check -->|No| trust --> wf1fail([Fail])
        wf1_user_check -->|Yes|wf1success([Succeed])

        push --> wf1success

    end

    workflow_run --> workflow

    classDef medium fill:#fb0;

[nwiltsie]

The second workflow will escape the dependabot restrictions with that workflow_run trigger. The first workflow will only succeed if the triggering user is a collaborator, making me more comfortable about adding permissions to the second workflow.

[dan-knight]

Thanks for looking into this. This all makes sense to me, and should be fairly safe. My only concern is that some of these measures rely on the GitHub Action workflow YAML file being correct (for example, workflow_dispatch being used correctly). Automation should surely help with this, but these YAML files always feel difficult to validate imo. Any thoughts on this? Do we just automate as much as possible, document well, and stay vigilant?

[nwiltsie]

My current strategy for managing the complexity is reusable workflows - we sink as much complexity as possible into workflows in this repository, then have simple cookie-cutter workflows in other repositories.