Skip to content
This repository has been archived by the owner on Feb 2, 2024. It is now read-only.

Handle OPTIONS content-length #83

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Handle OPTIONS content-length #83

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
26a4389
handle OPTIONS content-length
Maxime-J Mar 13, 2023
68b701a
update undici tests
Maxime-J Mar 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 4 additions & 5 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const {
filterPseudoHeaders,
copyHeaders,
stripHttp1ConnectionHeaders,
filterHeaders,
buildURL
} = require('./lib/utils')

Expand Down Expand Up @@ -53,11 +52,11 @@ function fastProxy (opts = {}) {

let body = null
// according to https://tools.ietf.org/html/rfc2616#section-4.3
// proxy should ignore message body when it's a GET or HEAD request
// proxy should ignore message body when it's a GET, HEAD or OPTIONS request
// when proxy this request, we should reset the content-length to make it a valid http request
if (req.method === 'GET' || req.method === 'HEAD') {
if (headers['content-length']) {
headers = filterHeaders(headers, 'content-length')
if (req.method === 'GET' || req.method === 'HEAD' || req.method === 'OPTIONS') {
if (headers.hasOwnProperty('content-length')) {
Copy link
Contributor

@Uzlopak Uzlopak Mar 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. If you want to use hasOwnProperty, than do it with

Object.prototype.hasOwnProperty.call(headers, 'content-length')

But imho the better way is

'content-length' in headers

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, safer and better to use in

delete headers['content-length']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using filterHeaders has maybe some deeper reason.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the context when it was introduced #38 (for the same undici reason I raised about OPTIONS)

-We could think it was made that way to prevent object mutation but headers are taken from a spread, so no risks of modifying elsewhere, and headers object is already mutated

headers['x-forwarded-host'] = headers.host
headers.host = url.hostname

-filterHeaders ensures a case insensitive removal, but first, headers are supposed to already be lowercase and anyway, a case-sensitive comparison is made before calling it.

Are you ok with delete then ?
But I can obviously leave it as it was, the priority being to fix that OPTIONS issue.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

delete is usually slower than just creating a new Object.
Maybe just fix the OPTIONS and keep that code and just open a new issue to discuss it.

}
} else {
if (req.body) {
Expand Down
15 changes: 14 additions & 1 deletion test/5.undici.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ describe('undici', () => {
res.setHeader('x-agent', 'fast-proxy')
res.send(req.headers)
})
service.options('/service/headers', (req, res) => {
res.send(null, 204, {...req.headers})
})
service.get('/service/timeout', async (req, res) => {
await sleep(200)
res.send('OK')
Expand Down Expand Up @@ -87,11 +90,21 @@ describe('undici', () => {
.set('content-length', '10')
.then((response) => {
expect(response.headers['x-agent']).to.equal('fast-proxy')
expect(response.body['content-length']).to.equal(undefined) // content-length is stripped for GET / HEAD requests
expect(response.body['content-length']).to.equal(undefined) // content-length is stripped for GET, HEAD and OPTIONS requests
expect(response.body.host).to.equal('127.0.0.1:3000')
})
})

it('should 204 on OPTIONS /service/headers', async () => {
await request(gHttpServer)
.options('/service/headers')
.expect(204)
.set('content-length', '0')
.then((response) => {
expect(response.headers['content-length']).to.equal(undefined) // content-length is stripped for GET, HEAD and OPTIONS requests
})
})

it('should 504 on GET /service/timeout', async () => {
await request(gHttpServer)
.get('/service/timeout')
Expand Down