Skip to content

feat: adds new github-env audit - #192

Merged
woodruffw merged 6 commits into
zizmorcore:mainfrom
tewokumu:ufs/patch-07
Nov 24, 2024
Merged

woodruffw merged 6 commits into
zizmorcore:mainfrom
tewokumu:ufs/patch-07

Conversation

@ubiratansoares

Copy link
Copy Markdown
Contributor
  • Pulls dangerous triggers detection logic from WorkflowAudit and moves to impl DangerousTriggers instance methods
  • Reuses only that logic in the context of to implement a new audit
cargo run -- tests/test-data/github_env.yml

error[github-env]: dangerous use of $GITHUB_ENV
  --> tests/test-data/github_env.yml:12:9
   |
12 |           run: |
   |  _________^
13 | |           message=$(echo "$TITLE" | grep -oP '[{\[][^}\]]+[}\]]' | sed 's/{\|}\|\[\|\]//g')
14 | |           echo "message=$message" >> $GITHUB_ENV
   | |_________________________________________________^ GITHUB_ENV used in the context of a dangerous Workflow trigger
   |
   = note: audit confidence → High

2 findings (1 ignored): 0 unknown, 0 informational, 0 low, 0 medium, 1 high

Some questions below!

Comment thread src/audit/github_env.rs Outdated
Comment thread src/audit/github_env.rs Outdated
@woodruffw

Copy link
Copy Markdown
Member

Thanks @ubiratansoares! I'll give this a more detailed review later today.

Pulls dangerous triggers detection logic from WorkflowAudit and moves to impl DangerousTriggers instance methods

Rather than use sharing APIs across audits, I think we can probably turn these into APIs on models::Workflow -- Workflow::has_pull_request_target and similar would probably be good.

@woodruffw woodruffw added the new-audit New audits label Nov 24, 2024
Comment thread src/audit/dangerous_triggers.rs Outdated
Comment thread src/audit/dangerous_triggers.rs Outdated
@ubiratansoares
ubiratansoares marked this pull request as ready for review November 24, 2024 21:40
@ubiratansoares

Copy link
Copy Markdown
Contributor Author

I address all the comments, including moving dangerous triggers detection logic to models::Workflow.

I also added some docs covering the basics based on the discussions we have in the related issue, but I did not go deep there are safe ways to use $GITHUB_ENV. Happy to extended those docs based on additional input!

@ubiratansoares

Copy link
Copy Markdown
Contributor Author

@woodruffw we miss an example here, happy to raise a PR there, lmk!

@woodruffw

Copy link
Copy Markdown
Member

@woodruffw we miss an example here, happy to raise a PR there, lmk!

Please do, thanks!

Comment thread src/audit/github_env.rs Outdated
Comment thread src/audit/github_env.rs
// eventually detecting how $GITHUB_ENV is being used
// and returning an Option<Confidence> instead

run_step_body.contains("$GITHUB_ENV") || run_step_body.contains("${GITHUB_ENV}")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just thinking out loud: in terms of improving this, we could probably use a regexp like:

^.+\s*>>?\s*"?\$\{?GITHUB_ENV\}?"?.*$

That'll catch a bunch of common cases, but it's also a mess. So we can defer that for a follow-up.

Comment thread docs/audits.md Outdated
Comment thread docs/audits.md Outdated
Comment thread docs/audits.md Outdated
@woodruffw woodruffw added the enhancement New feature or request label Nov 24, 2024

@woodruffw woodruffw left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, thanks @ubiratansoares!

I'll make a follow-up to track refinements here.

@woodruffw
woodruffw merged commit 6e33827 into zizmorcore:main Nov 24, 2024
@woodruffw woodruffw mentioned this pull request Nov 24, 2024
2 of 3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request new-audit New audits

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants