[NSUrlSessionHandler] Only update the request's RequestUri if a redirection occurred. Fixes #20629. #20633

rolfbjarne · 2024-05-22T16:35:46Z

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:

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 and CurrentRequest. 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.

…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

…pdate-requesturi-if-redirected

…irected

…pdate-requesturi-if-redirected

vs-mobiletools-engineering-service2 · 2024-05-28T08:21:44Z

📚 [PR Build] Artifacts 📚

Packages generated

View packages

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-28T08:26:06Z

💻 [PR Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

✅ All tests on macOS X64 - Mac Sonoma (14) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-28T08:31:28Z

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13) passed 💻

✅ All tests on macOS M1 - Mac Ventura (13) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-28T08:34:43Z

💻 [PR Build] Tests on macOS M1 - Mac Monterey (12) passed 💻

✅ All tests on macOS M1 - Mac Monterey (12) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-28T08:41:22Z

💻 [CI Build] Windows Integration Tests passed 💻

✅ All Windows Integration Tests passed.

Pipeline on Agent
Hash: bb516ff90751863a490d5914a7224f2bc7bfb837 [PR build]

vs-mobiletools-engineering-service2 · 2024-05-28T08:50:28Z

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11) passed 💻

✅ All tests on macOS M1 - Mac Big Sur (11) passed.

Pipeline on Agent
Hash: [PR build]

vs-mobiletools-engineering-service2 · 2024-05-28T09:00:13Z

✅ API diff for current PR / commit

Legacy Xamarin (No breaking changes)

~~iOS~~ (no change detected)
~~tvOS~~ (no change detected)
~~watchOS~~ (no change detected)
~~macOS~~ (no change detected)

NET (empty diffs)

~~iOS~~: (empty diff detected)
~~tvOS~~: (empty diff detected)
~~MacCatalyst~~: (empty diff detected)
~~macOS~~: (empty diff detected)

✅ API diff vs stable

Legacy Xamarin (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
watchOS: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)

.NET (No breaking changes)

iOS: vsdrops gist (No breaking changes)
tvOS: vsdrops gist (No breaking changes)
MacCatalyst: vsdrops gist (No breaking changes)
macOS: vsdrops gist (No breaking changes)
Microsoft.iOS vs Microsoft.MacCatalyst: vsdrops gist

Legacy Xamarin (stable) vs .NET

iOS: vsdrops gist
tvOS: vsdrops gist
macOS: vsdrops gist

ℹ️ Generator diff

Generator Diff: vsdrops (html) vsdrops (raw diff) gist (raw diff) - Please review changes)

Pipeline on Agent
Hash: bb516ff90751863a490d5914a7224f2bc7bfb837 [PR build]

vs-mobiletools-engineering-service2 · 2024-05-28T14:05:38Z

🚀 [CI Build] Test results 🚀

Test results

✅ All tests passed on VSTS: test results.

🎉 All 170 tests passed 🎉

Tests counts

✅ cecil: All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (iOS): All 1 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ dotnettests (MacCatalyst): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (macOS): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (Multiple platforms): All 1 tests passed. Html Report (VSDrops) Download
✅ dotnettests (tvOS): All 1 tests passed. Html Report (VSDrops) Download
✅ framework: All 8 tests passed. Html Report (VSDrops) Download
✅ fsharp: All 7 tests passed. Html Report (VSDrops) Download
✅ generator: All 2 tests passed. Html Report (VSDrops) Download
✅ install-source: All 1 tests passed. Html Report (VSDrops) Download
✅ interdependent-binding-projects: All 7 tests passed. Html Report (VSDrops) Download
✅ introspection: All 8 tests passed. Html Report (VSDrops) Download
✅ linker: All 65 tests passed. Html Report (VSDrops) Download
✅ mac-binding-project: All 1 tests passed. Html Report (VSDrops) Download
✅ mmp: All 2 tests passed. Html Report (VSDrops) Download
✅ mononative: All 6 tests passed. Html Report (VSDrops) Download
✅ monotouch (iOS): All 11 tests passed. Html Report (VSDrops) Download
✅ monotouch (MacCatalyst): All 7 tests passed. Html Report (VSDrops) Download
✅ monotouch (macOS): All 8 tests passed. Html Report (VSDrops) Download
✅ monotouch (tvOS): All 11 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ monotouch (watchOS): All 4 tests passed. [attempt 2] Html Report (VSDrops) Download
✅ msbuild: All 2 tests passed. Html Report (VSDrops) Download
✅ mtouch: All 1 tests passed. Html Report (VSDrops) Download
✅ xammac: All 3 tests passed. Html Report (VSDrops) Download
✅ xcframework: All 8 tests passed. Html Report (VSDrops) Download
✅ xtro: All 2 tests passed. Html Report (VSDrops) Download

Pipeline on Agent
Hash: [PR build]

rolfbjarne requested a review from mandel-macaque as a code owner May 22, 2024 16:35

This comment has been minimized.

Sign in to view

dalexsoto approved these changes May 23, 2024

View reviewed changes

This comment has been minimized.

Sign in to view

rolfbjarne added 2 commits May 23, 2024 08:48

Make tests .NET only.

f3f0436

Merge remote-tracking branch 'origin/main' into nsurlsessionhandler-u…

351f416

…pdate-requesturi-if-redirected

This comment has been minimized.

Sign in to view

Merge branch 'main' into nsurlsessionhandler-update-requesturi-if-red…

f46bda8

…irected

This comment has been minimized.

Sign in to view

haritha-mohan approved these changes May 25, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into nsurlsessionhandler-u…

bb516ff

…pdate-requesturi-if-redirected

This comment has been minimized.

Sign in to view

rolfbjarne merged commit 59e84a5 into xamarin:main May 28, 2024
64 checks passed

rolfbjarne deleted the nsurlsessionhandler-update-requesturi-if-redirected branch May 28, 2024 15:36

[NSUrlSessionHandler] Only update the request's RequestUri if a redirection occurred. Fixes #20629. #20633

[NSUrlSessionHandler] Only update the request's RequestUri if a redirection occurred. Fixes #20629. #20633

Conversation

rolfbjarne commented May 22, 2024

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vs-mobiletools-engineering-service2 commented May 28, 2024

📚 [PR Build] Artifacts 📚

Packages generated

vs-mobiletools-engineering-service2 commented May 28, 2024

💻 [PR Build] Tests on macOS X64 - Mac Sonoma (14) passed 💻

vs-mobiletools-engineering-service2 commented May 28, 2024

💻 [PR Build] Tests on macOS M1 - Mac Ventura (13) passed 💻

vs-mobiletools-engineering-service2 commented May 28, 2024

💻 [PR Build] Tests on macOS M1 - Mac Monterey (12) passed 💻

vs-mobiletools-engineering-service2 commented May 28, 2024

💻 [CI Build] Windows Integration Tests passed 💻

vs-mobiletools-engineering-service2 commented May 28, 2024

💻 [PR Build] Tests on macOS M1 - Mac Big Sur (11) passed 💻

vs-mobiletools-engineering-service2 commented May 28, 2024

✅ API diff for current PR / commit

✅ API diff vs stable

ℹ️ Generator diff

This comment has been minimized.

vs-mobiletools-engineering-service2 commented May 28, 2024

🚀 [CI Build] Test results 🚀

Test results

Tests counts