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

fix(lint-content): move variable to env to prevent command injection #35352

Merged
merged 2 commits into from
Aug 9, 2024

Conversation

svennergr
Copy link
Contributor

Description

This PR moves the existing DIFF_DOCUMENTS environment variable to be read from environment variables instead of interpolating that variable in the run script itself.

Motivation

This prevents command/script injections.

@svennergr svennergr requested a review from a team as a code owner August 7, 2024 08:02
@github-actions github-actions bot added system [PR only] Infrastructure and configuration for the project size/s [PR only] 6-50 LoC changed labels Aug 7, 2024
Copy link
Contributor

@fiji-flo fiji-flo left a comment

Choose a reason for hiding this comment

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

Thanks a lot for fixing!

@fiji-flo fiji-flo merged commit 53675bb into mdn:main Aug 9, 2024
7 of 8 checks passed
@svennergr svennergr deleted the svennergr/fix-env-workflow branch August 9, 2024 12:51
@OnkarRuikar
Copy link
Contributor

A few questions:

  1. Should we also use intermediate environment variables?
  2. We didn't do this change on line 35. Can we should we do it at least for sake of consistency?
  3. Should we do this in other workflows as well? Regardless of the pull_request_target trigger? If not, then for consistency?

@Josh-Cena
Copy link
Member

@OnkarRuikar I believe that this PR only fixes env variables created by us, as env.HEAD_SHA etc. are injected by GH and should be treated as secure. I'm +1 for consistency though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/s [PR only] 6-50 LoC changed system [PR only] Infrastructure and configuration for the project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants