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

Settings includes values on Object.prototype #4802

Closed
EvanHahn opened this issue Feb 3, 2022 · 7 comments
Closed

Settings includes values on Object.prototype #4802

EvanHahn opened this issue Feb 3, 2022 · 7 comments

Comments

@EvanHahn
Copy link
Contributor

EvanHahn commented Feb 3, 2022

App settings pull from Object.prototype if they're missing on the settings object.

const app = require('express')();

app.get('hasOwnProperty');
// => [Function: hasOwnProperty]

app.enabled('hasOwnProperty');
// => true

Is this intentional? If not, I'm happy to help fix.

@dougwilson
Copy link
Contributor

Hi, yea, definitely a useless feature 😀 . I'd be happy to accept a fix; app.settings.hasOwnProperty() should still be there for back compact, but using the .get and helpers probably shouldn't return them since it is quite a useless value. Ultimately it is not surprising this happens, as it is no different from just writing app.settings.hasOwnProperty.

We could also make settings a null prototype object in the 5.0 branch.

@EvanHahn
Copy link
Contributor Author

EvanHahn commented Feb 3, 2022

I'll make two changes:

  1. In 4.x, make app.disabled, app.enabled, app.get, and app.set(oneArgument) ignore inherited properties on app.settings.
  2. In 5.x, change settings from {} to Object.create(null).

Does that sound reasonable? If so, I can submit a patch.

@dougwilson
Copy link
Contributor

It sounds good, though I don't think you can just ignore any inherited value -- that is how the settings are inherited from parent apps to sub apps.

@dougwilson
Copy link
Contributor

Likely good enough to just filter out if it in Object.prototype.

@EvanHahn
Copy link
Contributor Author

EvanHahn commented Feb 3, 2022

Opened #4803. If that looks good, I'll make a similar PR for 5.0.

@dougwilson dougwilson mentioned this issue Feb 19, 2022
20 tasks
@dougwilson dougwilson added this to the 4.18 milestone Feb 19, 2022
@EvanHahn
Copy link
Contributor Author

Now that #4803 is merged, I'll plan to make a similar PR for the 5.x branch.

I hope to get to that in the next few days.

@EvanHahn
Copy link
Contributor Author

Opened #4835.

@dougwilson dougwilson removed this from the 4.18 milestone Mar 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants