Redact URLs before logging or returning in error #2643
Conversation
|
I looked at the time but didn't see any framework for testing the logged strings sanely, so I passed on it. I can do tests for the auxiliary helpers but that's not E2E so only tests that redaction works. |
|
@kozlovic may be able to assist, he has a dummy logger for checking that certain things get logged etc. |
|
@philpennock Here is an example on how to use a logger that capture debug statements: nats-server/server/mqtt_test.go Line 5271 in 208146a |
philpennock
force-pushed
the
pdp/redact-urls
branch
from
60ad149 to
e70f3c0
Compare
There was a problem hiding this comment.
LGTM. For the t.Errorf(), I personally think that they are not required and we should always use t.Fatalf(). Errorf() is ok if you want to list all possible issues, but again, as part of dev when fixing the issue(s), but once finalized, there is no point in having it. But this is not a blocker, more of a personal preference.
philpennock
force-pushed
the
pdp/redact-urls
branch
from
452320e to
e320be4
Compare
|
squashed down to 3 commits with distinct changes, and rebased to current main. Still need to sort out the solicited gone/back test scenario, but Ivan has provided me with guidance on triggering that; once that's in, we'll be good. |
This does not affect strings which failed to parse, and in such a scenario there's a mix of "which evil" to accept; we can't sanely find what should be redacted in those cases, so we leave them alone for debugging. The JWT library returns some errors for Operator URLs, but it rejects URLs which contain userinfo, so there can't be passwords in those and they're safe. Fixes #2597
Create internal/testhelper and move DummyLogger there, so it can be used from the test/ sub-dir too. Let DummyLogger optionally accumulate all log messages, not just retain the last-seen message. Confirm no passwords logged by TestLeafNodeBasicAuthFailover. Change TestNoPasswordsFromConnectTrace to check all trace messages, not just the most recent. Validate existing trace redaction in TestRouteToSelf.
philpennock
force-pushed
the
pdp/redact-urls
branch
from
e320be4 to
468cc13
Compare
This does not affect strings which failed to parse, and in such a scenario there's a mix of "which evil" to accept; we can't sanely find what should be redacted in those cases, so we leave them alone for debugging.
The JWT library returns some errors for Operator URLs, but it rejects URLs which contain userinfo, so there can't be passwords in those and they're safe.
Fixes #2597
/cc @nats-io/core