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

Check for newlines and carriage return in artifact paths and name #951

Merged
merged 5 commits into from
Dec 1, 2021

Conversation

konradpabjan
Copy link
Contributor

@konradpabjan konradpabjan commented Dec 1, 2021

Enforce checking for newline characters and carriage returns in the artifact name and file paths that are uploaded

Will close out: actions/upload-artifact#183

  • Restructuring some files + tests so that all the code for validating artifact names + paths is in a single file (not in utils.ts)
  • Make the error output more readible, the old code would split the message into multiple lines because the actual newline characters were displayed
  • Add tests for \\r and \\n

For invalid artifact name: https://github.com/konradpabjan/artifact-test/actions/runs/1527098177
image

For invalid path: https://github.com/konradpabjan/artifact-test/actions/runs/1527052660
image

Comment on lines +56 to +58
'some/invalid\rartifact/path',
'some/invalid\nartifact/path',
'some/invalid\r\nartifact/path',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New tests are here, I just moved everything into a separate file for better organization

@konradpabjan konradpabjan requested a review from a team December 1, 2021 18:45
@konradpabjan konradpabjan marked this pull request as ready for review December 1, 2021 19:00
@konradpabjan konradpabjan requested a review from a team December 1, 2021 19:00
Copy link
Contributor

@robherley robherley left a comment

Choose a reason for hiding this comment

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

👍 Just some small comments/nits

Should we worry about file name length? A lot of common fs limit to 255 iirc

@konradpabjan konradpabjan merged commit 88062ec into main Dec 1, 2021
@konradpabjan konradpabjan deleted the konradpabjan/newline-in-artifact-name branch December 1, 2021 21:31
at-wat pushed a commit to at-wat/actions-toolkit that referenced this pull request Aug 31, 2023
…tions#951)

* Check for newlines and carriage return in artifact paths and name

* Fix linting issue

* Update comments

* Add comment about spacing

* Remove extra space
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants