From 3fb6771ea6747a76c82d03516d3327d8368954fb Mon Sep 17 00:00:00 2001 From: Jay McDoniel Date: Fri, 12 Aug 2022 09:25:17 -0700 Subject: [PATCH 1/5] fix: add a check if the res is destroyed before sending response In the case of using `curl` or a similar tool on the command line, or if the client decides to end the request for a streamed file early without the check we would end up causing a server crash. Now, we will just not send the response as the client has already decided how to move on. fix: #10105 --- packages/common/file-stream/streamable-file.ts | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/packages/common/file-stream/streamable-file.ts b/packages/common/file-stream/streamable-file.ts index f57b857ee69..dae63beda01 100644 --- a/packages/common/file-stream/streamable-file.ts +++ b/packages/common/file-stream/streamable-file.ts @@ -4,6 +4,7 @@ import { isFunction } from '../utils/shared.utils'; import { StreamableFileOptions } from './streamable-options.interface'; export interface StreamableHandlerResponse { + destroyed: boolean; statusCode: number; send: (msg: string) => void; } @@ -15,8 +16,10 @@ export class StreamableFile { err: Error, response: StreamableHandlerResponse, ) => void = (err: Error, res) => { - res.statusCode = 400; - res.send(err.message); + if (!res.destroyed) { + res.statusCode = 400; + res.send(err.message); + } }; constructor(buffer: Uint8Array, options?: StreamableFileOptions); From e82bdd4dd46565bf2022bdbfdb6b6d6c495df11a Mon Sep 17 00:00:00 2001 From: Jay McDoniel Date: Fri, 12 Aug 2022 13:41:34 -0700 Subject: [PATCH 2/5] test: add test for res.destroyed handler I had to use node's `http` module for this because I could not get `supertest`s `abort` method to work properly. With the raw `http` module I was able to use `req.destroy()` to cancel the request early. We now see the error from a premature closure, but are still able to make extra requests afterwards --- integration/send-files/e2e/express.spec.ts | 41 ++++++++++++++++++++ integration/send-files/src/app.controller.ts | 5 +++ integration/send-files/src/app.service.ts | 9 +++++ 3 files changed, 55 insertions(+) diff --git a/integration/send-files/e2e/express.spec.ts b/integration/send-files/e2e/express.spec.ts index f95e923389b..e943337d6c7 100644 --- a/integration/send-files/e2e/express.spec.ts +++ b/integration/send-files/e2e/express.spec.ts @@ -5,6 +5,7 @@ import { import { Test } from '@nestjs/testing'; import { expect } from 'chai'; import { readFileSync } from 'fs'; +import * as http from 'http'; import { join } from 'path'; import * as request from 'supertest'; import { AppModule } from '../src/app.module'; @@ -68,4 +69,44 @@ describe('Express FileSend', () => { it('should return an error if the file does not exist', async () => { return request(app.getHttpServer()).get('/file/not/exist').expect(400); }); + it('should allow for the client to end the response and be able to make another', async () => { + await app.listen(0); + const url = await app.getUrl(); + const [protocol, host, port] = url.replace('[::1]', 'localhost').split(':'); + const httpOptions = { + host: host.replace('//', ''), + protocol: `${protocol}:`, + port, + path: '/file/slow', + method: 'GET', + }; + await new Promise(resolve => { + const req = http.request(httpOptions, res => { + res.on('data', () => { + req.destroy(); + }); + /* no op */ + res.on('close', resolve); + }); + req.end(); + }); + await new Promise((resolve, reject) => { + const req = http.request( + { ...httpOptions, path: '/file/stream' }, + res => { + res.on('data', chunk => { + /* no op */ + }); + res.on('error', err => { + reject(err); + }); + res.on('end', () => { + expect(res.statusCode).to.be.eq(200); + resolve(); + }); + }, + ); + req.end(); + }); + }).timeout(5000); }); diff --git a/integration/send-files/src/app.controller.ts b/integration/send-files/src/app.controller.ts index 9715784bdbf..69fa20ec069 100644 --- a/integration/send-files/src/app.controller.ts +++ b/integration/send-files/src/app.controller.ts @@ -36,4 +36,9 @@ export class AppController { getNonExistantFile(): StreamableFile { return this.appService.getFileThatDoesNotExist(); } + + @Get('/file/slow') + getSlowFile(): StreamableFile { + return this.appService.getSlowStream(); + } } diff --git a/integration/send-files/src/app.service.ts b/integration/send-files/src/app.service.ts index af94bf5b7f1..d7a25f64d47 100644 --- a/integration/send-files/src/app.service.ts +++ b/integration/send-files/src/app.service.ts @@ -1,7 +1,9 @@ import { Injectable, StreamableFile } from '@nestjs/common'; +import { randomBytes } from 'crypto'; import { createReadStream, readFileSync } from 'fs'; import { join } from 'path'; import { Observable, of } from 'rxjs'; +import { Readable } from 'stream'; import { NonFile } from './non-file'; @Injectable() @@ -39,4 +41,11 @@ export class AppService { getFileThatDoesNotExist(): StreamableFile { return new StreamableFile(createReadStream('does-not-exist.txt')); } + + getSlowStream(): StreamableFile { + const stream = new Readable(); + stream.push(Buffer.from(randomBytes(Math.pow(2, 31) - 1))); + stream._read = () => {}; + return new StreamableFile(stream); + } } From e66423b983f65c231ea86e6171b23a6b2f3e629f Mon Sep 17 00:00:00 2001 From: Jay McDoniel Date: Fri, 12 Aug 2022 15:02:40 -0700 Subject: [PATCH 3/5] test: split abrupt stop test into a utils file As we make use of the `http` module instead of axios or supertest we have tomake use of some rather low level code. Using this utils file we can have clearer names of what is happening in each request to make it easier to follow what's happening. I've also added in some comments about why each part is the way it is for clarity. --- integration/send-files/e2e/express.spec.ts | 47 ++++------------- integration/send-files/e2e/utils.ts | 61 ++++++++++++++++++++++ integration/send-files/src/app.service.ts | 6 ++- 3 files changed, 75 insertions(+), 39 deletions(-) create mode 100644 integration/send-files/e2e/utils.ts diff --git a/integration/send-files/e2e/express.spec.ts b/integration/send-files/e2e/express.spec.ts index e943337d6c7..04ff82699aa 100644 --- a/integration/send-files/e2e/express.spec.ts +++ b/integration/send-files/e2e/express.spec.ts @@ -5,10 +5,14 @@ import { import { Test } from '@nestjs/testing'; import { expect } from 'chai'; import { readFileSync } from 'fs'; -import * as http from 'http'; import { join } from 'path'; import * as request from 'supertest'; import { AppModule } from '../src/app.module'; +import { + getHttpBaseOptions, + sendCanceledHttpRequest, + sendHttpRequest, +} from './utils'; const readme = readFileSync(join(process.cwd(), 'Readme.md')); const readmeString = readme.toString(); @@ -71,42 +75,9 @@ describe('Express FileSend', () => { }); it('should allow for the client to end the response and be able to make another', async () => { await app.listen(0); - const url = await app.getUrl(); - const [protocol, host, port] = url.replace('[::1]', 'localhost').split(':'); - const httpOptions = { - host: host.replace('//', ''), - protocol: `${protocol}:`, - port, - path: '/file/slow', - method: 'GET', - }; - await new Promise(resolve => { - const req = http.request(httpOptions, res => { - res.on('data', () => { - req.destroy(); - }); - /* no op */ - res.on('close', resolve); - }); - req.end(); - }); - await new Promise((resolve, reject) => { - const req = http.request( - { ...httpOptions, path: '/file/stream' }, - res => { - res.on('data', chunk => { - /* no op */ - }); - res.on('error', err => { - reject(err); - }); - res.on('end', () => { - expect(res.statusCode).to.be.eq(200); - resolve(); - }); - }, - ); - req.end(); - }); + const httpOptions = await getHttpBaseOptions(app); + await sendCanceledHttpRequest(httpOptions, '/file/slow'); + const res = await sendHttpRequest(httpOptions, '/file/stream'); + expect(res.statusCode).to.be.eq(200); }).timeout(5000); }); diff --git a/integration/send-files/e2e/utils.ts b/integration/send-files/e2e/utils.ts new file mode 100644 index 00000000000..b1929e3c5f5 --- /dev/null +++ b/integration/send-files/e2e/utils.ts @@ -0,0 +1,61 @@ +import { INestApplication } from '@nestjs/common'; +import { IncomingMessage, request, RequestOptions } from 'http'; + +export const getHttpBaseOptions = async ( + app: INestApplication, +): Promise => { + const url = await app.getUrl(); + // replace IPv6 localhost with IPv4 localhost alias and split URL on : to get the protocol (http), the host (localhost) and the port (random port) + const [protocol, host, port] = url.replace('[::1]', 'localhost').split(':'); + return { + // protocol is expected to be http: or https: + protocol: `${protocol}:`, + // remove the // prefix left over from the split(':') + host: host.replace('//', ''), + port, + method: 'GET', + }; +}; + +export const sendCanceledHttpRequest = async ( + options: RequestOptions, + path: string, +) => { + return new Promise((resolve, reject) => { + const req = request({ ...options, path }, res => { + // close the request once we get the first response of data + res.on('data', () => { + req.destroy(); + }); + // response is closed, move on to next request and verify it's doable + res.on('close', resolve); + }); + // fire the request + req.end(); + }); +}; + +export const sendHttpRequest = async ( + options: RequestOptions, + path: string, +) => { + return new Promise((resolve, reject) => { + const req = request({ ...options, path }, res => { + // this makes sure that the response actually starts and is read. We could verify this value against the same + // that is in an earlier test, but all we care about in _this_ test is that the status code is 200 + res.on('data', chunk => { + /* no op */ + }); + // fail the test if somethin goes wrong + res.on('error', err => { + reject(err); + }); + // pass the response back so we can verify values in the test + res.on('end', () => { + resolve(res); + }); + }); + // fire the request + req.end(); + }); +}; diff --git a/integration/send-files/src/app.service.ts b/integration/send-files/src/app.service.ts index d7a25f64d47..e5f920c48b8 100644 --- a/integration/send-files/src/app.service.ts +++ b/integration/send-files/src/app.service.ts @@ -8,6 +8,9 @@ import { NonFile } from './non-file'; @Injectable() export class AppService { + // `randomBytes` has a max value of 2^31 -1. That's all this is + private readonly MAX_BITES = Math.pow(2, 31) - 1; + getReadStream(): StreamableFile { return new StreamableFile( createReadStream(join(process.cwd(), 'Readme.md')), @@ -44,7 +47,8 @@ export class AppService { getSlowStream(): StreamableFile { const stream = new Readable(); - stream.push(Buffer.from(randomBytes(Math.pow(2, 31) - 1))); + stream.push(Buffer.from(randomBytes(this.MAX_BITES))); + // necessary for a `new Readable()`. Doesn't do anything stream._read = () => {}; return new StreamableFile(stream); } From 78fc6bebc006ed89cf8eda3de972a675aef39fc3 Mon Sep 17 00:00:00 2001 From: Jay McDoniel Date: Sat, 13 Aug 2022 19:15:03 -0700 Subject: [PATCH 4/5] test: use url class instead of request options --- integration/send-files/e2e/express.spec.ts | 6 ++--- integration/send-files/e2e/utils.ts | 28 ++++++---------------- 2 files changed, 10 insertions(+), 24 deletions(-) diff --git a/integration/send-files/e2e/express.spec.ts b/integration/send-files/e2e/express.spec.ts index 04ff82699aa..cf78184c045 100644 --- a/integration/send-files/e2e/express.spec.ts +++ b/integration/send-files/e2e/express.spec.ts @@ -75,9 +75,9 @@ describe('Express FileSend', () => { }); it('should allow for the client to end the response and be able to make another', async () => { await app.listen(0); - const httpOptions = await getHttpBaseOptions(app); - await sendCanceledHttpRequest(httpOptions, '/file/slow'); - const res = await sendHttpRequest(httpOptions, '/file/stream'); + const url = await getHttpBaseOptions(app); + await sendCanceledHttpRequest(new URL('/file/slow', url)); + const res = await sendHttpRequest(new URL('/file/stream', url)); expect(res.statusCode).to.be.eq(200); }).timeout(5000); }); diff --git a/integration/send-files/e2e/utils.ts b/integration/send-files/e2e/utils.ts index b1929e3c5f5..4fe937a52c1 100644 --- a/integration/send-files/e2e/utils.ts +++ b/integration/send-files/e2e/utils.ts @@ -1,28 +1,17 @@ import { INestApplication } from '@nestjs/common'; import { IncomingMessage, request, RequestOptions } from 'http'; +import { URL } from 'url'; export const getHttpBaseOptions = async ( app: INestApplication, -): Promise => { +): Promise => { const url = await app.getUrl(); - // replace IPv6 localhost with IPv4 localhost alias and split URL on : to get the protocol (http), the host (localhost) and the port (random port) - const [protocol, host, port] = url.replace('[::1]', 'localhost').split(':'); - return { - // protocol is expected to be http: or https: - protocol: `${protocol}:`, - // remove the // prefix left over from the split(':') - host: host.replace('//', ''), - port, - method: 'GET', - }; + return new URL(url); }; -export const sendCanceledHttpRequest = async ( - options: RequestOptions, - path: string, -) => { +export const sendCanceledHttpRequest = async (url: URL) => { return new Promise((resolve, reject) => { - const req = request({ ...options, path }, res => { + const req = request(url, res => { // close the request once we get the first response of data res.on('data', () => { req.destroy(); @@ -35,12 +24,9 @@ export const sendCanceledHttpRequest = async ( }); }; -export const sendHttpRequest = async ( - options: RequestOptions, - path: string, -) => { +export const sendHttpRequest = async (url: URL) => { return new Promise((resolve, reject) => { - const req = request({ ...options, path }, res => { + const req = request(url, res => { // this makes sure that the response actually starts and is read. We could verify this value against the same // that is in an earlier test, but all we care about in _this_ test is that the status code is 200 res.on('data', chunk => { From 6d0595ba83c2fd84fbb1fedf6217834c0e5c9c99 Mon Sep 17 00:00:00 2001 From: Jay McDoniel Date: Sun, 14 Aug 2022 08:44:40 -0700 Subject: [PATCH 5/5] chore: change block comment to inline --- integration/send-files/e2e/utils.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration/send-files/e2e/utils.ts b/integration/send-files/e2e/utils.ts index 4fe937a52c1..88d6759e452 100644 --- a/integration/send-files/e2e/utils.ts +++ b/integration/send-files/e2e/utils.ts @@ -30,7 +30,7 @@ export const sendHttpRequest = async (url: URL) => { // this makes sure that the response actually starts and is read. We could verify this value against the same // that is in an earlier test, but all we care about in _this_ test is that the status code is 200 res.on('data', chunk => { - /* no op */ + // no op }); // fail the test if somethin goes wrong res.on('error', err => {