Skip to content

Avoid IndexOutOfRangeException in command-line parsing #7648

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.

Sign up for GitHub

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

Merged
Youssef1313 merged 2 commits into from
Apr 1, 2026
Merged

Avoid IndexOutOfRangeException in command-line parsing #7648

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
451b097
Avoid IndexOutOfRangeException in command-line parsing
Youssef1313 Apr 1, 2026
32a4cc2
Update test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLin…
Youssef1313 Apr 1, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Platform/Microsoft.Testing.Platform/CommandLine/Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ private static CommandLineParseResult Parse(List<string> args, IEnvironment envi

// If it's the first argument and it doesn't start with - then it's the tool name
// TODO: This won't work correctly if the first argument provided is a response file that contains the tool name.
if (isFirstRealArgument && currentArg[0] != '-')
if (isFirstRealArgument && currentArg.Length > 0 && currentArg[0] != '-')
Copy link
Copy Markdown
Member

@Evangelink Evangelink Apr 1, 2026

Choose a reason for hiding this comment

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

The production fix is correct and minimal — this was the only unguarded indexing on currentArg in the method. The StartsWith('@') above is safe on empty strings, and the length checks on lines 68-69 (Length > 1, Length > 2) already had proper guards. 👍

{
toolName = currentArg;
isFirstRealArgument = false;
Expand Down
22 changes: 22 additions & 0 deletions test/UnitTests/Microsoft.Testing.Platform.UnitTests/CommandLine/ArgumentArityTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,28 @@ public sealed class ArgumentArityTests
new ExtensionCommandLineProviderMockOptionsWithDifferentArity()
];

[TestMethod]
public async Task ParseAndValidate_EmptyArgument_ShouldNotThrowException()
{
// Arrange
string[] args = [string.Empty];
CommandLineParseResult parseResult = CommandLineParser.Parse(args, new SystemEnvironment());

// Act
ValidationResult result = await CommandLineOptionsValidator.ValidateAsync(parseResult, _systemCommandLineOptionsProviders,
_extensionCommandLineOptionsProviders, new Mock<ICommandLineOptions>().Object);

// Assert
Assert.IsFalse(result.IsValid);
#pragma warning disable SA1027 // Use tabs correctly
Comment on lines +38 to +39
Copy link
Copy Markdown
Member

@Evangelink Evangelink Apr 1, 2026

Choose a reason for hiding this comment

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

Disabling SA1027 to embed a literal tab in the raw string is a bit of code smell in a test. If you do keep the exact-match assertion, you can avoid the pragma by constructing the expected string with "\t":

var expected = string.Join(
    Environment.NewLine,
    "Invalid command line arguments:",
    "\t- Unexpected argument ",
    string.Empty);
result.ErrorMessage.Should().Be(expected);

Though as noted above, a Should().Contain("Unexpected argument") assertion would sidestep this entirely and still validate the regression.

Assert.AreEqual(
"""
Invalid command line arguments:
- Unexpected argument
""", result.ErrorMessage, StringComparer.Ordinal);
Comment on lines +34 to +44
Copy link
Copy Markdown
Member

@Evangelink Evangelink Apr 1, 2026

Choose a reason for hiding this comment

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

Per repo conventions in .github/copilot-instructions.md:

"All assertions must be written using FluentAssertions style of assertion."

The new test uses Assert.IsFalse and Assert.AreEqual. Consider switching to:

result.IsValid.Should().BeFalse();
result.ErrorMessage.Should().Contain("Unexpected argument");

Using Should().Contain(...) instead of exact multi-line match would also avoid the #pragma disable SA1027 and make the test more resilient to harmless formatting changes in the error message (indentation, newlines, trailing space).

(The existing tests in the same file also use Assert.* — that's a pre-existing inconsistency, but a new test is a good opportunity to follow the stated convention.)

#pragma warning restore SA1027 // Use tabs correctly
Comment thread
Youssef1313 marked this conversation as resolved.
}

[TestMethod]
public async Task ParseAndValidate_WhenOptionWithArityZeroIsCalledWithOneArgument_ReturnsFalse()
{
Comment thread
Youssef1313 marked this conversation as resolved.
Expand Down
Loading