Skip to content

Commit 2bac577

Browse files
committed
Add debug log for origin check
Extracted host detection into a separate function to avoid multiple log lines on each return and went with a thrown error to consolidate the common log text.
1 parent c32a31d commit 2bac577

File tree

2 files changed

+48
-27
lines changed

2 files changed

+48
-27
lines changed

src/node/http.ts

+34-20
Original file line numberDiff line numberDiff line change
@@ -323,61 +323,75 @@ function getFirstHeader(req: http.IncomingMessage, headerName: string): string |
323323
}
324324

325325
/**
326-
* Throw an error if origin checks fail. Call `next` if provided.
326+
* Throw a forbidden error if origin checks fail. Call `next` if provided.
327327
*/
328328
export function ensureOrigin(req: express.Request, _?: express.Response, next?: express.NextFunction): void {
329-
if (!authenticateOrigin(req)) {
329+
try {
330+
authenticateOrigin(req)
331+
if (next) {
332+
next()
333+
}
334+
} catch (error) {
335+
logger.debug(`${error instanceof Error ? error.message : error}; blocking request to ${req.originalUrl}`)
330336
throw new HttpError("Forbidden", HttpCode.Forbidden)
331337
}
332-
if (next) {
333-
next()
334-
}
335338
}
336339

337340
/**
338-
* Authenticate the request origin against the host.
341+
* Authenticate the request origin against the host. Throw if invalid.
339342
*/
340-
export function authenticateOrigin(req: express.Request): boolean {
343+
export function authenticateOrigin(req: express.Request): void {
341344
// A missing origin probably means the source is non-browser. Not sure we
342345
// have a use case for this but let it through.
343346
const originRaw = getFirstHeader(req, "origin")
344347
if (!originRaw) {
345-
return true
348+
return
346349
}
347350

348351
let origin: string
349352
try {
350353
origin = new URL(originRaw).host.trim().toLowerCase()
351354
} catch (error) {
352-
return false // Malformed URL.
355+
throw new Error(`unable to parse malformed origin "${originRaw}"`)
356+
}
357+
358+
const host = getHost(req)
359+
if (typeof host === "undefined") {
360+
// A missing host likely means the reverse proxy has not been configured to
361+
// forward the host which means we cannot perform the check. Emit an error
362+
// so an admin can fix the issue.
363+
logger.error("No host headers found")
364+
logger.error("Are you behind a reverse proxy that does not forward the host?")
365+
throw new Error("no host headers found")
353366
}
354367

368+
if (host !== origin) {
369+
throw new Error(`host "${host}" does not match origin "${origin}"`)
370+
}
371+
}
372+
373+
/**
374+
* Get the host from headers. It will be trimmed and lowercased.
375+
*/
376+
function getHost(req: express.Request): string | undefined {
355377
// Honor Forwarded if present.
356378
const forwardedRaw = getFirstHeader(req, "forwarded")
357379
if (forwardedRaw) {
358380
const parts = forwardedRaw.split(/[;,]/)
359381
for (let i = 0; i < parts.length; ++i) {
360382
const [key, value] = splitOnFirstEquals(parts[i])
361383
if (key.trim().toLowerCase() === "host" && value) {
362-
return origin === value.trim().toLowerCase()
384+
return value.trim().toLowerCase()
363385
}
364386
}
365387
}
366388

367389
// Honor X-Forwarded-Host if present.
368390
const xHost = getFirstHeader(req, "x-forwarded-host")
369391
if (xHost) {
370-
return origin === xHost.trim().toLowerCase()
392+
return xHost.trim().toLowerCase()
371393
}
372394

373-
// A missing host likely means the reverse proxy has not been configured to
374-
// forward the host which means we cannot perform the check. Emit a warning
375-
// so an admin can fix the issue.
376395
const host = getFirstHeader(req, "host")
377-
if (!host) {
378-
logger.warn(`no host headers found; blocking request to ${req.originalUrl}`)
379-
return false
380-
}
381-
382-
return origin === host.trim().toLowerCase()
396+
return host ? host.trim().toLowerCase() : undefined
383397
}

test/unit/node/http.test.ts

+14-7
Original file line numberDiff line numberDiff line change
@@ -24,32 +24,35 @@ describe("http", () => {
2424
{
2525
origin: "",
2626
host: "",
27-
expected: true,
2827
},
2928
{
3029
origin: "http://localhost:8080",
3130
host: "",
32-
expected: false,
31+
expected: "no host headers",
32+
},
33+
{
34+
origin: "http://localhost:8080",
35+
host: " ",
36+
expected: "does not match",
3337
},
3438
{
3539
origin: "http://localhost:8080",
3640
host: "localhost:8080",
37-
expected: true,
3841
},
3942
{
4043
origin: "http://localhost:8080",
4144
host: "localhost:8081",
42-
expected: false,
45+
expected: "does not match",
4346
},
4447
{
4548
origin: "localhost:8080",
4649
host: "localhost:8080",
47-
expected: false, // Gets parsed as host: localhost and path: 8080.
50+
expected: "does not match", // Gets parsed as host: localhost and path: 8080.
4851
},
4952
{
5053
origin: "test.org",
5154
host: "localhost:8080",
52-
expected: false, // Parsing fails completely.
55+
expected: "malformed", // Parsing fails completely.
5356
},
5457
].forEach((test) => {
5558
;[
@@ -67,7 +70,11 @@ describe("http", () => {
6770
[key]: value,
6871
},
6972
})
70-
expect(http.authenticateOrigin(req)).toBe(test.expected)
73+
if (typeof test.expected === "string") {
74+
expect(() => http.authenticateOrigin(req)).toThrow(test.expected)
75+
} else {
76+
expect(() => http.authenticateOrigin(req)).not.toThrow()
77+
}
7178
})
7279
})
7380
})

0 commit comments

Comments
 (0)