Adds ref and SHA as inputs, and sarif-id as output #889

cw-alexcroteau · 2022-01-24T19:18:49Z

Merge / deployment checklist

Confirm this change is backwards compatible with existing workflows.
Confirm the readme has been updated if necessary.
Confirm the changelog has been updated if necessary.

This PR might be useful for those facing #796 by allowing to specify a ref, but is NOT a direct fix for it.

aeisenberg

Thank you for your PR. This is looking really good. I have a few questions inline and suggestions inline.

We will also need to create an integration test for this to ensure that the new inputs are working as expected in real workflows. I'm not sure if you will be able to run them due to restrictions on running CI in forks. I'll figure out how to proceed later.

CHANGELOG.md

analyze/action.yml

aeisenberg · 2022-01-25T18:35:14Z

src/actions-util.ts

+  const ref = refInput || getRequiredEnvParam("GITHUB_REF");
+  const sha = getOptionalInput("sha") || getRequiredEnvParam("GITHUB_SHA");


It should probably be an error if only one of these inputs are specified.

Also, I'm not sure what would happen if you specify a SHA that is not part of the current branch. Hopefully, code scanning would error out..

I've added a check that throws an error message, and a unit test to confirm it works. Integration test to come with the rest of the integration tests.

I was initially targeting the upload-sarif action when I added this, so I did really see it as two arbitrary values provided by the user, who would ultimately be responsible to confirm their validity.
I can see two ways to handle this:
a. I can add an integration test for this specific use case, which would test that the code scanning would error out if an invalid ref is provided. If the error message is meaningful and relevant, we can rely on it.
b. I can add a check using git branch ${ref} --contains ${sha} in getRef(), with a fallback if the git command can't be called (see line 86). I would of course add the corresponding unit/integration tests.

For 2., I would prefer option b. This way, we don't have to rely on the underlying scanner. What's your preferred solution?

EDIT: Option 1 would not be possible, because I realized integration tests can only be positive with the current setup.

So, I can either leave it as-is, without a test, or use option b.

upload-sarif/action.yml

analyze/action.yml

aeisenberg · 2022-01-25T20:44:29Z

Please include the compiled sources (*.js and *.map files) in this PR. Because of the way actions are run, all build artifacts must be checked into the repo.

cw-alexcroteau · 2022-01-26T16:58:56Z

Hi @aeisenberg! Thanks for your feedback, I've included the compiled sources and fixed most inline suggestions.

I am trying to fix the integration tests, in the meantime there is one question remaining in inline suggestions. Can you approve the workflows here please (as I am a first time contributor, Github blocks them)?

FYI, Integration tests run in my fork. I had to enable workflows, then I created a draft PR in there just so workflows are executed. This information might be useful for future contributors.

cw-alexcroteau · 2022-01-26T22:42:21Z

All checks are green in my Draft, so as soon as running workflows is approved I think this can go forward.

aeisenberg

This is looking really good. I'm running the CI checks again. Hopefully, they will all pass. Just one minor question inline, but it's not a blocker.

pr-checks/checks/remote-config.yml

src/actions-util.ts

Code looks good. Waiting for CI checks to pass.

aeisenberg · 2022-01-31T01:48:12Z

Everything looks good, but the new CI jobs are failing. I'm digging in to try to figure out why.

benmoss · 2022-01-31T14:44:00Z

sendStatusReport doesn't respect TEST_MODE like some other functions do, not sure why it is getting called here but it appears that the test doesn't have permissions to do a PUT against https://api.github.com/repos/github/codeql-action/code-scanning/analysis/status

cw-alexcroteau · 2022-01-31T16:19:23Z

sendStatusReport doesn't respect TEST_MODE like some other functions do, not sure why it is getting called here but it appears that the test doesn't have permissions to do a PUT against https://api.github.com/repos/github/codeql-action/code-scanning/analysis/status

@benmoss Should I add a TEST_MODE for sendStatusReport or something?

aeisenberg · 2022-01-31T16:32:09Z

sendStatusReport doesn't respect TEST_MODE like some other functions do, not sure why it is getting called here but it appears that the test doesn't have permissions to do a PUT against https://api.github.com/repos/github/codeql-action/code-scanning/analysis/status

That's right, but it doesn't explain why similar integration tests, like the go autobuilder tests are passing. I am fairly certain they are uploading status reports as well.

The same job, when run in @cw-acroteau's non-forked PR is passing.

aeisenberg · 2022-01-31T16:33:14Z

In the non-forked version, I see that the permissions block is more permissive, which explains why it is passing there.

The permissions block for the runs in this repo, you can see that all permissions are read. But, it's the same se as in the go autobuilder tests.

So, I am still confused. Perhaps, the go autobuilder tests are not sending the status report somehow.

aeisenberg · 2022-02-01T00:00:49Z

Now things are starting to become clear. If I explicitly add security-events: write then the failing workflows will pass. The difference between the failing workflows and the passing ones is that the failing ones use a value for the ref property of the upload status report that is different from that of the PR. Upload status report is an internal API endpoint for telemetry into code scanning. I don't know if it is a bug that the endpoint sometimes accepts requests from user agents that do not have security-events: write permissions. I'm communicating with the team that maintains this part of the API.

Regardless, I think our test workflows should have this permission anyway. Here is what I propose:

I will create a new PR that will add the security-events: write permission to all of our generated workflows.
Can you rebase your PR, @cw-acroteau, on top of the one I will create? And make sure to regenerate the two integration test workflows you created. Also, make sure to fix the merge conflict in CHANGELOG.md.

Thanks for helping me find this bug (or at least odd inconsistency) in the action.

Ensure that all workflows are able to write security events.

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

cw-alexcroteau · 2022-02-01T01:08:32Z

Can you rebase your PR, @cw-acroteau, on top of the one I will create? And make sure to regenerate the two integration test workflows you created. Also, make sure to fix the merge conflict in CHANGELOG.md.

Hi @aeisenberg ! I just did that, hope everything is fine. Once again, the workflows won't run. I don't know what it takes for Github to consider me as not a "first-time contributor", but you'll need to approve the runs again. I'll wait for this, to confirm that the workflows will pass. If there's an issue I'll try to fix it. Thanks!

cw-alexcroteau · 2022-02-01T01:16:29Z

Hi again @aeisenberg ! Can you please check if the checks are okay in a copy similar to #902 please? It fails here because of "RequestError [HttpError]: Resource not accessible by integration", but it might be due to the fork.

cw-alexcroteau · 2022-02-01T01:23:19Z

Oh sorry I did a git rebase, and not changed the base branch. I thought this was what you meant.

aeisenberg · 2022-02-01T01:26:48Z

No problem. I wasn't clear about it. I changed the branch and it all looks good. Yes, the reason why the new workflows are failing is because the security-events permission is always downgraded to read for forks. I don't think it will be possible to get it to pass from a fork. I'm in discussion with the API maintainers to figure out if we should avoid uploading status reports for integration tests. This should allow your integration tests to pass.

aeisenberg · 2022-02-01T01:27:47Z

They are in Europe, so I probably won't hear anything until tomorrow.

aeisenberg · 2022-02-01T18:27:54Z

PRs from forks will always have their security-events permission downgraded to read. This makes sense because we don't want malicious PRs to come around and tamper with the security alerts for refs they are not associated with.

There is an exception to this. a PR from a fork can write security events to resources associated with that PR only. This allows the PR to report security concerns about itself without giving it the ability to overwrite other refs.

What this means is that using the standard PR workflow, these new ref and sha inputs will not be able to work in forks. We will need to document this accordingly.

It looks like there are ways around this using pull_request_target or possibly [Send write tokens to workflows from pull requests[(https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository#enabling-workflows-for-private-repository-forks). I haven't tried out either of these solutions yet, but they will probably work. However, there are added security considerations with this.

What this means is that we need to rebase on the latest tip of the aeisenberg/permissions in this PR (I'll do that shortly) and update the documentation of the inputs to point out the limitation with forks. This PR can then be merged. Later, we can work on some proper documentation on how to use these inputs in forks.

aeisenberg · 2022-02-01T18:38:10Z

I created a PR in your repo for these changes. If you are happy with them, please merge and then I can merge here.

aeisenberg · 2022-02-02T00:37:50Z

This PR should not have been closed. It should have been retargeted to main. Let me see what happened.

aeisenberg · 2022-02-02T00:41:35Z

@cw-acroteau I'm very sorry, but could you create a new PR targeting main? For some reason when #902 was merged, this PR was closed. Instead it should have been retargeted against main.

cw-alexcroteau · 2022-02-02T01:28:26Z

@cw-acroteau I'm very sorry, but could you create a new PR targeting main? For some reason when #902 was merged, this PR was closed. Instead it should have been retargeted against main.

@aeisenberg Please see #904

Move a couple of entries for #889 that should have been in the unreleased section but were inadvertently moved into the 1.0.31 release.

cw-alexcroteau requested a review from a team as a code owner January 24, 2022 19:18

cw-alexcroteau force-pushed the add-ref-input branch from 51971d1 to 1f8caea Compare January 24, 2022 19:47

aeisenberg previously requested changes Jan 25, 2022

View reviewed changes

cw-alexcroteau requested a review from aeisenberg January 26, 2022 13:19

cw-alexcroteau force-pushed the add-ref-input branch from 9674ae4 to f1ca338 Compare January 26, 2022 19:27

aeisenberg mentioned this pull request Jan 27, 2022

upload-serif@v1: Unable to override commit_sha #890

Closed

aeisenberg reviewed Jan 28, 2022

View reviewed changes

pr-checks/checks/remote-config.yml Show resolved Hide resolved

src/actions-util.ts Show resolved Hide resolved

aeisenberg mentioned this pull request Jan 31, 2022

[DONOTMERGE] Aeisenberg/test add ref input #898

Closed

Add a permissions block for generated workflows

e9aa2c6

Ensure that all workflows are able to write security events.

aeisenberg mentioned this pull request Feb 1, 2022

Add a permissions block for generated workflows #902

Merged

3 tasks

cw-alexcroteau and others added 10 commits January 31, 2022 20:06

Adds ref and SHA as inputs, and sarif-id as output

980fd4e

Apply documentation suggestions from code review

0dd4dbf

Co-authored-by: Andrew Eisenberg <aeisenberg@github.com>

Applies recomendation in upload-sarif/action.yml

5916f98

Adds check on inputs and compiled files

1eaaf07

Adds integration test and fixes linting

1bfa9ac

Fixes integration test

260b4d5

Updates javascript files

3cc8799

Fixes integration tests referred repo

63d0c78

Changes to commit hash in main branch

dfe2bc4

Splits integration tests

9f36b75

cw-alexcroteau force-pushed the add-ref-input branch from 32d8387 to 9f36b75 Compare February 1, 2022 01:06

Regenerates test workflows

72f9a88

aeisenberg changed the base branch from main to aeisenberg/permissions February 1, 2022 01:18

aeisenberg force-pushed the aeisenberg/permissions branch from 0dd07df to 36419a7 Compare February 1, 2022 18:13

aeisenberg deleted the branch github:aeisenberg/permissions February 1, 2022 18:54

aeisenberg closed this Feb 1, 2022

cw-alexcroteau mentioned this pull request Feb 2, 2022

Add ref input #904

Merged

3 tasks

aeisenberg mentioned this pull request Feb 2, 2022

Declare the merge base as base for code scanning comparisons #858

Merged

4 tasks

henrymercer added a commit that referenced this pull request Feb 2, 2022

Fix changelog

78eb2c9

Move a couple of entries for #889 that should have been in the unreleased section but were inadvertently moved into the 1.0.31 release.

henrymercer mentioned this pull request Feb 2, 2022

Fix changelog #905

Merged

3 tasks

totogtr mentioned this pull request Mar 29, 2022

upload-sarif@v1 : master does not match /^refs\/(heads|pull|tags)\/.*$/. #796

Open

This was referenced Oct 13, 2023

[Snyk] Security upgrade nyc from 12.0.2 to 13.1.0 aliscco/codeql-action#818

Open

[Snyk] Security upgrade nyc from 12.0.2 to 13.1.0 aliscco/codeql-action#819

Open

[Snyk] Security upgrade nyc from 12.0.2 to 13.1.0 aliscco/codeql-action#820

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds ref and SHA as inputs, and sarif-id as output #889

Adds ref and SHA as inputs, and sarif-id as output #889

cw-alexcroteau commented Jan 24, 2022 •

edited

aeisenberg left a comment

aeisenberg Jan 25, 2022

cw-alexcroteau Jan 25, 2022 •

edited

cw-alexcroteau Jan 26, 2022

aeisenberg commented Jan 25, 2022

cw-alexcroteau commented Jan 26, 2022 •

edited

cw-alexcroteau commented Jan 26, 2022

aeisenberg left a comment

aeisenberg commented Jan 31, 2022

benmoss commented Jan 31, 2022

cw-alexcroteau commented Jan 31, 2022 •

edited

aeisenberg commented Jan 31, 2022

aeisenberg commented Jan 31, 2022 •

edited

aeisenberg commented Feb 1, 2022

cw-alexcroteau commented Feb 1, 2022 •

edited

cw-alexcroteau commented Feb 1, 2022

cw-alexcroteau commented Feb 1, 2022

aeisenberg commented Feb 1, 2022

aeisenberg commented Feb 1, 2022

aeisenberg commented Feb 1, 2022

aeisenberg commented Feb 1, 2022

aeisenberg commented Feb 2, 2022

aeisenberg commented Feb 2, 2022

cw-alexcroteau commented Feb 2, 2022

		const ref = refInput \|\| getRequiredEnvParam("GITHUB_REF");
		const sha = getOptionalInput("sha") \|\| getRequiredEnvParam("GITHUB_SHA");

Adds ref and SHA as inputs, and sarif-id as output #889

Adds ref and SHA as inputs, and sarif-id as output #889

Conversation

cw-alexcroteau commented Jan 24, 2022 • edited

Merge / deployment checklist

aeisenberg left a comment

Choose a reason for hiding this comment

aeisenberg Jan 25, 2022

Choose a reason for hiding this comment

cw-alexcroteau Jan 25, 2022 • edited

Choose a reason for hiding this comment

cw-alexcroteau Jan 26, 2022

Choose a reason for hiding this comment

aeisenberg commented Jan 25, 2022

cw-alexcroteau commented Jan 26, 2022 • edited

cw-alexcroteau commented Jan 26, 2022

aeisenberg left a comment

Choose a reason for hiding this comment

aeisenberg commented Jan 31, 2022

benmoss commented Jan 31, 2022

cw-alexcroteau commented Jan 31, 2022 • edited

aeisenberg commented Jan 31, 2022

aeisenberg commented Jan 31, 2022 • edited

aeisenberg commented Feb 1, 2022

cw-alexcroteau commented Feb 1, 2022 • edited

cw-alexcroteau commented Feb 1, 2022

cw-alexcroteau commented Feb 1, 2022

aeisenberg commented Feb 1, 2022

aeisenberg commented Feb 1, 2022

aeisenberg commented Feb 1, 2022

aeisenberg commented Feb 1, 2022

aeisenberg commented Feb 2, 2022

aeisenberg commented Feb 2, 2022

cw-alexcroteau commented Feb 2, 2022

cw-alexcroteau commented Jan 24, 2022 •

edited

cw-alexcroteau Jan 25, 2022 •

edited

cw-alexcroteau commented Jan 26, 2022 •

edited

cw-alexcroteau commented Jan 31, 2022 •

edited

aeisenberg commented Jan 31, 2022 •

edited

cw-alexcroteau commented Feb 1, 2022 •

edited