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.

Sign up for GitHub

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

[browser][http] Add support for AllowAutoRedirect. #41394

Merged
merged 11 commits into from
Sep 10, 2020
+40 −4
Merged

[browser][http] Add support for AllowAutoRedirect. #41394

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
74fa272
[browser][http] Add support for AllowAutoRedirect.
kjpou1 Aug 26, 2020
edbef4d
Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…
kjpou1 Aug 27, 2020
aa48db1
Update src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpH…
kjpou1 Aug 27, 2020
97d72af
Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…
kjpou1 Aug 28, 2020
70a5f25
Address review comments and add flag to determine if the _allowAutoRe…
kjpou1 Aug 28, 2020
5e9423d
Remove Disposed and Handler checks to address review comments
kjpou1 Aug 28, 2020
677ea00
Update src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpH…
kjpou1 Aug 28, 2020
e3e3a3e
MaxAutomaticRedirections moved back to PlatformNotSupportedException.
kjpou1 Aug 28, 2020
424ca12
Merge branch 'wasm-http-redirect' of https://github.com/kjpou1/runtim…
kjpou1 Aug 28, 2020
b877f64
Address review comments about wrong order.
kjpou1 Aug 28, 2020
21d0489
Merge branch 'master' of https://github.com/dotnet/runtime into wasm-…
kjpou1 Sep 7, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions src/libraries/System.Net.Http/src/System.Net.Http.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -676,6 +676,8 @@
<Compile Include="System\Net\Http\BrowserHttpHandler\SystemProxyInfo.Browser.cs" />
<Compile Include="System\Net\Http\BrowserHttpHandler\SocketsHttpHandler.cs" />
<Compile Include="System\Net\Http\BrowserHttpHandler\BrowserHttpHandler.cs" />
<Compile Include="$(CommonPath)System\Net\Http\HttpHandlerDefaults.cs"
Link="Common\System\Net\Http\HttpHandlerDefaults.cs" />
</ItemGroup>
<ItemGroup>
<ProjectReference Include="$(LibrariesProjectRoot)System.Security.Principal.Windows\src\System.Security.Principal.Windows.csproj" />
Expand Down
55 changes: 49 additions & 6 deletions src/libraries/System.Net.Http/src/System/Net/Http/BrowserHttpHandler/BrowserHttpHandler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,10 @@ internal sealed class BrowserHttpHandler : HttpMessageHandler

private static readonly HttpRequestOptionsKey<bool> EnableStreamingResponse = new HttpRequestOptionsKey<bool>("WebAssemblyEnableStreamingResponse");
private static readonly HttpRequestOptionsKey<IDictionary<string, object>> FetchOptions = new HttpRequestOptionsKey<IDictionary<string, object>>("WebAssemblyFetchOptions");
private bool _allowAutoRedirect = HttpHandlerDefaults.DefaultAutomaticRedirection;
// flag to determine if the _allowAutoRedirect was explicitly set or not.
private bool _isAllowAutoRedirectTouched;
private int _maxAutomaticRedirections = HttpHandlerDefaults.DefaultMaxAutomaticRedirections;

/// <summary>
/// Gets whether the current Browser supports streaming responses
Expand Down Expand Up @@ -95,14 +99,26 @@ public ICredentials? Credentials

public bool AllowAutoRedirect
{
get => throw new PlatformNotSupportedException();
set => throw new PlatformNotSupportedException();
get => _allowAutoRedirect;
set
{
_isAllowAutoRedirectTouched = true;
kjpou1 marked this conversation as resolved.
Show resolved Hide resolved
_allowAutoRedirect = value;
}
}

public int MaxAutomaticRedirections
{
get => throw new PlatformNotSupportedException();
set => throw new PlatformNotSupportedException();
get => _maxAutomaticRedirections;
kjpou1 marked this conversation as resolved.
Show resolved Hide resolved
set
{
if (value <= 0)
{
throw new ArgumentOutOfRangeException(nameof(value), value, SR.Format(SR.net_http_value_must_be_greater_than, 0));
}

_maxAutomaticRedirections = value;
}
}

public int MaxConnectionsPerServer
Expand All @@ -125,7 +141,7 @@ public SslClientAuthenticationOptions SslOptions

public bool SupportsAutomaticDecompression => false;
public bool SupportsProxy => false;
public bool SupportsRedirectConfiguration => false;
public bool SupportsRedirectConfiguration => true;

private Dictionary<string, object?>? _properties;
public IDictionary<string, object?> Properties => _properties ??= new Dictionary<string, object?>();
Expand All @@ -146,6 +162,22 @@ protected internal override async Task<HttpResponseMessage> SendAsync(HttpReques

requestObject.SetObjectProperty("method", request.Method.Method);

// Only set if property was specifically modified and is not default value
if (_isAllowAutoRedirectTouched)
kjpou1 marked this conversation as resolved.
Show resolved Hide resolved
{
// Allowing or Disallowing redirects.
// Here we will set redirect to `manual` instead of error if AllowAutoRedirect is
// false so there is no exception thrown
//
// https://developer.mozilla.org/en-US/docs/Web/API/Response/type
//
// other issues from whatwg/fetch:
//
// https://github.com/whatwg/fetch/issues/763
// https://github.com/whatwg/fetch/issues/601
requestObject.SetObjectProperty("redirect", AllowAutoRedirect ? "follow" : "manual");
}

// We need to check for body content
if (request.Content != null)
{
Expand Down Expand Up @@ -224,9 +256,20 @@ protected internal override async Task<HttpResponseMessage> SendAsync(HttpReques
JSObject t = (JSObject)await response.ConfigureAwait(continueOnCapturedContext: true);

var status = new WasmFetchResponse(t, abortController, abortCts, abortRegistration);

HttpResponseMessage httpResponse = new HttpResponseMessage((HttpStatusCode)status.Status);

// Here we will set the ReasonPhrase so that it can be evaluated later.
// We do not have a status code but this will signal some type of what happened
// after interrogating the status code for success or not i.e. IsSuccessStatusCode
//
// https://developer.mozilla.org/en-US/docs/Web/API/Response/type
// opaqueredirect: The fetch request was made with redirect: "manual".
// The Response's status is 0, headers are empty, body is null and trailer is empty.
if (status.ResponseType == "opaqueredirect")
{
httpResponse.SetReasonPhraseWithoutValidation(status.ResponseType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karelz who could review this part? Should we try harder to map the result to status codes or expose that differently to the developers?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello everyone. Is there any update on this?

/cc @marek-safar @karelz @ManickaP

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there is a line in the PR comments about mapping a result and the approach for this PR:

Instead of faking a 3XX status code the implementation uses the ReasonPhrase to return the fetch response type opaqueredirect.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @dotnet/ncl

Hrm this is an interesting one. It seems like there is not a perfect solution here -- fetch API seems poorly made. ¯\_(ツ)_/¯

Without setting a normal redirect status code, we break compat with any existing code handling redirects.

But, it seems wrong to fake a status code when we don't know what the actual status code was. If we were to fake a status code, a temporary redirect seems like the best choice -- permanent redirect could cause dangerous handling e.g. updating a URI in a database.

To add a third bad option, what do we think of this? It at least gives the user some flexibility here.

public class BrowserHttpHandler
{
    public const HttpStatusCode OpaqueRedirect = (HttpStatusCode)1000;

    // if false, return TemporaryRedirect. if true, return OpaqueRedirect.
    public bool ReturnOpaqueRedirect { get; set; } = false;
}

(note, using a status code like this may not be advisable as an on-by-default option, as you might have some code doing e.g. if(StatusCode >= 500))

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I am understanding the above wouldn't this change the Public API as well?

}

bool streamingEnabled = false;
if (StreamingSupported)
{
Expand Down