Skip to content

fix: Redact password field in RemoteInfo type #1322

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mem
Copy link
Contributor

@mem mem commented May 26, 2025

When logging using zerolog or when marshaling to JSON, we don't really have a need to log the actual password and instead we have a requirement not to do so.

Add that behavior by implementing a couple of interfaces, one for zerolog and one for the JSON marshaller (stdlib).

@mem mem requested a review from a team as a code owner May 26, 2025 07:05
@mem mem requested review from the-it and ka3de May 26, 2025 07:05
type T RemoteInfo
var tmp T = T(ri)

tmp.Password = `<redacted>`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm probably messing up something by using redacted. It should be <encrypted>, or at least I think the frontend might be expecting that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed this to <encrypted> because that is what the API is using in some cases.

<encrypted> makes sense in that case because the API is encrypting these tokens in the database. <redacted> would be more appropriate in the general case, but we need to confirm we can make that change without breaking the frontend.

@mem mem force-pushed the mem/redact-password-in-remote-info branch from 99ff765 to 8ce00f8 Compare May 26, 2025 11:02
The-9880
The-9880 previously approved these changes Jun 2, 2025
When logging using zerolog or when marshaling to JSON, we don't really
have a need to log the actual password and instead we have a requirement
not to do so.

Add that behavior by implementing a couple of interfaces, one for
zerolog and one for the JSON marshaller (stdlib).

Signed-off-by: Marcelo E. Magallon <[email protected]>
@mem mem force-pushed the mem/redact-password-in-remote-info branch from 8ce00f8 to 8e85fe4 Compare June 9, 2025 13:50
@mem mem requested a review from The-9880 June 11, 2025 17:23
@The-9880
Copy link
Contributor

Implementation-wise, this looks fine to me.

I'll confirm with FE how this value is being used before approving so that we're sure it doesn't break some assumption.

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

Successfully merging this pull request may close these issues.

2 participants