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

Before tarring site, set file read permissions #34

Merged
merged 4 commits into from
Nov 15, 2022

Conversation

TooManyBees
Copy link
Contributor

@TooManyBees TooManyBees commented Nov 7, 2022

Adds read permission before tarring to prevent unreadable files from being deployed. This only affects files that are built on an actions runner, not files that are cloned from a git repoitory, because git's permissions are limited to the equivalents of chmod 644 and 755.

Uses gchmod on mac to conform with chmod on gnu/linux.

Pages DFS needs the "group read" permission set on any file it serves,
so add read permission before tarring to prevent unreadable files from
being deployed.

This only applies to files that are built on an actions runner, not
files that are cloned from a git repoitory, because git's permissions
are limited to the equivalents of chmod 644 and 755.
@TooManyBees TooManyBees requested a review from a team as a code owner November 7, 2022 18:55
@yoannchaudet
Copy link
Contributor

Should we parse the output and issue warnings?

The more I think about it the more I see it needing eventually to become a JavaScript Action 🙃

@TooManyBees
Copy link
Contributor Author

added warnings, they look like this

Screen Shot 2022-11-08 at 10 56 13 AM

@actions actions deleted a comment Nov 10, 2022
Copy link
Contributor

@JamesMGreene JamesMGreene left a comment

Choose a reason for hiding this comment

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

Love the annotations but curious about the use of gchmod for Mac. Also: do we need to do anything similar for Windows?

action.yml Outdated Show resolved Hide resolved
Copy link

@felipesu19 felipesu19 left a comment

Choose a reason for hiding this comment

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

I like the bash

Copy link
Contributor

@yoannchaudet yoannchaudet left a comment

Choose a reason for hiding this comment

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

Looks great to me 🚀 !

@@ -17,6 +17,10 @@ runs:
shell: sh
if: runner.os == 'Linux'
run: |
for f in $(chmod -c -R +r . | awk '{print substr($3, 2, length($3)-2)}')

Choose a reason for hiding this comment

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

If a filename has a blank in it, this'll pick up only the part of the name before the blank; a slightly more robust way could be to use ' as the field separator and loop over entire lines instead of splitting on words:

chmod -c -R +r . | awk -F\' '{ print $2 }' \
    | while IFS= read -r f; do
        echo "::warning::Added read permission to $f"
    done

but that breaks if the filename contains quotes. With GNU grep, a solution that should be able to handle all characters in filenames could look like

chmod -c -R +r . | grep -Po '^mode of (["'\''])\K.*(?=\1 changed from \d+)'

handling blanks, single and double quotes, and newlines in filenames, printing like

./subdir/file
./file'$'\n''newline
./file"'\''quotes
./file'quote
./has space

but that would require to rewrite the loop to

chmod -c -R +r . \
    | grep -Po '^mode of (["'\''])\K.*(?=\1 changed from \d+)' \
    | while IFS= read -r f; do
        echo "::warning::Added read permission to $f"
    done

This still works in sh, but I understand the action tries to support more limited environments like busybox, where grep won't have the -P option.

Probably not super important because it only affects the warning message, not the actual chmod operation, but I thought I'd point it out.

Choose a reason for hiding this comment

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

Or maybe avoid all parsing and have the entire output from chmod go into the warning?

Copy link
Contributor

Choose a reason for hiding this comment

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

Kind of like the suggestion of not parsing at all and just wrapping the output in a warning 👍

@yoannchaudet yoannchaudet mentioned this pull request Dec 8, 2022
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.

None yet

5 participants