fix(install.ps1): use ordinal IndexOf when stripping index URL credentials - #7286
danielhanchen merged 2 commits into
Conversation
…tials On non-English Windows locales, culture-aware String.IndexOf can mis-locate punctuation-only markers like ://, which corrupts scheme and authority parsing and crashes Remove-IndexUrlCredentials with a Substring ArgumentOutOfRangeException (issue 7279). Force Ordinal comparison for URL scheme/host parsing. Fixes #7279
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Remove-IndexUrlCredentials function in install.ps1 to use culture-invariant ordinal string comparisons, preventing potential crashes on non-English Windows locales. The reviewer suggests a more robust refactoring of the function using the built-in [System.Uri] and [System.UriBuilder] classes to avoid manual string parsing.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| function Remove-IndexUrlCredentials { | ||
| param([string]$Url) | ||
| $sep = $Url.IndexOf('://') | ||
| # URL parsing must be culture-invariant. On non-English Windows locales | ||
| # (e.g. th-TH), culture-aware IndexOf can mis-locate punctuation-only | ||
| # markers like "://", corrupting scheme/authority and crashing on | ||
| # Substring (issue #7279). Always use Ordinal comparison here. | ||
| $sep = $Url.IndexOf('://', [System.StringComparison]::Ordinal) | ||
| if ($sep -lt 0) { return $Url } | ||
| $scheme = $Url.Substring(0, $sep) | ||
| $rest = $Url.Substring($sep + 3) | ||
| # Drop query / fragment (may hold auth tokens). | ||
| $q = $rest.IndexOfAny([char[]]('?', '#')) | ||
| if ($q -ge 0) { $rest = $rest.Substring(0, $q) } | ||
| $slash = $rest.IndexOf('/') | ||
| $slash = $rest.IndexOf('/', [System.StringComparison]::Ordinal) | ||
| $authority = if ($slash -ge 0) { $rest.Substring(0, $slash) } else { $rest } | ||
| $at = $authority.LastIndexOf('@') | ||
| $at = $authority.LastIndexOf('@', [System.StringComparison]::Ordinal) | ||
| $host_ = if ($at -ge 0) { $authority.Substring($at + 1) } else { $authority } | ||
| if ($slash -ge 0) { return "${scheme}://${host_}$($rest.Substring($slash))" } | ||
| return "${scheme}://${host_}" |
There was a problem hiding this comment.
While using StringComparison.Ordinal is a good fix for the culture-specific issue, the entire function can be made more robust and maintainable by using the built-in [System.Uri] and [System.UriBuilder] classes for URL parsing and manipulation. This avoids manual string parsing, which can be error-prone.
This refactoring correctly removes user credentials, query strings, and fragments, while also handling various URL edge cases more gracefully. Note that this change will normalize URLs by adding a trailing slash to host-only URLs (e.g., https://example.com becomes https://example.com/), which is a standard practice.
function Remove-IndexUrlCredentials {
param([string]$Url)
if (-not ([System.Uri]::IsWellFormedUriString($Url, [System.UriKind]::Absolute))) {
return $Url
}
try {
$uri = [System.Uri]$Url
$builder = [System.UriBuilder]::new($uri.Scheme, $uri.Host, $uri.Port, $uri.AbsolutePath)
return $builder.Uri.AbsoluteUri
} catch {
return $Url
}
}
There was a problem hiding this comment.
Thanks, but keeping the minimal ordinal fix here. This helper is documented to match the string parsing in install.sh and studio/install_python_stack.py, so a UriBuilder rewrite would diverge from those siblings. It also changes output: System.UriBuilder normalizes host-only URLs by appending a trailing slash (https://download.pytorch.org becomes https://download.pytorch.org/), and System.Uri applies its own path, percent-encoding and IDN normalization, any of which can alter a wheel index or mirror URL. The ordinal fix resolves the crash with no behavior change on well-formed URLs, so it stays.
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
3 participants
Summary
String.IndexOfcan mis-locate punctuation-only markers like://.Remove-IndexUrlCredentialsand crashes withSubstringArgumentOutOfRangeException right after GPU detection.StringComparison.Ordinalfor scheme / path / userinfo parsing ininstall.ps1.Fixes #7279
Test plan
irm https://raw.githubusercontent.com/unslothai/unsloth/main/install.ps1 | iexshould pass PyTorch index URL step without Substring crash