GeezerVison

Working with dotnetcore team to figure out crossgen issue. Temporarily turned start-crossgen to a no-op so we can see the test results.

A bunch of webcmdlet tests are failing because the User-Agent string has a regression in dotnetcore 2.1-preview2. Looks like they already fixed it though.

Locally I tried use the older HttpClientHandler adding

AppContext.SetSwitch("System.Net.Http.UseSocketsHttpHandler", false);

and other web cmdlet tests failed. It seems better not to use the former HttpClientHandler and investigate current test failures. Mark Kraus (@markekraus) Could you please help?

We could set pending for User-Agent tests until we move to .Net Core 2.1 RC.

I see that another test completes abnormally.
They added new overlaps:

• before 2.1

$tableOutput.Contains

OverloadDefinitions
-------------------
bool Contains(string value)

• in 2.1

$tableOutput.Contains

OverloadDefinitions
-------------------
bool Contains(string value)
bool Contains(string value, System.StringComparison comparisonType)
bool Contains(char value)
bool Contains(char value, System.StringComparison comparisonType)

I'm fixing the test, but it seems that it can cause problems in scripts and not only in this case because other APIs can also be extended.

Ilya (@iSazonov) Steve Lee (@SteveL-MSFT)

I looked into the authorization issues. It looks like there is a change in HttpClient's behavior between 2.0 and 2.1 WRT authorization headers on redirect.

using this code in 6.0.2 and a build from Ilya (@iSazonov) branch:

$Handler = [System.Net.Http.HttpClientHandler]::new()
$client = [System.Net.Http.HttpClient]::new($Handler)
$req = [System.Net.Http.HttpRequestMessage]::new()
$req.Headers.Add('Authorization','test')
$req.RequestUri = 'https://httpbin.org/redirect-to?url=https://httpbin.org/get'
$res = $client.SendAsync($req).GetAwaiter().GetResult()
$res.Content.ReadAsStringAsync().GetAwaiter().GetResult()

in 6.0.2 (using dotnet 2.0):

{
  "args": {},
  "headers": {
    "Authorization": "test",
    "Connection": "close",
    "Host": "httpbin.org"
  },
  "origin": "173.239.232.139",
  "url": "https://httpbin.org/get"
}

in the build of this PR (dotnet 2.1):

{
  "args": {},
  "headers": {
    "Connection": "close",
    "Host": "httpbin.org"
  },
  "origin": "173.239.232.139",
  "url": "https://httpbin.org/get"
}

So it appears HttpClient no longer sends Authorization headers on redirect.

We can handle this in the web cmdlets with some change in logic. But I'd like to find out if this is a regression or by design breaking change in CoreFX. I might not have any free time until this weekend. if one of you could do a quick look or talk with the CoreFX folks that would be great. if not, i will look into deeper this weekend.

Mark Kraus (@markekraus) Many thanks! Could you please look https://stackoverflow.com/questions/28564961/authorization-header-is-lost-on-redirect ? My understanding is that a http client remove authorization headers by default because of security.

Ilya (@iSazonov) Yes. In .NET Framework (which is what that link is about) HttpClient did strip authorization by default. You can see this for yourself in Windows PowerShell 5.1 with the same code after running Add-Type -AssemblyName 'System.Net.Http'

In .NET Core 1.0-2.0, however, Authorization was not stripped by default. Code was added to the cmdlets to handle that. By default, we intercept the response if the authorization header is present. The pwsh web cmdlets do that by default and it can be overridden with -PreserveAuthorizationOnRedirect. This is a new feature we added to Core and it is something that we will need to continue to support as there were even recent issues opened about APIs requiring authorization headers on redirect.

If the change in 2.1 is by design we need to switch our logic around and instead intercept the redirect when the user wants to preserve the auth header.

Ilya (@iSazonov) Steve Lee (@SteveL-MSFT) ugh.. the change in the String.Contains() method in 2.1 is going to cause some pain in PowerShell.

https://docs.microsoft.com/en-us/dotnet/api/system.string.contains?view=netcore-2.1
vs
https://docs.microsoft.com/en-us/dotnet/api/system.string.contains?view=netcore-2.0

I hope there is something we can point users to in our release notes for 6.1 to changes in 2.1 APIs.

Clear Authorization Headers on Redirect dotnet/corefx#26864

Ilya (@iSazonov) thanks for finding that. It seems clear this is an intentional change so we will need to modify or logic.

The good news is that the current way was fragile (we fixed 2 issues with it since 6.0.0) and since it was default it ran more often. Now we can rely on .NET and the fragile code will run less often.

created #6728 to track the authorization logic changes needed.

My thought is that we shouldn't move to dotnetcore until PSCore6+2.1 is as good or better than current PSCore6 build. So I would wait until the fixes are available to pick up in this PR than get into a state where some things work and some things don't and we accidentally miss something on release of 6.1. I would be fine moving ahead to something newer than 2.1-Preview2 since it appears dotnetcore is moving towards RC and the window of opportunity to get them to fix issues in 2.1 before they declare GA is getting smaller. For this reason, I prefer not to mark tests as Pending specifically because of 2.1.

We also need to update the versions of the dotnet NuGet packages referred by PackageReference in our .csproj files. For example, Microsoft.Win32.Registry.AccessControl in system.management.automation.csproj should be updated to 4.5.0-rc1-26423-06.

Note that, we should pick the rc1-26423 ones, which are from the same build of the netcoreapp we are using with the new sdk (AssemblyFileVersion("4.6.26423.06")).

LGTM.

• Seems we don't open a tracking issue for web cmdlet proxy tests.
• Need review and make cleanups our workarounds for curl.
• Need review packaging. Perhaps in the PR (We've already missed the Preview2 time).

Ilya (@iSazonov) I'm not familiar with the issue regarding web cmdlet proxy tests and workaround for curl. You probably have more context, so can you please open a tracking issue for that?

As to packaging review, do you have any specific concerns about how this change would affect packaging?
Both the [Feature] and [Package] CI runs passed. We can kick off a release build after merging this PR to test the release build pipeline. /cc Travis Plunk (@TravisEz13), feel free to weigh in.

Steve Lee (@SteveL-MSFT), Aditya Patwardhan (@adityapatwardhan) and Mark Kraus (@markekraus), since new commits were pushed after you signed off, could you please take another look at this PR? Thanks!

Dongbo Wang (@daxian-dbw) I agree with starting a release build after this is merged.

About the Visual Studio build, I don't know if it will still work after this merge ... Travis Plunk (@TravisEz13) Does VS2017 allow configuring which dotnet sdk to use?

For Visual Studio you cannot specify the SDK.

This has been a long thread, Ilya (@iSazonov) can you summarize the proxy issue with the webcmdlet tests? I changed the tests as they were originally just wrong and incomplete. I think we should probably remove the tests for http_proxy env var which we skip on Windows because that is a side effect of corefx taking dependency on libcurl for non-Windows and not something supported explicitly by corefx nor the webcmdlets.

Currently I agree to remove this proxy tests. But I'll look this in depth only tomorrow. If I'll discover something important I'll open new tracking Issue.

The PR LGTM.

Ilya (@iSazonov) just to be clear, only the proxy tests for the http_proxy and https_proxy env vars. The ones using -proxy should stay and currently work. Created #6804

PR merged. Thanks everyone for getting this done!

Thanks everyone for this work!

Maybe it's a good experience of working together.