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

Redact passwords provided in URL when passing errors to callbacks #283

Merged

Conversation

alxndrsn
Copy link

@alxndrsn alxndrsn force-pushed the redact-passwords-in-error-meta branch 2 times, most recently from b8ee96b to a7527e0 Compare May 17, 2019 11:56
@alxndrsn alxndrsn changed the title Redact passwords provided in URL when passing errors to callbacks wip: Redact passwords provided in URL when passing errors to callbacks May 17, 2019
@alxndrsn alxndrsn force-pushed the redact-passwords-in-error-meta branch from a7527e0 to cbb04f5 Compare May 17, 2019 13:39
@alxndrsn alxndrsn force-pushed the redact-passwords-in-error-meta branch from cbb04f5 to bad0701 Compare May 17, 2019 13:44
@alxndrsn alxndrsn changed the title wip: Redact passwords provided in URL when passing errors to callbacks Redact passwords provided in URL when passing errors to callbacks May 17, 2019
Copy link
Member

@rachaelshaw rachaelshaw left a comment

Choose a reason for hiding this comment

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

@alxndrsn before I merge this in, would you take one more look to and see if there's another property that could be used to uniquely identify the type of error that's leaking these credentials? (Since using a regular expression for this has the potential for false positives, it'd be good to see if there's another way before resorting to that.)

@alxndrsn
Copy link
Author

@rachaelshaw I understand and share your hesitation - this is hardly pretty code. OTOH I've found it very hard to trace through exactly where these errors are generated, and at this level it would be hard to match which errors are affected (at least at this level), as it seems that:

  1. all errors generated in the database adapter will include the meta section from the sails config
  2. regex 1 is unlikely to have false-positives
  3. (all?) errors containing structured info (including .meta) are converted into strings at some lower level than this plugin, hence the match-all nature of the second regex. This isn't great, and I can't recall exactly where it happens (perhaps in machine itself?).

Perhaps a lower-level fix would happen in machinepack-postrgresql, as this appears to be where the meta object is attached to every error. At this point, the regex could be applied directly to the meta.url value, or perhaps other included values (is there also an option to specify username and password separately? https://sailsjs.com/documentation/concepts/extending-sails/adapters/available-adapters implies not.)

@alxndrsn
Copy link
Author

@rachaelshaw any more thoughts on this?

@Noitidart
Copy link

False positives is much better than leaving this out there. I would say merge this.

var REDACT_REGEX_MULTI = /(postgres:\/\/[^:\s]*):[^@\s]*@/g;

if(err) {
if(err.meta && typeof err.meta === 'object' && err.meta.url && typeof err.meta.url === 'string') {
Copy link
Author

Choose a reason for hiding this comment

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

I think this can be simplified to:

if(typeof err.meta === 'object' && typeof err.meta.url === 'string') {

if(err.meta && typeof err.meta === 'object' && err.meta.url && typeof err.meta.url === 'string') {
err.meta.url = err.meta.url.replace(REDACT_REGEX_SINGLE, REDACT_REPLACEMENT);
}
if(err.message && typeof err.message === 'string') {
Copy link
Author

Choose a reason for hiding this comment

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

As above, I think the initial check can be removed, so this line would just be:

if(typeof err.message === 'string') {

@johnabrams7
Copy link
Contributor

johnabrams7 commented Nov 18, 2019

@alxndrsn Sorry for the delay, after fully testing the new code, I was able to successfully hide/censor the PostgreSQL database passwords with asterisks. Merging this in, thanks again!

@johnabrams7 johnabrams7 merged commit 0e2e45a into balderdashy:master Nov 18, 2019
@alxndrsn
Copy link
Author

@johnabrams7 thanks for the merge. Any idea when this might get a public release?

@Noitidart
Copy link

Has this been publically released?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
4 participants