Skip to content

fix(toolkit-lib): credential plugin diagnostics no longer include the rejected value - #1976

Closed
sanjanaravikumar-az wants to merge 1 commit into
mainfrom
fix/no-rejected-value-in-credential-plugin-diagnostics
Closed

sanjanaravikumar-az wants to merge 1 commit into
mainfrom
fix/no-rejected-value-in-credential-plugin-diagnostics

Conversation

@sanjanaravikumar-az

Copy link
Copy Markdown
Contributor

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

  1. @aws-cdk/toolkit-liblib/api/aws-auth/credential-plugins.ts, initial resolution (InvalidPluginCredentials): dropped inspect(initial).
  2. @aws-cdk/toolkit-liblib/api/aws-auth/credential-plugins.ts, refresh path (PluginCredentialTypeMismatch): dropped inspect(newCreds).
  3. @aws-cdk/toolkit-liblib/api/plugin/plugin.ts, registerContextProviderAlpha (InvalidContextProvider): dropped inspect(provider).
  4. aws-cdklib/cli/pretty-print-error.ts, ensureError: dropped JSON.stringify of a non-Error thrown value / non-Error cause.

The util.inspect imports are gone from both toolkit-lib files. The shared helper is lib/util/describe-value.ts (exported via lib/util/index.ts); aws-cdk has 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/ToolkitError classes 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. Ordinary Error message/cause/stack output is byte-for-byte unaffected.

pretty-print-error.ts is additionally made exception-safe. value instanceof Error itself throws for a revoked proxy, so the check sits inside try/catch; the cause walk uses a seen-set (a self-referencing cause chain would otherwise recurse to stack overflow) and reads message/name/stack/cause through guarded readers that do not coerce non-strings.

Deliberate decision worth weighing: fail-silent console.error

printError/printDebug wrap console.error/console.debug in try {} catch {}, swallowing write failures. This is intentional but is a real trade-off, so please weigh it: prettyPrintError runs inside the CLI's top-level catch, and a chalk call over a throwing message getter (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-libtest/api/plugin + test/api/aws-auth: 9 suites, 98 tests passed.
aws-cdk — new test/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, working getPromise()); asserts error type, the stable prefix, and that no sentinel appears.
  • Initial-resolution coverage for the rejected-value site: a flat value with a falsy 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 as Error.cause, plus hostile inputs: cyclic, throwing toJSON, throwing getter, revoked proxy (bare and as a cause), an Error whose message/stack/cause getters 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 via jest.spyOn(console, 'error') — note that stream patching would pass vacuously here, because jest-bufferedconsole.ts already replaces process.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 around ensureError in pretty-print-error.ts was 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.ts is 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 via fixture.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 call registerSecrets(...) 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 the fix(toolkit-lib) subject above. There is no CHANGELOG.md to 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. If aws-cdk needs to ship on its own, packages/aws-cdk/release.txt may need touching — I have not done that, as the release policy is yours to set.

… 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.
@github-actions

Copy link
Copy Markdown
Contributor

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant