-
Notifications
You must be signed in to change notification settings - Fork 88
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
chore: secure hermetic_library_generation workflow #2317
Conversation
Thanks to @diogoteles08 for the inspection on our repos. This PR inlines environment variables to avoid overriding script injections.
outputs: | ||
REPO_FULL_NAME: ${{ env.REPO_FULL_NAME }} | ||
steps: | ||
- run: echo "" >> /dev/null # no op - we just need to declare the env var as output |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this empty step required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# skip pull requests come from a forked repository | ||
if: github.event.pull_request.head.repo.full_name == github.repository | ||
# skip pull requests coming from a forked repository | ||
if: needs.prepare-repo-full-name.outputs.REPO_FULL_NAME == github.repository |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we move this if condition to the run
section? Did we consider the approach before @JoeWang1127 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I modified it to just have an if
inside run
. I'll try a fork PR into this branch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried raising a PR from a fork
I tried setting the missing token in my fork without luck.
However I could confirm that the generation was properly triggered in this PR because it doesn't come from a fork.
https://github.com/googleapis/java-bigtable/actions/runs/10562926017/job/29261969452?pr=2317
Could you create a forked repo and test whether the workflow will be skipped? |
@JoeWang1127 we tried this in #2317 (comment). I think we still confirmed it works the other way around: non-forked repos actually get triggered. |
…ains common protos (#3162) In this PR: - Always perform a full generation in a non-monorepo or the repo contains common protos. - Secure generation workflow (use environment variable to avoid script injection), inspired by googleapis/java-bigtable#2317 This PR also brings changes in common protos and iam due to protoc updates (25.3 -> 25.4). --------- Co-authored-by: cloud-java-bot <[email protected]>
…ains common protos (#3162) In this PR: - Always perform a full generation in a non-monorepo or the repo contains common protos. - Secure generation workflow (use environment variable to avoid script injection), inspired by googleapis/java-bigtable#2317 This PR also brings changes in common protos and iam due to protoc updates (25.3 -> 25.4). --------- Co-authored-by: cloud-java-bot <[email protected]>
Thanks to @diogoteles08 for the inspection on our repos. This PR inlines environment variables to avoid overriding script injections.