Skip to content

Conversation

@johnnzhou
Copy link
Contributor

Enhance WebSocketProtocolErrorHandler to correctly add masking key for client/server.

Motivation:

In NIOWebSocket, the automatic error handling provided by WebSocketProtocolErrorHandler offers a convenient way for both clients and servers to handle protocol-level errors. However, the WebSocketFrame used in WebSocketProtocolErrorHandler does not fully adhere to the RFC 6455 standard. Specifically, as per RFC 6455 Section 5.1, a client must mask all frames it sends to the server, while a server must not mask any frames it sends to the client. This PR addressed this discrepancy to ensure compliance with the WebSocket protocol.

Modifications:

In the WebSocketProtocolErrorHandler initializer, the user can specify whether the handler is used for a WebSocket client or server. Within the errorCaught method, the WebSocketFrame will include a maskingKey if the handler is used by a client, or nil if it is used by a server.

Additionally, the @unchecked annotation is removed from NIOWebSocketServerUpgrader since the project is now using swift 5.9 toolchain.

Result:

The WebSocketProtocolErrorHandler is more robust and can correctly close the connection is error occurs.

@adam-fowler
Copy link
Contributor

Given previously a masking key wasn't added shouldn't the default continue to be that. Also adding a parameter to a function is considered a breaking change, even if it has a default value. You need to add a new function which sets the isServer parameter.

@johnnzhou
Copy link
Contributor Author

Hi @adam-fowler, thanks for the review. I addressed your comments in the most recent version. Please let me know if everything looks good now or if there’s anything else you’d like me to adjust.

Thanks again!

@adam-fowler
Copy link
Contributor

Instead of having a mutable isServer member variable. You could have two initializers. The original and a new one which takes a isServer parameter.

NIO folks will have last say on this though.

@Lukasa
Copy link
Contributor

Lukasa commented Jan 6, 2025

I like Adam's suggestion of using a let instead of a var, and setting it in the initializer. The value should never change at runtime anyway.

@Lukasa Lukasa added the 🆕 semver/minor Adds new public API. label Jan 6, 2025
Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Generally looks good, just a quick note!

@johnnzhou
Copy link
Contributor Author

Thanks @Lukasa for the review! I updated the PR to address your comments. Could you please review whenever you are available? Appreciate it!

@johnnzhou
Copy link
Contributor Author

johnnzhou commented Jan 16, 2025

Hi @Lukasa, whenever you are available, can you please re-review this PR? I modified it according to your comments. Let me know if there is any change I need to make. Thanks for your time in advance!

Copy link
Contributor

@Lukasa Lukasa left a comment

Choose a reason for hiding this comment

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

Looks great, thanks @johnnzhou!

@Lukasa Lukasa enabled auto-merge (squash) January 16, 2025 10:55
@Lukasa Lukasa merged commit 2620f8e into apple:main Jan 16, 2025
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🆕 semver/minor Adds new public API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants