Skip to content

Commit c662ef5

Browse files
code-ashercode-asher
committed
Add origin check on web sockets
1 parent 07cbf26 commit c662ef5

File tree

11 files changed

+264
-63
lines changed

11 files changed

+264
-63
lines changed

‎src/common/http.ts

+1
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ export enum HttpCode {
44
NotFound = 404,
55
BadRequest = 400,
66
Unauthorized = 401,
7+
Forbidden = 403,
78
LargePayload = 413,
89
ServerError = 500,
910
}

‎src/node/http.ts

+68-1
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,15 @@ import { version as codeServerVersion } from "./constants"
1212
import { Heart } from "./heart"
1313
import { CoderSettings, SettingsProvider } from "./settings"
1414
import { UpdateProvider } from "./update"
15-
import { getPasswordMethod, IsCookieValidArgs, isCookieValid, sanitizeString, escapeHtml, escapeJSON } from "./util"
15+
import {
16+
getPasswordMethod,
17+
IsCookieValidArgs,
18+
isCookieValid,
19+
sanitizeString,
20+
escapeHtml,
21+
escapeJSON,
22+
splitOnFirstEquals,
23+
} from "./util"
1624

1725
/**
1826
* Base options included on every page.
@@ -308,3 +316,62 @@ export const getCookieOptions = (req: express.Request): express.CookieOptions =>
308316
export const self = (req: express.Request): string => {
309317
return normalize(`${req.baseUrl}${req.originalUrl.endsWith("/") ? "/" : ""}`, true)
310318
}
319+
320+
function getFirstHeader(req: http.IncomingMessage, headerName: string): string | undefined {
321+
const val = req.headers[headerName]
322+
return Array.isArray(val) ? val[0] : val
323+
}
324+
325+
/**
326+
* Throw an error if origin checks fail. Call `next` if provided.
327+
*/
328+
export function ensureOrigin(req: express.Request, _?: express.Response, next?: express.NextFunction): void {
329+
if (!authenticateOrigin(req)) {
330+
throw new HttpError("Forbidden", HttpCode.Forbidden)
331+
}
332+
if (next) {
333+
next()
334+
}
335+
}
336+
337+
/**
338+
* Authenticate the request origin against the host.
339+
*/
340+
export function authenticateOrigin(req: express.Request): boolean {
341+
// A missing origin probably means the source is non-browser. Not sure we
342+
// have a use case for this but let it through.
343+
const originRaw = getFirstHeader(req, "origin")
344+
if (!originRaw) {
345+
return true
346+
}
347+
348+
const origin = new URL(originRaw).host.trim().toLowerCase()
349+
350+
// Honor X-Forwarded-Host and Forwarded if present.
351+
const forwardedRaw = getFirstHeader(req, "forwarded")
352+
if (forwardedRaw) {
353+
const parts = forwardedRaw.split(/[;,]/)
354+
for (let i = 0; i < parts.length; ++i) {
355+
const [key, value] = splitOnFirstEquals(parts[i])
356+
if (key.trim().toLowerCase() === "host" && value) {
357+
return origin == value.trim().toLowerCase()
358+
}
359+
}
360+
}
361+
362+
const xHostRaw = getFirstHeader(req, "x-forwarded-host")
363+
if (xHostRaw) {
364+
return origin === xHostRaw.trim().toLowerCase()
365+
}
366+
367+
// A missing host likely means the reverse proxy has not been configured to
368+
// forward the host which means we cannot perform the check. Emit a warning
369+
// so an admin can fix the issue.
370+
const host = getFirstHeader(req, "host")
371+
if (!host) {
372+
logger.warn(`no host headers found; blocking request to ${req.originalUrl}`)
373+
return false
374+
}
375+
376+
return origin == host.trim().toLowerCase()
377+
}

‎src/node/routes/domainProxy.ts

+2-4
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { Request, Router } from "express"
22
import { HttpCode, HttpError } from "../../common/http"
3-
import { authenticated, ensureAuthenticated, redirect, self } from "../http"
3+
import { authenticated, ensureAuthenticated, ensureOrigin, redirect, self } from "../http"
44
import { proxy } from "../proxy"
55
import { Router as WsRouter } from "../wsRouter"
66

@@ -78,10 +78,8 @@ wsRouter.ws("*", async (req, _, next) => {
7878
if (!port) {
7979
return next()
8080
}
81-
82-
// Must be authenticated to use the proxy.
81+
ensureOrigin(req)
8382
await ensureAuthenticated(req)
84-
8583
proxy.ws(req, req.ws, req.head, {
8684
ignorePath: true,
8785
target: `http://0.0.0.0:${port}${req.originalUrl}`,

‎src/node/routes/pathProxy.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@ import * as path from "path"
33
import * as qs from "qs"
44
import * as pluginapi from "../../../typings/pluginapi"
55
import { HttpCode, HttpError } from "../../common/http"
6-
import { authenticated, ensureAuthenticated, redirect, self } from "../http"
6+
import { authenticated, ensureAuthenticated, ensureOrigin, redirect, self } from "../http"
77
import { proxy as _proxy } from "../proxy"
88

99
const getProxyTarget = (req: Request, passthroughPath?: boolean): string => {
@@ -50,6 +50,7 @@ export async function wsProxy(
5050
passthroughPath?: boolean
5151
},
5252
): Promise<void> {
53+
ensureOrigin(req)
5354
await ensureAuthenticated(req)
5455
_proxy.ws(req, req.ws, req.head, {
5556
ignorePath: true,

‎src/node/routes/vscode.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import { WebsocketRequest } from "../../../typings/pluginapi"
77
import { logError } from "../../common/util"
88
import { CodeArgs, toCodeArgs } from "../cli"
99
import { isDevMode } from "../constants"
10-
import { authenticated, ensureAuthenticated, redirect, replaceTemplates, self } from "../http"
10+
import { authenticated, ensureAuthenticated, ensureOrigin, redirect, replaceTemplates, self } from "../http"
1111
import { SocketProxyProvider } from "../socket"
1212
import { isFile, loadAMDModule } from "../util"
1313
import { Router as WsRouter } from "../wsRouter"
@@ -173,7 +173,7 @@ export class CodeServerRouteWrapper {
173173
this.router.get("/", this.ensureCodeServerLoaded, this.$root)
174174
this.router.get("/manifest.json", this.manifest)
175175
this.router.all("*", ensureAuthenticated, this.ensureCodeServerLoaded, this.$proxyRequest)
176-
this._wsRouterWrapper.ws("*", ensureAuthenticated, this.ensureCodeServerLoaded, this.$proxyWebsocket)
176+
this._wsRouterWrapper.ws("*", ensureOrigin, ensureAuthenticated, this.ensureCodeServerLoaded, this.$proxyWebsocket)
177177
}
178178

179179
dispose() {

‎src/node/wsRouter.ts

+3
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ export class WebsocketRouter {
3232
/**
3333
* Handle a websocket at this route. Note that websockets are immediately
3434
* paused when they come in.
35+
*
36+
* If the origin header exists it must match the host or the connection will
37+
* be prevented.
3538
*/
3639
public ws(route: expressCore.PathParams, ...handlers: pluginapi.WebSocketHandler[]): void {
3740
this.router.get(

‎test/unit/node/http.test.ts

+95-42
Original file line numberDiff line numberDiff line change
@@ -1,55 +1,108 @@
11
import { getMockReq } from "@jest-mock/express"
2-
import { constructRedirectPath, relativeRoot } from "../../../src/node/http"
2+
import * as http from "../../../src/node/http"
3+
import { mockLogger } from "../../utils/helpers"
34

45
describe("http", () => {
6+
beforeEach(() => {
7+
mockLogger()
8+
})
9+
10+
afterEach(() => {
11+
jest.clearAllMocks()
12+
})
13+
514
it("should construct a relative path to the root", () => {
6-
expect(relativeRoot("/")).toStrictEqual(".")
7-
expect(relativeRoot("/foo")).toStrictEqual(".")
8-
expect(relativeRoot("/foo/")).toStrictEqual("./..")
9-
expect(relativeRoot("/foo/bar ")).toStrictEqual("./..")
10-
expect(relativeRoot("/foo/bar/")).toStrictEqual("./../..")
15+
expect(http.relativeRoot("/")).toStrictEqual(".")
16+
expect(http.relativeRoot("/foo")).toStrictEqual(".")
17+
expect(http.relativeRoot("/foo/")).toStrictEqual("./..")
18+
expect(http.relativeRoot("/foo/bar ")).toStrictEqual("./..")
19+
expect(http.relativeRoot("/foo/bar/")).toStrictEqual("./../..")
1120
})
12-
})
1321

14-
describe("constructRedirectPath", () => {
15-
it("should preserve slashes in queryString so they are human-readable", () => {
16-
const mockReq = getMockReq({
17-
originalUrl: "localhost:8080",
22+
describe("origin", () => {
23+
;[
24+
{
25+
origin: "",
26+
host: "",
27+
expected: true,
28+
},
29+
{
30+
origin: "http://localhost:8080",
31+
host: "",
32+
expected: false,
33+
},
34+
{
35+
origin: "http://localhost:8080",
36+
host: "localhost:8080",
37+
expected: true,
38+
},
39+
{
40+
origin: "http://localhost:8080",
41+
host: "localhost:8081",
42+
expected: false,
43+
},
44+
].forEach((test) => {
45+
;[
46+
["host", test.host],
47+
["x-forwarded-host", test.host],
48+
["forwarded", `for=127.0.0.1, host=${test.host}, proto=http`],
49+
["forwarded", `for=127.0.0.1;proto=http;host=${test.host}`],
50+
["forwarded", `proto=http;host=${test.host}, for=127.0.0.1`],
51+
].forEach(([key, value]) => {
52+
it(`${test.origin} -> [${key}: ${value}]`, () => {
53+
const req = getMockReq({
54+
originalUrl: "localhost:8080",
55+
headers: {
56+
origin: test.origin,
57+
[key]: value,
58+
},
59+
})
60+
expect(http.authenticateOrigin(req)).toBe(test.expected)
61+
})
62+
})
1863
})
19-
const mockQueryParams = { folder: "/Users/jp/dev/coder" }
20-
const mockTo = ""
21-
const actual = constructRedirectPath(mockReq, mockQueryParams, mockTo)
22-
const expected = "./?folder=/Users/jp/dev/coder"
23-
expect(actual).toBe(expected)
2464
})
25-
it("should use an empty string if no query params", () => {
26-
const mockReq = getMockReq({
27-
originalUrl: "localhost:8080",
65+
66+
describe("constructRedirectPath", () => {
67+
it("should preserve slashes in queryString so they are human-readable", () => {
68+
const mockReq = getMockReq({
69+
originalUrl: "localhost:8080",
70+
})
71+
const mockQueryParams = { folder: "/Users/jp/dev/coder" }
72+
const mockTo = ""
73+
const actual = http.constructRedirectPath(mockReq, mockQueryParams, mockTo)
74+
const expected = "./?folder=/Users/jp/dev/coder"
75+
expect(actual).toBe(expected)
2876
})
29-
const mockQueryParams = {}
30-
const mockTo = ""
31-
const actual = constructRedirectPath(mockReq, mockQueryParams, mockTo)
32-
const expected = "./"
33-
expect(actual).toBe(expected)
34-
})
35-
it("should append the 'to' path relative to the originalUrl", () => {
36-
const mockReq = getMockReq({
37-
originalUrl: "localhost:8080",
77+
it("should use an empty string if no query params", () => {
78+
const mockReq = getMockReq({
79+
originalUrl: "localhost:8080",
80+
})
81+
const mockQueryParams = {}
82+
const mockTo = ""
83+
const actual = http.constructRedirectPath(mockReq, mockQueryParams, mockTo)
84+
const expected = "./"
85+
expect(actual).toBe(expected)
3886
})
39-
const mockQueryParams = {}
40-
const mockTo = "vscode"
41-
const actual = constructRedirectPath(mockReq, mockQueryParams, mockTo)
42-
const expected = "./vscode"
43-
expect(actual).toBe(expected)
44-
})
45-
it("should append append queryParams after 'to' path", () => {
46-
const mockReq = getMockReq({
47-
originalUrl: "localhost:8080",
87+
it("should append the 'to' path relative to the originalUrl", () => {
88+
const mockReq = getMockReq({
89+
originalUrl: "localhost:8080",
90+
})
91+
const mockQueryParams = {}
92+
const mockTo = "vscode"
93+
const actual = http.constructRedirectPath(mockReq, mockQueryParams, mockTo)
94+
const expected = "./vscode"
95+
expect(actual).toBe(expected)
96+
})
97+
it("should append append queryParams after 'to' path", () => {
98+
const mockReq = getMockReq({
99+
originalUrl: "localhost:8080",
100+
})
101+
const mockQueryParams = { folder: "/Users/jp/dev/coder" }
102+
const mockTo = "vscode"
103+
const actual = http.constructRedirectPath(mockReq, mockQueryParams, mockTo)
104+
const expected = "./vscode?folder=/Users/jp/dev/coder"
105+
expect(actual).toBe(expected)
48106
})
49-
const mockQueryParams = { folder: "/Users/jp/dev/coder" }
50-
const mockTo = "vscode"
51-
const actual = constructRedirectPath(mockReq, mockQueryParams, mockTo)
52-
const expected = "./vscode?folder=/Users/jp/dev/coder"
53-
expect(actual).toBe(expected)
54107
})
55108
})

0 commit comments

Comments
 (0)