fix(core): log censoring is slow if response has many URLs #524
Conversation
There was a problem hiding this comment.
Ok, so the salient change here is that instead of using findSensitiveValues(data) (which finds sensitive keys and urls w/ querystrings), we instead only find sensitive keys (under the assumption that the logged body won't contain any new secrets not already present in authData.
I'm a little wary of censoring fewer things, but that does seem like a corner case. All else equal, I'd have focused on speed improvements in secret scrubber (if possible), but I think this is a reasonable first step.
Thanks for digging in here!
| json: { | ||
| data: [ | ||
| // An app could have thousands of URLs in a response. We don't | ||
| // want these to be in the sensitive vallues and make secret |
|
@xavdid I was wondering why we were censoring all the URLs (with query params) from the log extra in the first place. Did we do that to fix another issue where logs weren't censored enough? If yes, merging this PR would create another regression (where logs are not censored enough).
I also think fixing in secret-scrubber is better. Can you do it? We don't have to rush to merge this PR, since we can downgrade an app to 9.7.3 to fix the timeout issue. |
Yes, I think in a couple of different places. Especially during auth refresh, urls will have query params that need to be censored (but have new values that aren't in I think it's fair to exclude the request body from checking sensitive urls - that does seem like a corner case. But, we should probably keep checking them for auth requests and in messages itself. I think in interest of not causing regressions, downgrading affected apps for now is the way to go and I can take a pass at profiling and improving the url censoring in scrubber. If I can't get to the bottom of it in a day or two, we can move forward here and adjust if we notice any regressions. |
| // extra should be safe. | ||
| const sensitiveLogData = recurseExtract( | ||
| data, | ||
| (key, value) => _.isString(value) && isSensitiveKey(key) |
There was a problem hiding this comment.
sensitive key -> numeric value should also be caught. eg {secretNumber: 123456789}
|
fixed in secret scrubber, closing |
Fixes an issue where log censoring becomes too slow when an app gives a response that has many (like thousands) URLs. The new test case should be self-explanatory.