Skip to content

Commit 0f2dc5b

Browse files
chimuraichimurai
authored
fix(fixRequestBody): harden form-data stringification
1 parent 013dd9b commit 0f2dc5b

5 files changed

Lines changed: 213 additions & 23 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# Changelog
22

3+
## [Unreleased](https://github.com/chimurai/http-proxy-middleware/compare/v4.1.0...master)
4+
5+
- fix(fixRequestBody): harden form-data stringification
6+
37
## [v4.1.0](https://github.com/chimurai/http-proxy-middleware/releases/tag/v4.1.0)
48

59
- feat(definePlugin): helper function to create plugins

‎src/handlers/fix-request-body-utils/stringify-form-data.ts‎

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
const CR_OR_LF = /[\r\n]/;
2+
3+
/**
4+
* stringify FormData data
5+
* @param contentType
6+
* @param data
7+
* @returns
8+
*/
9+
export function stringifyFormData(contentType: string, data: object): string {
10+
const boundary = getMultipartBoundary(contentType);
11+
let str = '';
12+
13+
for (const [key, value] of Object.entries(data)) {
14+
const normalizedKey = String(key);
15+
const normalizedValue = String(value);
16+
17+
// Reject potentially dangerous sequences to prevent multipart header/body injection.
18+
validateMultipartField(normalizedKey, normalizedValue, boundary);
19+
20+
str += `--${boundary}\r\nContent-Disposition: form-data; name="${escapeMultipartFieldName(normalizedKey)}"\r\n\r\n${normalizedValue}\r\n`;
21+
}
22+
23+
return str;
24+
}
25+
26+
function getMultipartBoundary(contentType: string): string {
27+
const boundaryMatch = /(?:^|;)\s*boundary=(?:"([^"]+)"|([^;]+))/i.exec(contentType);
28+
29+
// Keep backward-compatible behavior when boundary is omitted: fall back to legacy extraction.
30+
const boundary = (boundaryMatch?.[1] ?? boundaryMatch?.[2] ?? contentType).trim();
31+
32+
if (!boundary || CR_OR_LF.test(boundary)) {
33+
throw new Error(
34+
'[HPM] unsafe multipart boundary detected. Request rejected per RFC 9112 obsolete line folding guidance.',
35+
);
36+
}
37+
38+
return boundary;
39+
}
40+
41+
function validateMultipartField(fieldName: string, fieldValue: string, boundary: string): void {
42+
const boundaryDelimiter = `--${boundary}`;
43+
44+
if (CR_OR_LF.test(fieldName)) {
45+
throw new Error(
46+
`[HPM] unsafe multipart field name "${fieldName}" detected. Request rejected per RFC 9112 obsolete line folding guidance.`,
47+
);
48+
}
49+
50+
if (CR_OR_LF.test(fieldValue) || fieldValue.includes(boundaryDelimiter)) {
51+
throw new Error(
52+
`[HPM] unsafe multipart field value for "${fieldName}" detected. Request rejected per RFC 9112 obsolete line folding guidance.`,
53+
);
54+
}
55+
}
56+
57+
function escapeMultipartFieldName(fieldName: string): string {
58+
return fieldName.replace(/\\/g, '\\\\').replace(/"/g, '\\"');
59+
}

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

Lines changed: 20 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,8 @@ import type * as http from 'node:http';
22
import * as querystring from 'node:querystring';
33
import * as zlib from 'node:zlib';
44

5+
import { stringifyFormData } from './fix-request-body-utils/stringify-form-data.js';
6+
57
export type BodyParserLikeRequest = http.IncomingMessage & { body?: any };
68

79
/**
@@ -65,30 +67,25 @@ export function fixRequestBody<TReq extends BodyParserLikeRequest = BodyParserLi
6567
proxyReq.write(proxyData);
6668
};
6769

68-
// Use if-elseif to prevent multiple writeBody/setHeader calls:
69-
// Error: "Cannot set headers after they are sent to the client"
70-
if (contentType.includes('application/json') || contentType.includes('+json')) {
71-
writeBody(JSON.stringify(requestBody));
72-
} else if (contentType.includes('application/x-www-form-urlencoded')) {
73-
writeBody(querystring.stringify(requestBody));
74-
} else if (contentType.includes('multipart/form-data')) {
75-
writeBody(handlerFormDataBodyData(contentType, requestBody));
76-
} else if (contentType.includes('text/plain')) {
77-
writeBody(requestBody);
70+
try {
71+
// Use if-elseif to prevent multiple writeBody/setHeader calls:
72+
// Error: "Cannot set headers after they are sent to the client"
73+
if (contentType.includes('application/json') || contentType.includes('+json')) {
74+
writeBody(JSON.stringify(requestBody));
75+
} else if (contentType.includes('application/x-www-form-urlencoded')) {
76+
writeBody(querystring.stringify(requestBody));
77+
} else if (contentType.includes('multipart/form-data')) {
78+
writeBody(stringifyFormData(contentType, requestBody));
79+
} else if (contentType.includes('text/plain')) {
80+
writeBody(requestBody);
81+
}
82+
} catch (error) {
83+
// proxyReq listeners run outside the middleware try/catch path; re-throwing here can bubble as
84+
// an unhandled exception in consumers, so destroy() is used to fail closed through proxy error handling.
85+
proxyReq.destroy(toError(error));
7886
}
7987
}
8088

81-
/**
82-
* format FormData data
83-
* @param contentType
84-
* @param data
85-
* @returns
86-
*/
87-
function handlerFormDataBodyData(contentType: string, data: any) {
88-
const boundary = contentType.replace(/^.*boundary=(.*)$/, '$1');
89-
let str = '';
90-
for (const [key, value] of Object.entries(data)) {
91-
str += `--${boundary}\r\nContent-Disposition: form-data; name="${key}"\r\n\r\n${value}\r\n`;
92-
}
93-
return str;
89+
function toError(error: unknown): Error {
90+
return error instanceof Error ? error : new Error(String(error));
9491
}

‎test/e2e/http-proxy-middleware.spec.ts‎

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,50 @@ describe('E2E http-proxy-middleware', () => {
128128
});
129129
await agent.post('/api').send({ foo: 'bar', bar: 'baz', doubleByte: '文' }).expect(200);
130130
});
131+
132+
it('should reject CRLF multipart injection when fixRequestBody serializes multipart', async () => {
133+
const targetSpy = vi.fn();
134+
135+
agent = request(
136+
createApp(
137+
bodyParser.urlencoded({ extended: false }),
138+
(req, res, next) => {
139+
if (req.body?.role === 'admin') {
140+
res.status(403).send('gateway: role=admin forbidden');
141+
return;
142+
}
143+
next();
144+
},
145+
createProxyMiddleware({
146+
target: mockTargetServer.url,
147+
pathFilter: '/api',
148+
on: {
149+
proxyReq: (proxyReq, req) => {
150+
proxyReq.setHeader('Content-Type', 'multipart/form-data; boundary=BB');
151+
fixRequestBody(proxyReq, req);
152+
},
153+
},
154+
}),
155+
),
156+
);
157+
158+
await mockTargetServer.forPost('/api').thenCallback(async () => {
159+
targetSpy();
160+
return { statusCode: 200 };
161+
});
162+
163+
const injectedValue =
164+
'alice\r\n--BB\r\nContent-Disposition: form-data; name="role"\r\n\r\nadmin\r\n--BB--';
165+
166+
const response = await agent
167+
.post('/api')
168+
.set('Content-Type', 'application/x-www-form-urlencoded')
169+
.send(`user=${encodeURIComponent(injectedValue)}`)
170+
.expect(500);
171+
172+
expect(response.text).toContain('Error occurred while trying to proxy');
173+
expect(targetSpy).toHaveBeenCalledTimes(0);
174+
});
131175
});
132176

133177
describe('custom pathFilter matcher/filter', () => {

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

Lines changed: 86 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,92 @@ describe('fixRequestBody', () => {
113113
expect(proxyRequest.write).toHaveBeenCalledWith(expectedBody);
114114
});
115115

116+
it('should reject multipart field values containing CRLF', () => {
117+
const proxyRequest = fakeProxyRequest();
118+
proxyRequest.setHeader('content-type', 'multipart/form-data; boundary=BB');
119+
120+
fixRequestBody(
121+
proxyRequest,
122+
createRequestWithBody({
123+
user: 'alice\r\n--BB\r\nContent-Disposition: form-data; name="role"\r\n\r\nadmin',
124+
}),
125+
);
126+
127+
expect(proxyRequest.write).toHaveBeenCalledTimes(0);
128+
expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
129+
});
130+
131+
it('should reject multipart field values containing LF only', () => {
132+
const proxyRequest = fakeProxyRequest();
133+
proxyRequest.setHeader('content-type', 'multipart/form-data; boundary=BB');
134+
135+
fixRequestBody(
136+
proxyRequest,
137+
createRequestWithBody({
138+
user: 'alice\nadmin',
139+
}),
140+
);
141+
142+
expect(proxyRequest.write).toHaveBeenCalledTimes(0);
143+
expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
144+
});
145+
146+
it('should reject multipart field values containing CR only', () => {
147+
const proxyRequest = fakeProxyRequest();
148+
proxyRequest.setHeader('content-type', 'multipart/form-data; boundary=BB');
149+
150+
fixRequestBody(
151+
proxyRequest,
152+
createRequestWithBody({
153+
user: 'alice\radmin',
154+
}),
155+
);
156+
157+
expect(proxyRequest.write).toHaveBeenCalledTimes(0);
158+
expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
159+
});
160+
161+
it('should reject multipart field names containing LF', () => {
162+
const proxyRequest = fakeProxyRequest();
163+
proxyRequest.setHeader('content-type', 'multipart/form-data; boundary=BB');
164+
165+
fixRequestBody(
166+
proxyRequest,
167+
createRequestWithBody({
168+
'bad\nname': 'alice',
169+
}),
170+
);
171+
172+
expect(proxyRequest.write).toHaveBeenCalledTimes(0);
173+
expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
174+
});
175+
176+
it('should reject multipart field values containing the active boundary delimiter', () => {
177+
const proxyRequest = fakeProxyRequest();
178+
proxyRequest.setHeader('content-type', 'multipart/form-data; boundary=BB');
179+
180+
fixRequestBody(
181+
proxyRequest,
182+
createRequestWithBody({
183+
user: '--BB',
184+
}),
185+
);
186+
187+
expect(proxyRequest.write).toHaveBeenCalledTimes(0);
188+
expect(proxyRequest.destroy).toHaveBeenCalledTimes(1);
189+
});
190+
191+
it('should escape quotes in multipart field names', () => {
192+
const proxyRequest = fakeProxyRequest();
193+
proxyRequest.setHeader('content-type', 'multipart/form-data; boundary=BB');
194+
195+
fixRequestBody(proxyRequest, createRequestWithBody({ 'field"name': 'value' }));
196+
197+
expect(proxyRequest.write).toHaveBeenCalledWith(
198+
'--BB\r\nContent-Disposition: form-data; name="field\\"name"\r\n\r\nvalue\r\n',
199+
);
200+
});
201+
116202
it('should write when body is not empty and Content-Type ends with +json', () => {
117203
const proxyRequest = fakeProxyRequest();
118204
proxyRequest.setHeader('content-type', 'application/merge-patch+json; charset=utf-8');

0 commit comments

Comments
 (0)