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

Remove unnecessary executable bit from several files #2645

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

jdufresne
Copy link
Contributor

The files modified are never used as executable entry points.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
The files modified are never used as executable entry points.
@stefannibrasil
Copy link
Contributor

@jdufresne I am not really sure what this PR is aiming to solve. Could you explain what issue you're solving and why are you adding empty files to the library? Thank you!

@jdufresne
Copy link
Contributor Author

jdufresne commented Dec 12, 2022

I'm note sure why GitHub says "Empty file." but I'm not adding empty files.

I'm removing the executable permission bit from files that do not need or use it. For example, on the main branch:

$ git checkout main
$ find . -name .git -prune -o -type f -executable -exec ls -l {} \+
-rwxrwxr-x. 1 jon jon  164 Mar 13  2021 ./bin/faker
-rwxr-xr-x. 1 jon jon  203 Dec 12 15:13 ./doc/blockchain/bitcoin.md
-rwxr-xr-x. 1 jon jon  160 Dec 12 15:13 ./doc/blockchain/ethereum.md
-rwxr-xr-x. 1 jon jon  441 Dec 12 15:13 ./doc/movies/harry_potter.md
-rwxr-xr-x. 1 jon jon  744 Dec 12 15:13 ./doc/movies/hitchhikers_guide_to_the_galaxy.md
-rwxr-xr-x. 1 jon jon  409 Dec 12 15:13 ./doc/movies/hobbit.md
-rwxr-xr-x. 1 jon jon  129 Dec 12 15:13 ./doc/music/rock_band.md
-rwxr-xr-x. 1 jon jon   92 Dec 12 15:13 ./doc/music/umphreys_mcgee.md
-rwxr-xr-x. 1 jon jon  140 Dec 12 15:13 ./doc/tv_shows/michael_scott.md
-rwxr-xr-x. 1 jon jon  187 Dec 12 15:13 ./doc/tv_shows/rupaul.md
-rwxr-xr-x. 1 jon jon 8990 Dec 12 15:13 ./lib/locales/en/heroes.yml
-rwxrwxr-x. 1 jon jon  395 Mar 13  2021 ./script/destroy
-rwxrwxr-x. 1 jon jon  397 Mar 13  2021 ./script/generate
-rwxr-xr-x. 1 jon jon 1958 Oct 18 03:38 ./script/txt2html
-rwxr-xr-x. 1 jon jon  435 Dec 12 15:13 ./test/faker/games/test_faker_heroes.rb

Note several files that are not zero bytes in size have the executable bit set (x).

If I checkout my branch, they no longer there:

$ git checkout exec-bit
$ find . -name .git -prune -o -type f -executable -exec ls -l {} \+
-rwxrwxr-x. 1 jon jon  164 Mar 13  2021 ./bin/faker
-rwxrwxr-x. 1 jon jon  395 Mar 13  2021 ./script/destroy
-rwxrwxr-x. 1 jon jon  397 Mar 13  2021 ./script/generate
-rwxr-xr-x. 1 jon jon 1958 Oct 18 03:38 ./script/txt2html

The files modified still have non-zero sizes:

$ ls -l $(git diff --name-only main)
-rw-r--r--. 1 jon jon  203 Dec 12 15:16 doc/blockchain/bitcoin.md
-rw-r--r--. 1 jon jon  160 Dec 12 15:16 doc/blockchain/ethereum.md
-rw-r--r--. 1 jon jon  441 Dec 12 15:16 doc/movies/harry_potter.md
-rw-r--r--. 1 jon jon  744 Dec 12 15:16 doc/movies/hitchhikers_guide_to_the_galaxy.md
-rw-r--r--. 1 jon jon  409 Dec 12 15:16 doc/movies/hobbit.md
-rw-r--r--. 1 jon jon  129 Dec 12 15:16 doc/music/rock_band.md
-rw-r--r--. 1 jon jon   92 Dec 12 15:16 doc/music/umphreys_mcgee.md
-rw-r--r--. 1 jon jon  140 Dec 12 15:16 doc/tv_shows/michael_scott.md
-rw-r--r--. 1 jon jon  187 Dec 12 15:16 doc/tv_shows/rupaul.md
-rw-r--r--. 1 jon jon 8990 Dec 12 15:16 lib/locales/en/heroes.yml
-rw-r--r--. 1 jon jon  435 Dec 12 15:16 test/faker/games/test_faker_heroes.rb

I noticed this by auditing all gems installed in my production and CI environments. Having unnecessary executable bit set is a slight security risk and it makes it easier for an unintended subprocess to run these script files. I don't think there would be a real danger in practice, but might as well apply best practices here.

In general, a file should only have the executable permission set if it is intended to be run directly by the user as the main entry point to a program. That is not the case here.

@stefannibrasil
Copy link
Contributor

hum @jdufresne this is indeed strange. I am not sure about where this comes from.

@koic @vbrazo @Zeragamba do you have any context about this?

@Zeragamba
Copy link
Contributor

This probably came about due to some contributors using Windows while developing their feature

@jdufresne
Copy link
Contributor Author

This probably came about due to some contributors using Windows while developing their feature

Yeah, that seems like a likely source to me.

@stefannibrasil
Copy link
Contributor

@jdufresne do you mind copying and pasting the explanation you shared here in the PR description? We want to document the why for the future.

@Zeragamba this one looks good to me. Could I assign it to you to approve/merge? Thank you!

@Zeragamba Zeragamba merged commit 3283761 into faker-ruby:main Dec 20, 2022
@Zeragamba
Copy link
Contributor

Merged :), and mentioned the comment in the commit message, so that should be good too

@jdufresne jdufresne deleted the exec-bit branch December 20, 2022 13:49
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

3 participants