fix(toolkit-lib): credential plugin diagnostics no longer include the rejected value - #1976
Closed
sanjanaravikumar-az wants to merge 1 commit into
Closed
sanjanaravikumar-az wants to merge 1 commit into
sanjanaravikumar-az wants to merge 1 commit into
Conversation
… rejected value When a credential plugin or a context provider returned a value that did not satisfy the expected contract, the resulting diagnostic interpolated that value into the error message. Error diagnostics now describe an unexpected value by a fixed type category only, and never render its contents. To keep those errors actionable, the diagnostics instead report the credential source name, the phase in which the value was rejected (initial resolution or refresh) and the specific contract that was violated. The same hardening is applied to the CLI's top-level error handler in `aws-cdk` (`lib/cli/pretty-print-error.ts`), which no longer serializes a non-Error thrown value or a non-Error `cause`. That code path is also made exception-safe, so a value with a throwing accessor cannot turn a handled error into an unhandled rejection and skip the telemetry flush. Error types and the stable message prefixes are unchanged. Tests: the credential-plugin and plugin-host suites now assert the ABSENCE of sentinel secret values in the rendered diagnostics for a rejected initial value and a rejected refresh value, and a new `pretty-print-error` suite covers secret-bearing thrown values and causes plus hostile inputs (cyclic, throwing `toJSON`, throwing getter, revoked proxy, BigInt, symbol, function, null, undefined) while confirming ordinary Error output is unaffected. The coverage-exclusion block around `ensureError` was removed now that the function is directly covered.
sanjanaravikumar-az
had a problem deploying
to
automation
September 20, 2026 20:16 — with
GitHub Actions
Failure
sanjanaravikumar-az
had a problem deploying
to
no-approval
September 20, 2026 20:16 — with
GitHub Actions
Failure
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
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.
What changed and why
Security hardening of error diagnostics. When a credential plugin or a context provider returned a value that did not satisfy the expected contract, the resulting diagnostic interpolated that rejected value into the error message. Everything in those messages goes to stderr and is routinely collected by CI logs, so a value that happened to hold AWS credentials could be written there.
An unexpected value is now described by a fixed type category only (
typeof, plus'null'), and its contents are never rendered. The categorization deliberately performs no property access, no key enumeration and no prototype/constructor inspection — property names can themselves be sensitive, and traversal can trigger getters or proxy traps from inside an error path.To keep the errors actionable without the value, the diagnostics now report the CDK-side identifiers instead: the credential source name, the phase in which the value was rejected (initial resolution vs. refresh), and the specific contract that was violated.
This was reported internally as a security issue. No exploit detail is included here or in the code.
Sites touched
@aws-cdk/toolkit-lib—lib/api/aws-auth/credential-plugins.ts, initial resolution (InvalidPluginCredentials): droppedinspect(initial).@aws-cdk/toolkit-lib—lib/api/aws-auth/credential-plugins.ts, refresh path (PluginCredentialTypeMismatch): droppedinspect(newCreds).@aws-cdk/toolkit-lib—lib/api/plugin/plugin.ts,registerContextProviderAlpha(InvalidContextProvider): droppedinspect(provider).aws-cdk—lib/cli/pretty-print-error.ts,ensureError: droppedJSON.stringifyof a non-Error thrown value / non-Errorcause.The
util.inspectimports are gone from both toolkit-lib files. The shared helper islib/util/describe-value.ts(exported vialib/util/index.ts);aws-cdkhas its own local copy rather than a cross-package private import.Note for reviewers
Error types and the stable message prefixes are preserved. The
AuthenticationError/ToolkitErrorclasses and codes (InvalidPluginCredentials,PluginCredentialTypeMismatch,InvalidContextProvider) are unchanged, and each message keeps its original leading sentence (e.g.Plugin returned a value that doesn't resemble AWS credentials:,Plugin initially returned static V3 credentials but now returned something else:,Object you gave me does not look like a ContextProviderPlugin:) so existing matchers and docs keep working. OrdinaryErrormessage/cause/stackoutput is byte-for-byte unaffected.pretty-print-error.tsis additionally made exception-safe.value instanceof Erroritself throws for a revoked proxy, so the check sits insidetry/catch; the cause walk uses a seen-set (a self-referencing cause chain would otherwise recurse to stack overflow) and readsmessage/name/stack/causethrough guarded readers that do not coerce non-strings.Deliberate decision worth weighing: fail-silent
console.errorprintError/printDebugwrapconsole.error/console.debugintry {} catch {}, swallowing write failures. This is intentional but is a real trade-off, so please weigh it:prettyPrintErrorruns inside the CLI's top-level catch, and a chalk call over a throwingmessagegetter (or a broken stdio stream) would otherwise convert an already-handled error into an unhandled rejection, skipping the telemetry flush that runs after it. The cost is that a genuine console failure is invisible. The alternative — letting it propagate — trades a silent diagnostic for a lost telemetry flush and a worse crash signature. Happy to change this if you prefer the propagating behaviour.Test evidence
@aws-cdk/toolkit-lib—test/api/plugin+test/api/aws-auth: 9 suites, 98 tests passed.aws-cdk— newtest/cli/pretty-print-error.test.ts: 24 tests passed; coverage of that file 98.6% statements / 97.29% branches / 100% functions.The new/repaired assertions check the absence of sentinel secret values in rendered diagnostics, rather than just asserting the new wording:
credential-plugin.test.ts— refresh now returns a genuine V2-compatible object carrying sentinels (accessKeyId,secretAccessKey,sessionToken,expireTime, workinggetPromise()); asserts error type, the stable prefix, and that no sentinel appears.accessKeyId, plus a nested{ region, credentials: { ... } }shape (named so it is not "fixed" into a valid top-level V3 shape).plugin-host.test.ts— an invalid context provider carrying a nested credential object; asserts prefix, provider name, and no sentinels.pretty-print-error.test.ts— secret-bearing object both as the thrown value and asError.cause, plus hostile inputs: cyclic, throwingtoJSON, throwing getter, revoked proxy (bare and as a cause), an Error whosemessage/stack/causegetters all throw, BigInt, symbol, function,null,undefined, a self-looping cause chain, and a failing console write. Ordinary Error output is asserted unchanged. Capture is viajest.spyOn(console, 'error')— note that stream patching would pass vacuously here, becausejest-bufferedconsole.tsalready replacesprocess.stderr.write.Negative check: temporarily re-introducing the
inspect(...)interpolation made all three new/repaired toolkit-lib tests fail on the sentinels; reverting made them green again. So the tests genuinely pin the fix and are not tautological.Baseline check: a path-limited stash of these edits confirmed the single unrelated
cli.test.ts"notices autodetection" failure is pre-existing on the base commit.Removed coverage exclusion
The
/* c8 ignore start */…/* c8 ignore stop */block aroundensureErrorinpretty-print-error.tswas removed — the function is now directly covered by the new suite, so the exclusion is no longer warranted.Integ test: authored but NOT executed
packages/@aws-cdk-testing/cli-integ/tests/cli-integ-tests/plugins/cdk-credential-plugin-does-not-leak.integtest.tsis included but was not run — it needs real AWS accounts and a built CLI, which I could not exercise here. Please treat it as unverified and run it in CI. It drives the real CLI as a child process viafixture.cdk, with a plugin that returns expiring V3 credentials initially and then a sentinel-bearing V2 object on refresh; an explicit target account forces the lookup. It asserts a nonzero exit, the stable message prefix, and the absence of the sentinels. It is keyed on account+mode rather than a call counter, because the CLI sets up read and write providers separately and a plain counter would send the second initial resolution down the wrong path. It deliberately does not callregisterSecrets(...)on the sentinels, since the harness would redact them and the assertion would pass vacuously.Release note
Releases are projen +
commit-and-tag-version, generating the changelog from conventional-commit subjects, so the commit/PR subject is the release note — hence thefix(toolkit-lib)subject above. There is noCHANGELOG.mdto edit.One thing for maintainers to decide: this also changes
packages/aws-cdk(lib/cli/pretty-print-error.ts), but the subject carries a single scope. Ifaws-cdkneeds to ship on its own,packages/aws-cdk/release.txtmay need touching — I have not done that, as the release policy is yours to set.