WIP/BLD: daff perms

[tangkong]

• Transition comment diff workflow to trigger on pull_request to lock down write permission tokens
• Try using github-scripts for posting comments

Maybe:
scope out a token with comment permissions and use that for the script specifically

[github-actions]

--- a/Spreadsheet/KFE/QRIX-alarms.csv
+++ b/Spreadsheet/KFE/QRIX-alarms.csv

@@	#Indent	Branch	PV	Description	Latch	Delay	...
---			MR1K1:BEND:RTD:US:1_RBV	TEMPERATURE			...

[tangkong]

sigh, secrets are not passed when pull requests are made from forked repositories. pull_request_target may be the what we have to go with here.

I wanted to use github-scripts for the sake of using an action that's supported by github explicitly, but it should be functionally identical to the tholliander workflow

[KaushikMalapati]

There's a description on https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ that uses a pull-request triggered workflow with another job triggered on the workflow_run of the pull-request workflow, and only the second one uses a secret.

It would be something like

daff.yml

name: Comment Diff

on:
  pull_request:
    paths:
      - 'Spreadsheet/**'

jobs:
  comment-diff:
    runs-on: ubuntu-20.04
    defaults:
      run:
        shell: bash --login -eo pipefail {0}

    steps:
    - uses: actions/checkout@v4
      with:
        fetch-depth: 0

    - name: install requirements
      run: |
        pip install --upgrade pip
        pip install daff

    - name: make diff
      id: make_diff
      run: |
        daff git csv
        git config set --global diff.daff-csv.command "daff diff --git --output-format html --fragment --context 0"
        git remote add target https://github.com/pcdshub/pcds-nalms.git
        git fetch target
        git diff target/${{ github.event.pull_request.base.ref }}...HEAD Spreadsheet/ > ${{ runner.temp }}/diff.txt
    - uses: actions/upload-artifact@v4
      with:
        name: diff
        path: ${{ runner.temp }}/diff.txt

And subsequently

makecomment.yaml (almost completely copy pasted from the link)

name: Comment on the pull request
on:
  workflow_run:
    workflows: ["Comment Diff"]
    types:
      - completed

jobs:
  upload:
    runs-on: ubuntu-latest
    if: >
      github.event.workflow_run.event == 'pull_request' &&
      github.event.workflow_run.conclusion == 'success'
    steps:
      - name: 'Download artifact'
        uses: actions/github-script@v7
        with:
          script: |
            var artifacts = await github.actions.listWorkflowRunArtifacts({
               owner: context.repo.owner,
               repo: context.repo.repo,
               run_id: ${{github.event.workflow_run.id }},
            });
            var matchArtifact = artifacts.data.artifacts.filter((artifact) => {
              return artifact.name == "diff"
            })[0];
            var download = await github.actions.downloadArtifact({
               owner: context.repo.owner,
               repo: context.repo.repo,
               artifact_id: matchArtifact.id,
               archive_format: 'zip',
            });
            var fs = require('fs');
            fs.writeFileSync('${{github.workspace}}/pr.zip', Buffer.from(download.data));
      - run: unzip pr.zip

      - name: 'Comment on PR'
        uses: actions/github-script@v7
        with:
          github-token: ${{ secrets.TOKEN_WITH_ONLY_PR_WRITE_PERMS }}
          script: |
            var fs = require('fs');
            var issue_number = Number(fs.readFileSync('./NR'));
            await github.issues.createComment({
              owner: context.repo.owner,
              repo: context.repo.repo,
              issue_number: issue_number,
              body: 'Everything is OK. Thank you for the PR!'
            });

[tangkong]

Interesting, I guess I should have tried that first. 😆

[tangkong]

This is again something that will require a merge to test

[tangkong]

In some more concise terms, we're splitting this up into two workflows in order to give each part of the process the most restrictive set of permissions:

• checking out the code and generating the diff: READ-ONLY
• Writing the comment: Write-PR permissions only

By splitting this up we then require passing the diff information through artifacts, but it allows us to more strictly limit permissions.

pull_request_target gives read and write permissions, and should not be used in workflows that checkout PR contents (which can contain malicious code). This is balanced by pull_request_target triggered workflows only running based on the base of the PR (not the modified code), but it is possible to circumvent this (I did this at an earlier part of this commit history)

Mostly putting this into words for my own education, but maybe also for the education of others.

[ZLLentz]

I'm confused, where is the part where we write out the diff to a comment?
I'm on board with learning and applying best-practices for security.

[tangkong]

I need to stop working on this in the morning. 💀 (he says as he logs off for the day to fix this tomorrow morning)

[tangkong]

Thanks for the careful eye, I've clearly not been careful enough qa'ing this on my own.

Reading over this again I think might want some escape hatch for when there is no diff generated, in the case where commits are added that don't affect the csvs. Maybe that doesn't matter 🤷

I think I addressed all the comments 🤞

[KaushikMalapati]

Reading over this again I think might want some escape hatch for when there is no diff generated, in the case where commits are added that don't affect the csvs. Maybe that doesn't matter 🤷

Does this

pcds-nalms/.github/workflows/daff.yml

Line 6 in 1ccadc3

- 'Spreadsheet/**'

not cover that? Am I wrong in assuming that only csvs would be under that path?

[tangkong]

Ah no you're right, that should hopefully be sufficient

[ZLLentz]

This looks good to me. Maybe it's time to merge and check the result in a follow-up.

[tangkong]

Merging, thanks for all the QA and sorry for the sloppiness throughout (I'll do better).

Turns out the required checks needed exactly the name of the job as defined in the yaml, not the name as displayed in the check itself. ("generate-xml", not "Process on PR / generate-xml"). This conflicts with my past experience with pcds-ci-helpers, but I won't lose any sleep