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

Handle redirects in the pipeline #22876

Merged
merged 5 commits into from
Jul 26, 2021
Merged

Conversation

pakrym
Copy link
Contributor

@pakrym pakrym commented Jul 26, 2021

Fixes: #22746

@pakrym pakrym requested a review from KrzysztofCwalina as a code owner July 26, 2021 17:23
@ghost ghost added the Azure.Core label Jul 26, 2021
Comment on lines -156 to +164
return new SocketsHttpHandler();
return new SocketsHttpHandler()
{
AllowAutoRedirect = false
};
#else
return new HttpClientHandler();
return new HttpClientHandler()
{
AllowAutoRedirect = false
};
Copy link
Member

Choose a reason for hiding this comment

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

nit:

#if NETCOREAPP
return new SocketsHttpHandler()
#else
return new HttpClientHandler();
#endif
{
    AllowAutoRedirect = false
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's evil :)

// Clear the authorization header.
request.Headers.Remove(HttpHeader.Names.Authorization);

if (AzureCoreEventSource.Singleton.IsEnabled())
Copy link
Member

Choose a reason for hiding this comment

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

Just curious why we don't do these checks in the eventSource instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To avoid converting Uris to strings if the ES is disabled.

@chrwhit
Copy link
Member

chrwhit commented Jul 26, 2021

After this update, I assume I should remove my changes in Communications, correct?

@pakrym
Copy link
Contributor Author

pakrym commented Jul 26, 2021

After this update, I assume I should remove my changes in Communications, correct?

Correct. You would also need to add a ProjectReference to Azure.Core (to get the new feature) and flip it back to PackageReference when Azure.Core ships with the fix.

@chrwhit
Copy link
Member

chrwhit commented Jul 26, 2021

After this update, I assume I should remove my changes in Communications, correct?

Correct. You would also need to add a ProjectReference to Azure.Core (to get the new feature) and flip it back to PackageReference when Azure.Core ships with the fix.

Can we just wait for the package?

@pakrym
Copy link
Contributor Author

pakrym commented Jul 26, 2021

Can we just wait for the package?

That works too!

Co-authored-by: Christopher Scott <chriscott@hotmail.com>
@pakrym pakrym merged commit 1b5f42b into Azure:main Jul 26, 2021
@pakrym pakrym deleted the pakrym/Add-RedirectPolicy branch July 26, 2021 20:06
response.Dispose();

// Clear the authorization header.
request.Headers.Remove(HttpHeader.Names.Authorization);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

dotnet/runtime#26475 (comment)

TLDR to prevent accidental secret leaks.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.

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.

Handle redirects as part of the pipeline
4 participants