Skip to content

fix: stop sanitizeSkopeoErrorForLogging swallowing non-skopeo errors#1689

Open
parker-snyk wants to merge 1 commit into
stagingfrom
CN-1360-sanitize-skopeo-error
Open

fix: stop sanitizeSkopeoErrorForLogging swallowing non-skopeo errors#1689
parker-snyk wants to merge 1 commit into
stagingfrom
CN-1360-sanitize-skopeo-error

Conversation

@parker-snyk
Copy link
Copy Markdown
Contributor

@parker-snyk parker-snyk commented May 22, 2026

  • Tests written and linted
  • Documentation written (n/a — internal log-sanitization helper)
  • Commit history is tidy

What this does

Fixes CN-1360 (parent CN-1359).

sanitizeSkopeoErrorForLogging in src/scanner/images/index.ts unconditionally deletes message from every error caught in pullImages. The intent was to scrub the skopeo command line, which embeds credentials passed via --src-creds <user:pass>. The side effect: non-skopeo errors (e.g. ECR credential-resolution failures under IRSA) also lose their message, leaving only a useless log line:

failed to pull image docker/oci archive image

…with no actionable detail. Tessl AI hit this on v2.22.20.

This PR branches on the error shape:

  • Skopeo ChildProcessErrors (identified by a populated stderr field) — drop message (creds), drop childProcess and stack, keep stderr (the actual skopeo failure detail).
  • Every other error (no stderr) — keep message so the underlying cause survives. Still drop childProcess and stack defensively.

The helper now also returns a new object instead of mutating the input, and the error: any is replaced with error: unknown plus narrowing.

Notes for the reviewer

  • Function is now exported (// Exported for testing) so unit tests can hit it directly.
  • New tests in test/unit/scanner/images.spec.ts cover both branches plus non-object / empty-stderr edge cases, and explicitly assert that --src-creds / the secret value never appear in the sanitized output.
  • No behavioural change to pullImages itself; only the contents of the logged error object differ.

Test plan

  • npm run lint:eslint clean
  • npm run test:unit — 312 tests pass (18 suites)
  • New unit test asserts --src-creds and the credential value are absent from the sanitized skopeo error
  • New unit test asserts message is preserved for non-skopeo errors (e.g. "ECR token fetch failed")
  • Manual review: original error object is not mutated by the sanitizer

`sanitizeSkopeoErrorForLogging` was unconditionally deleting `message`
from every error caught in `pullImages`. The original intent was to
avoid logging skopeo command lines (which contain
`--src-creds <user:pass>`), but the same scrubbing was applied to
non-skopeo failures such as ECR credential-resolution errors. That
produced log lines like "failed to pull image docker/oci archive image"
with no underlying cause, blocking root-cause analysis for IRSA / ECR
pull failures (Tessl AI hit this in v2.22.20).

Branch on the error shape instead:

- Skopeo `ChildProcessError`s (identified by a populated `stderr`):
  drop `message` (creds), keep `stderr` (the real failure detail),
  drop `childProcess` and `stack`.
- Every other error: keep `message`, drop `childProcess` (defensive)
  and `stack`.

Also stop mutating the original error object, type the input as
`unknown`, and add unit tests covering both branches plus the
non-object / empty-stderr edge cases.

Refs: CN-1360, CN-1359
@parker-snyk parker-snyk requested a review from a team as a code owner May 22, 2026 20:01
@parker-snyk parker-snyk requested a review from bdemeo12 May 22, 2026 20:01
@snyk-pr-review-bot
Copy link
Copy Markdown

PR Reviewer Guide 🔍

🧪 PR contains tests
🔒 Security concerns

Sensitive information exposure:
The PR modifies how credentials are scrubbed from logs. By conditioning the removal of the message property (which contains the full command line including credentials) on the presence of stderr, there is a risk that atypical Skopeo failures (where stderr is empty) will lead to credentials being logged. However, this is a trade-off made to fix a confirmed bug where critical system errors were being silenced. A safer approach might involve regex-based scrubbing of the message string instead of unconditional deletion or conditional preservation.

⚡ Recommended focus areas for review

Credential Leakage Risk 🟡 [minor]

The logic for isSkopeoChildProcessError relies on stderr being a non-empty string. If skopeo or the underlying process wrapper fails before writing to stderr (e.g., an internal execution error), isSkopeoChildProcessError will be false. In this scenario, the message property—which contains the raw command line including --src-creds—will be preserved and logged. While the PR improves observability for non-Skopeo errors, it creates a narrow window where credentials might still be logged if a Skopeo execution fails silently.

const isSkopeoChildProcessError =
  typeof source.stderr === 'string' && source.stderr.length > 0;
📚 Repository Context Analyzed

This review considered 10 relevant code sections from 9 files (average relevance: 0.77)

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant