Skip to content

Commit 62b3a6f

Browse files
Teffencode-asher
Teffen
and
code-asher
authored
Proxy path fixes (#4548)
* Fix issue where HTTP error status codes are not read. * Fix issues surrounding sessions when accessed from a proxy. - Updated vscode args to match latest upstream. - Fixed issues surrounding trailing slashes affecting base paths. - Updated cookie names to better match upstream's usage, debuggability. * Bump vendor. * Update tests. * Fix issue where tests lack cookie key. Co-authored-by: Asher <ash@coder.com>
1 parent 6a2740f commit 62b3a6f

File tree

11 files changed

+39
-27
lines changed

11 files changed

+39
-27
lines changed

‎src/common/http.ts

+5-1
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,12 @@ export enum HttpCode {
1313
* used in the HTTP response.
1414
*/
1515
export class HttpError extends Error {
16-
public constructor(message: string, public readonly status: HttpCode, public readonly details?: object) {
16+
public constructor(message: string, public readonly statusCode: HttpCode, public readonly details?: object) {
1717
super(message)
1818
this.name = this.constructor.name
1919
}
2020
}
21+
22+
export enum CookieKeys {
23+
Session = "code-server-session",
24+
}

‎src/node/http.ts

+2-2
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import * as net from "net"
66
import path from "path"
77
import qs from "qs"
88
import { Disposable } from "../common/emitter"
9-
import { HttpCode, HttpError } from "../common/http"
9+
import { CookieKeys, HttpCode, HttpError } from "../common/http"
1010
import { normalize } from "../common/util"
1111
import { AuthType, DefaultedArgs } from "./cli"
1212
import { version as codeServerVersion } from "./constants"
@@ -93,7 +93,7 @@ export const authenticated = async (req: express.Request): Promise<boolean> => {
9393
const passwordMethod = getPasswordMethod(hashedPasswordFromArgs)
9494
const isCookieValidArgs: IsCookieValidArgs = {
9595
passwordMethod,
96-
cookieKey: sanitizeString(req.cookies.key),
96+
cookieKey: sanitizeString(req.cookies[CookieKeys.Session]),
9797
passwordFromArgs: req.args.password || "",
9898
hashedPasswordFromArgs: req.args["hashed-password"],
9999
}

‎src/node/main.ts

+6-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { field, logger } from "@coder/logger"
2-
import * as os from "os"
32
import http from "http"
3+
import * as os from "os"
44
import path from "path"
55
import { Disposable } from "../common/emitter"
66
import { plural } from "../common/util"
@@ -37,8 +37,11 @@ export const runVsCodeCli = async (args: DefaultedArgs): Promise<void> => {
3737
try {
3838
await spawnCli({
3939
...args,
40-
// For some reason VS Code takes the port as a string.
41-
port: typeof args.port !== "undefined" ? args.port.toString() : undefined,
40+
/** Type casting. */
41+
"accept-server-license-terms": true,
42+
help: !!args.help,
43+
version: !!args.version,
44+
port: args.port?.toString(),
4245
})
4346
} catch (error: any) {
4447
logger.error("Got error from VS Code", error)

‎src/node/routes/login.ts

+4-7
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,11 @@ import { promises as fs } from "fs"
33
import { RateLimiter as Limiter } from "limiter"
44
import * as os from "os"
55
import * as path from "path"
6+
import { CookieKeys } from "../../common/http"
67
import { rootPath } from "../constants"
78
import { authenticated, getCookieDomain, redirect, replaceTemplates } from "../http"
89
import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString, escapeHtml } from "../util"
910

10-
export enum Cookie {
11-
Key = "key",
12-
}
13-
1411
// RateLimiter wraps around the limiter library for logins.
1512
// It allows 2 logins every minute plus 12 logins every hour.
1613
export class RateLimiter {
@@ -62,7 +59,7 @@ router.get("/", async (req, res) => {
6259
res.send(await getRoot(req))
6360
})
6461

65-
router.post("/", async (req, res) => {
62+
router.post<{}, string, { password: string; base?: string }, { to?: string }>("/", async (req, res) => {
6663
const password = sanitizeString(req.body.password)
6764
const hashedPasswordFromArgs = req.args["hashed-password"]
6865

@@ -87,13 +84,13 @@ router.post("/", async (req, res) => {
8784
if (isPasswordValid) {
8885
// The hash does not add any actual security but we do it for
8986
// obfuscation purposes (and as a side effect it handles escaping).
90-
res.cookie(Cookie.Key, hashedPassword, {
87+
res.cookie(CookieKeys.Session, hashedPassword, {
9188
domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
9289
// Browsers do not appear to allow cookies to be set relatively so we
9390
// need to get the root path from the browser since the proxy rewrites
9491
// it out of the path. Otherwise code-server instances hosted on
9592
// separate sub-paths will clobber each other.
96-
path: req.body.base ? path.posix.join(req.body.base, "..") : "/",
93+
path: req.body.base ? path.posix.join(req.body.base, "..", "/") : "/",
9794
sameSite: "lax",
9895
})
9996

‎src/node/routes/logout.ts

+9-5
Original file line numberDiff line numberDiff line change
@@ -1,17 +1,21 @@
11
import { Router } from "express"
2+
import { CookieKeys } from "../../common/http"
23
import { getCookieDomain, redirect } from "../http"
3-
import { Cookie } from "./login"
4+
5+
import { sanitizeString } from "../util"
46

57
export const router = Router()
68

7-
router.get("/", async (req, res) => {
9+
router.get<{}, undefined, undefined, { base?: string; to?: string }>("/", async (req, res) => {
10+
const path = sanitizeString(req.query.base) || "/"
11+
const to = sanitizeString(req.query.to) || "/"
12+
813
// Must use the *identical* properties used to set the cookie.
9-
res.clearCookie(Cookie.Key, {
14+
res.clearCookie(CookieKeys.Session, {
1015
domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
11-
path: req.query.base || "/",
16+
path: decodeURIComponent(path),
1217
sameSite: "lax",
1318
})
1419

15-
const to = (typeof req.query.to === "string" && req.query.to) || "/"
1620
return redirect(req, res, to, { to: undefined, base: undefined })
1721
})

‎src/node/routes/vscode.ts

+6-3
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ export class CodeServerRouteWrapper {
2424
const isAuthenticated = await authenticated(req)
2525

2626
if (!isAuthenticated) {
27-
return redirect(req, res, "login", {
27+
return redirect(req, res, "login/", {
2828
// req.baseUrl can be blank if already at the root.
2929
to: req.baseUrl && req.baseUrl !== "/" ? req.baseUrl : undefined,
3030
})
@@ -88,9 +88,12 @@ export class CodeServerRouteWrapper {
8888

8989
try {
9090
this._codeServerMain = await createVSServer(null, {
91-
connectionToken: "0000",
91+
"connection-token": "0000",
92+
"accept-server-license-terms": true,
9293
...args,
93-
// For some reason VS Code takes the port as a string.
94+
/** Type casting. */
95+
help: !!args.help,
96+
version: !!args.version,
9497
port: args.port?.toString(),
9598
})
9699
} catch (createServerError) {

‎src/node/util.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -321,7 +321,7 @@ export async function isCookieValid({
321321
* - greater than 0 characters
322322
* - trims whitespace
323323
*/
324-
export function sanitizeString(str: string): string {
324+
export function sanitizeString(str: unknown): string {
325325
// Very basic sanitization of string
326326
// Credit: https://stackoverflow.com/a/46719000/3015595
327327
return typeof str === "string" && str.trim().length > 0 ? str.trim() : ""

‎test/unit/common/http.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,7 @@ describe("http", () => {
1919
const httpError = new HttpError(message, HttpCode.BadRequest)
2020

2121
expect(httpError.message).toBe(message)
22-
expect(httpError.status).toBe(400)
22+
expect(httpError.statusCode).toBe(400)
2323
expect(httpError.details).toBeUndefined()
2424
})
2525
it("should have details if provided", () => {

‎test/utils/globalSetup.ts

+2-1
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
import { Cookie } from "playwright"
2+
import { CookieKeys } from "../../src/common/http"
23
import { hash } from "../../src/node/util"
34
import { PASSWORD, workspaceDir } from "./constants"
45
import { clean } from "./helpers"
@@ -27,7 +28,7 @@ export default async function () {
2728
domain: "localhost",
2829
expires: -1,
2930
httpOnly: false,
30-
name: "key",
31+
name: CookieKeys.Session,
3132
path: "/",
3233
sameSite: "Lax",
3334
secure: false,

‎vendor/package.json

+1-1
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,6 @@
77
"postinstall": "./postinstall.sh"
88
},
99
"devDependencies": {
10-
"code-oss-dev": "cdr/vscode#a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6"
10+
"code-oss-dev": "cdr/vscode#5e0c6f3b95ed032e62c49101ae502a46c62ef202"
1111
}
1212
}

‎vendor/yarn.lock

+2-2
Original file line numberDiff line numberDiff line change
@@ -296,9 +296,9 @@ clone-response@^1.0.2:
296296
dependencies:
297297
mimic-response "^1.0.0"
298298

299-
code-oss-dev@cdr/vscode#a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6:
299+
code-oss-dev@cdr/vscode#5e0c6f3b95ed032e62c49101ae502a46c62ef202:
300300
version "1.61.1"
301-
resolved "https://codeload.github.com/cdr/vscode/tar.gz/a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6"
301+
resolved "https://codeload.github.com/cdr/vscode/tar.gz/5e0c6f3b95ed032e62c49101ae502a46c62ef202"
302302
dependencies:
303303
"@microsoft/applicationinsights-web" "^2.6.4"
304304
"@vscode/sqlite3" "4.0.12"

0 commit comments

Comments
 (0)