Skip to content

Commit 92de2ae

Browse files
committed
Rewrite cookie path logic
Before we relied on the client to resolve the base given to it by the backend against the path. Instead have the client pass that information along so we can resolve it on the backend. This means the client has to do less work.
1 parent f70a32f commit 92de2ae

File tree

7 files changed

+47
-28
lines changed

7 files changed

+47
-28
lines changed

src/browser/pages/login.html

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,8 @@ <h1 class="main">Welcome to code-server</h1>
3030
<div class="content">
3131
<form class="login-form" method="post">
3232
<input class="user" type="text" autocomplete="username" />
33-
<input id="base" type="hidden" name="base" value="/" />
33+
<input id="base" type="hidden" name="base" value="{{BASE}}" />
34+
<input id="href" type="hidden" name="href" value="" />
3435
<div class="field">
3536
<input
3637
required
@@ -51,9 +52,9 @@ <h1 class="main">Welcome to code-server</h1>
5152
<script>
5253
// Inform the backend about the path since the proxy might have rewritten
5354
// it out of the headers and cookies must be set with absolute paths.
54-
const el = document.getElementById("base")
55+
const el = document.getElementById("href")
5556
if (el) {
56-
el.value = window.location.pathname
57+
el.value = location.href
5758
}
5859
</script>
5960
</body>

src/node/http.ts

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,3 +255,36 @@ export function disposer(server: http.Server): Disposable["dispose"] {
255255
})
256256
}
257257
}
258+
259+
/**
260+
* Get the options for setting a cookie. The options must be identical for
261+
* setting and unsetting cookies otherwise they are considered separate.
262+
*/
263+
export const getCookieOptions = (req: express.Request): express.CookieOptions => {
264+
// Normally we set paths relatively. However browsers do not appear to allow
265+
// cookies to be set relatively which means we need an absolute path. We
266+
// cannot be guaranteed we know the path since a reverse proxy might have
267+
// rewritten it. That means we need to get the path from the frontend.
268+
269+
// The reason we need to set the path (as opposed to defaulting to /) is to
270+
// avoid code-server instances on different sub-paths clobbering each other or
271+
// from accessing each other's tokens (and to prevent other services from
272+
// accessing code-server's tokens).
273+
274+
// When logging in or out the request must include the href (the full current
275+
// URL of that page) and the relative path to the root as given to it by the
276+
// backend. Using these two we can determine the true absolute root.
277+
let domain = req.headers.host || ""
278+
let pathname = "/"
279+
const href = req.query.href || req.body.href
280+
if (href) {
281+
const url = new URL(req.query.base || req.body.base || "/", href)
282+
domain = url.host
283+
pathname = url.pathname
284+
}
285+
return {
286+
domain: getCookieDomain(domain, req.args["proxy-domain"]),
287+
path: normalize(pathname) || "/",
288+
sameSite: "lax",
289+
}
290+
}

src/node/routes/login.ts

Lines changed: 2 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ import * as os from "os"
55
import * as path from "path"
66
import { CookieKeys } from "../../common/http"
77
import { rootPath } from "../constants"
8-
import { authenticated, getCookieDomain, redirect, replaceTemplates } from "../http"
8+
import { authenticated, getCookieOptions, redirect, replaceTemplates } from "../http"
99
import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString, escapeHtml } from "../util"
1010

1111
// RateLimiter wraps around the limiter library for logins.
@@ -84,15 +84,7 @@ router.post<{}, string, { password: string; base?: string }, { to?: string }>("/
8484
if (isPasswordValid) {
8585
// The hash does not add any actual security but we do it for
8686
// obfuscation purposes (and as a side effect it handles escaping).
87-
res.cookie(CookieKeys.Session, hashedPassword, {
88-
domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
89-
// Browsers do not appear to allow cookies to be set relatively so we
90-
// need to get the root path from the browser since the proxy rewrites
91-
// it out of the path. Otherwise code-server instances hosted on
92-
// separate sub-paths will clobber each other.
93-
path: req.body.base ? path.posix.join(req.body.base, "..", "/") : "/",
94-
sameSite: "lax",
95-
})
87+
res.cookie(CookieKeys.Session, hashedPassword, getCookieOptions(req))
9688

9789
const to = (typeof req.query.to === "string" && req.query.to) || "/"
9890
return redirect(req, res, to, { to: undefined })

src/node/routes/logout.ts

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1,21 +1,14 @@
11
import { Router } from "express"
22
import { CookieKeys } from "../../common/http"
3-
import { getCookieDomain, redirect } from "../http"
4-
3+
import { getCookieOptions, redirect } from "../http"
54
import { sanitizeString } from "../util"
65

76
export const router = Router()
87

98
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-
139
// Must use the *identical* properties used to set the cookie.
14-
res.clearCookie(CookieKeys.Session, {
15-
domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
16-
path: decodeURIComponent(path),
17-
sameSite: "lax",
18-
})
10+
res.clearCookie(CookieKeys.Session, getCookieOptions(req))
1911

20-
return redirect(req, res, to, { to: undefined, base: undefined })
12+
const to = sanitizeString(req.query.to) || "/"
13+
return redirect(req, res, to, { to: undefined, base: undefined, href: undefined })
2114
})

src/node/util.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -324,7 +324,7 @@ export async function isCookieValid({
324324
export function sanitizeString(str: unknown): string {
325325
// Very basic sanitization of string
326326
// Credit: https://stackoverflow.com/a/46719000/3015595
327-
return typeof str === "string" && str.trim().length > 0 ? str.trim() : ""
327+
return typeof str === "string" ? str.trim() : ""
328328
}
329329

330330
const mimeTypes: { [key: string]: string } = {

vendor/package.json

Lines changed: 1 addition & 1 deletion
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#c2a251c6afaa13fbebf97fcd8a68192f8cf46031"
10+
"code-oss-dev": "cdr/vscode#478224aa345e9541f2427b30142dd13ee7e14d39"
1111
}
1212
}

vendor/yarn.lock

Lines changed: 2 additions & 2 deletions
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#c2a251c6afaa13fbebf97fcd8a68192f8cf46031:
299+
code-oss-dev@cdr/vscode#478224aa345e9541f2427b30142dd13ee7e14d39:
300300
version "1.61.1"
301-
resolved "https://codeload.github.com/cdr/vscode/tar.gz/c2a251c6afaa13fbebf97fcd8a68192f8cf46031"
301+
resolved "https://codeload.github.com/cdr/vscode/tar.gz/478224aa345e9541f2427b30142dd13ee7e14d39"
302302
dependencies:
303303
"@microsoft/applicationinsights-web" "^2.6.4"
304304
"@vscode/sqlite3" "4.0.12"

0 commit comments

Comments
 (0)