fix: redact sensitive fields in logs and remove auto-logging of args/results (WAL-9455)#1687
Merged
albertoelias-crossmint merged 3 commits intowallets-v1from Mar 20, 2026
Conversation
…results (WAL-9455) - Add redactSensitiveFields() utility that recursively strips JWTs, API keys, tokens, passwords, and other credentials from log context objects - Apply redaction in SdkLogger.log() before context reaches any sink - Remove automatic args/result logging from WithLoggerContext decorator (methods already have their own curated log statements with safe fields) - Fix buildContext in wallet-factory.ts to only pass chain instead of full args - Fix getWallet success log in client.ts to log only address instead of full response Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
|
Contributor
Original prompt from Alberto Elias
|
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/common/base/src/logger/utils.ts
Line: 45-63
Comment:
**Error instances silently reduced to `{}`**
`redactSensitiveFields` calls `Object.entries(obj)` on whatever value it encounters. For native `Error` instances (the most common thing thrown in JavaScript), `message`, `name`, and `stack` are **non-enumerable** properties, so `Object.entries(error)` returns `[]` and the function returns an empty `{}` object.
This matters because the decorator now calls:
```ts
options.logger.error(`${options.methodName} threw an error`, { error });
```
After `redactSensitiveFields` runs in `SdkLogger.log()`, the `error` key in the log context will be `{}` — the stack trace, message, and error name are completely gone. This is a silent regression in error observability.
A minimal fix would be to detect Error instances and handle them like `serializeLogArgs` already does:
```ts
export function redactSensitiveFields(obj: unknown, depth = 0): unknown {
if (depth > MAX_REDACTION_DEPTH || obj == null || typeof obj !== "object") {
return obj;
}
// Error instances have non-enumerable properties; preserve them explicitly
if (obj instanceof Error) {
return {
name: obj.name,
message: obj.message,
stack: obj.stack,
// Any enumerable own properties (e.g. custom error fields)
...redactSensitiveFields(Object.fromEntries(Object.entries(obj)), depth + 1) as object,
};
}
// ... rest unchanged
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/common/base/src/logger/utils.ts
Line: 29-34
Comment:
**Partial redaction leaks too much of short secrets**
For a string of length 9 (e.g. a 9-character password or short API key), `redactValue` returns `${value.slice(0, 4)}...${value.slice(-4)}`, which exposes 8 of the 9 characters. That effectively defeats redaction for anything close to the 8-character threshold.
The PR description calls this out as a known trade-off, but it's worth resolving before merge. For truly sensitive keys (JWTs, API keys, passwords, private keys), there is no safe use-case for showing partial content — showing `[REDACTED]` universally is unambiguously safer:
```suggestion
function redactValue(_value: unknown): string {
return "[REDACTED]";
}
```
If correlation/debugging value from partial values is desired in the future, it can be added back with a more deliberate length threshold (e.g., only reveal last 4 chars for strings > 32 chars, similar to credit-card masking conventions).
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/common/base/src/logger/utils.ts
Line: 7-21
Comment:
**Common OAuth key names not covered**
`SENSITIVE_KEYS` includes `"refreshtoken"` (matching `refreshToken` after lowercasing), but is missing several common variants:
- `"access_token"` / `"accesstoken"` — standard OAuth access token field names
- `"client_secret"` — OAuth client secret (the set has `"secret"` which requires an exact match on the key `secret`, not `client_secret`)
- `"bearer"` — occasionally used as a context key for bearer tokens
```suggestion
const SENSITIVE_KEYS = new Set([
"jwt",
"apikey",
"api_key",
"authorization",
"x-api-key",
"token",
"accesstoken",
"access_token",
"refreshtoken",
"refresh_token",
"secret",
"client_secret",
"password",
"privatekey",
"private_key",
"credential",
"authdata",
"bearer",
]);
```
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix: redact sensitiv..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
🔥 Smoke Test Results✅ Status: Passed Statistics
✅ All smoke tests passed!All critical flows are working correctly. This is a non-blocking smoke test. Full regression tests run separately. |
maxsch-xmint
approved these changes
Mar 20, 2026
…handling, simplify redactValue - Remove generic 'token' from SENSITIVE_KEYS to avoid false-positive redaction of blockchain token identifiers (e.g. tokenLocator) - Replace with specific auth token keys: accesstoken, access_token, authtoken, auth_token, bearertoken, bearer_token, idtoken, id_token, refresh_token - Add client_secret and bearer to SENSITIVE_KEYS - Simplify redactValue to always return '[REDACTED]' (no partial leaking) - Handle Error instances explicitly to preserve name/message/stack (non-enumerable properties were lost with Object.entries) Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/common/base/src/logger/utils.ts
Line: 7-31
Comment:
**`"token"` missing from `SENSITIVE_KEYS` despite PR description**
The PR description explicitly states: *"The `token` key in `SENSITIVE_KEYS` is intentionally broad — it will redact any field named `token` including non-secret tokens (e.g. pagination tokens). Confirm this trade-off is acceptable."*
However, `"token"` is not present in the set. Any log context field literally named `token` (a very common pattern for auth tokens, e.g. `{ token: jwt }`) will pass through unredacted. Since this was called out as a deliberate inclusion in the description, its absence appears to be an accidental omission rather than an intentional removal.
```suggestion
const SENSITIVE_KEYS = new Set([
"jwt",
"token",
"apikey",
"api_key",
"authorization",
"x-api-key",
"accesstoken",
"access_token",
"authtoken",
"auth_token",
"bearertoken",
"bearer_token",
"idtoken",
"id_token",
"refreshtoken",
"refresh_token",
"secret",
"client_secret",
"password",
"privatekey",
"private_key",
"credential",
"authdata",
"bearer",
]);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/common/base/src/logger/utils.ts
Line: 35-37
Comment:
**Unused `_value` parameter in `redactValue`**
The `_value` parameter is prefixed with `_` to suppress the "unused variable" lint warning, but it's never read. This suggests the function was originally intended to do partial masking (as the PR description discusses) and was then simplified to always return `[REDACTED]`. The parameter can be removed entirely to make the intent clear.
```suggestion
function redactValue(): string {
return "[REDACTED]";
}
```
The call sites would need to be updated too:
```ts
result[key] = redactValue();
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: packages/common/base/src/logger/utils.ts
Line: 48-77
Comment:
**No unit tests for `redactSensitiveFields`**
This function is the core of the security fix and has several non-trivial code paths (array handling, `Error` instance serialisation, recursive depth capping, case-insensitive key matching). The existing custom engineering rule requires adding unit tests when implementing new validation logic or error handling features.
Given the security-sensitive nature of this function — a regression here could silently re-expose credentials — test coverage would be particularly valuable. Key cases to cover include:
- A top-level sensitive key is redacted
- Nested objects are recursively redacted
- `Error` instances retain `name`/`message`/`stack`
- Arrays are recursively processed
- Depth capping stops at `MAX_REDACTION_DEPTH`
- Non-sensitive keys pass through unchanged
**Rule Used:** Add unit tests when implementing new validation lo... ([source](https://app.greptile.com/review/custom-context?memory=9dc35ad5-5868-49a3-bbcb-a42edc5ee697))
**Learnt From**
[Paella-Labs/crossbit-main#21014](https://github.com/Paella-Labs/crossbit-main/pull/21014)
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix: address review ..." |
Comment on lines
+35
to
+37
| function redactValue(_value: unknown): string { | ||
| return "[REDACTED]"; | ||
| } |
Contributor
There was a problem hiding this comment.
_value parameter in redactValue
The _value parameter is prefixed with _ to suppress the "unused variable" lint warning, but it's never read. This suggests the function was originally intended to do partial masking (as the PR description discusses) and was then simplified to always return [REDACTED]. The parameter can be removed entirely to make the intent clear.
Suggested change
| function redactValue(_value: unknown): string { | |
| return "[REDACTED]"; | |
| } | |
| function redactValue(): string { | |
| return "[REDACTED]"; | |
| } |
The call sites would need to be updated too:
result[key] = redactValue();Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/logger/utils.ts
Line: 35-37
Comment:
**Unused `_value` parameter in `redactValue`**
The `_value` parameter is prefixed with `_` to suppress the "unused variable" lint warning, but it's never read. This suggests the function was originally intended to do partial masking (as the PR description discusses) and was then simplified to always return `[REDACTED]`. The parameter can be removed entirely to make the intent clear.
```suggestion
function redactValue(): string {
return "[REDACTED]";
}
```
The call sites would need to be updated too:
```ts
result[key] = redactValue();
```
How can I resolve this? If you propose a fix, please make it concise.
| if (typeof value === "string" && value.length > 8) { | ||
| return `${value.slice(0, 4)}...${value.slice(-4)}`; | ||
| } | ||
| function redactValue(_value: unknown): string { |
Collaborator
Author
There was a problem hiding this comment.
@devinai we want the previous behaviour
Contributor
There was a problem hiding this comment.
Restored the previous redactValue behavior in caa2e6e — strings >8 chars show first/last 4 chars (${value.slice(0, 4)}...${value.slice(-4)}), shorter values get [REDACTED].
Co-Authored-By: Alberto Elias <alberto.elias@paella.dev>
Contributor
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/common/base/src/logger/SdkLogger.ts
Line: 416-422
Comment:
**Log message string not covered by redaction**
`redactSensitiveFields` is applied to the merged context object, but the `serializedMessage` string is passed to log sinks as-is. If a caller interpolates a sensitive value directly into the message (e.g. `logger.info(\`Authenticated with token ${jwt}\`)`), it will appear unredacted in the final `LogEntry.message`.
This is a known limitation of key-based redaction, but it is worth documenting explicitly so consumers know the contract: sensitive values must go into the context object (where they will be redacted by key), not into the message string.
Consider adding a note to the `log` method or the public `info`/`debug`/`error` API comments clarifying that message strings are **not** scanned for sensitive patterns.
How can I resolve this? If you propose a fix, please make it concise.Last reviewed commit: "fix: restore partial..." |
albertoelias-crossmint
deleted the
devin/1773964118-wal-9455-redact-sensitive-logs
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Console logs in dev were exposing full JWTs and API keys via the
WithLoggerContextdecorator's automatic argument/result logging and through some explicit log statements that passed entire response objects. This PR addresses WAL-9455 with two layers of defense:Redaction utility (
utils.ts): AddsredactSensitiveFields()which recursively strips known-sensitive keys (jwt,apiKey,authorization,password,privateKey, auth tokens, etc.) from log context objects before they reach any sink. Applied centrally inSdkLogger.log(). HandlesErrorinstances explicitly to preserve non-enumerablename/message/stackproperties. String values >8 chars are partially truncated (first/last 4 chars shown); shorter values are fully replaced with[REDACTED].Remove auto-logging from decorator (
decorators.ts): TheWithLoggerContextdecorator no longer automatically logs{ args }on entry or{ result }on success. Error logging is preserved. Individual methods already have their own curated log statements with safe fields.Targeted log fixes:
getWalletsuccess log inclient.tsnow logs onlyaddress+locatorinstead of the full response.buildContextinwallet-factory.tsnow passes onlychaininstead of the full args object (which could contain signer configs).Things for reviewer to verify
.then()handler was removed along with the success log). Methods that lack their own success logging will now be silent on completion.redactSensitiveFields. Consider whether test coverage is needed before merge (especially theErrorinstance handling path)."token"key was intentionally excluded fromSENSITIVE_KEYSto avoid false-positive redaction of blockchain token identifiers (e.g.tokenLocator). Specific auth token variants (accesstoken,authtoken,bearertoken,idtoken,refreshtoken+ underscore variants) are covered instead.redactValueshows first/last 4 chars for strings >8 chars. For a 9-char secret this exposes 8 of 9 characters — this trade-off was explicitly approved by the reviewer for debugging value.Updates since last revision
"token"fromSENSITIVE_KEYSper reviewer feedback — replaced with specific auth token keys (accesstoken,access_token,authtoken,auth_token,bearertoken,bearer_token,idtoken,id_token,refresh_token). Addedclient_secretandbearer.Errorinstance handling inredactSensitiveFieldsto preservename/message/stack(non-enumerable properties that were previously lost viaObject.entries).redactValue(first/last 4 chars for strings >8 chars) per reviewer request.Test plan
biome check)pnpm build:libs)Package updates
None — internal-only changes to the logger pipeline, no public API surface changes.
Link to Devin session: https://crossmint.devinenterprise.com/sessions/a88ab08eaccc4c1bab8d4b5718be6638
Requested by: @albertoelias-crossmint