-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
There was a problem hiding this 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 } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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:
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.
There was a problem hiding this 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.
There was a problem hiding this 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.
Refactored the
getErrorHeaders
function to use object spread syntax to copy error headers, replacing the manual loop.