Skip to content

Conversation

@grahamlyus
Copy link

@grahamlyus grahamlyus commented Jan 9, 2022

Description

Fixes #9310

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

Why

AllowAutoRedirect is true by default, and this causes network requests to fail without much information other than "Unhandled exception during request" . Using the debugger, I found this was due to 0x80072F88 - The HTTP redirect request must be confirmed by the user.

react-native on iOS/Android/macOS does not automatically follow redirects.
Resolves #9310

What

Uses HttpBaseProtocolFilter to disable auto redirects.

Microsoft Reviewers: Open in CodeFlow

@grahamlyus grahamlyus requested a review from a team as a code owner January 9, 2022 21:11
@ghost ghost added the Area: Networking label Jan 9, 2022
@NickGerleman
Copy link
Contributor

@JunielKatarn do you have insight into the correct behavior here?

@JunielKatarn
Copy link
Contributor

@JunielKatarn do you have insight into the correct behavior here?

Based on react-native's sources, neither iOS, Android or Office's closed-source HTTP module seem to explicitly declare allowing/disabling redirects.

@grahamlyus if your statement in #9310 holds true, this change seems correct in principle.
However, I'm concerned if certain applications would require redirect on their particular HTTP endpoints and this change could potentially break their scenarios.

Ideally, this change should:

  1. Enable redirects to be configurable.
  2. Add a test validating this (probably non-trivial).

@grahamlyus
Copy link
Author

@JunielKatarn as I mentioned in #9310, the auto redirect isn't even handled as it's expecting the user to confirm the redirect somehow - error: 0x80072F88 - The HTTP redirect request must be confirmed by the user.

I'm not sure how that could fixed or how to make the AllowAutoRedirect configurable, I'm afraid this is the first Windows code I've seen in 15 years! :)

@chrisglein
Copy link
Member

@JunielKatarn Can you take a look fresh look at this PR? Is it possible help bridge those last requirements on the bug fix (configuration and test coverage)?

@JunielKatarn
Copy link
Contributor

JunielKatarn commented Jan 19, 2023

Addressed by #10534.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

AllowAutoRedirect should be false

4 participants