Skip to content
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

workflows/eval: avoid potential script injection attack #357753

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

azuwis
Copy link
Contributor

@azuwis azuwis commented Nov 21, 2024

Although matrix.system is supposed to be generated from trusted code, we'd better follow Github Actions good practices.

CC: @infinisil

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: policy discussion 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Nov 21, 2024
@JohnRTitor
Copy link
Contributor

Could you make the changes for #355847 (comment), in a seperate commit?

@JohnRTitor JohnRTitor requested a review from infinisil November 21, 2024 03:37
Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

Oh yeah nice catch. This workflow doesn't (currently) deal with secrets, so nothing bad could've happened, but still

@JohnRTitor
Copy link
Contributor

@infinisil I am seeing another CI fail (likely also related to high per chunk memory)?

https://github.com/NixOS/nixpkgs/actions/runs/11940805989/job/33284337358

Would reducing chunk size to 750 be sufficient?

@infinisil
Copy link
Member

I'm thinking 9000, we can easily lower it further if needed

@JohnRTitor
Copy link
Contributor

oh I missed an extra 0. 🤦

--arg attrpathFile ./paths/paths.json \
--arg chunkSize 10000
--arg chunkSize 9000
Copy link
Member

Choose a reason for hiding this comment

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

Can we first just merge the first commit: #357800

@Mic92
Copy link
Member

Mic92 commented Nov 21, 2024

I'm thinking 9000, we can easily lower it further if needed

There seems to be plenty of memory available. Infinite recursion errors are related to stack memory limits. Should we not rather increase the stack size?

@JohnRTitor
Copy link
Contributor

Sorry shouldn't have mixed these two

@Mic92
Copy link
Member

Mic92 commented Nov 21, 2024

Here is my proposal to better understand where the infinite recursion errors come from: #357805

@JohnRTitor JohnRTitor merged commit 8677027 into NixOS:master Nov 21, 2024
43 of 44 checks passed
@JohnRTitor
Copy link
Contributor

Oof, merged this one, and #357805 seemed to merge automatically as well

(Github has a nice way of detecting this)

@Mic92
Copy link
Member

Mic92 commented Nov 21, 2024

@JohnRTitor that's normal and not problematic. It's not #357805 that was merged but #357800

@JohnRTitor
Copy link
Contributor

By the way, any idea why backport PRs are not automatically evaluated? #357821

@infinisil
Copy link
Member

@JohnRTitor It's because the backport was created by the default GitHub Actions account, which GitHub prevents from triggering other actions to prevent too easy infinite CI recursions. If we use a GitHub App instead tp create the PR that would be fixed

@JohnRTitor
Copy link
Contributor

Could we create a listener to automatically start the eval action, like @ofborg eval?

@azuwis
Copy link
Contributor Author

azuwis commented Nov 21, 2024

By the way, any idea why backport PRs are not automatically evaluated? #357821

Github Actions limitation.

Some documentations and workarounds https://github.com/peter-evans/create-pull-request/blob/main/docs/concepts-guidelines.md#triggering-further-workflow-runs

@azuwis azuwis deleted the push-wxyxpyzlzmnm branch November 22, 2024 06:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions 6.topic: policy discussion
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants