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

Remove password from stringified outputs #2070

Conversation

charmander
Copy link
Collaborator

No description provided.

@charmander charmander force-pushed the make-password-non-enumerable branch 3 times, most recently from fd53cf0 to e8080cf Compare January 13, 2020 19:20
`Client` writes to it when `password` is a function.
when it didn’t exist previously.
to avoid breaking uses like `new Pool(existingPool.options)`.
in formatting and configurability.
@charmander charmander marked this pull request as ready for review January 13, 2020 19:23
@brianc brianc merged commit 1091d7c into brianc:bmc/make-password-non-enumerable Jan 13, 2020
brianc added a commit that referenced this pull request Jan 13, 2020
* Remove password from stringified outputs

Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password.  To widen the pit of success I'm making that field non-enumerable.  You can still get at it...it just wont show up "by accident" when you're logging things now.

The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0.

* Implement feedback

* Fix more whitespace the autoformatter changed

* Simplify code a bit

* Remove password from stringified outputs (#2070)

* Keep ConnectionParameters’s password property writable

`Client` writes to it when `password` is a function.

* Avoid creating password property on pool options

when it didn’t exist previously.

* Allow password option to be non-enumerable

to avoid breaking uses like `new Pool(existingPool.options)`.

* Make password property definitions consistent

in formatting and configurability.

Co-authored-by: Charmander <~@charmander.me>
@charmander charmander deleted the make-password-non-enumerable branch January 13, 2020 20:34
@charmander
Copy link
Collaborator Author

I just realized that 410b89b fixes the problem for Pool when used from pg-pool, but not when used from pg (because its BoundPool extends the pool’s options with an Object.assign). Should the same fix go in there to support new pg.Pool(otherPool.options), or does that seem like too much of a hack to avoid swapping out Object.assign with something that doesn’t care about whether properties are inherited or enumerable? @brianc

@brianc
Copy link
Owner

brianc commented Jan 15, 2020

I'm not following what you're saying exactly? If you do

const newPool = new pg.Pool(otherPool.options)
console.log(JSON.stringify(newPool))

the password shows up again? Dang...yeah I mean ideally there's no way to accidentally log out a password...regardless of how objects are constructed.

@charmander
Copy link
Collaborator Author

@brianc The password doesn’t show up, but that pattern will stop working, because the Object.assign that copies otherPool.options doesn’t include the password property.

@brianc
Copy link
Owner

brianc commented Jan 15, 2020

oh i see what you're saying - so we could special case it to be copied in if you do that?  ugg....hmmm I mean we could special case copying password over, mutate the options argument and append the Client directly to it, make the pg.Pool constructor take a second (undocumented) argument for the Client constructor to use (gross).  We could also do something like...

class BoundPool {
 constructor (options) { 
    super(options)
    this.options.Client = Client
  }
}

That's actually pretty clean IMO and isn't even more code or anything. Should fix the problem too, right?

@charmander
Copy link
Collaborator Author

@brianc Ah, nice solution! That would be perfect, except there’s this in pg-pool:

this.Client = this.options.Client || Client || require('pg').Client

which makes it do require('pg'), which is probably fine because of the peer dependency but still a little weird. However, I think || Client might achieve exactly what’s needed:

super(options, Client)

which also defaults to the Client being passed into BoundPool if one exists, instead of overwriting it. \o/

brianc added a commit that referenced this pull request Jan 28, 2020
* Remove password from stringified outputs

Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password.  To widen the pit of success I'm making that field non-enumerable.  You can still get at it...it just wont show up "by accident" when you're logging things now.

The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0.

* Implement feedback

* Fix more whitespace the autoformatter changed

* Simplify code a bit

* Remove password from stringified outputs (#2070)

* Keep ConnectionParameters’s password property writable

`Client` writes to it when `password` is a function.

* Avoid creating password property on pool options

when it didn’t exist previously.

* Allow password option to be non-enumerable

to avoid breaking uses like `new Pool(existingPool.options)`.

* Make password property definitions consistent

in formatting and configurability.

Co-authored-by: Charmander <~@charmander.me>
brianc added a commit that referenced this pull request Mar 30, 2020
* Drop support for EOL versions of node (#2062)

* Drop support for EOL versions of node

* Re-add testing for node@8.x

* Revert changes to .travis.yml

* Update packages/pg-pool/package.json

Co-Authored-By: Charmander <~@charmander.me>

Co-authored-by: Charmander <~@charmander.me>

* Remove password from stringified outputs (#2066)

* Remove password from stringified outputs

Theres a security concern where if you're not careful and you include your client or pool instance in console.log or stack traces it might include the database password.  To widen the pit of success I'm making that field non-enumerable.  You can still get at it...it just wont show up "by accident" when you're logging things now.

The backwards compatiblity impact of this is very small, but it is still technically somewhat an API change so...8.0.

* Implement feedback

* Fix more whitespace the autoformatter changed

* Simplify code a bit

* Remove password from stringified outputs (#2070)

* Keep ConnectionParameters’s password property writable

`Client` writes to it when `password` is a function.

* Avoid creating password property on pool options

when it didn’t exist previously.

* Allow password option to be non-enumerable

to avoid breaking uses like `new Pool(existingPool.options)`.

* Make password property definitions consistent

in formatting and configurability.

Co-authored-by: Charmander <~@charmander.me>

* Make `native` non-enumerable (#2065)

* Make `native` non-enumerable

Making it non-enumerable means less spurious "Cannot find module"
errors in your logs when iterating over `pg` objects.

`Object.defineProperty` has been available since Node 0.12.

See #1894 (comment)

* Add test for `native` enumeration

Co-authored-by: Gabe Gorelick <gabegorelick@gmail.com>

* Use class-extends to wrap Pool (#1541)

* Use class-extends to wrap Pool

* Minimize diff

* Test `BoundPool` inheritance

Co-authored-by: Charmander <~@charmander.me>
Co-authored-by: Brian C <brian.m.carlson@gmail.com>

* Continue support for creating a pg.Pool from another instance’s options (#2076)

* Add failing test for creating a `BoundPool` from another instance’s settings

* Continue support for creating a pg.Pool from another instance’s options

by dropping the requirement for the `password` property to be enumerable.

* Use user name as default database when user is non-default (#1679)

Not entirely backwards-compatible.

* Make native client password property consistent with others

i.e. configurable.

* Make notice messages not an instance of Error (#2090)

* Make notice messages not an instance of Error

Slight API cleanup to make a notice instance the same shape as it was, but not be an instance of error.  This is a backwards incompatible change though I expect the impact to be minimal.

Closes #1982

* skip notice test in travis

* Pin node@13.6 for regression in async iterators

* Check and see if node 13.8 is still borked on async iterator

* Yeah, node still has changed edge case behavior on stream

* Emit notice messages on travis

* Revert "Revert "Support additional tls.connect() options (#1996)" (#2010)" (#2113)

This reverts commit 510a273.

* Fix ssl tests (#2116)

* Convert Query to an ES6 class (#2126)

The last missing `new` deprecation warning for pg 8.

Co-authored-by: Charmander <~@charmander.me>
Co-authored-by: Gabe Gorelick <gabegorelick@gmail.com>
Co-authored-by: Natalie Wolfe <natalie@lifewanted.com>
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.

None yet

2 participants