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.

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: handle Headers in RedirectHandler #3777

Merged
merged 4 commits into from
Nov 7, 2024

Conversation

iiAku
Copy link
Contributor

@iiAku iiAku commented Oct 27, 2024

This relates to...

This should close #3773

Rationale

Changes

Features

Bug Fixes

Breaking Changes and Deprecations

Status

@iiAku iiAku force-pushed the fix/redirect-handler-headers branch from e4ddac9 to c1c78c2 Compare October 27, 2024 23:36
Copy link
Member

@metcoder95 metcoder95 left a comment

Choose a reason for hiding this comment

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

Can you add tests for it?

@iiAku iiAku force-pushed the fix/redirect-handler-headers branch from b941ad4 to 71c9baf Compare October 28, 2024 18:00
@iiAku iiAku requested a review from metcoder95 October 28, 2024 18:10
@@ -239,3 +240,4 @@ function cleanRequestHeaders (headers, removeContent, unknownOrigin) {
}

module.exports = RedirectHandler
module.exports.cleanRequestHeaders = cleanRequestHeaders
Copy link
Member

Choose a reason for hiding this comment

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

Let's not do that if we are just exporting it to testing it out.

You can attempt to test the same on the redirect-request test suite where you submit a Headers instance instead of the array/object way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added tests in appropriate file

lib/handler/redirect-handler.js Show resolved Hide resolved
@iiAku iiAku force-pushed the fix/redirect-handler-headers branch from 7c891d5 to e4768c4 Compare October 29, 2024 20:18
@iiAku iiAku requested a review from metcoder95 October 29, 2024 20:42
@ronag ronag merged commit dade9de into nodejs:main Nov 7, 2024
29 of 36 checks passed
@@ -227,9 +227,10 @@ function cleanRequestHeaders (headers, removeContent, unknownOrigin) {
}
}
} else if (headers && typeof headers === 'object') {
for (const key of Object.keys(headers)) {
const entries = headers instanceof Headers ? headers.entries() : Object.entries(headers)
Copy link
Member

Choose a reason for hiding this comment

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

this won't work for Maps, only Headers

Copy link
Member

Choose a reason for hiding this comment

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

maybe better to check for Symbol.Iterable

Copy link
Member

Choose a reason for hiding this comment

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

@iiAku can you create a follow up PR?

const entries = utils.isIterable(headers) ? headers : Object.entries(headers)

Copy link
Member

Choose a reason for hiding this comment

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

this also only works with the global Headers, not Headers imported from undici

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@iiAku can you create a follow up PR?

const entries = utils.isIterable(headers) ? headers : Object.entries(headers)

Yes I can look into this, thanks for reviewing raising that point @KhafraDev

flakey5 pushed a commit to flakey5/undici that referenced this pull request Nov 14, 2024
@github-actions github-actions bot mentioned this pull request Dec 3, 2024
This was referenced Dec 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RedirectHandler cleans headers too aggressively
4 participants