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

Expose WebSocketMessageFlags in ClientWebSocket #77783

Closed
greenEkatherine opened this issue Nov 2, 2022 · 7 comments · Fixed by #79412
Closed

Expose WebSocketMessageFlags in ClientWebSocket #77783

greenEkatherine opened this issue Nov 2, 2022 · 7 comments · Fixed by #79412
Assignees
Milestone

Comments

@greenEkatherine
Copy link
Contributor

In #31088 compression support in WebSocket was introduced. There was added WebSocketMessageFlags.DisableCompression flag which is needed to turn off compression i.e. when sending data containing secrets -- it helps to avoid CRIME/BREACH attacks.

However, this flag was added only in WebSocket and not in ClientWebSocket which is supposed to manage messages. We need to expose DisableCompression flag in SendAsync methods as well as it is done for EndOfMessage flag.

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Nov 2, 2022
@ghost
Copy link

ghost commented Nov 2, 2022

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

Issue Details

In #31088 compression support in WebSocket was introduced. There was added WebSocketMessageFlags.DisableCompression flag which is needed to turn off compression i.e. when sending data containing secrets -- it helps to avoid CRIME/BREACH attacks.

However, this flag was added only in WebSocket and not in ClientWebSocket which is supposed to manage messages. We need to expose DisableCompression flag in SendAsync methods as well as it is done for EndOfMessage flag.

Author: greenEkatherine
Assignees: -
Labels:

area-System.Net

Milestone: -

@MihaZupan MihaZupan added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 2, 2022
@stephentoub
Copy link
Member

I've not quite followed the proposal. ClientWebSocket derives from WebSocket, inheriting all the APIs added there, and its implementation wraps ManagedWebSocket that implements the compression. What is the specific API that would be added to ClientWebSocket?

@greenEkatherine
Copy link
Contributor Author

@stephentoub we should add SendAsync method that handles DisableCompression flag. Currently we handle only EndOfMessage flag in both ClientWebSocket

public override Task SendAsync(ArraySegment<byte> buffer, WebSocketMessageType messageType, bool endOfMessage, CancellationToken cancellationToken) =>
ConnectedWebSocket.SendAsync(buffer, messageType, endOfMessage, cancellationToken);
public override ValueTask SendAsync(ReadOnlyMemory<byte> buffer, WebSocketMessageType messageType, bool endOfMessage, CancellationToken cancellationToken) =>
ConnectedWebSocket.SendAsync(buffer, messageType, endOfMessage, cancellationToken);
and parent WebSocket
public virtual ValueTask SendAsync(ReadOnlyMemory<byte> buffer, WebSocketMessageType messageType, bool endOfMessage, CancellationToken cancellationToken) =>
MemoryMarshal.TryGetArray(buffer, out ArraySegment<byte> arraySegment) ?
new ValueTask(SendAsync(arraySegment, messageType, endOfMessage, cancellationToken)) :
SendWithArrayPoolAsync(buffer, messageType, endOfMessage, cancellationToken);
public virtual ValueTask SendAsync(ReadOnlyMemory<byte> buffer, WebSocketMessageType messageType, WebSocketMessageFlags messageFlags, CancellationToken cancellationToken = default)
{
return SendAsync(buffer, messageType, messageFlags.HasFlag(WebSocketMessageFlags.EndOfMessage), cancellationToken);
}

@stephentoub
Copy link
Member

stephentoub commented Nov 3, 2022

Thanks. This isn't about missing API, then, it's a bug. ClientWebSocket should be overriding public virtual ValueTask SendAsync(ReadOnlyMemory<byte> buffer, WebSocketMessageType messageType, WebSocketMessageFlags messageFlags, CancellationToken cancellationToken = default) to forward it to the wrapped ManagedWebSocket's corresponding overload.

@stephentoub stephentoub added bug and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Nov 3, 2022
@MihaZupan
Copy link
Member

Triage: We should fix this in 8.0, looks like we're missing test coverage as well.

@MihaZupan MihaZupan removed the untriaged New issue has not been triaged by the area owner label Nov 3, 2022
@MihaZupan MihaZupan added this to the 8.0.0 milestone Nov 3, 2022
@greenEkatherine greenEkatherine self-assigned this Nov 30, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 8, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Dec 12, 2022
@CarnaViire
Copy link
Member

Reopening for backport

@CarnaViire CarnaViire reopened this Dec 12, 2022
@karelz karelz modified the milestones: 8.0.0, 6.0.x Dec 13, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Dec 13, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 5, 2023
@karelz
Copy link
Member

karelz commented Jan 6, 2023

Fixed in 8.0 (main) in PR #79412, and in 7.0.3 in PR #79547, and in 6.0.14 in PR #79549.

@karelz karelz closed this as completed Jan 6, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants