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

[BUG, WEBSOCKETS] Not properly handling the Origin header #727

Closed
SerafimArts opened this issue Jun 15, 2021 · 2 comments · Fixed by #730
Closed

[BUG, WEBSOCKETS] Not properly handling the Origin header #727

SerafimArts opened this issue Jun 15, 2021 · 2 comments · Fixed by #730
Assignees
Labels
B-bug Bug: bug, exception F-need-verification
Milestone

Comments

@SerafimArts
Copy link
Contributor

SerafimArts commented Jun 15, 2021

CORS headers are incorrectly processed.

First Playback

Configuration

server:
    command: "php worker.php"
    relay: "pipes"

http:
    address: 127.0.0.1:80
    middleware: [ "websockets" ]

websockets:
    pubsubs: [ "memory" ]
    path: "/ws"

Worker Code

use Spiral\RoadRunner\Worker;
use Spiral\RoadRunner\Http\HttpWorker;

require __DIR__ . '/vendor/autoload.php';

$http = new HttpWorker(Worker::create());

while ($req = $http->waitRequest()) {
    $http->respond(200, 'Response', [
        'Access-Control-Allow-Origin' => ['*']
    ]);
}

Actual Behaviour

fetch('http://127.0.0.1'); // HTTP OK
new WebSocket('ws://127.0.0.1/ws'); // WebSocket connection to 'ws://127.0.0.1/ws' failed: (anonymous) | @ | VM455:1

RR Trace:

2021-06-15T23:53:52.275+0300    ?[91mERROR?[0m  ?[92mwebsockets  ?[0m   websockets/plugin.go:207        upgrade connection      {"error": "websocket: request origin not allowed by Upgrader.Ch
eckOrigin"}
github.com/spiral/roadrunner/v2/plugins/websockets.(*Plugin).Middleware.func1
        github.com/spiral/roadrunner/[email protected]/plugins/websockets/plugin.go:207
net/http.HandlerFunc.ServeHTTP
        net/http/server.go:2069
net/http.serverHandler.ServeHTTP
        net/http/server.go:2887
net/http.(*conn).serve
        net/http/server.go:1952

Expected Behaviour

Correct HTTP 101 Response

Second Variant

Configuration

server:
    command: "php worker.php"
    relay: "pipes"

http:
    address: 127.0.0.1:80
    middleware: [ "headers", "websockets" ]
    headers:
        cors:
            allowed_origin: "*"

websockets:
    pubsubs: [ "memory" ]
    path: "/ws"

Worker Code

use Spiral\RoadRunner\Worker;
use Spiral\RoadRunner\Http\HttpWorker;

require __DIR__ . '/vendor/autoload.php';

$http = new HttpWorker(Worker::create());

while ($req = $http->waitRequest()) {
    $http->respond(200, 'Response');
}

Actual Behaviour

fetch('http://127.0.0.1'); // HTTP OK
new WebSocket('ws://127.0.0.1/ws'); // WebSocket connection to 'ws://127.0.0.1/ws' failed: (anonymous) | @ | VM471:1

RR Trace:

2021-06-15T23:59:12.755+0300    ?[91mERROR?[0m  ?[92mwebsockets  ?[0m   websockets/plugin.go:207        upgrade connection      {"error": "websocket: request origin not allowed by Upgrader.Ch
eckOrigin"}
github.com/spiral/roadrunner/v2/plugins/websockets.(*Plugin).Middleware.func1
        github.com/spiral/roadrunner/[email protected]/plugins/websockets/plugin.go:207
net/http.HandlerFunc.ServeHTTP
        net/http/server.go:2069
github.com/spiral/roadrunner/v2/plugins/headers.(*Plugin).Middleware.func1
        github.com/spiral/roadrunner/[email protected]/plugins/headers/plugin.go:64
net/http.HandlerFunc.ServeHTTP
        net/http/server.go:2069
net/http.serverHandler.ServeHTTP
        net/http/server.go:2887
net/http.(*conn).serve
        net/http/server.go:1952

Expected Behaviour

Correct HTTP 101 Response

@SerafimArts SerafimArts added B-bug Bug: bug, exception F-need-verification labels Jun 15, 2021
@rustatian
Copy link
Member

rustatian commented Jun 16, 2021

Actually, this is not a whole CORS but only a request Origin header. Because according to a standard RFC6455, WebSocket connection might have such header to verify, but not other CORS headers.
In the RR1 that value checking was skipped. In the actual implementation this part was not completely discussed, so, let me discuss this with @wolfy-j.


@wolfy-j In the broadcast-ws plugin default behaviour was to skip Origin header check for the WS connections:

	u.CheckOrigin = func(r *http.Request) bool {
		// allow all connections by default
		return true
	}

This is a code sample from the previous gorilla Upgrade connection method. This option had no actual effect:
https://github.com/spiral/broadcast-ws/blob/56cc0ca297891a26773db14e3fcb8b4f50e6f1e6/config.go#L12
Because by default Origin in any case wasn't checked.
But new implementation has the check implemented:

	checkOrigin := u.CheckOrigin
	if checkOrigin == nil {
		checkOrigin = checkSameOrigin // <---------
	}
	if !checkOrigin(r) { // <-------
		return u.returnError(w, r, http.StatusForbidden, "websocket: request origin not allowed by Upgrader.CheckOrigin")
	}

As the summary, we have 2 options here:

  1. Skip the check (just return true on every connection) as was in the RR1.
  2. Add a new option to the WebSocket configuration section. We also have to check all values according to the RFC4790 (ASCII case folding).
websockets:
origin:
    allow: ["localhost", "1.1.1.1", "*"]
    forbid: ["2.2.2.2"]

@rustatian rustatian changed the title [BUG] Broadcast CORS error [BUG, WEBSOCKETS] Not properly handling the Origin header Jun 16, 2021
@rustatian rustatian self-assigned this Jun 16, 2021
@rustatian rustatian added this to the 2.3.1 milestone Jun 16, 2021
@rustatian
Copy link
Member

As discussed with @wolfy-j , I'll include the Origin option in the websockets configuration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-bug Bug: bug, exception F-need-verification
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants