Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: 301 and 302 change method to GET #3862

Merged
merged 8 commits into from
Nov 21, 2024
Merged

fix: 301 and 302 change method to GET #3862

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a7024ed
fix: 301 and 302 change method to GET
DTrombett Nov 21, 2024
a070253
tests: update tests to reflect changes
DTrombett Nov 21, 2024
bc4cbac
tests: update other tests too
DTrombett Nov 21, 2024
3772d50
fix: destroy body
DTrombett Nov 21, 2024
6a78c2a
Apply suggestions from code review
ronag Nov 21, 2024
e5b5894
fix: missing )
DTrombett Nov 21, 2024
be844b6
fix: check if stream is valid
DTrombett Nov 21, 2024
8840c9e
refactor: extract noop function
DTrombett Nov 21, 2024
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
12 changes: 12 additions & 0 deletions lib/handler/redirect-handler.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ const redirectableStatusCodes = [300, 301, 302, 303, 307, 308]

const kBody = Symbol('body')

const noop = () => {}

class BodyAsyncIterable {
constructor (body) {
this[kBody] = body
Expand Down Expand Up @@ -132,10 +134,20 @@ class RedirectHandler {
this.opts.maxRedirections = 0
this.opts.query = null

// https://tools.ietf.org/html/rfc7231#section-6.4.2
// https://fetch.spec.whatwg.org/#http-redirect-fetch
// In case of HTTP 301 or 302 with POST, change the method to GET
if ((statusCode === 301 || statusCode === 302) && this.opts.method === 'POST') {
this.opts.method = 'GET'
if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', noop))
this.opts.body = null
DTrombett marked this conversation as resolved.
Show resolved Hide resolved
}

// https://tools.ietf.org/html/rfc7231#section-6.4.4
// In case of HTTP 303, always replace method to be either HEAD or GET
if (statusCode === 303 && this.opts.method !== 'HEAD') {
this.opts.method = 'GET'
if (util.isStream(this.opts.body)) util.destroy(this.opts.body.on('error', noop))
this.opts.body = null
}
}
Expand Down
8 changes: 4 additions & 4 deletions test/interceptors/redirect.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ for (const factory of [
await t.completed
})

test('should follow redirection after a HTTP 301', async t => {
test('should follow redirection after a HTTP 301 changing method to GET', async t => {
t = tspl(t, { plan: 3 })

const server = await startRedirectingServer()
Expand All @@ -188,7 +188,7 @@ for (const factory of [
t.ok(!headers.location)
t.strictEqual(
body,
`POST /5 :: host@${server} connection@keep-alive content-length@7 :: REQUEST`
`GET /5 :: host@${server} connection@keep-alive`
)
})

Expand Down Expand Up @@ -540,7 +540,7 @@ for (const factory of [
headers,
body: bodyStream
} = await request(t, server, undefined, `http://${server}/301`, {
method: 'POST',
method: 'PUT',
body: createReadableStream('REQUEST'),
maxRedirections: 10
})
Expand Down Expand Up @@ -608,7 +608,7 @@ test('should follow redirections when going cross origin', async t => {
`http://${server1}/end`
]
)
t.strictEqual(body, 'POST')
t.strictEqual(body, 'GET')

await t.completed
})
Expand Down
6 changes: 3 additions & 3 deletions test/redirect-request.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ for (const factory of [

t.strictEqual(statusCode, 200)
t.ok(!headers.location)
t.strictEqual(body, `POST /5 :: host@${server} connection@keep-alive content-length@7 :: REQUEST`)
t.strictEqual(body, `GET /5 :: host@${server} connection@keep-alive`)
})

test('should follow redirection after a HTTP 302', async t => {
Expand Down Expand Up @@ -485,7 +485,7 @@ for (const factory of [
const server = await startRedirectingServer()

const { statusCode, headers, body: bodyStream } = await request(t, server, undefined, `http://${server}/301`, {
method: 'POST',
method: 'PUT',
body: createReadableStream('REQUEST'),
maxRedirections: 10
})
Expand Down Expand Up @@ -541,7 +541,7 @@ test('should follow redirections when going cross origin', async t => {
`http://${server3}/end`,
`http://${server1}/end`
])
t.strictEqual(body, 'POST')
t.strictEqual(body, 'GET')

await t.completed
})
Expand Down
6 changes: 3 additions & 3 deletions test/redirect-stream.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ test('should follow redirection after a HTTP 300', async t => {
t.strictEqual(body.join(''), `GET /5 key=value :: host@${server} connection@keep-alive`)
})

test('should follow redirection after a HTTP 301', async t => {
test('should follow redirection after a HTTP 301 changing method to GET', async t => {
t = tspl(t, { plan: 3 })

const body = []
Expand All @@ -97,7 +97,7 @@ test('should follow redirection after a HTTP 301', async t => {
}
)

t.strictEqual(body.join(''), `POST /5 :: host@${server} connection@keep-alive content-length@7 :: REQUEST`)
t.strictEqual(body.join(''), `GET /5 :: host@${server} connection@keep-alive`)
})

test('should follow redirection after a HTTP 302', async t => {
Expand Down Expand Up @@ -316,7 +316,7 @@ test('should follow redirections when going cross origin', async t => {
}
)

t.strictEqual(body.join(''), 'POST')
t.strictEqual(body.join(''), 'GET')
})

describe('when a Location response header is NOT present', async () => {
Expand Down
Loading