Skip to content

Commit 5cebc2b

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 5cebc2b

File tree

2 files changed

+46
-27
lines changed

2 files changed

+46
-27
lines changed

src/node/http.ts

+32-20
Original file line numberDiff line numberDiff line change
@@ -323,61 +323,73 @@ 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+
// A missing host likely means the reverse proxy has not been configured to
359+
// forward the host which means we cannot perform the check. Emit a warning
360+
// so an admin can fix the issue.
361+
const host = getHost(req)
362+
if (typeof host === "undefined") {
363+
throw new Error("no host headers found")
353364
}
354365

366+
if (host !== origin) {
367+
throw new Error(`host "${host}" does not match origin "${origin}"`)
368+
}
369+
}
370+
371+
/**
372+
* Get the host from headers. It will be trimmed and lowercased.
373+
*/
374+
function getHost(req: express.Request): string | undefined {
355375
// Honor Forwarded if present.
356376
const forwardedRaw = getFirstHeader(req, "forwarded")
357377
if (forwardedRaw) {
358378
const parts = forwardedRaw.split(/[;,]/)
359379
for (let i = 0; i < parts.length; ++i) {
360380
const [key, value] = splitOnFirstEquals(parts[i])
361381
if (key.trim().toLowerCase() === "host" && value) {
362-
return origin === value.trim().toLowerCase()
382+
return value.trim().toLowerCase()
363383
}
364384
}
365385
}
366386

367387
// Honor X-Forwarded-Host if present.
368388
const xHost = getFirstHeader(req, "x-forwarded-host")
369389
if (xHost) {
370-
return origin === xHost.trim().toLowerCase()
390+
return xHost.trim().toLowerCase()
371391
}
372392

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.
376393
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()
394+
return host ? host.trim().toLowerCase() : undefined
383395
}

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)