TST: small change to csvs to test xml generation

[tangkong]

Previous PR is working and (presumably) checking out the repo with an ssh key. This aims to fully flex the xml-generation

[tangkong]

I don't think that specifying the ref in a workflow works for PRs originating from forks. I also think we have to merge this to see if I'm right

[KaushikMalapati]

SInce this job triggers on pull_request_target and not pull_request, the default checkout would be the branch we are merging into. So if we are trying to merge into the master branch, we would just compare it with itself and post an empty diff.

I think what's happening is that it's trying to checkout the source branch from the target repo (I hope I'm using the right terminology) which doesn't exist. Maybe this would work?

ref: "${{ github.event.pull_request.head.ref }}"
repository: "${{ github.event.pull_request.head.repo.full_name }}"

where we explicitly define where the branch is coming from

[tangkong]

Good call 👍

[KaushikMalapati]

Unfortunately, I think we still have to merge this to test it. Was this change meant to just test the daff workflow or do you actually want to add it?

[tangkong]

This PR wasn't meant to test the daff workflow at all, it was meant to test the xml generation with some modified repository permissions / push-access-bypass settings.

I just got side-tracked by the big red ❌ from the daff workflow

[tangkong]

A bit more reading revealed that the pull_request_target workflow trigger gives the GITHUB_TOKEN write permissions, which we don't need here. I'm going to try setting this to the standard pull-request trigger, which only grants read access.

https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/
https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target

Take 2: The comment workflow requires write permissions or pull_request_target. I don't know how I feel about this currently. While I'm uncomfortable granting write permissions to a workflow generally, according to documentation the pull_request_target trigger only ever runs on the base of the PR'd branch (not the modified PR contents). This protects us slightly, and also explains our difficulty testing the workflow in a PR.

[KaushikMalapati]

A bit more reading revealed that the pull_request_target workflow trigger gives the GITHUB_TOKEN write permissions, which we don't need here. I'm going to try setting this to the standard pull-request trigger, which only grants read access.

https://securitylab.github.com/resources/github-actions-preventing-pwn-requests/ https://docs.github.com/en/actions/writing-workflows/choosing-when-your-workflow-runs/events-that-trigger-workflows#pull_request_target

Take 2: The comment workflow requires write permissions or pull_request_target. I don't know how I feel about this currently. While I'm uncomfortable granting write permissions to a workflow generally, according to documentation the pull_request_target trigger only ever runs on the base of the PR'd branch (not the modified PR contents). This protects us slightly, and also explains our difficulty testing the workflow in a PR.

I didn't consider anything security wise when I wrote this, but could we make a secret github key that only has write permissions on pull requests? Alternatively we could follow the security lab paper and sotre the diff in an artifact in a pull-request workflow, and have another job trigger on workflow-run to make the comment.

[tangkong]

If we want to fully restrict access, I think the github approved way of posting a comment is through the rest API through github-script: https://github.com/actions/github-script#comment-on-an-issue

We can scope out a fine-grained access token specifically allowed to post comments only

[tangkong]

I'm going to request that we focus on the daff workflow in a different PR, since it's distracting from the goal of this effort.

The diff workflow is close (I think)

[ZLLentz]

To finish here, the last step is probably to undo the tiny csv change, right?

[tangkong]

The csv change was to test xml push generation on PR merge, so it's actually necessary here. The goal of this was to test if we could give this job permissions via deploy-key, rather than by just omitting branch protections

This was poorly established / lost in the nerd snipe 😆

[tangkong]

I'm going to merge so I can find out how I messed this one up and try again

[tangkong]

ahahahahahaahahit worked