Remove NotForwardedHttpHeaders in SpaServices.Extensions SpaProxy

[mattywong]

• You've read the Contributor Guide and Code of Conduct.
• You've included unit or integration tests for your change, where applicable.
• You've included inline docs for your change, where applicable.
• There's an open issue for the PR that you are making. If you'd like to propose a new feature or change, please open an issue to discuss the change or find an existing issue.

PR Title
Remove NotForwardedHttpHeaders in SpaProxy

PR Description
Removes previously added code in SpaServices.Extensions that removes the Connection header being proxied back to webpack-dev-server/sockjs when using UseProxyToSpaDevelopmentServer on Windows.

Addresses #bugnumber (in this specific format)
#29478

[Tratcher]

It looks like you used the wrong branch, please rebase on main.

Keeping the connection header isn't correct either. Only some values should be allowed in the connection header like Upgrade.

[Tratcher]

If this is a WebSocket request it shouldn't even hit the modified code path, it should be handled earlier here:

aspnetcore/src/Middleware/Spa/SpaServices.Extensions/src/Proxying/SpaProxy.cs

Lines 66 to 70 in cc45d1b

	if (context.WebSockets.IsWebSocketRequest)
	{
	await AcceptProxyWebSocketRequest(context, ToWebSocketScheme(targetUri), proxyCancellationToken);
	return true;
	}

[mattywong]

If this is a WebSocket request it shouldn't even hit the modified code path, it should be handled earlier here:

aspnetcore/src/Middleware/Spa/SpaServices.Extensions/src/Proxying/SpaProxy.cs

Lines 66 to 70 in cc45d1b

	if (context.WebSockets.IsWebSocketRequest)
	{
	await AcceptProxyWebSocketRequest(context, ToWebSocketScheme(targetUri), proxyCancellationToken);
	return true;
	}

I think this is a sockjs protocol problem. Connecting straight with web sockets works fine, but when using sockjs (default transport mode in webpack dev server, which angular cli uses) it makes 2 http requests first (which I think then get upgraded to the web socket connection?) . Using web sockets as the transport mode directly in webpack dev server circumvents this process and connects straight to wss/ws.

[Tratcher]

Can you share a fiddler/network trace of the two requests? I'm curious what's in their headers.

[mattywong]

Can you share a fiddler/network trace of the two requests? I'm curious what's in their headers.

GET https://localhost:44316/sockjs-node/info?t=1621012538295 HTTP/1.1
Host: localhost:44316
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
sec-ch-ua: " Not A;Brand";v="99", "Chromium";v="90", "Google Chrome";v="90"
sec-ch-ua-mobile: ?0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.212 Safari/537.36
Accept: */*
Sec-Fetch-Site: same-origin
Sec-Fetch-Mode: cors
Sec-Fetch-Dest: empty
Referer: https://localhost:44316/
Accept-Encoding: gzip, deflate, br
Accept-Language: en-GB,en-US;q=0.9,en;q=0.
Cookie: OMITTED

GET https://localhost:44316/sockjs-node/info?t=1621012538295 HTTP/1.1
Host: localhost:44316
Connection: keep-alive
Pragma: no-cache
Cache-Control: no-cache
sec-ch-ua: " Not A;Brand";v="99", "Chromium";v="90", "Google Chrome";v="90"
sec-ch-ua-mobile: ?0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/90.0.4430.212 Safari/537.36
Accept: */*
Sec-Fetch-Site: same-origin
Sec-Fetch-Mode: cors
Sec-Fetch-Dest: empty
Referer: https://localhost:44316/
Accept-Encoding: gzip, deflate, br
Accept-Language: en-GB,en-US;q=0.9,en;q=0.8
Cookie: OMITTED

[Tratcher]

Hmm, removing Connection: keep-alive shouldn't break it, that's a no-op on HTTP/1.1. Which request fails, 1, 2, or the subsequent WebSocket request?

[mattywong]

Hmm, removing Connection: keep-alive shouldn't break it, that's a no-op on HTTP/1.1. Which request fails, 1, 2, or the subsequent WebSocket request?

Subsequent WebSocket requests:

(left is with fixed dll, right is .NET 5)

I couldn't really test any more of the requests through Fiddler as it seems when Fiddler is turned on the WebSocket connection issue continues happening even with the fixed dll. You can see in Fiddler that it reverts to xhr-streaming.

[Tratcher]

And that's where it gets even more confusing, WebSockets shouldn't even use that modified code path. See #29478 (comment).

[mattywong]

And that's where it gets even more confusing, WebSockets shouldn't even use that modified code path. See #29478 (comment).

My hunch is that it's still in the http request proxy pipeline and that http request isn't getting 'Upgraded' to a web socket, which I can only guess Fiddler would pick up if it weren't for it to be blocked when trying to intercept the traffic (this is just my theory however, I am not that familiar with sockjs). Using native websockets as the transport mode in webpack-dev-server does not have the same issue mentioned and works just fine, so this seems specific to sockjs and the sockjs protocol.

The only certain facts that we know is that this was introduced in a change from .NET core 3.1 -> .NET 5. In the pr (#16863) where it looks like it was introduced, the lines of code seemed to be brought in as an optimization.

[javiercn]

@mattywong thanks for the contribution.

Unfortunately we don't think we want to take this PR in as it is. We are changing how the proxying works in 6.0 to leverage the CLI proxy from the underlying SPA framework (Angular, React, etc.) which is the approach we recommend going forward (and even for existing 5.0 projects). This will be available in preview4 in the near future.

[mattywong]

@javiercn what about for people that have custom webpack configurations and don't use create-react-app or Angular cli? It seems with every major change of .NET core so far there's always been deprecations/changes that affect SPA developers (like the SpaServices deprecation and npm packages...). I understand the concern for wanting to streamline a lot of these processes as I agree it's not simple, but I feel there has to be a better way then just saying sorry, use the new approach way and throwing devs in the dark here (i mean just look at the announcement and community dislikes on the SpaServices deprecation issue a couple years back #12890).

I think the UseProxyToSpaDevelopmentServer method is still a valid and simple solution to the problem, especially for those that don't use pre setup SPA frameworks, and I'd argue how it works in its current form is fine except for this sockjs issue in development, which did not exist in 3.1 (this issue has no effect in production as the SPA will be built in to static files majority of the time, and therefore there is no need to use UseProxyToSpaDevelopmentServer in production).

I have no idea how .NET 6 proxying will work, so I can't really comment on how it will integrate with such a vast combination of different projects leveraging completely different bundlers and setups in development.

I urge you to reconsider, and/or suggest a better solution to the current problem this pr is trying to resolve (that affects everyone using the official angular spa template, and pretty much anyone using default webpack-dev-server and UseProxyToSpaDevelopmentServer on Windows). As much as I support aspnetcore and the direction it may be going, I feel the trust is waning between each major version change.

[ghost]

Hi @mattywong. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[javiercn]

@mattywong the approach that we are taking works with any frontend you want. All of them use the webpack-dev-server or the http-proxy-middleware package directly.

what about for people that have custom webpack configurations and don't use create-react-app or Angular cli? It seems with every major change of .NET core so far there's always been deprecations/changes that affect SPA developers (like the SpaServices deprecation and npm packages...). I understand the concern for wanting to streamline a lot of these processes as I agree it's not simple, but I feel there has to be a better way then just saying sorry, use the new approach way and throwing devs in the dark here (i mean just look at the announcement and community dislikes on the SpaServices deprecation issue a couple years back #12890).

I understand your frustration and your perspective on the topic, however let me point out a few things that I think make an important difference compared to the changes we made a couple years back:

• We are not "removing" anything in 6.0 and we think all supported scenarios in 5.0 and 3.1 should still be supported by the new approach and will work better.
  • If you have an scenario that works with the "old" asp.net core proxy and that doesn't work with the new approach, we are interested to know.
• We are making this change because we believe we will improve/fix an entire class of issues introduced by our own non-standard proxying middleware.
  • An example of that is precisely this issue.
• We are providing migration instructions from the approach used in 5.0 and earlier to the one in 6.0

I think the UseProxyToSpaDevelopmentServer method is still a valid and simple solution to the problem, especially for those that don't use pre setup SPA frameworks, and I'd argue how it works in its current form is fine except for this sockjs issue in development, which did not exist in 3.1 (this issue has no effect in production as the SPA will be built in to static files majority of the time, and therefore there is no need to use UseProxyToSpaDevelopmentServer in production).

Just to clarify and re-iterate, you don't need to use a "pre-setup" framework with the work that we are doing in 6.0 in this area. There are many issues related to the existing SPA proxy that we are aiming to fix by the work we are doing.

I have no idea how .NET 6 proxying will work, so I can't really comment on how it will integrate with such a vast combination of different projects leveraging completely different bundlers and setups in development.

It will integrate in the same way all those setups integrate with all other backend frameworks out there, which is important because it makes sure:

• The approach we are following is standard across the JS community; any front-end framework and bundler provides a solution and documentation for this.
• When something doesn't work the way you expect, you can leverage the larger JavaScript community for solutions.
• Any changes in how proxying works are tied to the lifecycle of your front-end framework, that means for example that when you update to a new Angular version you don't run into issues with the proxy in ASP.NET Core and you'll have an easier time upgrading your JS in between .NET releases.

I will suggest you checkout the community standup talk we did a few weeks back here and give it a try once it comes out on preview4 to judge by yourself.

I urge you to reconsider, and/or suggest a better solution to the current problem this pr is trying to resolve (that affects everyone using the official angular spa template, and pretty much anyone using default webpack-dev-server and UseProxyToSpaDevelopmentServer on Windows). As much as I support aspnetcore and the direction it may be going, I feel the trust is waning between each major version change.

We think that the solution we are providing moving forward is the better choice here, including old projects from 5.0 and onwards. Instead of having to deal with a long tail of integration issues introduced by our own proxy, we think that switching to a standard approach (while maintaining a the same/similar developer experience) is the way to go, since it makes these class of issues completely disappear and leverages all the work done by the JS community in this space.

I would ask of you to give us the benefit of the doubt here, try the new approach once it comes out in a couple of weeks (you can apply it to a 5.0 project without issue) and evaluate then. If something is not supported with our new approach or doesn't work as well as in the old approach, we are interested to know and open to feedback.

We are hoping that the changes we are doing, even though are not cost free (you have to do a 1 time update to upgrade your project), are substantially beneficial for existing and new projects.

I understand that we have made hard choices in this area in the past, however, I want to be transparent that the goal for us here is to better align with existing best practices in the area and improve the existing scenarios moving forward.

Hope this helps.

[mcardoalm]

@javiercn

This issue is causing live reload for us to fail as well (#29478) and we had to downgrade Microsoft.AspNetCore.SpaServices.Extensions to version 3.1.16 to get things to work again.

Could you reconsider this pull request just for the .NET 5 branch, I know a better solution is coming in .NET 6 but we can't run preview packages and need a working solution for .NET 5 until the new version is released.

The change that @mattywong was suggesting just removes a contribution that caused a slowdown in UseProxyToSpaDevelopmentServer (#16797). It would be better to have live reload work at all and have the slowdown then not work at all.

Thanks for considering!

[ghost]

Hi @mcardoalm. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

[mattywong]

Thanks @mcardoalm, it might be worth mentioning that this pr initially was based on the v5.0.6 tag at the time before the rebase on main. I probably should have mentioned this in one of my comments at the time as I felt it should've been a patch to v5, and I'm not sure if that intention was clear. Glad to see there's more traction on this issue however.

[ghost]

Hi @mattywong. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.