fix(core): censor novel secrets in querystring #526
Conversation
| @@ -14,22 +14,29 @@ const { | |||
| } = require('../src/http-middlewares/after/log-response'); | |||
|
|
|||
| // little helper to prepare a req/res pair like the http logger does | |||
| const prepareTestRequest = (reqBody = {}, resBody = {}) => | |||
| const prepareTestRequest = ({ | |||
| reqBody = {}, | |||
There was a problem hiding this comment.
Declaring params like this make them mostly function like python kwargs
| @@ -124,7 +128,13 @@ const buildSensitiveValues = (event, data) => { | |||
| const authData = bundle.authData || {}; | |||
| // for the most part, we should censor all the values from authData | |||
| // the exception is safe urls, which should be filtered out - we want those to be logged | |||
| // but, we _should_ censor-no-matter-what sensitive keys, even if their value is a safe url | |||
There was a problem hiding this comment.
weirdly, the logger.safe urls test was failing for me locally, but not in CI in the last PR. Bumping secret-scrubber locally didn't seem to change that. Not sure why CI didn't flag anything ¯\_(ツ)_/¯
There was a problem hiding this comment.
That's odd. It passes for me locally. Just curious, what's your failing message?
There was a problem hiding this comment.
Right? Weirdest thing.
Running yarn main-tests -g "safe urls" in core directory gives:
- "password": "https://a-url-like.password.com"
+ "password": ":censored:31:0dbc81268a:"
Same thing happens when running the full suite.
It sort of makes sense- the value is a url and doesn't have basic auth or a querystring. So I'm not sure why the test passed before. But, I think the current behavior is what we want. If something is explicitly a sensitive key, we should censor it. If it's not (but is in auth data), it's probably safe.
Follow up to #525 that covers similar behavior in the querystring.
Didn't catch it before because I passed the url
example.com?api_key=secret, but prep request separates them, so we have to searchrequest_paramsexplicitly too.Underscores the importance of our unit tests matching what actually gets called at runtime