Skip to content

Commit 1c98a40

Browse files
code-ashercode-asher
committed
Check origin header on web sockets
1 parent 07cbf26 commit 1c98a40

File tree

11 files changed

+278
-62
lines changed

11 files changed

+278
-62
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

+73-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,67 @@ 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+
let origin: string
349+
try {
350+
origin = new URL(originRaw).host.trim().toLowerCase()
351+
} catch (error) {
352+
return false // Malformed URL.
353+
}
354+
355+
// Honor X-Forwarded-Host and Forwarded if present.
356+
const forwardedRaw = getFirstHeader(req, "forwarded")
357+
if (forwardedRaw) {
358+
const parts = forwardedRaw.split(/[;,]/)
359+
for (let i = 0; i < parts.length; ++i) {
360+
const [key, value] = splitOnFirstEquals(parts[i])
361+
if (key.trim().toLowerCase() === "host" && value) {
362+
return origin === value.trim().toLowerCase()
363+
}
364+
}
365+
}
366+
367+
const xHostRaw = getFirstHeader(req, "x-forwarded-host")
368+
if (xHostRaw) {
369+
return origin === xHostRaw.trim().toLowerCase()
370+
}
371+
372+
// A missing host likely means the reverse proxy has not been configured to
373+
// forward the host which means we cannot perform the check. Emit a warning
374+
// so an admin can fix the issue.
375+
const host = getFirstHeader(req, "host")
376+
if (!host) {
377+
logger.warn(`no host headers found; blocking request to ${req.originalUrl}`)
378+
return false
379+
}
380+
381+
return origin === host.trim().toLowerCase()
382+
}

‎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

+105-42
Original file line numberDiff line numberDiff line change
@@ -1,55 +1,118 @@
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+
{
45+
origin: "localhost:8080",
46+
host: "localhost:8080",
47+
expected: false, // Gets parsed as host: localhost and path: 8080.
48+
},
49+
{
50+
origin: "test.org",
51+
host: "localhost:8080",
52+
expected: false, // Parsing fails completely.
53+
},
54+
].forEach((test) => {
55+
;[
56+
["host", test.host],
57+
["x-forwarded-host", test.host],
58+
["forwarded", `for=127.0.0.1, host=${test.host}, proto=http`],
59+
["forwarded", `for=127.0.0.1;proto=http;host=${test.host}`],
60+
["forwarded", `proto=http;host=${test.host}, for=127.0.0.1`],
61+
].forEach(([key, value]) => {
62+
it(`${test.origin} -> [${key}: ${value}]`, () => {
63+
const req = getMockReq({
64+
originalUrl: "localhost:8080",
65+
headers: {
66+
origin: test.origin,
67+
[key]: value,
68+
},
69+
})
70+
expect(http.authenticateOrigin(req)).toBe(test.expected)
71+
})
72+
})
1873
})
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)
2474
})
25-
it("should use an empty string if no query params", () => {
26-
const mockReq = getMockReq({
27-
originalUrl: "localhost:8080",
75+
76+
describe("constructRedirectPath", () => {
77+
it("should preserve slashes in queryString so they are human-readable", () => {
78+
const mockReq = getMockReq({
79+
originalUrl: "localhost:8080",
80+
})
81+
const mockQueryParams = { folder: "/Users/jp/dev/coder" }
82+
const mockTo = ""
83+
const actual = http.constructRedirectPath(mockReq, mockQueryParams, mockTo)
84+
const expected = "./?folder=/Users/jp/dev/coder"
85+
expect(actual).toBe(expected)
2886
})
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",
87+
it("should use an empty string if no query params", () => {
88+
const mockReq = getMockReq({
89+
originalUrl: "localhost:8080",
90+
})
91+
const mockQueryParams = {}
92+
const mockTo = ""
93+
const actual = http.constructRedirectPath(mockReq, mockQueryParams, mockTo)
94+
const expected = "./"
95+
expect(actual).toBe(expected)
3896
})
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",
97+
it("should append the 'to' path relative to the originalUrl", () => {
98+
const mockReq = getMockReq({
99+
originalUrl: "localhost:8080",
100+
})
101+
const mockQueryParams = {}
102+
const mockTo = "vscode"
103+
const actual = http.constructRedirectPath(mockReq, mockQueryParams, mockTo)
104+
const expected = "./vscode"
105+
expect(actual).toBe(expected)
106+
})
107+
it("should append append queryParams after 'to' path", () => {
108+
const mockReq = getMockReq({
109+
originalUrl: "localhost:8080",
110+
})
111+
const mockQueryParams = { folder: "/Users/jp/dev/coder" }
112+
const mockTo = "vscode"
113+
const actual = http.constructRedirectPath(mockReq, mockQueryParams, mockTo)
114+
const expected = "./vscode?folder=/Users/jp/dev/coder"
115+
expect(actual).toBe(expected)
48116
})
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)
54117
})
55118
})

0 commit comments

Comments
 (0)