ci: fix semver-checks master by storopoli · Pull Request #3028 · rust-bitcoin/rust-bitcoin

storopoli · 2024-07-13T12:31:14Z

Ok now I think I've nailed this! 🚀
Closes #3018.

Summary

This PR fix the permissions by adding a secondary job, semver-checks-pr-label, that runs only if the semver-checks completes and does not fail, i.e. we have either Semver API breaks.
This is a secure job according to internal discussions and https://docs.github.com/en/actions/using-workflows/events-that-trigger-workflows#workflow_run

Implementation notes

We split the semver checks into the actual check and labeling. This is due to the fact that we were having several permission issues and the original implementation was insecure. Also it is currently failing on several PRs.

semver-checks.yml: checks for API semver breaks. Upon finding API semver-checks, it creates a file named semver-break that has the PR number inside. If this file is present we upload an artifact representing that the API semver was broken with the PR number.
semver-checks-pr-label.yml: runs after the semver-checks.yml is completed. It uses actions/github-script (in JavaScript) to list the previous workflow artifacts and, if finds that an API semver break is present, adds a corresponding label and a comment to the PR that is inside the file semver-break. We sanitize the hell out of the thing to be a number only, both in BASH but also in JavaScript.

How I tested this?

I merged this PR to my master branch in my storopoli/rust-bitcoin repo.
I've set the following settings in the repo settings to mimic what we have for rust-bitcoin:
I've made a PR (cherry-picked from Toby) that breaks semver, see https://github.com/storopoli/rust-bitcoin/pull/10. It correctly labelled as API break and added a comment to the PR

.github/workflows/semver-checks-pr-label.yml

storopoli · 2024-07-13T12:31:58Z

.github/workflows/semver-checks.yml

@Kixunil I cannot read the PR number in the semver-checks-pr-label.yml from the previous semver-checks-pr.yml workflow.
I've tried a couple of things, most notoriously this:

# in semver-checks-pr-label.yml env: # hack to get PR number from the workflow_run object PR_NUMBER: ${{ github.event.workflow_run.conclusion.pull_requests[0].number }}

That sucks. But if someone griefs us we'll have an argument for GH to fix that.

coveralls · 2024-07-13T12:33:38Z

Pull Request Test Coverage Report for Build 9939267405

Details

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 83.153%

Totals
Change from base Build 9931259856:	0.0%
Covered Lines:	19634
Relevant Lines:	23612

💛 - Coveralls

Kixunil

The remaining vulnerability needs to be fixed.

.github/workflows/semver-checks-pr-label.yml

Kixunil · 2024-07-13T15:02:11Z

.github/workflows/semver-checks.yml

That sucks. But if someone griefs us we'll have an argument for GH to fix that.

.github/workflows/semver-checks-pr-label.yml

obi1kenobi

I'm generally very risk-averse when it comes to security things, so my personal preference is to obsessively quote everything and eliminate anything that might be a vector for code injection, even if it is currently not exploitable due to an existing check. I basically don't trust that a future refactoring won't accidentally remove or reduce the check just enough that an attacker could slip through.

I've left some suggestions in that spirit, but it's entirely possible your preferences are not the same. So consider my comments purely advisory and feel free to ignore them if you don't think they are right for your style and development process.

.github/workflows/semver-checks-pr-label.yml

.github/workflows/semver-checks.yml

Kixunil

ACK 6ab8f99

My comments can be addressed in a follow up PR since I believe this is sufficiently secure even without it and we desperately need to fix master. Note that I'm going AFK soon, so pushing will remove my ACK for around 8 hours.

Kixunil · 2024-07-15T19:23:38Z

.github/workflows/semver-checks-pr-label.yml

+            fs.writeFileSync(`${process.env.GITHUB_WORKSPACE}/semver-break.zip`, Buffer.from(download.data));
+      - name: "Unzip artifact"
+        if: ${{ hashFiles('semver-break.zip') != '' }}
+        run: unzip semver-break.zip


FTR, I did check whether unzip is vulnerable to path traversal attacks. It looks like it's not. But maybe we should use the -n switch just to be sure. (Why the f didn't GH provide a first class API for this?!)

You mean the -n that I get when I run unzip -h in my linux machine:

-n never overwrite existing files

?

Kixunil · 2024-07-15T19:26:04Z

.github/workflows/semver-checks.yml

+          PR_NUMBER: ${{ github.event.number }}
+        run: |
+          # check if PR_NUMBER is a number
+          if ! [[ "$PR_NUMBER" =~ ^-?[0-9]+$ ]]; then


This isn't really needed since the attacker won't attack himself.

But I prefer to have it in there. It is not doing anything now since the ENV var is set by Github.
Do you really see that it could be a problem in the near future?

It's just a bloat. Not a big deal but I'd remove it if it was entirely my choice.

tcharding

FWIW ACK merging this. I can't put a proper ACK because I don't even know what scripting language is being used so my review is too shallow to warrant any worth.

Kixunil · 2024-07-15T20:13:36Z

because I don't even know what scripting language is being used

Ignorance is a bliss. I wish I didn't know either.

tcharding · 2024-07-15T20:19:53Z

Lol, glad I didn't look it up :)

storopoli · 2024-07-15T21:39:52Z

FWIW ACK merging this. I can't put a proper ACK because I don't even know what scripting language is being used so my review is too shallow to warrant any worth.

I think I am more a JS hater (it belongs only in the browser to make things do things) than Rust maximalist.
The Geiger counter is going crazy with all the exposure to radioactive JS over these past days.
But I trimmed it down to the bare minimum.

I've updated the PR description with an overview and some implementation notes.
That might help people that want to review this.

obi1kenobi

This looks good to me 👍

Just a note that the unzip -n conversation got resolved but the -n argument doesn't look like it made it into the PR. Entirely possible this is intentional and I've just been out of the loop, so I'm just flagging it for your consideration.

Kixunil · 2024-07-16T07:35:51Z

it belongs only in the browser

WASM belongs there ;)

Thanks @obi1kenobi we had a rule to resolve conversations before but removed it. It was intentionally left out of the PR to not trigger re-review but should not have been marked as resolved.

storopoli · 2024-07-16T08:57:19Z

Yeah my bad, I got trigger-happy with the resolve to unclutter things for the reviewers since we are planning to merge this without the -n and tackle in a new PR. I'll add in the TODO to #3003.

apoelstra

ACK 6ab8f99 thanks for sticking with this!!

storopoli · 2024-07-16T17:39:09Z

ACK 6ab8f99 thanks for sticking with this!!

Thanks, after this is merged I'll get back to #3003 TODO list.

github-actions bot added the ci label Jul 13, 2024

storopoli commented Jul 13, 2024

View reviewed changes

.github/workflows/semver-checks-pr-label.yml Outdated Show resolved Hide resolved

storopoli commented Jul 13, 2024

View reviewed changes

storopoli mentioned this pull request Jul 13, 2024

ci: fix semver-checks in master #3027

Closed

storopoli mentioned this pull request Jul 13, 2024

ci: semver-checks for non-additive cargo features #3003

Merged

Kixunil requested changes Jul 13, 2024

View reviewed changes

storopoli requested review from Kixunil and obi1kenobi July 14, 2024 11:41

obi1kenobi reviewed Jul 14, 2024

View reviewed changes

ci: fix semver-checks master

6ab8f99

Kixunil approved these changes Jul 15, 2024

View reviewed changes

tcharding approved these changes Jul 15, 2024

View reviewed changes

storopoli requested a review from obi1kenobi July 15, 2024 21:42

obi1kenobi reviewed Jul 16, 2024

View reviewed changes

apoelstra approved these changes Jul 16, 2024

View reviewed changes

apoelstra merged commit 59c8069 into rust-bitcoin:master Jul 16, 2024

storopoli deleted the ci/fix-semver-checks branch July 16, 2024 20:29

Conversation

storopoli commented Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Implementation notes

How I tested this?

Uh oh!

Uh oh!

storopoli Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Jul 13, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 9939267405

Details

💛 - Coveralls

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

obi1kenobi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcharding left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tcharding commented Jul 15, 2024

Uh oh!

storopoli commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

obi1kenobi left a comment

Choose a reason for hiding this comment

Uh oh!

Kixunil commented Jul 16, 2024

Uh oh!

storopoli commented Jul 16, 2024

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

storopoli commented Jul 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

storopoli commented Jul 13, 2024 •

edited

Loading

storopoli Jul 13, 2024 •

edited

Loading

coveralls commented Jul 13, 2024 •

edited

Loading

Kixunil commented Jul 15, 2024 •

edited

Loading

storopoli commented Jul 15, 2024 •

edited

Loading