Skip to content

Commit

Permalink
feat: treat unhandled interceptor exceptions as 500 responses (#532)
Browse files Browse the repository at this point in the history
  • Loading branch information
kettanaito authored Apr 16, 2024
1 parent 0db6966 commit 61f9459
Show file tree
Hide file tree
Showing 15 changed files with 355 additions and 92 deletions.
42 changes: 34 additions & 8 deletions src/interceptors/ClientRequest/NodeClientRequest.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import { toInteractiveRequest } from '../../utils/toInteractiveRequest'
import { emitAsync } from '../../utils/emitAsync'
import { getRawFetchHeaders } from '../../utils/getRawFetchHeaders'
import { isPropertyAccessible } from '../../utils/isPropertyAccessible'
import { isNodeLikeError } from '../../utils/isNodeLikeError'
import { INTERNAL_REQUEST_ID_HEADER_NAME } from '../../Interceptor'
import { createRequestId } from '../../createRequestId'

Expand Down Expand Up @@ -259,16 +260,36 @@ export class NodeClientRequest extends ClientRequest {
}
}

// Halt the request whenever the resolver throws an exception.
if (resolverResult.error) {
this.logger.info(
'encountered resolver exception, aborting request...',
'unhandled resolver exception, coercing to an error response...',
resolverResult.error
)

this.destroyed = true
this.emit('error', resolverResult.error)
this.terminate()
// Allow throwing Node.js-like errors, like connection rejection errors.
// Treat them as request errors.
if (isNodeLikeError(resolverResult.error)) {
this.errorWith(resolverResult.error)
} else {
// Coerce unhandled exceptions in the "request" listeners
// as 500 responses.
this.respondWith(
new Response(
JSON.stringify({
name: resolverResult.error.name,
message: resolverResult.error.message,
stack: resolverResult.error.stack,
}),
{
status: 500,
statusText: 'Unhandled Exception',
headers: {
'Content-Type': 'application/json',
},
}
)
)
}

return this
}
Expand Down Expand Up @@ -300,15 +321,14 @@ export class NodeClientRequest extends ClientRequest {
mockedResponse.type === 'error'
) {
this.logger.info(
'received network error response, aborting request...'
'received network error response, erroring request...'
)

/**
* There is no standardized error format for network errors
* in Node.js. Instead, emit a generic TypeError.
*/
this.emit('error', new TypeError('Network error'))
this.terminate()
this.errorWith(new TypeError('Network error'))

return this
}
Expand Down Expand Up @@ -581,6 +601,12 @@ export class NodeClientRequest extends ClientRequest {
})
}

private errorWith(error: Error): void {
this.destroyed = true
this.emit('error', error)
this.terminate()
}

/**
* Terminates a pending request.
*/
Expand Down
21 changes: 20 additions & 1 deletion src/interceptors/XMLHttpRequest/XMLHttpRequestProxy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -94,12 +94,31 @@ export function createXMLHttpRequestProxy({
resolverResult.error
)

// Treat unhandled exceptions in the "request" listener
// as 500 server errors.
xhrRequestController.respondWith(
new Response(
JSON.stringify({
name: resolverResult.error.name,
message: resolverResult.error.message,
stack: resolverResult.error.stack,
}),
{
status: 500,
statusText: 'Unhandled Exception',
headers: {
'Content-Type': 'application/json',
},
}
)
)

/**
* @todo Consider forwarding this error to the stderr as well
* since not all consumers are expecting to handle errors.
* If they don't, this error will be swallowed.
*/
xhrRequestController.errorWith(resolverResult.error)
// xhrRequestController.errorWith(resolverResult.error)
return
}

Expand Down
18 changes: 17 additions & 1 deletion src/interceptors/fetch/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,23 @@ export class FetchInterceptor extends Interceptor<HttpRequestEventMap> {
}

if (resolverResult.error) {
return Promise.reject(createNetworkError(resolverResult.error))
// Treat unhandled exceptions from the "request" listeners
// as 500 errors from the server. Fetch API doesn't respect
// Node.js internal errors so no special treatment for those.
return new Response(
JSON.stringify({
name: resolverResult.error.name,
message: resolverResult.error.message,
stack: resolverResult.error.stack,
}),
{
status: 500,
statusText: 'Unhandled Exception',
headers: {
'Content-Type': 'application/json',
},
}
)
}

const mockedResponse = resolverResult.data
Expand Down
13 changes: 13 additions & 0 deletions src/utils/isNodeLikeError.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
export function isNodeLikeError(
error: unknown
): error is NodeJS.ErrnoException {
if (error == null) {
return false
}

if (!(error instanceof Error)) {
return false
}

return 'code' in error && 'errno' in error
}
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const httpServer = new HttpServer((app) => {
app.get('/resource', (req, res) => {
res.send('hello')
})
app.get('/error', (req, res) => {
app.get('/error-response', (req, res) => {
res.status(500).send('Internal Server Error')
})
app.get('/exception', (req, res) => {
Expand All @@ -26,10 +26,14 @@ beforeAll(async () => {
interceptor.on('request', ({ request }) => {
switch (true) {
case request.url.endsWith('/exception'): {
throw new Error('Network error')
throw new Error('Custom error')
}

case request.url.endsWith('/error'): {
case request.url.endsWith('/network-error'): {
return request.respondWith(Response.error())
}

case request.url.endsWith('/error-response'): {
return request.respondWith(
new Response('Internal Server Error', { status: 500 })
)
Expand Down Expand Up @@ -76,8 +80,8 @@ it.each<[name: string, getUrl: () => string]>([
)

it.each<[name: string, getUrl: () => string]>([
['passthrough', () => httpServer.https.url('/error')],
['mocked', () => 'http://localhost/error'],
['passthrough', () => httpServer.https.url('/error-response')],
['mocked', () => 'http://localhost/error-response'],
])(
`does not fail when unsetting event handlers for a %s error response`,
async (_, getUrl) => {
Expand All @@ -103,8 +107,8 @@ it.each<[name: string, getUrl: () => string]>([
)

it.each<[name: string, getUrl: () => string]>([
['passthrough', () => httpServer.https.url('/exception')],
['mocked', () => 'http://localhost/exception'],
['passthrough', () => httpServer.https.url('/network-error')],
['mocked', () => 'http://localhost/network-error'],
])(
`does not fail when unsetting event handlers for a %s request error`,
async (_, getUrl) => {
Expand All @@ -128,3 +132,28 @@ it.each<[name: string, getUrl: () => string]>([
expect(request.responseText).toBe('')
}
)

it('does not fail when unsetting event handlers during unhandled exception in the interceptor', async () => {
const request = await createXMLHttpRequest((request) => {
request.responseType = 'json'
request.open('GET', httpServer.https.url('/exception'))

request.onreadystatechange = null
request.onloadstart = null
request.onprogress = null
request.onload = null
request.onloadend = null
request.ontimeout = null

request.send()
})

expect(request.readyState).toBe(4)
expect(request.status).toBe(500)
expect(request.statusText).toBe('Unhandled Exception')
expect(request.response).toEqual({
name: 'Error',
message: 'Custom error',
stack: expect.any(String),
})
})
82 changes: 75 additions & 7 deletions test/modules/XMLHttpRequest/compliance/xhr-event-handlers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ const httpServer = new HttpServer((app) => {
app.get('/resource', (req, res) => {
res.send('hello')
})
app.get('/error', (req, res) => {
app.get('/error-response', (req, res) => {
res.status(500).send('Internal Server Error')
})
app.get('/exception', (req, res) => {
Expand All @@ -26,10 +26,14 @@ beforeAll(async () => {
interceptor.on('request', ({ request }) => {
switch (true) {
case request.url.endsWith('/exception'): {
throw new Error('Network error')
throw new Error('Custom error')
}

case request.url.endsWith('/error'): {
case request.url.endsWith('/network-error'): {
return request.respondWith(Response.error())
}

case request.url.endsWith('/error-response'): {
return request.respondWith(
new Response('Internal Server Error', { status: 500 })
)
Expand Down Expand Up @@ -115,8 +119,8 @@ it.each<[name: string, getUrl: () => string]>([
)

it.each<[name: string, getUrl: () => string]>([
['passthrough', () => httpServer.https.url('/error')],
['mocked', () => 'http://localhost/error'],
['passthrough', () => httpServer.https.url('/error-response')],
['mocked', () => 'http://localhost/error-response'],
])(`dispatches relevant events upon a %s error response`, async (_, getUrl) => {
const url = getUrl()

Expand Down Expand Up @@ -178,8 +182,8 @@ it.each<[name: string, getUrl: () => string]>([
})

it.each<[name: string, getUrl: () => string]>([
['passthrough', () => httpServer.https.url('/exception')],
['mocked', () => 'http://localhost/exception'],
['passthrough', () => httpServer.https.url('/network-error')],
['mocked', () => 'http://localhost/network-error'],
])(`dispatches relevant events upon a %s request error`, async (_, getUrl) => {
const url = getUrl()

Expand Down Expand Up @@ -235,3 +239,67 @@ it.each<[name: string, getUrl: () => string]>([
expect(loadListener).not.toHaveBeenCalled()
expect(loadEndListener).toHaveBeenCalledTimes(1)
})

it('dispatched relevant events upon an unhandled exception in the interceptor', async () => {
const onReadyStateChangeHandler = vi.fn(function (this: XMLHttpRequest) {
return this.readyState
})
const onLoadStartHandler = vi.fn()
const onProgressHandler = vi.fn()
const onLoadHandler = vi.fn()
const onLoadEndHandler = vi.fn()

const onReadyStateChangeListener = vi.fn(function (this: XMLHttpRequest) {
return this.readyState
})
const loadStartListener = vi.fn()
const progressListener = vi.fn()
const loadListener = vi.fn()
const loadEndListener = vi.fn()

const request = await createXMLHttpRequest((request) => {
request.responseType = 'json'
request.open('GET', httpServer.https.url('/exception'))

request.onreadystatechange = onReadyStateChangeHandler
request.onloadstart = onLoadStartHandler
request.onprogress = onProgressHandler
request.onload = onLoadHandler
request.onloadend = onLoadEndHandler

request.addEventListener('readystatechange', onReadyStateChangeListener)
request.addEventListener('loadstart', loadStartListener)
request.addEventListener('progress', progressListener)
request.addEventListener('load', loadListener)
request.addEventListener('loadend', loadEndListener)

request.send()
})

expect(request.readyState).toBe(4)
expect(request.status).toBe(500)
expect(request.statusText).toBe('Unhandled Exception')
expect(request.response).toEqual({
name: 'Error',
message: 'Custom error',
stack: expect.any(String),
})

expect(onReadyStateChangeHandler).toHaveBeenCalledTimes(3)
expect(onReadyStateChangeHandler).toHaveNthReturnedWith(1, 2)
expect(onReadyStateChangeHandler).toHaveNthReturnedWith(2, 3)
expect(onReadyStateChangeHandler).toHaveNthReturnedWith(3, 4)
expect(onLoadStartHandler).toHaveBeenCalledTimes(1)
expect(onProgressHandler).toHaveBeenCalledTimes(1)
expect(onLoadHandler).toHaveBeenCalledTimes(1)
expect(onLoadEndHandler).toHaveBeenCalledTimes(1)

expect(onReadyStateChangeListener).toHaveBeenCalledTimes(3)
expect(onReadyStateChangeListener).toHaveNthReturnedWith(1, 2)
expect(onReadyStateChangeListener).toHaveNthReturnedWith(2, 3)
expect(onReadyStateChangeListener).toHaveNthReturnedWith(3, 4)
expect(loadStartListener).toHaveBeenCalledTimes(1)
expect(progressListener).toHaveBeenCalledTimes(1)
expect(loadListener).toHaveBeenCalledTimes(1)
expect(loadEndListener).toHaveBeenCalledTimes(1)
})
Loading

0 comments on commit 61f9459

Please sign in to comment.