Use HTML meta charset attribute value, if present, when the Context-Type header does not specify it. #4338

dantraMSFT · 2017-07-25T00:06:47Z

The change enables checking for the charset meta attribute when the Content-Type header does not define the charset. When found, the text content is re-encoded to the target charset.

A new property has been added to BasicHtmlWebResponseObject and HtmlWebResponseObject to support verifying the encoding. Attempting to do so from the returned content is non-deterministic.
See MicrosoftDocs/PowerShell-Docs#1543 for example usage.

NOTE: Three tests are pending the resolution for issue #2867

SteveL-MSFT · 2017-07-25T00:44:14Z

...erShell.Commands.Utility/commands/utility/WebCmdlet/CoreCLR/HtmlWebResponseObject.CoreClr.cs

Need doc change as you're adding to public api

SteveL-MSFT · 2017-08-03T03:57:13Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/StreamHelper.cs

HTML5 doesn't require quotes around attribute values and single quotes are also legal

TravisEz13 · 2017-07-29T20:48:53Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/StreamHelper.cs

between meta and charset one or more spaces are allowed not only one.

I think a regular expression might be better here

JamesWTruher · 2017-08-03T19:50:43Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

I'm worried about platforms where these encodings don't exist (linux/mac), which will cause the tests to fail. Do additional steps need to be taken to register new encodings, or does that happen elsewhere?

dantraMSFT · 2017-08-08T17:18:10Z

Moving to regex for detecting charset. Will reopen when done.

…ype header does not specify it.

Add negative test to validate handling of invalid/unsupported encodings in StreamHelper.DecodeStream

Revise regex expression to be less restrictive of the charset attribute value and use a named group for the value.

Updated regex to handle http-equiv charset declarations Update tests to validate http-equiv charset declarations

Strip trailing whitespace.

dantraMSFT · 2017-08-09T22:19:06Z

@TravisEz13 and @JamesWTruher, please take a look again. I've completed the changes to use regex.

TravisEz13 · 2017-08-10T16:09:32Z

src/Microsoft.PowerShell.Commands.Utility/commands/utility/WebCmdlet/StreamHelper.cs

+            if (isDefaultEncoding) do
+            {
+                // check for a charset attribute on the meta element to override the default.
+                Match match = s_metaexp.Match(content);


In order to cache the regex, you need to use the static regex methods. See: https://msdn.microsoft.com/en-us/library/system.text.regularexpressions.regex(v=vs.110).aspx#static_vs_instance

@TravisEz13 - According to my reading of the page, I'm doing exactly what it says...

"To prevent recompilation, you should instantiate a single Regex object that is accessible to all code that requires it, as shown in the following rewritten example."

Since s_metaexp is static/readonly, there is only one compilation. That appears to be the point of the of the caching; avoiding recompilation. Am I missing something?

Under the Important note:

In the .NET Framework versions 1.0 and 1.1, all compiled regular expressions, whether they were used in instance or static method calls, were cached. Starting with the .NET Framework 2.0, only regular expressions used in static method calls are cached.`

Basically, they changed the behavior and only added a not.

My review of the corefx shows that constructing and reusing an instance (regex.Match) is equivalent to calling the static Match method. The constructor manages cache lookups/updates. The static Match method constructs a new instance and calls through the instance Match method.
The relevant static method is:
public static Match Match(string input, string pattern, RegexOptions options, TimeSpan matchTimeout)
{
return new Regex(pattern, options, matchTimeout, true).Match(input);
}

Sound good, thanks for looking into it.

PR is update

TravisEz13 · 2017-08-11T23:00:52Z

test/powershell/Modules/Microsoft.PowerShell.Utility/WebCmdlets.Tests.ps1

+    #region charset encoding tests
+
+    Context  "BasicHtmlWebResponseObject Encoding tests" {
+        It "Verifies Invoke-WebRequest detects charset meta value when the ContentType header does not define it." {


Do you need to add test cases for any of the issues that caused the change to regex?

I did add test cases covering variations of <meta charset...?> and , including various newline variations and unquoted attribute value.

dantraMSFT · 2017-08-16T17:09:14Z

@TravisEz13: would you merge this. Thanks.

SteveL-MSFT · 2017-08-28T21:31:32Z

@chuanjiao10 since this is already merged, @dantraMSFT will submit new PR to address your issue

dantraMSFT · 2017-08-28T23:02:42Z

@chuanjiao10, see #4692

dantraMSFT requested review from JamesWTruher and SteveL-MSFT July 25, 2017 00:06

msftclas added the cla-not-required label Jul 25, 2017

daxian-dbw assigned TravisEz13 Jul 28, 2017

SteveL-MSFT previously requested changes Aug 3, 2017

View reviewed changes

TravisEz13 added the Documentation Needed in this repo Documentation is needed in this repo label Aug 3, 2017

TravisEz13 requested changes Aug 3, 2017

View reviewed changes

JamesWTruher reviewed Aug 3, 2017

View reviewed changes

dantraMSFT closed this Aug 8, 2017

dantraMSFT mentioned this pull request Aug 9, 2017

Document new Encoding property on BasicHtmlWebResponseObject and HtmlWebResponseObject MicrosoftDocs/PowerShell-Docs#1543

Merged

1 task

dantraMSFT reopened this Aug 9, 2017

msftclas added the cla-not-required label Aug 9, 2017

dantraMSFT and others added 6 commits August 9, 2017 13:00

Use HTML meta charset attribute value, if present, when the Context-T…

2fa0f51

…ype header does not specify it.

Switch to regex for searching for meta/charset attribute

74d816c

Add negative test to validate handling of invalid/unsupported encodings in StreamHelper.DecodeStream

Update tests to use encodings that are available on Linux

e5cfbb1

[Feature]

d1a5b2b

Revise regex expression to be less restrictive of the charset attribute value and use a named group for the value.

[Feature]

4f4066b

Updated regex to handle http-equiv charset declarations Update tests to validate http-equiv charset declarations

[Heading]

09e7a46

Strip trailing whitespace.

TravisEz13 reviewed Aug 10, 2017

View reviewed changes

TravisEz13 approved these changes Aug 11, 2017

View reviewed changes

TravisEz13 merged commit 75a2949 into PowerShell:master Aug 16, 2017

dantraMSFT deleted the dantra/issue3267 branch August 16, 2017 23:27

joeyaiello mentioned this pull request Oct 15, 2018

Docs needed for 'Use HTML meta charset attribute value, if present, when the Context-Type header does not specify it.' MicrosoftDocs/PowerShell-Docs#3093

Closed

joeyaiello removed Documentation Needed in this repo Documentation is needed in this repo labels Oct 15, 2018

Use HTML meta charset attribute value, if present, when the Context-Type header does not specify it. #4338

Use HTML meta charset attribute value, if present, when the Context-Type header does not specify it. #4338

Uh oh!

Conversation

dantraMSFT commented Jul 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantraMSFT commented Aug 8, 2017

Uh oh!

dantraMSFT commented Aug 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TravisEz13 Aug 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dantraMSFT commented Aug 16, 2017

Uh oh!

SteveL-MSFT commented Aug 28, 2017

Uh oh!

dantraMSFT commented Aug 28, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dantraMSFT commented Jul 25, 2017 •

edited

Loading

TravisEz13 Aug 11, 2017 •

edited

Loading