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

WebSocket Origin Restriction #9447

Merged
merged 11 commits into from
Nov 6, 2018
Merged

WebSocket Origin Restriction #9447

merged 11 commits into from
Nov 6, 2018

Conversation

BrennanConroy
Copy link
Member

@BrennanConroy BrennanConroy commented Nov 5, 2018

Fixes #9363

Internal Review Link
There are some moniker ranges, so switch the versions to verify they're working

});
```

This helps ensure that browser users are using a trusted client and not a malicious one from a different site.
Copy link
Member Author

Choose a reason for hiding this comment

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

Review: Should we keep this or remove it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove from here, but consider working it into the section intro.

@BrennanConroy BrennanConroy changed the title wip WebSocket Origin Restriction WebSocket Origin Restriction Nov 5, 2018
Copy link
Contributor

@tdykstra tdykstra left a comment

Choose a reason for hiding this comment

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

Looks good, a few comments on style issues.

The following settings can be configured:

* `KeepAliveInterval` - How frequently to send "ping" frames to the client to ensure proxies keep the connection open.
* `ReceiveBufferSize` - The size of the buffer used to receive data. Advanced users may need to change this for performance tuning based on the size of the data.
* `AllowedOrigins` - List of the Origin header values allowed for WebSocket requests to prevent Cross-Site WebSocket Hijacking. By default all Origins are allowed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Lowercase Cross-Site and Hijacking

});
```

This helps ensure that browser users are using a trusted client and not a malicious one from a different site.
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove from here, but consider working it into the section intro.

```c#
app.UseWebSockets(new WebSocketOptions()
{
AllowedOrigins.Add("https://client.com");
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the delimiter for multiple origins?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's a list so you can call .Add() for as many origins as you want

Copy link
Contributor

Choose a reason for hiding this comment

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

Show multiple origins (like https://client.com and https://www.client.com) to illustrate that

@@ -39,6 +39,14 @@ For example, the following CORS policy allows a SignalR browser client hosted on

## WebSocket Origin Restriction

::: moniker range=">= aspnetcore-2.2"

The protections provided by CORS don't apply to WebSockets. For origin restriction on WebSockets read [WebSockets Origin Restriction](xref:fundamentals/websockets#websocket-origin-restriction).
Copy link
Contributor

Choose a reason for hiding this comment

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

Lowercase origin restriction

@@ -123,6 +137,33 @@ The code shown earlier that accepts the WebSocket request passes the `WebSocket`

When accepting the WebSocket connection before beginning the loop, the middleware pipeline ends. Upon closing the socket, the pipeline unwinds. That is, the request stops moving forward in the pipeline when the WebSocket is accepted. When the loop is finished and the socket is closed, the request proceeds back up the pipeline.

::: moniker range=">= aspnetcore-2.2"

### WebSocket Origin Restriction
Copy link
Contributor

Choose a reason for hiding this comment

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

Lowercase origin restriction

sentence case headings; lowercase "origin" except when it refers specifically to "Origin" as the name of a header.

This helps ensure that browser users are using a trusted client and not a malicious one from a different site.

> [!NOTE]
> The `Origin` header is controlled by the client and, like the `Referer` header, can be faked. These headers should **not** be used as an authentication mechanism.
Copy link
Contributor

Choose a reason for hiding this comment

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

Or more directly

Do not use these headers as an authentication mechanism.

Could also make that sentence first to make it stand out more.

@BrennanConroy
Copy link
Member Author

@tdykstra FYI there are unrelated warnings in the publishing check

@tdykstra
Copy link
Contributor

tdykstra commented Nov 5, 2018

@BrennanConroy Thanks, we're aware of them and are looking into it.

The following settings can be configured:

* `KeepAliveInterval` - How frequently to send "ping" frames to the client to ensure proxies keep the connection open.
* `ReceiveBufferSize` - The size of the buffer used to receive data. Advanced users may need to change this for performance tuning based on the size of the data.
Copy link
Member

Choose a reason for hiding this comment

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

What are the default values?


* `KeepAliveInterval` - How frequently to send "ping" frames to the client to ensure proxies keep the connection open. The default is two minutes.
* `ReceiveBufferSize` - The size of the buffer used to receive data. Advanced users may need to change this for performance tuning based on the size of the data. The default is 4kb.
* `AllowedOrigins` - List of the Origin header values allowed for WebSocket requests to prevent cross-site WebSocket hijacking. By default all Origins are allowed.
Copy link
Member

@halter73 halter73 Nov 5, 2018

Choose a reason for hiding this comment

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

Nit: Uppercase O in "Origin header" seems right, but it should probably be lowercase for "By default, all origins are allowed."

Co-Authored-By: BrennanConroy <brecon@microsoft.com>
* `KeepAliveInterval` - How frequently to send "ping" frames to the client to ensure proxies keep the connection open.
* `ReceiveBufferSize` - The size of the buffer used to receive data. Advanced users may need to change this for performance tuning based on the size of the data.
* `KeepAliveInterval` - How frequently to send "ping" frames to the client to ensure proxies keep the connection open. The default is two minutes.
* `ReceiveBufferSize` - The size of the buffer used to receive data. Advanced users may need to change this for performance tuning based on the size of the data. The default is 4kb.
Copy link
Member

Choose a reason for hiding this comment

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

  • kb --> KB
  • Add space between 4 and KB

The following settings can be configured:

* `KeepAliveInterval` - How frequently to send "ping" frames to the client to ensure proxies keep the connection open. The default is two minutes.
* `ReceiveBufferSize` - The size of the buffer used to receive data. Advanced users may need to change this for performance tuning based on the size of the data. The default is 4kb.
Copy link
Member

Choose a reason for hiding this comment

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

  • kb --> KB
  • Add space between 4 and KB


* `KeepAliveInterval` - How frequently to send "ping" frames to the client to ensure proxies keep the connection open. The default is two minutes.
* `ReceiveBufferSize` - The size of the buffer used to receive data. Advanced users may need to change this for performance tuning based on the size of the data. The default is 4kb.
* `AllowedOrigins` - List of the Origin header values allowed for WebSocket requests to prevent cross-site WebSocket hijacking. By default, all origins are allowed.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* `AllowedOrigins` - List of the Origin header values allowed for WebSocket requests to prevent cross-site WebSocket hijacking. By default, all origins are allowed.
* `AllowedOrigins` - List of the Origin header values allowed for WebSocket requests to prevent cross-site WebSocket hijacking. By default, all origins are allowed.

@@ -39,6 +39,14 @@ For example, the following CORS policy allows a SignalR browser client hosted on

## WebSocket Origin Restriction

::: moniker range=">= aspnetcore-2.2"

The protections provided by CORS don't apply to WebSockets. For origin restriction on WebSockets read [WebSockets origin restriction](xref:fundamentals/websockets#websocket-origin-restriction).
Copy link
Member

Choose a reason for hiding this comment

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

Add a comma after "For origin restrictions on WebSockets".

```c#
app.UseWebSockets(new WebSocketOptions()
{
AllowedOrigins.Add("https://client.com");
Copy link
Contributor

Choose a reason for hiding this comment

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

Show multiple origins (like https://client.com and https://www.client.com) to illustrate that

aspnetcore/fundamentals/websockets.md Outdated Show resolved Hide resolved
analogrelay and others added 2 commits November 6, 2018 11:28
Co-Authored-By: BrennanConroy <brecon@microsoft.com>
aspnetcore/fundamentals/websockets.md Outdated Show resolved Hide resolved

However, browsers do send the `Origin` header when issuing WebSocket requests. Applications should be configured to validate these headers to ensure that only WebSockets coming from the expected origins are allowed.

If you are hosting your server on "https://server.com" and hosting your client on "https://client.com", add "https://client.com" to the `AllowedOrigins` list for WebSockets to verify.
Copy link
Member

Choose a reason for hiding this comment

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

you are --> you're

scottaddie and others added 3 commits November 6, 2018 13:22
@scottaddie scottaddie merged commit d396dff into master Nov 6, 2018
@delete-merged-branch delete-merged-branch bot deleted the brecon/websocketOrigin branch November 6, 2018 21:55
@scottaddie
Copy link
Member

Thank you for your help, @BrennanConroy!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants