Skip to content

Settings includes values on Object.prototype #4802

Closed
@EvanHahn

Description

@EvanHahn
Contributor

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.

Activity

dougwilson

dougwilson commented on Feb 3, 2022

@dougwilson
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

EvanHahn commented on Feb 3, 2022

@EvanHahn
ContributorAuthor

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

dougwilson commented on Feb 3, 2022

@dougwilson
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

dougwilson commented on Feb 3, 2022

@dougwilson
Contributor

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

EvanHahn

EvanHahn commented on Feb 3, 2022

@EvanHahn
ContributorAuthor

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

added this to the 4.18 milestone on Feb 19, 2022
added a commit that references this issue on Feb 21, 2022
2ddae30
EvanHahn

EvanHahn commented on Feb 21, 2022

@EvanHahn
ContributorAuthor

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

EvanHahn commented on Feb 21, 2022

@EvanHahn
ContributorAuthor

Opened #4835.

added a commit that references this issue on Mar 24, 2022
c17fe05

2 remaining items

Loading
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

      Development

      No branches or pull requests

        Participants

        @dougwilson@EvanHahn

        Issue actions

          Settings includes values on Object.prototype · Issue #4802 · expressjs/express