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

git-completion: sanitize the command names #229

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

MrMebelMan
Copy link

When autocompleting git commands, ensure that the command name does not contain special characters.

I've found this by misspelling pull as [ull.

Steps to reproduce:

git [ <TAB>

The output is following:

bash: declare: `_git_[': not a valid identifier
error: invalid key: alias.[

@gitgitgadget
Copy link

gitgitgadget bot commented May 28, 2019

Welcome to GitGitGadget

Hi @MrMebelMan, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests.

Please make sure that this Pull Request has a good description, as it will be used as cover letter.

Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "commit:", and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code.

Contributing the patches

Before you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a PR comment of the form /allow <username>.

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions.

If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox ("raw") file corresponding to the mail you want to reply to from the Git mailing list. If you use GMail, you can upload that raw mbox file via:

curl -g --user "<EMailAddress>:<Password>" --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

@MrMebelMan MrMebelMan force-pushed the autocomplete_command_sanitation branch from 43baf29 to dd8df8f Compare May 28, 2019 16:38
@dscho
Copy link
Member

dscho commented May 28, 2019

/allow MrMebelMan

@gitgitgadget
Copy link

gitgitgadget bot commented May 28, 2019

User MrMebelMan is now allowed to use GitGitGadget.

@dscho
Copy link
Member

dscho commented May 28, 2019

Welcome, @MrMebelMan !

This seems like a desirable change, but the commit message probably wants to sprout some convincing arguments to that end.

Also, the commit message starts with a *, something that no other commit message in git.git has, so maybe drop that?

Further, the commit message could possibly say that yes, this patch adds some Bash'ism, but that's okay because it is in the Bash-specific part of the completions. I would do that if I were you, as the rest of Git's shell scripts is held to a pretty high (portable) standard that does not allow any Bash-specific constructs.

@MrMebelMan
Copy link
Author

MrMebelMan commented May 29, 2019

Thanks for the feedback, @dscho! I'll remove the asterisk and add the note about BASHism to the commit message as you suggested 🙏

I didn't come up with any "convincing" arguments though... I just think that it shouldn't throw an error when user misspells the command name :)

@MrMebelMan MrMebelMan force-pushed the autocomplete_command_sanitation branch from dd8df8f to 3d367ce Compare May 29, 2019 14:17
@dscho
Copy link
Member

dscho commented May 29, 2019

I didn't come up with any "convincing" arguments though... I just think that it shouldn't throw an error when user misspells the command name :)

But if you expand on that, it would be a convincing argument, no? Something along the lines "if the user specified a command name containing illegal characters, it clearly is not a Git command that they were talking about" or some such...

Do not declare/execute commands that contain invalid or special characters.

If the autocompleted command contains illegal characters, for example when
misspelling `git pull` as `git [ull`, then the user will see an error. This
patch adds a character whitelist for commands that strips all but
lowercase alphabetic characters and dashes, so that misspells fail silently.

This patch uses the `[[` keyword that is not sh-compatible, but it's
okay since the change affects BASH and ZSH-specific autocomplete
scripts.

Signed-off-by: Vladyslav Burzakovskyy <vladislav.burzakovskij@satoshilabs.com>
@MrMebelMan MrMebelMan force-pushed the autocomplete_command_sanitation branch from 3d367ce to 4abd284 Compare May 30, 2019 11:05
@MrMebelMan
Copy link
Author

@dscho, thanks again, I think I finally got it :D I've altered the commit message as you said 🙏

@dscho
Copy link
Member

dscho commented May 30, 2019

@MrMebelMan nice!

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