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

Jump to bottom

Add ignored http headers to SpaProxy #16863

Merged
merged 2 commits into from Nov 11, 2019
Merged

Add ignored http headers to SpaProxy #16863

merged 2 commits into from Nov 11, 2019

Conversation

ChristopherHaws
Copy link
Contributor

@ChristopherHaws ChristopherHaws commented Nov 6, 2019 •
edited

Ignore Connection header when using UseProxyToSpaDevelopmentServer

Addresses #16797

@Tratcher Tratcher removed their request for review November 6, 2019 05:26
@javiercn javiercn added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 6, 2019
javiercn
javiercn reviewed Nov 6, 2019
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

The original issue only mentions the Connection header, why are we also ignoring cookies?

@mkArtakMSFT
Copy link
Member

Thanks for your PR, @ChristopherHaws.
Cookie headers can be important in some cases, and we don't feel that it's the right thing to do to always remove these. For the Connection headers, we like your proposal. Can you tweak your PR to remove only the Connection headers?
As for the Cookie header, you can have a custom middleware registered in the middleware pipeline, which can remove the Cookie header for your specific application.

@analogrelay analogrelay removed their request for review November 6, 2019 17:30
@ChristopherHaws
Copy link
Contributor Author

I have removed the cookie header from the NotForwardedHttpHeaders.

javiercn
javiercn approved these changes Nov 7, 2019
View reviewed changes
Copy link
Member

@javiercn javiercn left a comment

Choose a reason for hiding this comment

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

Changes look good. Seems like the build got stuck somewhere, re-triggered to see what happens. Will merge once it passes.

@mkArtakMSFT mkArtakMSFT added this to the 5.0.0-preview1 milestone Nov 11, 2019
@mkArtakMSFT
Copy link
Member

Thanks for your contribution, @ChristopherHaws.

@mkArtakMSFT mkArtakMSFT reopened this Nov 11, 2019
@mkArtakMSFT mkArtakMSFT merged commit 4eebc16 into dotnet:master Nov 11, 2019
This was referenced May 14, 2021
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiercn javiercn javiercn approved these changes

Assignees

@javiercn javiercn

Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Milestone
5.0.0-preview1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ChristopherHaws @mkArtakMSFT @javiercn