Skip to content

Commit 1bdccbe

Browse files
chimuraichimurai
authored
fix(fixRequestBody): check readableLength (#1096)
1 parent 01934d3 commit 1bdccbe

File tree

2 files changed

+25
-123
lines changed

2 files changed

+25
-123
lines changed

‎src/handlers/fix-request-body.ts

+5-38
Original file line numberDiff line numberDiff line change
@@ -1,26 +1,20 @@
11
import type * as http from 'http';
22
import * as querystring from 'querystring';
33

4-
import type { Options } from '../types';
5-
import { getLogger } from '../logger';
6-
74
export type BodyParserLikeRequest = http.IncomingMessage & { body?: any };
85

9-
type HandleBadRequestArgs = {
10-
proxyReq: http.ClientRequest;
11-
req: BodyParserLikeRequest;
12-
res: http.ServerResponse<http.IncomingMessage>;
13-
};
14-
156
/**
167
* Fix proxied body if bodyParser is involved.
178
*/
189
export function fixRequestBody<TReq extends BodyParserLikeRequest = BodyParserLikeRequest>(
1910
proxyReq: http.ClientRequest,
2011
req: TReq,
21-
res: http.ServerResponse<http.IncomingMessage>,
22-
options: Options,
2312
): void {
13+
// skip fixRequestBody() when req.readableLength not 0 (bodyParser failure)
14+
if (req.readableLength !== 0) {
15+
return;
16+
}
17+
2418
const requestBody = req.body;
2519

2620
if (!requestBody) {
@@ -33,22 +27,6 @@ export function fixRequestBody<TReq extends BodyParserLikeRequest = BodyParserLi
3327
return;
3428
}
3529

36-
const logger = getLogger(options);
37-
38-
// Handle bad request when unexpected "Connect: Upgrade" header is provided
39-
if (/upgrade/gi.test(proxyReq.getHeader('Connection') as string)) {
40-
handleBadRequest({ proxyReq, req, res });
41-
logger.error(`[HPM] HPM_UNEXPECTED_CONNECTION_UPGRADE_HEADER. Aborted request: ${req.url}`);
42-
return;
43-
}
44-
45-
// Handle bad request when invalid request body is provided
46-
if (hasInvalidKeys(requestBody)) {
47-
handleBadRequest({ proxyReq, req, res });
48-
logger.error(`[HPM] HPM_INVALID_REQUEST_DATA. Aborted request: ${req.url}`);
49-
return;
50-
}
51-
5230
const writeBody = (bodyData: string) => {
5331
proxyReq.setHeader('Content-Length', Buffer.byteLength(bodyData));
5432
proxyReq.write(bodyData);
@@ -79,14 +57,3 @@ function handlerFormDataBodyData(contentType: string, data: any) {
7957
}
8058
return str;
8159
}
82-
83-
function hasInvalidKeys(obj) {
84-
return Object.keys(obj).some((key) => /[\n\r]/.test(key));
85-
}
86-
87-
function handleBadRequest({ proxyReq, req, res }: HandleBadRequestArgs) {
88-
res.writeHead(400);
89-
res.end('Bad Request');
90-
proxyReq.destroy();
91-
req.destroy();
92-
}

‎test/unit/fix-request-body.spec.ts

+20-85
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,7 @@ describe('fixRequestBody', () => {
3838
jest.spyOn(proxyRequest, 'setHeader');
3939
jest.spyOn(proxyRequest, 'write');
4040

41-
fixRequestBody(proxyRequest, createRequestWithBody(undefined), fakeProxyResponse(), {});
41+
fixRequestBody(proxyRequest, createRequestWithBody(undefined));
4242

4343
expect(proxyRequest.setHeader).not.toHaveBeenCalled();
4444
expect(proxyRequest.write).not.toHaveBeenCalled();
@@ -51,7 +51,7 @@ describe('fixRequestBody', () => {
5151
jest.spyOn(proxyRequest, 'setHeader');
5252
jest.spyOn(proxyRequest, 'write');
5353

54-
fixRequestBody(proxyRequest, createRequestWithBody({}), fakeProxyResponse(), {});
54+
fixRequestBody(proxyRequest, createRequestWithBody({}));
5555

5656
expect(proxyRequest.setHeader).toHaveBeenCalled();
5757
expect(proxyRequest.write).toHaveBeenCalled();
@@ -64,12 +64,7 @@ describe('fixRequestBody', () => {
6464
jest.spyOn(proxyRequest, 'setHeader');
6565
jest.spyOn(proxyRequest, 'write');
6666

67-
fixRequestBody(
68-
proxyRequest,
69-
createRequestWithBody({ someField: 'some value' }),
70-
fakeProxyResponse(),
71-
{},
72-
);
67+
fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
7368

7469
const expectedBody = JSON.stringify({ someField: 'some value' });
7570
expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -83,12 +78,7 @@ describe('fixRequestBody', () => {
8378
jest.spyOn(proxyRequest, 'setHeader');
8479
jest.spyOn(proxyRequest, 'write');
8580

86-
fixRequestBody(
87-
proxyRequest,
88-
createRequestWithBody({ someField: 'some value' }),
89-
fakeProxyResponse(),
90-
{},
91-
);
81+
fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
9282

9383
const expectedBody = handlerFormDataBodyData('multipart/form-data', {
9484
someField: 'some value',
@@ -112,12 +102,7 @@ describe('fixRequestBody', () => {
112102
jest.spyOn(proxyRequest, 'setHeader');
113103
jest.spyOn(proxyRequest, 'write');
114104

115-
fixRequestBody(
116-
proxyRequest,
117-
createRequestWithBody({ someField: 'some value' }),
118-
fakeProxyResponse(),
119-
{},
120-
);
105+
fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
121106

122107
const expectedBody = handlerFormDataBodyData('multipart/form-data', {
123108
someField: 'some value',
@@ -142,12 +127,7 @@ describe('fixRequestBody', () => {
142127
jest.spyOn(proxyRequest, 'setHeader');
143128
jest.spyOn(proxyRequest, 'write');
144129

145-
fixRequestBody(
146-
proxyRequest,
147-
createRequestWithBody({ someField: 'some value' }),
148-
fakeProxyResponse(),
149-
{},
150-
);
130+
fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
151131
const expectedBody = JSON.stringify({ someField: 'some value' });
152132
expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
153133
expect(proxyRequest.write).toHaveBeenCalledWith(expectedBody);
@@ -160,12 +140,7 @@ describe('fixRequestBody', () => {
160140
jest.spyOn(proxyRequest, 'setHeader');
161141
jest.spyOn(proxyRequest, 'write');
162142

163-
fixRequestBody(
164-
proxyRequest,
165-
createRequestWithBody({ someField: 'some value' }),
166-
fakeProxyResponse(),
167-
{},
168-
);
143+
fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
169144

170145
const expectedBody = querystring.stringify({ someField: 'some value' });
171146
expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -179,12 +154,7 @@ describe('fixRequestBody', () => {
179154
jest.spyOn(proxyRequest, 'setHeader');
180155
jest.spyOn(proxyRequest, 'write');
181156

182-
fixRequestBody(
183-
proxyRequest,
184-
createRequestWithBody({ someField: 'some value' }),
185-
fakeProxyResponse(),
186-
{},
187-
);
157+
fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
188158

189159
const expectedBody = querystring.stringify({ someField: 'some value' });
190160
expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
@@ -198,69 +168,34 @@ describe('fixRequestBody', () => {
198168
jest.spyOn(proxyRequest, 'setHeader');
199169
jest.spyOn(proxyRequest, 'write');
200170

201-
fixRequestBody(
202-
proxyRequest,
203-
createRequestWithBody({ someField: 'some value' }),
204-
fakeProxyResponse(),
205-
{},
206-
);
171+
fixRequestBody(proxyRequest, createRequestWithBody({ someField: 'some value' }));
207172

208173
const expectedBody = JSON.stringify({ someField: 'some value' });
209174
expect(proxyRequest.setHeader).toHaveBeenCalledWith('Content-Length', expectedBody.length);
210175
expect(proxyRequest.write).toHaveBeenCalledTimes(1);
211176
expect(proxyRequest.write).toHaveBeenCalledWith(expectedBody);
212177
});
213178

214-
it('should return 400 and abort request on "Connection: Upgrade" header', () => {
179+
it('should not fixRequestBody() when there bodyParser fails', () => {
215180
const proxyRequest = fakeProxyRequest();
216-
const request = createRequestWithBody({ someField: 'some value' });
217-
const proxyResponse = fakeProxyResponse();
218-
proxyRequest.setHeader('connection', 'upgrade');
219-
proxyRequest.setHeader('content-type', 'application/x-www-form-urlencoded');
220-
221-
jest.spyOn(proxyRequest, 'destroy');
222-
jest.spyOn(request, 'destroy');
223-
jest.spyOn(proxyResponse, 'writeHead');
224-
jest.spyOn(proxyResponse, 'end');
225-
226-
const logger = {
227-
error: jest.fn(),
228-
};
181+
const request = {
182+
get readableLength() {
183+
return 4444; // simulate bodyParser failure
184+
},
185+
} as BodyParserLikeRequest;
229186

230-
fixRequestBody(proxyRequest, request, proxyResponse, { logger });
231-
232-
expect(proxyResponse.writeHead).toHaveBeenCalledWith(400);
233-
expect(proxyResponse.end).toHaveBeenCalledTimes(1);
234-
expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
235-
expect(request.destroy).toHaveBeenCalledTimes(1);
236-
expect(logger.error).toHaveBeenCalledWith(
237-
`[HPM] HPM_UNEXPECTED_CONNECTION_UPGRADE_HEADER. Aborted request: /test_path`,
238-
);
239-
});
240-
241-
it('should return 400 and abort request on invalid request data', () => {
242-
const proxyRequest = fakeProxyRequest();
243-
const request = createRequestWithBody({ 'INVALID \n\r DATA': '' });
244187
const proxyResponse = fakeProxyResponse();
245188
proxyRequest.setHeader('content-type', 'application/x-www-form-urlencoded');
246189

190+
jest.spyOn(proxyRequest, 'write');
247191
jest.spyOn(proxyRequest, 'destroy');
248-
jest.spyOn(request, 'destroy');
249192
jest.spyOn(proxyResponse, 'writeHead');
250193
jest.spyOn(proxyResponse, 'end');
251194

252-
const logger = {
253-
error: jest.fn(),
254-
};
255-
256-
fixRequestBody(proxyRequest, request, proxyResponse, { logger });
195+
fixRequestBody(proxyRequest, request);
257196

258-
expect(proxyResponse.writeHead).toHaveBeenCalledWith(400);
259-
expect(proxyResponse.end).toHaveBeenCalledTimes(1);
260-
expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
261-
expect(request.destroy).toHaveBeenCalledTimes(1);
262-
expect(logger.error).toHaveBeenCalledWith(
263-
`[HPM] HPM_INVALID_REQUEST_DATA. Aborted request: /test_path`,
264-
);
197+
expect(proxyResponse.end).toHaveBeenCalledTimes(0);
198+
expect(proxyRequest.write).toHaveBeenCalledTimes(0);
199+
expect(proxyRequest.destroy).toHaveBeenCalledTimes(0);
265200
});
266201
});

0 commit comments

Comments
 (0)