Add CI job to enforce security assessments #5709
Conversation
.github/workflows/require-labels.yml
Outdated
There was a problem hiding this comment.
Just curious if there is a particular reason for using v1 vs. the latest v2 here?
There was a problem hiding this comment.
No reason aside from that I more or less copied this workflow from an internal repo that was using v1
|
Reviewing the action code for |
I'm assuming this is fine given that the source code is short enough to quickly read over + doesn't seem problematic, and also we're already using this action elsewhere for the same purpose (CC @sfc-gh-hpathak to double-check) Given we can manually audit the source code of the action and pin the version being used, I think it makes more sense to do that vs essentially just copy-pasting the same code into our repo to be able to use it |
396cda4 to
c52b26d
Compare
|
Kudos to @mayagbarnes for bringing up the guidelines (I didn't even think of that 😅). @vdonato Can we add an explicit permissions tag from this? IIRC by default GH gives all perms to a running workflow, we can restrict that to just read. Not sure which on of those keys will allow reading just the label, At some point someone may flag the "using different GH action" as a violation but I agree that this is low risk. |
c52b26d to
2791217
Compare
Done! Looks like adding read permissions to |
|
@vdonato the problem with third parties is not only that they can write malicious code, but they are also more vulnerable to phishing and other attacks, and then replacing the code with malicious code in the future. But I think we can prevent this by pinning actions to a full-length commit SHA. It is aligned with the Github Action hardening guideline. |
Co-authored-by: Kamil Breguła <kamil.bregula@snowflake.com>
Thanks @sfc-gh-kbregula, done! |
* develop: cli.py cleanup (streamlit#5737) Add CI job to enforce security assessments (streamlit#5709)
📚 Context
This PR adds a simple CI job that fails unless the
security-assessment-completedlabel has beenadded to the PR. The intention here is to simply ensure that we don't merge PRs without being reminded
to go through the security assessment process if the PR warrants it. The vast majority of our PRs likely
won't require an in-depth security review, but we do want to make sure that we don't prematurely merge
PRs that we should be getting security reviews for just because we forgot about the process.
NOTE: This security process is only something that maintainers will need to think about. Open source
contributors, of course, may be asked to make changes to their PRs in case that there is a security implication
that needs to be addressed, but the security process itself is a process internal to the Streamlit team.
What kind of change does this PR introduce?