Skip to content

Conversation

@nbayati
Copy link
Contributor

@nbayati nbayati commented Nov 3, 2025

The executable command for pluggable auth was being parsed with strings.Fields, which does not handle quoted arguments containing spaces correctly. This caused errors when the executable path or its arguments contained spaces.

This change replaces strings.Fields with shellwords.Parse from the github.com/mattn/go-shellwords library. This library correctly parses shell command strings, respecting quotes.

This change adds a new dependency on github.com/mattn/go-shellwords.

This was reported in b/238143555

The executable command for pluggable auth was being parsed with
`strings.Fields`, which does not handle quoted arguments containing
spaces correctly. This caused errors when the executable path or its
arguments contained spaces.

This change replaces `strings.Fields` with `shellwords.Parse` from the
`github.com/mattn/go-shellwords` library. This library correctly
parses shell command strings, respecting quotes.

This change adds a new dependency on `github.com/mattn/go-shellwords`.
@nbayati nbayati requested a review from a team as a code owner November 3, 2025 21:54
@gemini-code-assist
Copy link
Contributor

Summary of Changes

Hello @nbayati, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request addresses a critical parsing flaw in the pluggable authentication mechanism where executable commands with spaces in their paths or arguments were incorrectly interpreted. By switching to a more sophisticated shellwords parser, the system can now reliably execute commands, significantly improving the stability and compatibility of the authentication process. This change ensures that complex command structures are handled as intended, preventing authentication failures due to malformed command execution.

Highlights

  • Bug Fix: Corrected an issue where executable commands for pluggable authentication failed to parse correctly when paths or arguments contained spaces, leading to errors.
  • Parsing Logic Update: Replaced the use of strings.Fields with shellwords.Parse from the github.com/mattn/go-shellwords library to accurately handle shell command strings, including those with quoted arguments.
  • New Dependency: Introduced a new indirect dependency on github.com/mattn/go-shellwords to facilitate robust command-line argument parsing.
  • Improved Robustness: Added error handling for the new parsing logic, specifically checking for parsing errors and empty commands to prevent unexpected behavior.
  • Test Coverage: Added new unit tests to executable_provider_test.go to verify the correct behavior of shellwords.Parse with various scenarios involving quoted and unquoted arguments.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request correctly fixes an issue with parsing executable commands that contain quoted arguments with spaces. The change replaces strings.Fields with shellwords.Parse, which is the right approach for this problem. The introduction of the new dependency is justified. My feedback includes a suggestion to enhance the test coverage for the new error handling path that was introduced.

Comment on lines +33 to +64
tests := []struct {
name string
command string
want []string
wantErr bool
}{
{
name: "simple command",
command: "a b c",
want: []string{"a", "b", "c"},
},
{
name: "quoted argument",
command: `a "b c"`,
want: []string{"a", "b c"},
},
{
name: "quoted executable",
command: `"a b" c`,
want: []string{"a b", "c"},
},
{
name: "quoted executable and argument",
command: `"a b" "c d"`,
want: []string{"a b", "c d"},
},
{
name: "single quotes",
command: `'a b' 'c d'`,
want: []string{"a b", "c d"},
},
}
Copy link
Contributor

Choose a reason for hiding this comment

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

medium

This test is a good addition for verifying the new parsing logic. To improve test coverage for the new error handling in the run function, please consider adding test cases for empty and whitespace-only command strings. These inputs cause shellwords.Parse to return an empty slice, which triggers the new if len(splitCommand) == 0 check.

 	tests := []struct {
 		name    string
 		command string
 		want    []string
 		wantErr bool
 	}{
 		{
 			name:    "simple command",
 			command: "a b c",
 			want:    []string{"a", "b", "c"},
 		},
 		{
 			name:    "quoted argument",
 			command: `a "b c"`,
 			want:    []string{"a", "b c"},
 		},
 		{
 			name:    "quoted executable",
 			command: `"a b" c`,
 			want:    []string{"a b", "c"},
 		},
 		{
 			name:    "quoted executable and argument",
 			command: `"a b" "c d"`,
 			want:    []string{"a b", "c d"},
 		},
 		{
 			name:    "single quotes",
 			command: `'a b' 'c d'`,
 			want:    []string{"a b", "c d"},
 		},
 		{
 			name:    "empty command",
 			command: "",
 			want:    []string{},
 		},
 		{
 			name:    "whitespace only",
 			command: "   ",
 			want:    []string{},
 		},
 	}

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.

1 participant