Repository navigation
Conversation
With GOOGLE_SDK_NODE_LOGGING=auth (or * or all), the AuthClient response interceptor logs each response body. Token refreshes go through the same transporter, so access_token, id_token and refresh_token were printed to stderr. Replace access_token, id_token, refresh_token, accessToken and token in a copy of the body before logging it. The response the caller gets is unchanged. Refs googleapis#7690
There was a problem hiding this comment.
Code Review
This pull request introduces credential redaction for logged response and error bodies in the AuthClient class to prevent sensitive tokens from being exposed in logs. It defines a list of sensitive fields and a redactForLog helper method, which is integrated into the default response and error interceptors, along with corresponding unit tests. The review feedback correctly points out a potential issue where passing non-plain objects like Buffers or Streams to redactForLog could lead to performance degradation or circular reference errors during object spreading, and suggests a robust check to only process plain objects.
| static redactForLog(data: unknown): unknown { | ||
| if (data === null || typeof data !== 'object' || Array.isArray(data)) { | ||
| return data; | ||
| } | ||
| const copy: Record<string, unknown> = {...data}; | ||
| for (const field of AuthClient.REDACTED_LOG_FIELDS) { | ||
| if (copy[field] !== undefined) { | ||
| copy[field] = '***REDACTED***'; | ||
| } | ||
| } | ||
| return copy; | ||
| } |
There was a problem hiding this comment.
If data is a Buffer or a Stream (which can happen when downloading files or handling binary payloads), typeof data === 'object' is true and Array.isArray(data) is false. Spreading a Buffer or Stream using {...data} will:
- For
Buffer, create a massive object with numeric keys for every single byte, causing severe performance/memory issues and rendering the log output useless. - For
Stream, copy internal stream properties, which can lead to circular reference errors or verbose internal state logs.
To prevent this, we should ensure we only attempt to copy and redact plain objects (e.g., those parsed from JSON). We can achieve this robustly by checking that the prototype of data is either Object.prototype or null.
static redactForLog(data: unknown): unknown {
if (data === null || typeof data !== 'object') {
return data;
}
const proto = Object.getPrototypeOf(data);
if (proto !== Object.prototype && proto !== null) {
return data;
}
const copy: Record<string, unknown> = {...(data as Record<string, unknown>)};
for (const field of AuthClient.REDACTED_LOG_FIELDS) {
if (copy[field] !== undefined) {
copy[field] = '***REDACTED***';
}
}
return copy;
}References
- Always validate and reject missing or falsy arguments upfront before performing property checks, to prevent TypeErrors.
- Do not use value?.toString() === '[object Object]' to verify if a variable is a plain object. Use robust checks instead.
There was a problem hiding this comment.
Fixed: only plain objects are copied now; Buffers, streams and other bodies are logged as before. Added a test.
Refs #7690
With
GOOGLE_SDK_NODE_LOGGING=auth(or*,all), theAuthClientresponse and error interceptors log the response body of every request. Token refreshes go through the same transporter, soaccess_token,id_tokenandrefresh_tokenare printed to stderr. The repro is in #7690 (comment).This change logs a copy of the body with
access_token,id_token,refresh_token,accessTokenandtokenreplaced by***REDACTED***, asgdchclient.tsalready does intoJSON. The response the caller gets is unchanged. Other fields are still logged.It does not cover gcp-metadata's
instance metadata is %slog from the original report.Tests: a new case in
test/test.authclient.tschecks both log paths and that the caller's body is unchanged. It fails without the change. The package's tests pass (957 passing), and eslint is clean on both changed files.