OCram85/code-server
Archived
1
0
Fork 0
Code Pull Requests 2 Activity

node/routes: Fix error handling

Browse Source 
We should always send HTML if the user agent expects it.

If they do not, they should clearly indicate such via the Accept header.

Closes #2297
This commit is contained in:
Anmol Sethi 2020-11-13 15:32:47 -05:00
parent 7afa689285
commit 40a7c11ce3
No known key found for this signature in database
GPG Key ID: 8CEF1878FF10ADEB
2 changed files with 13 additions and 9 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/node/routes/domainProxy.ts
View File

@ -48,8 +48,8 @@ router.all("*", (req, res, next) => {
// Assume anything that explicitly accepts text/html is a user browsing a // Assume anything that explicitly accepts text/html is a user browsing a
// page (as opposed to an xhr request). Don't use `req.accepts()` since // page (as opposed to an xhr request). Don't use `req.accepts()` since
// *every* request that I've seen (in Firefox and Chromium at least) // *every* request that I've seen (in Firefox and Chromium at least)
// includes `*/*` making it always truthy. // includes `*/*` making it always truthy. Even for css/javascript.
if (typeof req.headers.accepts === "string" && req.headers.accepts.split(",").includes("text/html")) { if (req.headers.accept && req.headers.accept.includes("text/html")) {
// Let the login through. // Let the login through.
if (/\/login\/?/.test(req.path)) { if (/\/login\/?/.test(req.path)) {
return next() return next()

18
src/node/routes/index.ts
View File

@ -125,7 +125,7 @@ export const register = async (
throw new HttpError("Not Found", HttpCode.NotFound) throw new HttpError("Not Found", HttpCode.NotFound)
}) })
const errorHandler: express.ErrorRequestHandler = async (err, req, res) => { const errorHandler: express.ErrorRequestHandler = async (err, req, res, next) => {
if (err.code === "ENOENT" || err.code === "EISDIR") { if (err.code === "ENOENT" || err.code === "EISDIR") {
err.status = HttpCode.NotFound err.status = HttpCode.NotFound
} }
@ -133,12 +133,11 @@ export const register = async (
const status = err.status ?? err.statusCode ?? 500 const status = err.status ?? err.statusCode ?? 500
res.status(status) res.status(status)
if (req.accepts("application/json")) { // Assume anything that explicitly accepts text/html is a user browsing a
res.json({ // page (as opposed to an xhr request). Don't use `req.accepts()` since
error: err.message, // *every* request that I've seen (in Firefox and Chromium at least)
...(err.details || {}), // includes `*/*` making it always truthy. Even for css/javascript.
}) if (req.headers.accept && req.headers.accept.includes("text/html")) {
} else {
const resourcePath = path.resolve(rootPath, "src/browser/pages/error.html") const resourcePath = path.resolve(rootPath, "src/browser/pages/error.html")
res.set("Content-Type", getMediaMime(resourcePath)) res.set("Content-Type", getMediaMime(resourcePath))
const content = await fs.readFile(resourcePath, "utf8") const content = await fs.readFile(resourcePath, "utf8")
@ -148,6 +147,11 @@ export const register = async (
.replace(/{{ERROR_HEADER}}/g, status) .replace(/{{ERROR_HEADER}}/g, status)
.replace(/{{ERROR_BODY}}/g, err.message), .replace(/{{ERROR_BODY}}/g, err.message),
) )
} else {
res.json({
error: err.message,
...(err.details || {}),
})
} }
} }