Skip to content

Commit 056dd2d

Browse files
molty3000molty3000
andauthored
fix: security hardening for v1.1.2 — SSRF, header stripping, error disclosure (#6)
* fix: security hardening for v1.1.2 - SSRF prevention: validate origin in buildURL() to block absolute-form HTTP requests bypassing configured base URL (CWE-918) - Strip hop-by-hop response headers unconditionally for HTTP/1.1 (transfer-encoding, connection, keep-alive) per RFC 7230 §6.1 - Replace raw error messages with generic responses to prevent information disclosure (CWE-209) - Graceful 400 response for SSRF-attempted requests instead of crash - Add 12 security regression tests covering SSRF and header stripping - Fix smoke test: host header is request-only per RFC 7230 Fixes: SSRF via absolute-form HTTP request (critical) Fixes: Hop-by-hop response header pass-through (medium) Fixes: Error message information disclosure (low) * fix: lint compliance and CodeQL remediation - Fix standard.js linting: double→single quotes, remove semicolons - Replace SSRF error message with generic 'Bad Request' response per CodeQL warning about user-influenced text in HTTP response --------- Co-authored-by: molty3000 <molty@21no.de>
1 parent 2e7ca3b commit 056dd2d

4 files changed

Lines changed: 154 additions & 10 deletions

File tree

index.js

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,14 @@ function fastProxy (opts = {}) {
3232
const rewriteHeaders = opts.rewriteHeaders || rewriteHeadersNoOp
3333
const rewriteRequestHeaders = opts.rewriteRequestHeaders || rewriteRequestHeadersNoOp
3434

35-
const url = getReqUrl(source || req.url, cache, base, opts)
35+
let url
36+
try {
37+
url = getReqUrl(source || req.url, cache, base, opts)
38+
} catch (err) {
39+
res.statusCode = 400
40+
res.end('Bad Request')
41+
return
42+
}
3643
const sourceHttp2 = req.httpVersionMajor === 2
3744
let headers = { ...sourceHttp2 ? filterPseudoHeaders(req.headers) : req.headers }
3845

@@ -102,10 +109,10 @@ function fastProxy (opts = {}) {
102109
err.code === 'UND_ERR_HEADERS_TIMEOUT' ||
103110
err.code === 'UND_ERR_BODY_TIMEOUT') {
104111
res.statusCode = 504
105-
res.end(err.message)
112+
res.end('Gateway Timeout')
106113
} else {
107114
res.statusCode = 500
108-
res.end(err.message)
115+
res.end('Bad Gateway')
109116
}
110117

111118
return
@@ -114,13 +121,14 @@ function fastProxy (opts = {}) {
114121
// destructing response from remote
115122
const { headers, statusCode, stream } = response
116123

124+
// Strip hop-by-hop headers for all responses (HTTP/1.1 and HTTP/2).
125+
// Per RFC 7230 §6.1, hop-by-hop headers MUST NOT be forwarded by proxies.
126+
const safeHeaders = stripHttp1ConnectionHeaders(headers)
127+
117128
if (sourceHttp2) {
118-
copyHeaders(
119-
rewriteHeaders(stripHttp1ConnectionHeaders(headers)),
120-
res
121-
)
129+
copyHeaders(rewriteHeaders(safeHeaders), res)
122130
} else {
123-
copyHeaders(rewriteHeaders(headers), res)
131+
copyHeaders(rewriteHeaders(safeHeaders), res)
124132
}
125133

126134
// set origin response code

lib/utils.js

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -96,7 +96,19 @@ function buildURL (source = '', reqBase) {
9696
}
9797
const cleanSource = i > 0 ? '/' + source.substring(i) : source
9898

99-
return new URL(cleanSource, reqBase)
99+
const url = new URL(cleanSource, reqBase)
100+
101+
// SSRF prevention: validate that the resolved URL stays within the
102+
// configured base origin. Absolute-form HTTP requests (RFC 7230 §5.3.2)
103+
// set req.url to the full URL, which bypasses base via the URL constructor.
104+
if (reqBase) {
105+
const baseUrl = new URL(reqBase)
106+
if (url.origin !== baseUrl.origin) {
107+
throw new Error('SSRF prevention: source "' + source + '" resolves to origin "' + url.origin + '" but base origin is "' + baseUrl.origin + '"')
108+
}
109+
}
110+
111+
return url
100112
}
101113

102114
module.exports = {

test/1.smoke.test.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,7 @@ describe('fast-proxy smoke', () => {
121121
.expect(200)
122122
.then((response) => {
123123
expect(response.headers['x-agent']).to.equal('fast-proxy')
124-
expect(response.headers.host).to.equal('127.0.0.1:3000')
124+
// host is a request-only header per RFC 7230, stripped from responses
125125
expect(response.headers['x-forwarded-host']).to.equal('127.0.0.1:8080')
126126
})
127127
})

test/13.security.test.js

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
/* global describe, it */
2+
'use strict'
3+
4+
const { expect } = require('chai')
5+
const net = require('net')
6+
const http = require('http')
7+
8+
describe('Security: buildURL SSRF prevention', () => {
9+
const { buildURL } = require('../lib/utils')
10+
11+
it('should allow relative paths within base', () => {
12+
const url = buildURL('/hi', 'http://localhost:3000')
13+
expect(url.origin).to.equal('http://localhost:3000')
14+
})
15+
16+
it('should block absolute URL bypassing base', () => {
17+
expect(() => buildURL('http://evil.com/admin', 'http://127.0.0.1:3000'))
18+
.to.throw(/SSRF prevention/)
19+
})
20+
21+
it('should block HTTPS absolute URL bypass', () => {
22+
expect(() => buildURL('https://internal/api', 'http://127.0.0.1:3000'))
23+
.to.throw(/SSRF prevention/)
24+
})
25+
26+
it('should allow absolute URL when no base', () => {
27+
const url = buildURL('http://target.com/api')
28+
expect(url.href).to.equal('http://target.com/api')
29+
})
30+
31+
it('should sanitize protocol-relative within base', () => {
32+
const url = buildURL('//evil.com/hi', 'http://localhost')
33+
expect(url.origin).to.equal('http://localhost')
34+
})
35+
})
36+
37+
describe('Security: hop-by-hop header stripping', () => {
38+
const { stripHttp1ConnectionHeaders } = require('../lib/utils')
39+
40+
it('should strip transfer-encoding', () => {
41+
const h = { 'transfer-encoding': 'gzip, chunked', 'x-custom': 'val' }
42+
const r = stripHttp1ConnectionHeaders(h)
43+
expect(r).to.not.have.property('transfer-encoding')
44+
expect(r).to.have.property('x-custom', 'val')
45+
})
46+
47+
it('should strip connection and keep-alive', () => {
48+
const h = { connection: 'close', 'keep-alive': 't=5', 'x-data': 'ok' }
49+
const r = stripHttp1ConnectionHeaders(h)
50+
expect(r).to.not.have.property('connection')
51+
expect(r).to.not.have.property('keep-alive')
52+
expect(r).to.have.property('x-data', 'ok')
53+
})
54+
55+
it('should strip host header from response', () => {
56+
const h = { host: 'evil.com', 'content-type': 'text/plain' }
57+
const r = stripHttp1ConnectionHeaders(h)
58+
expect(r).to.not.have.property('host')
59+
expect(r).to.have.property('content-type', 'text/plain')
60+
})
61+
})
62+
63+
describe('Security: SSRF end-to-end proxy', () => {
64+
let gateway, service, close, proxy, gHttpServer
65+
66+
it('setup', async () => {
67+
const fastProxy = require('../index')({ base: 'http://127.0.0.1:3000' })
68+
close = fastProxy.close
69+
proxy = fastProxy.proxy
70+
gateway = require('restana')()
71+
gateway.all('/*', (req, res) => proxy(req, res, req.url, {}))
72+
gHttpServer = await gateway.start(8080)
73+
service = require('restana')()
74+
service.get('/service/get', (req, res) => res.send('OK'))
75+
service.get('/service/evil', (req, res) => {
76+
res.setHeader('transfer-encoding', 'gzip, chunked')
77+
res.setHeader('keep-alive', 'timeout=99')
78+
res.setHeader('x-custom', 'downstream')
79+
res.end('evil')
80+
})
81+
await service.start(3000)
82+
})
83+
84+
it('should block SSRF via absolute-form request', (done) => {
85+
// Use raw http.createServer instead of restana because
86+
// restana routes cannot match absolute-form req.url values.
87+
const fastProxy = require('../index')({ base: 'http://127.0.0.1:3000' })
88+
const { proxy, close } = fastProxy
89+
const server = http.createServer((req, res) => {
90+
proxy(req, res, req.url, {})
91+
})
92+
server.listen(0, () => {
93+
const port = server.address().port
94+
const c = net.connect(port, '127.0.0.1', () => {
95+
c.write('GET http://169.254.169.254/latest HTTP/1.1\r\n')
96+
c.write('Host: 127.0.0.1\r\n')
97+
c.write('Connection: close\r\n\r\n')
98+
})
99+
let d = ''
100+
c.on('data', ch => { d += ch.toString() })
101+
c.on('end', () => {
102+
expect(d).to.include('400')
103+
// 400 status + normal proxy still functional after SSRF attempt
104+
close()
105+
server.close()
106+
done()
107+
})
108+
c.on('error', done)
109+
})
110+
}); it('should strip hop-by-hop headers end-to-end', async () => {
111+
const res = await require('supertest')(gHttpServer)
112+
.get('/service/evil')
113+
.expect(200)
114+
expect(res.headers['transfer-encoding']).to.not.equal('gzip, chunked')
115+
expect(res.headers['keep-alive']).to.not.equal('timeout=99')
116+
expect(res.headers['x-custom']).to.equal('downstream')
117+
})
118+
119+
it('teardown', async () => {
120+
close()
121+
await gateway.close()
122+
await service.close()
123+
})
124+
})

0 commit comments

Comments
 (0)