Fix PSUserAgent Generation Exception on Windows 7 #5256
Conversation
markekraus
requested review from
JamesWTruher,
PaulHigin and
dantraMSFT
as code owners
| Regex pattern = new Regex(@"\d+(\.\d+)+"); | ||
| string versionText = pattern.Match(OS).Value; | ||
| Version windowsPlatformversion = new Version(versionText); | ||
| s_windowsUserAgent = $"Windows NT {windowsPlatformversion.Major}.{windowsPlatformversion.Minor}"; |
There was a problem hiding this comment.
Why do we parse? Maybe just return OS (OSDescription) ?
There was a problem hiding this comment.
Because of the format expected in User-Agents. the OS portion should be Windows NT <Major Version>.<Minor Version> That's not native anywhere in the CoreFX name space, System.Environment.OSVersion.Version reports 6.2 for major/minor on windows 10, but User-Agent expects 10.0. ugh.
There was a problem hiding this comment.
Please clarify for me what is "the format expected in User-Agents"? My understanding is that the string is sent to a web server as UserAgent string and falls in a web log file so if I see the OSDescription in the log it is useful.
There was a problem hiding this comment.
User-Agents have an expected format. The first part, Platform, has a specific format to detect Windows/Linux/macOS. the full OSDescription follows the Platform field. here is the full user-agent strings with this change:
windows 10:
Mozilla/5.0 (Windows NT 10.0; Microsoft Windows 10.0.15063; en-US) PowerShell/6.0.0
windows 7
Mozilla/5.0 (Windows NT 6.1; Microsoft Windows 6.1.7601 S; en-US) PowerShell/6.0.0
The change in this PR is for the Windows NT 10.0 and Windows NT 6.1 Platform fields.
There was a problem hiding this comment.
@markekraus I think we must merge and fix the bug and I'll open new Issue to discuss User-Agents.
| string versionText = OS.Substring(OS.LastIndexOf(" ") +1); | ||
| Regex pattern = new Regex(@"\d+(\.\d+)+"); | ||
| string versionText = pattern.Match(OS).Value; | ||
| Version windowsPlatformversion = new Version(versionText); |
There was a problem hiding this comment.
Can we trust OSDescription? Maybe add Assert at least?
There was a problem hiding this comment.
I believe we can for all the versions of Windows this will run on. What assert did you have in mind?
There was a problem hiding this comment.
Diagnostics.Assert(versionText != null, "...")
There was a problem hiding this comment.
Hmm it would probably be better to wrap it in a try/catch because if the system was detected as Windows, then returning Windows NT without the version is better than nothing at all.
iSazonov
changed the title
3 participants
fixes #5242
On Windows 7
RuntimeInformation.OSDescriptionproducesMicrosoft Windows 6.1.7601 S. This resulted inSattempting to be evaluated as aVersionand aSystem.ArgumentException: Version string portion was too short or too long.exception.This PR switches to a regex capture of the version from the
OSDescriptionString.I have verfied this is working on windows 10 and Windows 7