Repository navigation
fix(client): redact credentials from SDK object diagnostics - #351
Merged
Merged
Conversation
Quality Keeper — Advisory ReviewSource:
Quality Keeper assessed the repository, executed checks, and collected results, covering 19 quality expectations. Five repository-quality areasHard expectations keep their exact Evidence State. Advisory applicability questions are separate. Implementation or suite presence is not proof of a trusted run.
Observability — 0 hard, 0 advisoryCan operators understand and diagnose repository-owned runtime behavior? Hard expectations None selected. Applicability Review Gaps (advisory) None. Product Analytics — 0 hard, 0 advisoryDoes repository-owned product analytics instrumentation behave safely and as declared? Hard expectations None selected. Applicability Review Gaps (advisory) None. User Feedback — 0 hard, 0 advisoryDo repository-owned feedback and support-intake flows behave safely and accessibly? Hard expectations None selected. Applicability Review Gaps (advisory) None. Reliability — 3 hard, 0 advisoryDoes repository-owned software fail, recover, time out, and report health predictably? Hard expectations
Applicability Review Gaps (advisory) None. Security — 4 hard, 0 advisoryDo static controls and repository-owned security behaviors protect the intended boundaries? Hard expectations
Applicability Review Gaps (advisory) None. Other registered quality — 12 hard, 0 advisoryRegistered Testing Types not assigned to one of the five focused areas. Hard expectations
Applicability Review Gaps (advisory) None. Result
CoveredQuality Keeper found acceptable evidence for one or more expectations in these Testing Types.
Needs attentionQuality Keeper does not yet have acceptable evidence for these Testing Types. Some have partial checks that need to run, be fixed, or be connected. Others need new proof or seeded tests. In both cases, Quality Keeper cannot count them as covered yet.
Quality Keeper changes no Evidence State. Pending, missing, executed-but-not-exercised, and review items are not passing, and seeded scaffolds are next actions, not evidence, until applied and re-evaluated. Full breakdown (19 expectations)
New to these terms? Start with the Quality Keeper Standards Wiki to learn what each testing type means, why it applies, and what counts as evidence. |
Contributor
|
Thanks for working on this. We ran our repository's secrets-in-output check against the current head of this branch (905fea1), and it still reports remaining credential-bearing fields in the objects returned by authorize(), getUploadUrl() and getDownloadAuthorization(). Details are in our team's internal tracker. We have a small patch for the remaining fields and are happy to share it here if that would help. |
sophiecarreras
left a comment
sophiecarreras
left a comment
Contributor
There was a problem hiding this comment.
Thanks Felipe. Review from Sofia, with Claude's help. No credential leak found, so this is a comment, not a blocker, but the framing needs changes before merge.
What was verified (by running, on head of this PR): tsc --noEmit passes, both redaction suites pass (44/44), full suite passes (87 files, 2555 passed, 9 skipped). Reverting one layer at a time makes the tests fail (in-memory.ts 9, upload-url-pool.ts 3, bucket.ts 1, file.ts 3, client inspect.custom 3), so the tests do detect each fix. Symbol.toStringTag, structuredClone, getOwnPropertyDescriptors and a subclass with an extra secret field showed no leak.
Please change before merge
- The Proxy break is under "Security", not "Changed" (CHANGELOG.md:27-31).
new Proxy(client, {}).listBuckets()throwsCannot read private member #accountInfo, andnew Proxy(new InMemoryAccountInfo(), {}).getApiUrl()throws for#auth. That breaks anyone wrapping these in Vuereactive, MobX or an instrumentation proxy. The note names onlyclient;Bucket,B2Object,InMemoryAccountInfo,FileAccountInfoandUploadUrlPoolare affected too. - Semver and the
toJSONchange. The package is 0.4.0. The accessor change, the Proxy break andJSON.stringify(accountInfo)now returning{authorized}(in-memory.ts:396,file.ts:259) are breaking. Anyone who persisted a store withJSON.stringifysilently loses data, and the changelog does not say so. Suggest a 0.5.0 bump, a "Breaking" label, and a line on thetoJSONbehavior.
Nits
{...client}still exposesraw.transport, the rawrealmUrlandreauthCoalescer. No secret with the default setup, but a realm with userinfo (https://user:pass@host) shows in a spread whiletoJSONandinspectredact it.redaction.test.ts:1572covers onlytoJSON.client.accountInforeturns the user's own store, so logging a customAccountInfodirectly prints whatever it exposes (the JSDoc atclient.ts:820says so).- The reviewed-export list is a name snapshot: a contributor can append a name to pass it, and it checks a hand-picked fixture per class. A fine tripwire, not proof.
Not checked: error messages and RawClient request/response errors (untouched by this PR, not audited for token content); Bun, Deno and browser behavior of Symbol.for('nodejs.util.inspect.custom').
Needs a human: does any downstream package or the harness wrap SDK objects in a Proxy or read client.accountInfo as an own property? Does anyone rely on JSON.stringify(store)? Release owner to decide 0.5.0 vs patch.
Contributor
|
Patch for the remaining credential-bearing fields, as offered. Sofia, with Claude's help. Fields our secrets-in-output check flags on
Check (from
What the patch does: the four raw-client responses get non-enumerable Two things to know before taking it
Patch (git diff 905fea1..50d3a70, 6 files)diff --git a/CHANGELOG.md b/CHANGELOG.md
index f477184..943b9b2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -19,13 +19,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
`client.accountInfo` directly, and pass a custom store through the
constructor's `accountInfo` option.
+- **BREAKING: responses that carry `authorizationToken` hide it when serialized or
+ printed.** `JSON.stringify`, `console.log`, and `util.inspect` of the objects
+ returned by `authorize()`, `getUploadUrl()`, `getUploadPartUrl()`, and
+ `getDownloadAuthorization()` (and of `client.accountInfo.getAuth()`) show
+ `[redacted token]` instead of the token. The `authorizationToken` property is
+ unchanged, so `response.authorizationToken` and every SDK code path still read
+ the real token. To serialize a response that carries a token on purpose (for
+ example to hand a download authorization to a browser), copy it first:
+ `JSON.stringify({ ...response })`. A copy made that way is a plain object with
+ the real token, so treat it as credential material. Code that persisted
+ `JSON.stringify(response)` now stores the placeholder and must switch to the
+ copy.
+
### Security
- **SDK objects hide application keys and authorization tokens in diagnostics.**
Logging, inspecting, or serializing clients, bucket and file handles, auth
- stores, and upload URL pools no longer prints stored credentials. Explicit
- `authorize()` and `getAuth()` responses still contain the real token, and file
- auth caches still persist it.
+ stores, and upload URL pools no longer prints stored credentials. File auth
+ caches still persist the real token, and `response.authorizationToken` still
+ holds it.
Methods called through a `Proxy` wrapper (for example Vue `reactive(client)`)
now throw `Cannot read private member`, the same constraint as the Partner
client. Keep these SDK instances outside
diff --git a/src/auth/file.ts b/src/auth/file.ts
index d183c95..b3308f5 100644
--- a/src/auth/file.ts
+++ b/src/auth/file.ts
@@ -333,7 +333,7 @@ export class FileAccountInfo implements AccountInfo, AuthContextAwareAccountInfo
: {}),
},
})
- : JSON.stringify(auth)
+ : JSON.stringify({ ...auth })
await this.writePrivateFileAtomically(contents)
})
this.writeQueue = next.catch((error: unknown) => {
diff --git a/src/raw/index.ts b/src/raw/index.ts
index 0d9b8d5..1fb6903 100644
--- a/src/raw/index.ts
+++ b/src/raw/index.ts
@@ -17,6 +17,7 @@ import { assertValidBucketConfiguration } from '../bucket-validation.ts'
import { FinishLargeFileResponseBodyError, UploadResponseBodyError } from '../errors/index.ts'
import type { RetryOptions } from '../http/retry.ts'
import type { HttpTransport } from '../http/transport.ts'
+import { withRedactedTokenDiagnostics } from '../token-redaction.ts'
import type {
AllowedBucket,
ApplicationKey,
@@ -371,7 +372,9 @@ export class RawClient {
...(options?.signal !== undefined ? { signal: options.signal } : {}),
...(options?.retry !== undefined ? { retry: options.retry } : {}),
})
- return normalizeAuthorizeAccountResponse(await response.json<WireAuthorizeAccountResponse>())
+ return withRedactedTokenDiagnostics(
+ normalizeAuthorizeAccountResponse(await response.json<WireAuthorizeAccountResponse>()),
+ )
}
// --- Buckets ---
@@ -495,12 +498,14 @@ export class RawClient {
optionsOrSignal?: RawRequestOptions | AbortSignal,
retry?: Partial<RetryOptions>,
): Promise<GetUploadUrlResponse> {
- return this.postJson<GetUploadUrlResponse>(
- apiUrl,
- authToken,
- 'b2_get_upload_url',
- request,
- normalizeRawRequestOptions(optionsOrSignal, retry),
+ return withRedactedTokenDiagnostics(
+ await this.postJson<GetUploadUrlResponse>(
+ apiUrl,
+ authToken,
+ 'b2_get_upload_url',
+ request,
+ normalizeRawRequestOptions(optionsOrSignal, retry),
+ ),
)
}
@@ -911,12 +916,14 @@ export class RawClient {
optionsOrSignal?: RawRequestOptions | AbortSignal,
retry?: Partial<RetryOptions>,
): Promise<GetUploadPartUrlResponse> {
- return this.postJson<GetUploadPartUrlResponse>(
- apiUrl,
- authToken,
- 'b2_get_upload_part_url',
- request,
- normalizeRawRequestOptions(optionsOrSignal, retry),
+ return withRedactedTokenDiagnostics(
+ await this.postJson<GetUploadPartUrlResponse>(
+ apiUrl,
+ authToken,
+ 'b2_get_upload_part_url',
+ request,
+ normalizeRawRequestOptions(optionsOrSignal, retry),
+ ),
)
}
@@ -1207,11 +1214,13 @@ export class RawClient {
authToken: string,
request: DownloadAuthorizationRequest,
): Promise<DownloadAuthorizationResponse> {
- return this.postJson<DownloadAuthorizationResponse>(
- apiUrl,
- authToken,
- 'b2_get_download_authorization',
- request,
+ return withRedactedTokenDiagnostics(
+ await this.postJson<DownloadAuthorizationResponse>(
+ apiUrl,
+ authToken,
+ 'b2_get_download_authorization',
+ request,
+ ),
)
}
diff --git a/src/redaction.test.ts b/src/redaction.test.ts
index 2c097b2..f024a89 100644
--- a/src/redaction.test.ts
+++ b/src/redaction.test.ts
@@ -191,7 +191,8 @@ describe('credential redaction JSON and spread', () => {
const response = await client.authorize()
expect(response.authorizationToken).toBe(ACCOUNT_TOKEN)
expect(store.getAuth()?.authorizationToken).toBe(ACCOUNT_TOKEN)
- expect(JSON.parse(JSON.stringify(response)).authorizationToken).toBe(ACCOUNT_TOKEN)
+ expect(JSON.stringify(response)).not.toContain(ACCOUNT_TOKEN)
+ expect(JSON.parse(JSON.stringify({ ...response })).authorizationToken).toBe(ACCOUNT_TOKEN)
expect(client.accountInfo).toBe(store)
poolTokens(store)
expect(store.checkoutUploadUrl(BID)?.authorizationToken).toBe(UPLOAD_TOKEN)
diff --git a/src/response-token-redaction.node.test.ts b/src/response-token-redaction.node.test.ts
new file mode 100644
index 0000000..96811d0
--- /dev/null
+++ b/src/response-token-redaction.node.test.ts
@@ -0,0 +1,128 @@
+import { mkdtemp, readFile, rm } from 'node:fs/promises'
+import { tmpdir } from 'node:os'
+import { join } from 'node:path'
+import { inspect } from 'node:util'
+import { afterEach, describe, expect, it } from 'vitest'
+import { FileAccountInfo } from './auth/file.ts'
+import type { HttpRequest, HttpResponse, HttpTransport } from './http/transport.ts'
+import { RawClient } from './raw/index.ts'
+import { jsonResponse } from './test-utils/index.ts'
+import { RESPONSE_TOKEN_REDACTED } from './token-redaction.ts'
+import { bucketId, largeFileId } from './types/ids.ts'
+
+const AUTH_TOKEN = 'sentinel-account-token-0001'
+const UPLOAD_TOKEN = 'sentinel-upload-token-0002'
+const PART_TOKEN = 'sentinel-part-token-0003'
+const DOWNLOAD_TOKEN = 'sentinel-download-token-0004'
+const API = 'https://api.example.test'
+
+function transport(): HttpTransport {
+ return {
+ async send(request: HttpRequest): Promise<HttpResponse> {
+ if (request.url.includes('b2_authorize_account')) {
+ return jsonResponse({
+ accountId: 'account-id',
+ authorizationToken: AUTH_TOKEN,
+ apiInfo: {
+ storageApi: {
+ apiUrl: API,
+ downloadUrl: 'https://download.example.test',
+ s3ApiUrl: 'https://s3.example.test',
+ absoluteMinimumPartSize: 5_000_000,
+ recommendedPartSize: 100_000_000,
+ allowed: { capabilities: [], buckets: null, namePrefix: null },
+ },
+ },
+ applicationKeyExpirationTimestamp: null,
+ })
+ }
+ if (request.url.includes('b2_get_upload_part_url')) {
+ return jsonResponse({
+ fileId: 'large-file',
+ uploadUrl: 'https://upload.example.test/part',
+ authorizationToken: PART_TOKEN,
+ })
+ }
+ if (request.url.includes('b2_get_upload_url')) {
+ return jsonResponse({
+ bucketId: 'bucket',
+ uploadUrl: 'https://upload.example.test/file',
+ authorizationToken: UPLOAD_TOKEN,
+ })
+ }
+ return jsonResponse({
+ bucketId: 'bucket',
+ fileNamePrefix: 'p',
+ authorizationToken: DOWNLOAD_TOKEN,
+ })
+ },
+ } as HttpTransport
+}
+
+const calls = {
+ 'authorizeAccount (b2_authorize_account)': [
+ AUTH_TOKEN,
+ (raw: RawClient) => raw.authorizeAccount('key-id', 'key-secret', API),
+ ],
+ 'getUploadUrl (b2_get_upload_url)': [
+ UPLOAD_TOKEN,
+ (raw: RawClient) => raw.getUploadUrl(API, 'auth', { bucketId: bucketId('bucket') }),
+ ],
+ 'getUploadPartUrl (b2_get_upload_part_url)': [
+ PART_TOKEN,
+ (raw: RawClient) => raw.getUploadPartUrl(API, 'auth', { fileId: largeFileId('large-file') }),
+ ],
+ 'getDownloadAuthorization (b2_get_download_authorization)': [
+ DOWNLOAD_TOKEN,
+ (raw: RawClient) =>
+ raw.getDownloadAuthorization(API, 'auth', {
+ bucketId: bucketId('bucket'),
+ fileNamePrefix: 'p',
+ validDurationInSeconds: 60,
+ }),
+ ],
+} as const
+
+const directories: string[] = []
+afterEach(async () => {
+ await Promise.all(directories.splice(0).map((d) => rm(d, { recursive: true, force: true })))
+})
+
+describe('B2 response token redaction', () => {
+ it.each(Object.entries(calls))(
+ '%s hides its token in diagnostics but keeps the property',
+ async (_name, [token, call]) => {
+ const response = await call(new RawClient({ transport: transport() }))
+
+ expect(response.authorizationToken).toBe(token)
+ for (const rendered of [
+ JSON.stringify(response),
+ inspect(response, { depth: Infinity, showHidden: true }),
+ inspect(response),
+ ]) {
+ expect(rendered).not.toContain(token)
+ expect(rendered).toContain(RESPONSE_TOKEN_REDACTED)
+ }
+ expect({ ...response }.authorizationToken).toBe(token)
+ },
+ )
+
+ it('FileAccountInfo without a realm still persists and reloads the real token', async () => {
+ const directory = await mkdtemp(join(tmpdir(), 'b2-response-token-'))
+ directories.push(directory)
+ const path = join(directory, 'auth.json')
+ const auth = await new RawClient({ transport: transport() }).authorizeAccount(
+ 'key-id',
+ 'key-secret',
+ API,
+ )
+ const store = new FileAccountInfo(path)
+ store.setAuth(auth)
+ await store.flushed()
+
+ expect(JSON.parse(await readFile(path, 'utf8')).authorizationToken).toBe(AUTH_TOKEN)
+ const reloaded = new FileAccountInfo(path)
+ await reloaded.load()
+ expect(reloaded.getAuthToken()).toBe(AUTH_TOKEN)
+ })
+})
diff --git a/src/token-redaction.ts b/src/token-redaction.ts
new file mode 100644
index 0000000..bbb2fd7
--- /dev/null
+++ b/src/token-redaction.ts
@@ -0,0 +1,32 @@
+/** Placeholder printed instead of a B2 authorization token in diagnostics. */
+export const RESPONSE_TOKEN_REDACTED = '[redacted token]'
+
+const INSPECT_CUSTOM = Symbol.for('nodejs.util.inspect.custom')
+
+/**
+ * Makes a B2 response that carries `authorizationToken` safe to print.
+ *
+ * `JSON.stringify`, `console.log` and `util.inspect` show the token as
+ * {@link RESPONSE_TOKEN_REDACTED}. The `authorizationToken` property itself is unchanged, so code
+ * that reads it (account info, upload pools, retries) works as before. The two hooks are
+ * non-enumerable, so a spread copy (`{ ...response }`) is a plain object with the real token:
+ * persist a copy like that, never `JSON.stringify(response)`.
+ *
+ * @internal
+ */
+export function withRedactedTokenDiagnostics<T extends { authorizationToken: string }>(
+ response: T,
+): T {
+ const redacted = (): T => ({ ...response, authorizationToken: RESPONSE_TOKEN_REDACTED })
+ Object.defineProperty(response, 'toJSON', {
+ value: redacted,
+ enumerable: false,
+ configurable: true,
+ })
+ Object.defineProperty(response, INSPECT_CUSTOM, {
+ value: redacted,
+ enumerable: false,
+ configurable: true,
+ })
+ return response
+} |
Contributor
Author
|
@sophiecarreras — before I push the 1. Redacting the handed-out tokens breaks documented browser flows. The redactor hooks
With the redaction, both now serialize without the token, so the browser gets a tokenless response and the upload/download fails with 401. So Scope question for you: redact only 2. Minor, regardless of the scope decision: the Requesting your review on the scope question — I'll fold the persistence fix and the rest in once you've called it. |
Assert that util.inspect, console.log, JSON.stringify and object spread of clients, bucket and file handles, auth stores and upload URL pools never contain the application key or authorization tokens, and that getAuth(), authorize() and file auth caches still carry the real token.
Hold the application key, stored authorization and upload URL pools in private fields, give B2Client and the account-info stores a redacted toJSON/inspect summary, and expose accountInfo through a getter, mirroring the Partner client.
ffumero2003
force-pushed
the
fix-client-redact-diagnostics
branch
from
November 5, 2026 18:55
905fea1 to
83c00b8
Compare
This was referenced Nov 6, 2026
Merged
ffumero2003
added a commit
that referenced
this pull request
Nov 10, 2026
) * test: cover credential redaction in SDK object diagnostics Assert that util.inspect, console.log, JSON.stringify and object spread of clients, bucket and file handles, auth stores and upload URL pools never contain the application key or authorization tokens, and that getAuth(), authorize() and file auth caches still carry the real token. * fix(client): redact credentials from SDK object diagnostics Hold the application key, stored authorization and upload URL pools in private fields, give B2Client and the account-info stores a redacted toJSON/inspect summary, and expose accountInfo through a getter, mirroring the Partner client. * docs: note credential redaction and the accountInfo accessor change
ffumero2003
added a commit
that referenced
this pull request
Nov 10, 2026
* fix(upload): record large-file SHA-1 and support resume (#354) * fix(upload): retry upload POST connection failures by default (#344) * fix(upload): preserve SHA-1 integrity during large-file resume (#365) * chore: update vulnerable transitive dependencies (#340) * chore: update development tooling for dependency audit (#361) * chore: allowlist minimatch audit advisory (#363) * chore(release): prepare isolated v0.5.0 * test(upload): verify connection-failure phases and SHA-1 * docs: make the upload retry migration example self-contained * fix(client): redact credentials from SDK object diagnostics (#351) (#375) * test: cover credential redaction in SDK object diagnostics Assert that util.inspect, console.log, JSON.stringify and object spread of clients, bucket and file handles, auth stores and upload URL pools never contain the application key or authorization tokens, and that getAuth(), authorize() and file auth caches still carry the real token. * fix(client): redact credentials from SDK object diagnostics Hold the application key, stored authorization and upload URL pools in private fields, give B2Client and the account-info stores a redacted toJSON/inspect summary, and expose accountInfo through a getter, mirroring the Partner client. * docs: note credential redaction and the accountInfo accessor change * feat(upload): add single-file connection-fault retry opt-out (#376) * feat(upload): add single-file connection-fault retry opt-out * refactor(upload): resolve connection-fault retry options --------- Co-authored-by: Sophie <49928680+sophiecarreras@users.noreply.github.com>
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
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.
Summary
Make SDK objects safe to log.
console.log,util.inspect,JSON.stringifyand object spread ofB2Client,Bucket,B2Object, the account-info stores and the upload URL pools no longer include the application key or authorization tokens. This mirrors whatPartnerClientalready does.Changes
B2Clientholds the application key in a#private field and gains atoJSON()/util.inspectsummary (B2ClientJson): type, key ID, a credentials placeholder, the realm URL (sanitized), authorized state, and URL-guard suffixes.InMemoryAccountInfoandFileAccountInfohold their stored authorization and upload URL pools in#fields, with matching summaries (InMemoryAccountInfoJson,FileAccountInfoJson).BucketandB2Objecthold their client reference in a#field; their own data (names, IDs) is unchanged.UploadUrlPoolholds its entries in a#field.B2Client.accountInfois now a prototype accessor, so spreading or copying a client doesn't copy a custom store.No API is removed:
client.accountInfo,authorize()andgetAuth()work as before and return the full response.FileAccountInfostill persists and reloads the real token.Compatibility (CHANGELOG "Changed")
accountInfois no longer an own property:{ ...client },Object.keys,Object.assign,Object.hasOwnand own-property descriptors don't see it;Reading
client.accountInfoand'accountInfo' in clientstill work. As withPartnerClient, methods called through aProxywrapper (for example Vuereactive(client)) throwCannot read private member.Tests
src/redaction.test.ts(Node and browser) andsrc/redaction.node.test.ts(util.inspect, file persistence):AccountInfochecks the client summary,Object.keysand spread check the#fields, and the pool is tested directly.src/test-utils/redaction-export-review.ts, test-only and not built) fails when a new public export appears without a review, so a future credential-holding class can't skip this.Each layer was checked by reverting it alone; the matching test failed. Reverted that way:
#fields;toJSONandinspectmethods;getAuth()and file persistence.Verification
pnpm typecheck,pnpm lint,pnpm lint:docs,pnpm lint:spelling,pnpm test,pnpm test:browserandpnpm test:coverage(gates unchanged) pass.