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

refactor: use object spread to copy error headers #79

Merged
merged 1 commit into from
Feb 10, 2025
Merged

refactor: use object spread to copy error headers #79

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions HISTORY.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ unreleased
* remove unnecessary devDependency `safe-buffer`
* remove `unpipe` package and use native `unpipe()` method
* remove unnecessary devDependency `readable-stream`
* refactor: use object spread to copy error headers

v2.0.0 / 2024-09-02
==================
Expand Down
10 changes: 1 addition & 9 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,15 +144,7 @@ function getErrorHeaders (err) {
return undefined
}

var headers = Object.create(null)
var keys = Object.keys(err.headers)

for (var i = 0; i < keys.length; i++) {
var key = keys[i]
headers[key] = err.headers[key]
}

return headers
return { ...err.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 is functionally different than the previous because this object does not have a null prototype. There are a few ways we could fix it, but I am not sure what would be best.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need the null prototype here because the only usage of the headers is to iterate over them and set them on the response. I think we don't even need to copy the headers we can iterate over them directly.

Copy link
Member

Choose a reason for hiding this comment

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

The null prototype helps protect against some forms of mistakes consumers of the package can make which lead to security issues, so it is important to be vigilant on this type of change. BUT! Now that I have had a second to look at it, I think you are correct because in this case the method is not exported and we are only using the return value within this package. I think that protects from all the possible mistakes end users could make.

Copy link
Member

Choose a reason for hiding this comment

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

Also, just be be sure, I went through the history of this a bit, links for reference:

7bfa2f7
ab539c3

I am guessing this was done out of an abundance of caution back then. I don't see a way this could become a security issue even in the original form. Maybe I am missing something, but I am again thinking this is safe to remove.

}

/**
Expand Down
Loading