Declare permissions #15493

jsoref · 2024-01-31T14:00:18Z

Repositories can be configured with Default access (restricted) https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token

Best practice says that workflows should declare the minimal permissions they require. Without declaring permissions, paranoid forks fail miserably.

closes #15462

jsoref · 2024-01-31T14:05:23Z

To make the Check change note workflow happy, please add label:no-change-note-required

angelapwen

I looked at your fork of the repo, which has identical permissions and works 👍 thank you for the contribution!

csharp/actions/create-extractor-pack/action.yml

angelapwen · 2024-02-01T11:16:43Z

.github/workflows/check-implicit-this.yml

+permissions:
+  contents: read
+


I've been reminded to check that all of these permissions are necessary. Undoubtedly the security-events:write one for the QL-for-QL tests workflow is, as we saw elsewhere.

I tried this one in particular out on my own fork (where the current commit is main and where I've set the default token permissions to read-only) and it looks like this one passes without adding the extra permission. Is this because read is allowed with the least permissive setting?

These are the options I have:

It would be nice to confirm that all the added permissions are necessary.

While this looks like a grant, it's actually a restriction. By default you get some additional permissions:

Contents: read Metadata: read Packages: read

By specifying contents: read, it's blocking packages: read.

contents: read is theoretically needed for any workflow that uses actions/checkout (it might not realistically fail in public forks of a repository, but it would absolutely fail if someone made a private repository with the same contents).

I should note that I do things like that regularly when I'm testing repositories looking for vulnerabilities.

(It isn't my day job, but I do it often enough. It's definitely considered best practice. Admittedly, w/o a GHES license w/ CodeQL it'd be a bit of a PITA here, but it's technically possible to manage if you used a second public repository with a PAT for the codeql bits. But the general idea here is to show best practices so that if people copy+paste they get best practices.)

https://github.com/check-spelling-sandbox/codeql/actions/runs/7820458947/job/21335230118

GITHUB_TOKEN Permissions Contents: read Metadata: read

Warning: CodeQL Action v2 will be deprecated on December 5th, 2024. Please update all occurrences of the CodeQL Action in your workflow files to v3. For more information, see https://github.blog/changelog/2024-01-12-code-scanning-deprecation-of-codeql-action-v2/ RequestError [HttpError]: Resource not accessible by integration at /home/runner/work/_actions/github/codeql-action/v2/node_modules/@octokit/request/dist-node/index.js:86:21 at processTicksAndRejections (node:internal/process/task_queues:96:5) at async requestWithGraphqlErrorHandling (/home/runner/work/_actions/github/codeql-action/v2/node_modules/@octokit/plugin-retry/dist-node/index.js:71:20) at async Job.doExecute (/home/runner/work/_actions/github/codeql-action/v2/node_modules/bottleneck/light.js:405:18) { status: 403, response: { url: 'https://api.github.com/repos/check-spelling-sandbox/codeql/code-scanning/analysis/status', status: 403,

codeql-action/init currently requires a lot of stuff, that's currently github/codeql-action#2110

I guess I should try to polish that PR off before I finish this PR.

I can test this branch (as main to deal w/ all the frustrating checks that only run on main) with my hacked version of the codeql action to see how this would fare...

.github/workflows/labeler.yml

angelapwen

Blocking merge until the other comments are addressed 😄

Repositories can be configured with Default access (restricted) https://docs.github.com/en/actions/security-guides/automatic-token-authentication#permissions-for-the-github_token Best practice says that workflows should declare the minimal permissions they require. Without declaring permissions, paranoid forks fail miserably.

aeisenberg

Thanks for contributing this. I like that this narrows the permissions of all of our tokens.

I think .github/workflows/csv-coverage-update.yml is broken. I have a few other suggestions that will allow you to narrow the permissions. security-events is only required if we are reading or writing SARIF to or from code scanning.

I do have a concern that these jobs haven't been run. Are you able to trigger them in your fork to make sure all permissions are correct?

What we do in other repos and can do here (but best to wait for a followup PR) is to add a chunk that ensures the workflow file is run whenever the workflow file itself is modified.

eg-

  pull_request:
    paths:
      - '.github/workflows/csv-coverage-timeseries.yml'

(and similar for all other workflow files)

.github/workflows/csv-coverage-update.yml

.github/workflows/ql-for-ql-tests.yml

aeisenberg · 2024-02-06T22:30:59Z

.github/workflows/query-list.yml

@@ -13,6 +13,10 @@ on:
      - '.github/actions/fetch-codeql/action.yml'
      - 'misc/scripts/generate-code-scanning-query-list.py'

+permissions:
+  contents: read
+  security-events: read


Necessary?

Suggested change

security-events: read

.github/workflows/ruby-dataset-measure.yml

aeisenberg · 2024-02-06T22:31:58Z

.github/workflows/ruby-qltest.yml

@@ -29,6 +29,10 @@ defaults:
  run:
    working-directory: ruby

+permissions:
+  contents: read
+  security-events: read


?

Suggested change

security-events: read

Maybe?

I can't get ruby to build (I think I need larger runners or to try to kill a bunch of processes to get my runs more memory):
https://github.com/check-spelling-sandbox/codeql/actions/runs/7820458922/job/21335248948

We'll see what things look like when I update this PR.

aeisenberg · 2024-02-06T22:32:24Z

.github/workflows/swift.yml

@@ -33,6 +33,10 @@ on:
      - rc/*
      - codeql-cli-*

+permissions:
+  contents: read
+  security-events: read


?

Suggested change

security-events: read

Maybe?

This job consistently dies:
https://github.com/check-spelling-sandbox/codeql/actions/runs/7820458953/job/21335237762

We'll see what things look like when I update this PR.

But, I'd expect it to consistently die.

.github/workflows/validate-change-notes.yml

jsoref · 2024-02-07T17:17:14Z

@aeisenberg: the design of these workflows is really painful.

Many of them run only on specific branches.
Many of them only run if specific files change.
Many of them really really only work if run from a PR. -- I tried to add workflow_dispatch to test things and things exploded spectacularly: https://github.com/check-spelling-sandbox/codeql/actions/runs/7818256643/job/21328047264

jsoref requested review from a team as code owners January 31, 2024 14:00

github-actions bot added C# QL-for-QL labels Jan 31, 2024

angelapwen previously approved these changes Jan 31, 2024

View reviewed changes

csharp/actions/create-extractor-pack/action.yml Outdated Show resolved Hide resolved

angelapwen added the no-change-note-required This PR does not need a change note label Jan 31, 2024

jsoref dismissed angelapwen’s stale review via c5a047d January 31, 2024 14:09

jsoref force-pushed the declare-permissions branch from b997f22 to c5a047d Compare January 31, 2024 14:09

github-actions bot removed the C# label Jan 31, 2024

angelapwen approved these changes Jan 31, 2024

View reviewed changes

angelapwen reviewed Feb 1, 2024

View reviewed changes

.github/workflows/labeler.yml Show resolved Hide resolved

angelapwen requested changes Feb 1, 2024

View reviewed changes

jsoref force-pushed the declare-permissions branch from 9da590e to 301cfcc Compare February 1, 2024 15:39

jsoref requested a review from angelapwen February 4, 2024 16:21

aeisenberg requested changes Feb 6, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declare permissions #15493

Declare permissions #15493

jsoref commented Jan 31, 2024

jsoref commented Jan 31, 2024

angelapwen left a comment

angelapwen Feb 1, 2024

jsoref Feb 1, 2024

jsoref Feb 1, 2024

jsoref Feb 1, 2024

jsoref Feb 7, 2024

angelapwen left a comment

aeisenberg left a comment

aeisenberg Feb 6, 2024

aeisenberg Feb 6, 2024

jsoref Feb 8, 2024

aeisenberg Feb 6, 2024

jsoref Feb 8, 2024

jsoref commented Feb 7, 2024

Declare permissions #15493

Are you sure you want to change the base?

Declare permissions #15493

Conversation

jsoref commented Jan 31, 2024

jsoref commented Jan 31, 2024

angelapwen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelapwen left a comment

Choose a reason for hiding this comment

aeisenberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsoref commented Feb 7, 2024