knex icon indicating copy to clipboard operation
knex copied to clipboard

fix: clone config in client constructor

Open castarco opened this issue 11 months ago • 13 comments

This PR ensures that config objects passed to client constructors are deep-cloned before marking some of its sensitive properties as non-enumerable.

Fixes #5629 .

castarco avatar Jul 16 '23 17:07 castarco

Coverage Status

coverage: 92.784% (-0.003%) from 92.787% when pulling 0567fecf7a69ce28cbcfc2bdf205441004ef01f0 on castarco:regression-check into 4ca3dd5bc28e0665c5bed55026fac2ec45489d81 on knex:master.

coveralls avatar Jul 16 '23 17:07 coveralls

Thanks for you work @castarco I will review as soon as possible (it's seems CI fail because of Node12...xd)

OlivierCavadenti avatar Jul 18 '23 07:07 OlivierCavadenti

🙏

falkenhawk avatar Aug 29 '23 10:08 falkenhawk

Rebased, seeing that my branch was a bit outdated.

castarco avatar Aug 29 '23 17:08 castarco

Seems like CI is being randomly flaky again 😅

daniellockyer avatar Aug 31 '23 15:08 daniellockyer

Any update on this?

SeraphimRP avatar Oct 26 '23 18:10 SeraphimRP

I don't know. Nobody gave me any feedback regarding this patch.

castarco avatar Oct 26 '23 20:10 castarco

will check it out this weeek, sorry

kibertoad avatar Oct 26 '23 21:10 kibertoad

Adding my completely unhelpful context that I'm running into this using mssql, and am hopeful this would resolve the login error we're running into that is currently preventing us from adopting 3.0.1 (doesn't occur on 2.4.2)

michaelpinnell avatar Dec 04 '23 21:12 michaelpinnell

I am waiting for the pull requests to be merged and the library versioned.😉

araera111 avatar Dec 13 '23 05:12 araera111

When will this fix be merged?

araera111 avatar Jan 24 '24 06:01 araera111

When will this fix be merged?

@araera111 I really don't know. I didn't get any extra feedback (besides a comment on test code that was only relevant for production code).

I guess I should rebase at some point, but I'm not sure it's worth investing time on it unless we get some guarantees that this has any chance of being merged.

castarco avatar Jan 24 '24 07:01 castarco

@OlivierCavadenti @kibertoad When will this fix be merged?

araera111 avatar Feb 15 '24 03:02 araera111

What's needed to move this PR across the line? How can I help? I just lost hours of debugging to figure out that knex was unexpectedly mutating a configuration object.

nbeyer avatar Mar 10 '24 16:03 nbeyer

@rluvaton could you please take a look?

kibertoad avatar Mar 10 '24 21:03 kibertoad

Would it be quicker to release a patch with https://github.com/knex/knex/pull/5559 reverted?

daniellockyer avatar Mar 11 '24 13:03 daniellockyer