Skip to content

Ignore __proto__ key when copying properties - #162

Open
spokodev wants to merge 1 commit into
jshttp:masterfrom
spokodev:fix/ignore-proto-in-properties
Open

Ignore __proto__ key when copying properties#162
spokodev wants to merge 1 commit into
jshttp:masterfrom
spokodev:fix/ignore-proto-in-properties

Conversation

@spokodev

Copy link
Copy Markdown

createError(status, [msg|err], properties) copies properties onto the error with err[key] = props[key]. When properties has an own __proto__ key (for example from JSON.parse of partially-untrusted data), the bracket assignment invokes the __proto__ setter and calls Object.setPrototypeOf(err, ...), so err instanceof Error, err instanceof createError.HttpError and createError.isHttpError(err) all become false on an error the module just produced.

The README states the (status, error, properties) form "will not alter the inheritance of the given error object", and an existing test already asserts the prototype is preserved. This skips the __proto__ key alongside status/statusCode in the copy loop. The effect is scoped to the created object; there is no global Object.prototype pollution.

@kilisamemarisaaakilisamemarisaaa left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I reviewed this change at head b4ed2f43815f4d96888b58e13a220d4f0368e812.

The guard correctly prevents both own and inherited enumerable __proto__ properties from being assigned while preserving ordinary properties. I independently verified:

  • JSON-parsed own __proto__ does not add evil or change the error prototype
  • an inherited enumerable __proto__ is also ignored
  • Error.prototype and createError.isHttpError() remain correct
  • Object.prototype is not polluted
  • project tests: 53 passing, 1 pending; lint passes

No blocking issues found.

Sign up for freeto 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.

2 participants

@spokodev@kilisamemarisaaa