Skip to content

Adds zizmor sarif #1214

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Jun 5, 2025
Merged

Adds zizmor sarif #1214

merged 11 commits into from
Jun 5, 2025

Conversation

kingbuzzman
Copy link
Contributor

@kingbuzzman kingbuzzman commented Jun 4, 2025

A followup to a comment @webknjaz made.

Related to #1203 but not dependent on it.


Adds the issues directly onto the PR (this was the example)

image

@github-advanced-security
Copy link

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

@kingbuzzman kingbuzzman marked this pull request as ready for review June 4, 2025 13:51
@kingbuzzman kingbuzzman requested a review from bluetech June 4, 2025 14:16
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work unless you give the job the security events privilege.

@kingbuzzman
Copy link
Contributor Author

This won't work unless you give the job the security events privilege.

image

@webknjaz But it did.. what am i missing?

@webknjaz
Copy link
Member

webknjaz commented Jun 4, 2025

Dunno, maybe because it's a public repo?

Let's ask @woodruffw to share ideas...

@kingbuzzman kingbuzzman requested a review from webknjaz June 4, 2025 20:06
@kingbuzzman
Copy link
Contributor Author

@webknjaz OK, I added the permission.. doesnt seem like it matters, but I'd be glad to see your approval 😇 -- if its warranted.

Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, approving. Regarding the sh hack, it would probably be more elegant in a toxfile.py...

@woodruffw
Copy link

Dunno, maybe because it's a public repo?

Let's ask @woodruffw to share ideas...

Thanks for the ping -- that might be incidentally working if your GITHUB_TOKEN defaults are the old ("permissive") ones.

You can see that under Actions > General in the repo's settings, or potentially under the org-level settings if you have an org-level permissive policy. For example, this is what I have on zizmor itself:

Screenshot 2025-06-04 at 5 53 55 PM

(it's faded out because I have it disabled on the org level.)

@woodruffw
Copy link

The above can happen if this repo is older, since older repos are "grandfathered" into the old permissive token defaults. For example, here's a random older repo of mine:

Screenshot 2025-06-04 at 5 55 15 PM

@webknjaz
Copy link
Member

webknjaz commented Jun 4, 2025

@woodruffw I see, but wouldn't the workflow-level permissions setting reset it to zero privileges? And then the job-level one setting just one privilege (in the previous commit).

@woodruffw
Copy link

I see, but wouldn't the workflow-level permissions setting reset it to zero privileges? And then the job-level one setting just one privilege (in the previous commit).

Hmm, true. Yeah, I'm at a loss to explain this, then...

@webknjaz webknjaz changed the title Adds zizmor serif Adds zizmor sarif Jun 4, 2025
@webknjaz
Copy link
Member

webknjaz commented Jun 5, 2025

@woodruffw I remember how some privileges are needed for private repos and not for public ones. Wonder if this is the case here.

@woodruffw
Copy link

Could be that, although I thought that was the case only for the contents and actions permissions with read (since they're "latent" with a public repo). security-events: write OTOH should never be latently present except in a an older default token case, which as you've noted doesn't seem to be applicable here.

@woodruffw
Copy link

Hmm, it's also possible there are two different things here: GH might allow "advanced security" to make notifications/alerts on the enabling PR, but then forbid alerts within the security pane itself after merge if the permission isn't set. That would be pretty confusing on GH's part, but it would explain the observed behavior here...

@kingbuzzman
Copy link
Contributor Author

@webknjaz i've been playing around with toxfile.py.. i dont like the alternative, i prefer the hack. @bluetech do i have your blessings to merge?

@webknjaz
Copy link
Member

webknjaz commented Jun 5, 2025

@woodruffw well, thanks for entertaining my assumptions.. Will leave it as is.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, except a comment about using sh in tox

@kingbuzzman kingbuzzman enabled auto-merge (squash) June 5, 2025 20:01
@kingbuzzman kingbuzzman merged commit ab218f3 into main Jun 5, 2025
21 checks passed
@kingbuzzman kingbuzzman deleted the dev/zizmore-serif branch June 5, 2025 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants