Skip to content

Proxy path fixes #4548

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
Dec 2, 2021
Merged

Proxy path fixes #4548

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9e2c7a7
Fix issue where HTTP error status codes are not read.
GirlBossRush Nov 24, 2021
59db017
Fix issues surrounding sessions when accessed from a proxy.
GirlBossRush Nov 24, 2021
fcfb87d
Bump vendor.
GirlBossRush Nov 24, 2021
5adc97d
Update tests.
GirlBossRush Nov 24, 2021
ba87ecc
Fix issue where tests lack cookie key.
GirlBossRush Nov 25, 2021
5e046f7
Merge branch 'main' into proxy-path-fixes
code-asher Dec 1, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/common/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,12 @@ export enum HttpCode {
* used in the HTTP response.
*/
export class HttpError extends Error {
public constructor(message: string, public readonly status: HttpCode, public readonly details?: object) {
public constructor(message: string, public readonly statusCode: HttpCode, public readonly details?: object) {
super(message)
this.name = this.constructor.name
}
}

export enum CookieKeys {
Session = "code-server-session",
}
4 changes: 2 additions & 2 deletions src/node/http.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import * as net from "net"
import path from "path"
import qs from "qs"
import { Disposable } from "../common/emitter"
import { HttpCode, HttpError } from "../common/http"
import { CookieKeys, HttpCode, HttpError } from "../common/http"
import { normalize } from "../common/util"
import { AuthType, DefaultedArgs } from "./cli"
import { version as codeServerVersion } from "./constants"
Expand Down Expand Up @@ -93,7 +93,7 @@ export const authenticated = async (req: express.Request): Promise<boolean> => {
const passwordMethod = getPasswordMethod(hashedPasswordFromArgs)
const isCookieValidArgs: IsCookieValidArgs = {
passwordMethod,
cookieKey: sanitizeString(req.cookies.key),
cookieKey: sanitizeString(req.cookies[CookieKeys.Session]),
passwordFromArgs: req.args.password || "",
hashedPasswordFromArgs: req.args["hashed-password"],
}
Expand Down
9 changes: 6 additions & 3 deletions src/node/main.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { field, logger } from "@coder/logger"
import * as os from "os"
import http from "http"
import * as os from "os"
import path from "path"
import { Disposable } from "../common/emitter"
import { plural } from "../common/util"
Expand Down Expand Up @@ -37,8 +37,11 @@ export const runVsCodeCli = async (args: DefaultedArgs): Promise<void> => {
try {
await spawnCli({
...args,
// For some reason VS Code takes the port as a string.
port: typeof args.port !== "undefined" ? args.port.toString() : undefined,
/** Type casting. */
"accept-server-license-terms": true,
help: !!args.help,
version: !!args.version,
port: args.port?.toString(),
})
} catch (error: any) {
logger.error("Got error from VS Code", error)
Expand Down
11 changes: 4 additions & 7 deletions src/node/routes/login.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,11 @@ import { promises as fs } from "fs"
import { RateLimiter as Limiter } from "limiter"
import * as os from "os"
import * as path from "path"
import { CookieKeys } from "../../common/http"
import { rootPath } from "../constants"
import { authenticated, getCookieDomain, redirect, replaceTemplates } from "../http"
import { getPasswordMethod, handlePasswordValidation, humanPath, sanitizeString, escapeHtml } from "../util"

export enum Cookie {
Key = "key",
}

// RateLimiter wraps around the limiter library for logins.
// It allows 2 logins every minute plus 12 logins every hour.
export class RateLimiter {
Expand Down Expand Up @@ -62,7 +59,7 @@ router.get("/", async (req, res) => {
res.send(await getRoot(req))
})

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Previously this would assign the cookie to a resource path instead of a route path (e.g. /code instead of /code/). From what I’ve researched, there’s a number of opinions in both camps. I believe that since we’re now leaning so heavily on relative routes in VS Code, there’s little we can do to avoid this aside from convenient redirects and providing guidance for users.

  • code-server may be accessed either at the root or at /vscode with no slash
  • If you’re using a proxy that includes a path, it must end in a slash
  • If you’d like to omit the slash with your proxy, this must be configured on your end

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you’d like to omit the slash with your proxy, this must be configured on your end

I feel like this is a fair tradeoff but I didn't use proxies with code-server before so can't fully speak to that UX. @code-asher what are your thoughts?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of putting the cookie at /code/ instead of /code? The current behavior works for both cases while the new behavior breaks for the no trailing slash case (I think?) and I am not sure what we would be gaining in exchange for making authentication more brittle.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue with omitting the trailing slash from the cookie is that logging out from any arbitrary path requires that we send that very same path in the logout request. Trailing slashes denote the depth and access of a cookie, and while the spec is pretty forgiving on cookies to omit it, we can’t identify what part of the URL is where authenticated “starts” without knowing if the user first authenticated from / or /code. Both share the same path base / while adding a trailing slash defines cookies from /code/.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah so a cookie set to the path /code actually scopes the cookie to /? That does seem like a good reason to add the trailing slash.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need the depth either way right?

For example assuming the base path is /apps/coder/code-server/ when we log out from /apps/coder/code-server/vscode/ we now get redirected to /apps/coder/code-server/logout?base=/apps/coder/code-server/vscode/ and from there we need to set the cookie path using path.posix.join(base, "..") or path.posix.join(base, "..", "/") (only difference is the trailing slash).

If we log out from /apps/coder/code-server/ then we need to do the same thing in both cases except without a ... If there was some future route like /apps/coder/code-server/my/new/route/ we would need ../../.. in both cases.

Maybe there is some way we can get rid of needing to know what the current depth is? Hmmmm... 🤔

In previous versions of code-server the way we made this work was to pass the relative path to the root to the frontend (so in the three examples above it would be .., ., and ../../.. respectively). The client then resolved this against its current URL (in all three cases it would resolve to /apps/coder/code-server) and then we would redirect to logout?base=/apps/coder/code-server and we could set the path directly to that query variable since it was already resolved to the root.

Copy link
Member

@code-asher code-asher Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe where we pass things like logoutEndpoint we can also include the current route (from which we can derive the depth) so we can then derive the correct base.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To summarize, @code-asher you're saying this change would break current behavior?

I think you and I will need to test and verify this ourselves. If it does lead to a breaking change, we should see what our options are like you said.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sort of. This is mostly academic right now since you cannot host code-server at a base path without a trailing slash anyway because of the way browsers resolve relative paths (i.e. /code does not work because we would need the asset paths to be ./code/path/to/asset which means we need to get code from somewhere which means we need to ask the user for the base path or we need to inject the asset paths on the client). Current code-server does not work without a trailing slash (on the base path, it does work on our own routes like /vscode of course---I think I misspoke about this in Slack the other day so hopefully this clarifies).

There is one area we could potentially be breaking which is if existing users have a cookie at /code and we change the logic to use /code/ then they can no longer log out. But since we are also changing the cookie name that point is also moot.

I do still think we should avoid an arbitrary restriction though. If one day we decide to support a slash-less base path it would be nice if the cookie logic already works. So it is really just more about leaving support as broad as possible as long as there are no downsides in doing so.

Copy link
Member

@code-asher code-asher Dec 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is just this login portion though. My comments about the logout stuff are still relevant though and I think need to be implemented (see other comment for repro).

sameSite: "lax",
})

Expand Down
14 changes: 9 additions & 5 deletions src/node/routes/logout.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,17 +1,21 @@
import { Router } from "express"
import { CookieKeys } from "../../common/http"
import { getCookieDomain, redirect } from "../http"
import { Cookie } from "./login"

import { sanitizeString } from "../util"

export const router = Router()

router.get("/", async (req, res) => {
router.get<{}, undefined, undefined, { base?: string; to?: string }>("/", async (req, res) => {
const path = sanitizeString(req.query.base) || "/"
const to = sanitizeString(req.query.to) || "/"

// Must use the *identical* properties used to set the cookie.
res.clearCookie(Cookie.Key, {
res.clearCookie(CookieKeys.Session, {
domain: getCookieDomain(req.headers.host || "", req.args["proxy-domain"]),
path: req.query.base || "/",
path: decodeURIComponent(path),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is encoded on the VS Code side:

https://github.com/cdr/vscode/blob/5e0c6f3b95ed032e62c49101ae502a46c62ef202/src/vs/workbench/browser/client.ts#L184-L197

Copy link
Member

@code-asher code-asher Nov 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this path has to be the same as the other cookie in order to overwrite it? So I think we will need to apply the same logic here we did in the login, namely the whole path.posix.join() bit.

I suppose we will want .. if browsing at /vscode/ or /vscode and can use it as-is otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm guessing we'll need to test that to verify. If you're correct, I think one of us will have to implement that then @code-asher. Do you want to or do you want me to?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I think if you go to http://localhost:8080/vscode/ and click "log out" then it will (un)set the cookie at /vscode/ which will not log you out since the cookie would have been initially set at /.

If you get to it before I do feel free but otherwise I will take it on!

sameSite: "lax",
})

const to = (typeof req.query.to === "string" && req.query.to) || "/"
return redirect(req, res, to, { to: undefined, base: undefined })
})
9 changes: 6 additions & 3 deletions src/node/routes/vscode.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ export class CodeServerRouteWrapper {
const isAuthenticated = await authenticated(req)

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

try {
this._codeServerMain = await createVSServer(null, {
connectionToken: "0000",
"connection-token": "0000",
"accept-server-license-terms": true,
...args,
// For some reason VS Code takes the port as a string.
/** Type casting. */
help: !!args.help,
version: !!args.version,
port: args.port?.toString(),
})
} catch (createServerError) {
Expand Down
2 changes: 1 addition & 1 deletion src/node/util.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,7 @@ export async function isCookieValid({
* - greater than 0 characters
* - trims whitespace
*/
export function sanitizeString(str: string): string {
export function sanitizeString(str: unknown): string {
// Very basic sanitization of string
// Credit: https://stackoverflow.com/a/46719000/3015595
return typeof str === "string" && str.trim().length > 0 ? str.trim() : ""
Expand Down
2 changes: 1 addition & 1 deletion test/unit/common/http.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe("http", () => {
const httpError = new HttpError(message, HttpCode.BadRequest)

expect(httpError.message).toBe(message)
expect(httpError.status).toBe(400)
expect(httpError.statusCode).toBe(400)
expect(httpError.details).toBeUndefined()
})
it("should have details if provided", () => {
Expand Down
3 changes: 2 additions & 1 deletion test/utils/globalSetup.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { Cookie } from "playwright"
import { CookieKeys } from "../../src/common/http"
import { hash } from "../../src/node/util"
import { PASSWORD, workspaceDir } from "./constants"
import { clean } from "./helpers"
Expand Down Expand Up @@ -27,7 +28,7 @@ export default async function () {
domain: "localhost",
expires: -1,
httpOnly: false,
name: "key",
name: CookieKeys.Session,
path: "/",
sameSite: "Lax",
secure: false,
Expand Down
2 changes: 1 addition & 1 deletion vendor/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@
"postinstall": "./postinstall.sh"
},
"devDependencies": {
"code-oss-dev": "cdr/vscode#a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6"
"code-oss-dev": "cdr/vscode#5e0c6f3b95ed032e62c49101ae502a46c62ef202"
}
}
4 changes: 2 additions & 2 deletions vendor/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -296,9 +296,9 @@ clone-response@^1.0.2:
dependencies:
mimic-response "^1.0.0"

code-oss-dev@cdr/vscode#a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6:
code-oss-dev@cdr/vscode#5e0c6f3b95ed032e62c49101ae502a46c62ef202:
version "1.61.1"
resolved "https://codeload.github.com/cdr/vscode/tar.gz/a1d3f915454bb88e508c269a3c5bafb3cfa6f9f6"
resolved "https://codeload.github.com/cdr/vscode/tar.gz/5e0c6f3b95ed032e62c49101ae502a46c62ef202"
dependencies:
"@microsoft/applicationinsights-web" "^2.6.4"
"@vscode/sqlite3" "4.0.12"
Expand Down