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

refactor: use object spread to copy error headers #79

Merged
merged 1 commit into from
Feb 10, 2025

Conversation

Phillip9587
Copy link
Contributor

Refactored the getErrorHeaders function to use object spread syntax to copy error headers, replacing the manual loop.

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

Just dropping this quickly while I did a first pass. I would need to run a few examples to see if there is a meaningful difference in the ways we could simplify this while keeping the null prototype, and I don't have time for that at the moment.

}

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.

Copy link
Member

@wesleytodd wesleytodd left a comment

Choose a reason for hiding this comment

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

Ok, lets let it sit to see if we get any other feedback, but I am pretty convinced the null prototype was not strictly necessary.

Copy link
Contributor

@bjohansebas bjohansebas left a comment

Choose a reason for hiding this comment

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

Yep, I think we can safely make this change.

@wesleytodd wesleytodd merged commit 87ffb23 into pillarjs:master Feb 10, 2025
8 checks passed
@wesleytodd wesleytodd mentioned this pull request Feb 10, 2025
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.

3 participants