Skip to content
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.

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

Native iOS/macOS HttpClientHandler incorrectly encodes query strings #20629

Closed
abnegate opened this issue May 22, 2024 · 3 comments · Fixed by #20633
Closed

Native iOS/macOS HttpClientHandler incorrectly encodes query strings #20629

abnegate opened this issue May 22, 2024 · 3 comments · Fixed by #20633
Labels
bug If an issue is a bug or a pull request a bug fix networking If an issue or pull request is related to networking
Milestone

Comments

@abnegate
Copy link

Steps to Reproduce

  1. Instantiate HttpClient
  2. Instantiate HttpRequestMessage for a GET request, with a request URI including a query string with special characters
  3. Send the request using the client with the SendAsync method
  4. Observe that during the SendAsync call, the query string is encoded incorrectly
  5. Add <UseNativeHandler>false</UseNativeHandler> to project file
  6. Observe that query string is not encoded incorrectly

Expected Behavior

Query string is encoded correctly

Actual Behavior

Query string is encoded incorrectly

Environment

Version information
.NET SDK:
 Version:           8.0.300
 Commit:            326f6e68b2
 Workload version:  8.0.300-manifests.4c099cbd
 MSBuild version:   17.10.4+10fbfbf2e

Runtime Environment:
 OS Name:     Mac OS X
 OS Version:  14.4
 OS Platform: Darwin
 RID:         osx-arm64
 Base Path:   /usr/local/share/dotnet/sdk/8.0.300/

.NET workloads installed:
 [maui]
   Installation Source: SDK 8.0.300
   Manifest Version:    8.0.21/8.0.100
   Manifest Path:       /usr/local/share/dotnet/sdk-manifests/8.0.100/microsoft.net.sdk.maui/8.0.21/WorkloadManifest.json
   Install Type:        FileBased


Host:
  Version:      8.0.5
  Architecture: arm64
  Commit:       087e15321b

.NET SDKs installed:
  7.0.301 [/usr/local/share/dotnet/sdk]
  6.0.400 [/usr/local/share/dotnet/sdk]
  6.0.408 [/usr/local/share/dotnet/sdk]
  6.0.412 [/usr/local/share/dotnet/sdk]
  6.0.413 [/usr/local/share/dotnet/sdk]
  8.0.306 [/usr/local/share/dotnet/sdk]
  7.0.307 [/usr/local/share/dotnet/sdk]
  9.0.201 [/usr/local/share/dotnet/sdk]
  8.0.300 [/usr/local/share/dotnet/sdk]

.NET runtimes installed:
  Microsoft.AspNetCore.App 6.0.6 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.8 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.16 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.20 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 6.0.21 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.9 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 7.0.10 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.2 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.AspNetCore.App 8.0.5 [/usr/local/share/dotnet/shared/Microsoft.AspNetCore.App]
  Microsoft.NETCore.App 6.0.8 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.16 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.20 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 6.0.21 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.9 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 7.0.10 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.2 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]
  Microsoft.NETCore.App 8.0.5 [/usr/local/share/dotnet/shared/Microsoft.NETCore.App]

Other architectures found:
  x64   [/usr/local/share/dotnet/x64]
    registered at [/etc/dotnet/install_location_x64]

Environment variables:
  Not set

global.json file:
  Not found

Learn more:
  https://aka.ms/dotnet/info

Download .NET:
  https://aka.ms/dotnet/download

Build Logs

msbuild.binlog.zip

Example Project (If Possible)

MauiApp1.zip

Before request sent
1
After request sent
2

@rolfbjarne
Copy link
Member

I can reproduce the behavior.

The HttpResponse.RequestUri is changed/updated here:

httpResponse.RequestMessage.RequestUri = absoluteUri;

this code is rather old, but apparently RequestUri is updated to reflect any redirects:

xamarin/ModernHttpClient@518ac1b

@rolfbjarne
Copy link
Member

Updating RequestUri after a redirect seems correct, a console app does that.

Test code:

async static Task Main ()
{
    var client = new HttpClient () { BaseAddress = new Uri("https://httpbin.org") };
    var request = new HttpRequestMessage (HttpMethod.Get,
        "/redirect-to?url=https%3A%2F%2Fmicrosoft.com&status_code=302&queries[]={}"
    );
    Console.WriteLine ($"Request uri 1: {request.RequestUri}");
    var response = await client.SendAsync(request);
    Console.WriteLine ($"Request uri 2: {request.RequestUri}");
    var content = await response.Content.ReadAsStringAsync();
    Console.WriteLine ((int) response.StatusCode);
    Console.WriteLine(content.Length);
}

prints:

Request uri 1: /redirect-to?url=https%3A%2F%2Fmicrosoft.com&status_code=302&queries[]={}
Request uri 2: https://www.microsoft.com/es-es/
200
201252

@rolfbjarne
Copy link
Member

@rolfbjarne rolfbjarne added bug If an issue is a bug or a pull request a bug fix networking If an issue or pull request is related to networking labels May 22, 2024
@rolfbjarne rolfbjarne added this to the Future milestone May 22, 2024
rolfbjarne added a commit to rolfbjarne/xamarin-macios that referenced this issue May 22, 2024
…ection occurred. Fixes xamarin#20629.

The logic to update the request's RequestUri is somewhat old:

xamarin/ModernHttpClient@518ac1b

but it makes sense to do so.

Some testing with a console app, also revealed that a plain .NET does this in
case of a redirect:

```cs
async static Task Main ()
{
    var client = new HttpClient () { BaseAddress = new Uri("https://httpbin.org") };
    var request = new HttpRequestMessage (HttpMethod.Get,
        "/redirect-to?url=https%3A%2F%2Fmicrosoft.com&status_code=302&queries[]={}"
    );
    Console.WriteLine ($"Request uri 1: {request.RequestUri}");
    var response = await client.SendAsync(request);
    Console.WriteLine ($"Request uri 2: {request.RequestUri}");
    var content = await response.Content.ReadAsStringAsync();
    Console.WriteLine ((int) response.StatusCode);
    Console.WriteLine(content.Length);
}
```

prints:

```
Request uri 1: /redirect-to?url=https%3A%2F%2Fmicrosoft.com&status_code=302&queries[]={}
Request uri 2: https://www.microsoft.com/es-es/
200
201252
```

Further looking into .NET's code, `SocketsHttpHandler` updates RequestUri after
following a redirect:

https://github.com/dotnet/runtime/blob/f5eb26e4da0d0d7a5874553d8a793a551c34af0a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs#L65-L66

So it seems the expected behavior is to update RequestUri only in case of a
redirect.

Now the question becomes: how to detect whether we're a redirect or not?
Contrary to `SocketsHttpHandler`, we don't do the actual redirect, it's
handled transparently by `NSUrlSession`.

Fortunately `NSUrlSessionTask` has two properties which help:
[`OriginalRequest`][1] and [`CurrentRequest`][2]. According to Apple's
documentation, these are the same, "except when the server has responded to
the initial request with a redirect to a different URL."

This sounds perfect for us, so use these two properties to determine whether a redirect occurred.

Note that we can't use object identity, because these are 'copy' properties,
which means they'll never be the same instances, only at most a copy of
eachother. So instead compare the `AbsoluteString` property to check for
equality. As far as I'm aware, none of the other properties (request body,
headers, cookies, etc.) can be set by the server in a redirect, so there's no
need to compare those.

Fixes xamarin#20629.

[1]: https://developer.apple.com/documentation/foundation/nsurlsessiontask/1411572-originalrequest
[2]: https://developer.apple.com/documentation/foundation/nsurlsessiontask/1411649-currentrequest
rolfbjarne added a commit that referenced this issue May 28, 2024
…ection occurred. Fixes #20629. (#20633)

The logic to update the request's RequestUri is somewhat old:

xamarin/ModernHttpClient@518ac1b

but it makes sense to do so.

Some testing with a console app, also revealed that a plain .NET does this in
case of a redirect:

```cs
async static Task Main ()
{
    var client = new HttpClient () { BaseAddress = new Uri("https://httpbin.org") };
    var request = new HttpRequestMessage (HttpMethod.Get,
        "/redirect-to?url=https%3A%2F%2Fmicrosoft.com&status_code=302&queries[]={}"
    );
    Console.WriteLine ($"Request uri 1: {request.RequestUri}");
    var response = await client.SendAsync(request);
    Console.WriteLine ($"Request uri 2: {request.RequestUri}");
    var content = await response.Content.ReadAsStringAsync();
    Console.WriteLine ((int) response.StatusCode);
    Console.WriteLine(content.Length);
}
```

prints:

```
Request uri 1: /redirect-to?url=https%3A%2F%2Fmicrosoft.com&status_code=302&queries[]={}
Request uri 2: https://www.microsoft.com/es-es/
200
201252
```

Further looking into .NET's code, `SocketsHttpHandler` updates RequestUri after
following a redirect:

https://github.com/dotnet/runtime/blob/f5eb26e4da0d0d7a5874553d8a793a551c34af0a/src/libraries/System.Net.Http/src/System/Net/Http/SocketsHttpHandler/RedirectHandler.cs#L65-L66

So it seems the expected behavior is to update RequestUri only in case of a
redirect.

Now the question becomes: how to detect whether we're a redirect or not?
Contrary to `SocketsHttpHandler`, we don't do the actual redirect, it's
handled transparently by `NSUrlSession`.

Fortunately `NSUrlSessionTask` has two properties which help:
[`OriginalRequest`][1] and [`CurrentRequest`][2]. According to Apple's
documentation, these are the same, "except when the server has responded to
the initial request with a redirect to a different URL."

This sounds perfect for us, so use these two properties to determine whether a redirect occurred.

Note that we can't use object identity, because these are 'copy' properties,
which means they'll never be the same instances, only at most a copy of
eachother. So instead compare the `AbsoluteString` property to check for
equality. As far as I'm aware, none of the other properties (request body,
headers, cookies, etc.) can be set by the server in a redirect, so there's no
need to compare those.

Fixes #20629.

[1]: https://developer.apple.com/documentation/foundation/nsurlsessiontask/1411572-originalrequest
[2]: https://developer.apple.com/documentation/foundation/nsurlsessiontask/1411649-currentrequest
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug If an issue is a bug or a pull request a bug fix networking If an issue or pull request is related to networking
Projects
None yet
2 participants