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

Updating native WebSocket header logic to match RN #13366

Merged
merged 3 commits into from
Jul 16, 2024

Conversation

slswalker
Copy link
Contributor

@slswalker slswalker commented Jun 19, 2024

Description

Type of Change

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

Why

There are two bugs at play. This fixes both bugs, allowing headers to be properly used with web socket requests.

Bug #1
In Libraries/WebSocket/WebSocket.js, the expected type for headers is an object. This is passed down as-is to the native layer in the call NativeWebSocketModule.connect(url, protocols, {headers}, this._socketId);. The turbo module implementation attempts to iterative over this box type by using .AsArray(), which immediately continues past the for-loop. The result is that any headers passed down from Javascript are ignored.

Bug #2
React Native WebSocket implementations do not put a port on the Origin header when a port is not provided. RNW does, which results in adding a port of 0 to every default Origin header. Port 0 is reserved and is not meant to be used.

The combination of both of these bugs means every web socket request will only have one header and it will be a improperly constructed Origin header.

Resolves [Add Relevant Issue Here]
While I don't know which version of RNW this was introduced in, when Xbox updated an app from RNW 0.69 to 0.73, all web socket requests broke due to services rejecting origin headers wiht port 0. This works fine on mobile apps due.

What

  • WebSocketModule.cpp: Changed the usage of .AsArray() to .AsObject() to match expectations of headers type that is passed down by Javascript
  • WinRTWebSocketResource.cpp: Changed default origin header construction to only add a port if it is not 0.

Testing

Xbox is currently using a patch that applies both of these changes.

Changelog

yes?

WebSocket headers are now properly applied when using Turbo Modules

Microsoft Reviewers: Open in CodeFlow

@slswalker slswalker requested a review from a team as a code owner June 19, 2024 13:13
@acoates-ms acoates-ms requested a review from JunielKatarn June 19, 2024 14:55
@slswalker slswalker force-pushed the user/sawalker/ws-headers branch from e2807d0 to 22a9c62 Compare June 28, 2024 13:27
@slswalker slswalker requested a review from a team as a code owner June 28, 2024 13:27
@slswalker slswalker changed the title Updating native WS header logic to match RN Updating native WebSocket header logic to match RN Jun 28, 2024
@slswalker slswalker force-pushed the user/sawalker/ws-headers branch from 112231c to 0ce4401 Compare July 16, 2024 17:24
@acoates-ms acoates-ms merged commit 63d919e into microsoft:main Jul 16, 2024
57 checks passed
@@ -382,7 +382,10 @@ void WinRTWebSocketResource::Connect(string &&url, const Protocols &protocols, c
scheme = L"https";
}

auto origin = winrt::hstring{scheme + L"://" + host + L":" + winrt::to_hstring(port)};
// Only add a port if a port is defined
winrt::hstring originPort = port != 0 ? L":" + winrt::to_hstring(port) : L"";
Copy link
Contributor

@JunielKatarn JunielKatarn Jul 17, 2024

Choose a reason for hiding this comment

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

For follow-up:

While this change prevents from appending a port with value 0 when the intended value may have been the protocol defaults (80, 443), this would prevent a client app from explicitly using such port, even though it's against conventions and possibly standards.

We should use the same URL processing as OriginPolicyHttpFilter.cpp to prevent a few corner cases such as explicitly set default ports.
See #12626 and OfficeDev/office-js#4144.

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.

4 participants