Stop reproducing file-upload field values in config decode errors - #1113
Conversation
…e errors FileUploadDecodeHook's path-resolution branch wrapped os.Stat, os.ReadFile, and url.Parse errors directly, all three of which embed the raw input in their Error() string. Since this field type carries credential files (service-account JSON, certs, private keys), a value supplied in the wrong shape wrote the value itself to stderr/logs. WithIsSecret does not protect this path: it only feeds GUI schema rendering and is never visible to the decode hook. Unwrap each error down to its underlying OS/parse reason (e.g. "no such file or directory") before wrapping, discarding the embedded path/URL/value. Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| // Unwrap to the underlying parse failure so it isn't echoed back. | ||
| var urlErr *url.Error | ||
| if errors.As(err, &urlErr) { | ||
| return nil, fmt.Errorf("invalid data URL: %w", urlErr.Err) |
There was a problem hiding this comment.
🟡 Suggestion (low confidence): this redacts the url.Parse failure, but two sibling error sites in the same function still format value-derived text — expected base64 data, got: %s (line 161) and expected MIME type application/json, got: %s (line 164) both print mediaType, which is the raw field value from after data: up to the first comma. Reachability is narrow (the value must literally start with data: and contain a comma before any real media type), so this is a consistency gap rather than the same exposure the PR fixes, but redacting or truncating mediaType would close the function completely.
| func redactPathError(err error) error { | ||
| var pathErr *fs.PathError | ||
| if errors.As(err, &pathErr) { | ||
| return pathErr.Err | ||
| } | ||
| return err | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion (low confidence, SDK compatibility): returning pathErr.Err drops *fs.PathError from the error chain, so a downstream caller doing errors.As(err, &pathErr) to inspect Op/Path no longer matches. Sentinel checks are unaffected (errors.Is(err, fs.ErrNotExist) / os.IsNotExist still work, since the syscall.Errno is preserved), and this error only surfaces through the config decode path, so real breakage is unlikely — worth a line in the PR description as an intentional error-shape change rather than a code change.
General PR Review: Stop reproducing file-upload field values in config decode errorsBlocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. The change adds Risk triage (per The two prior findings ( Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
…unwrap Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
| if err != nil { | ||
| var urlErr *url.Error | ||
| if errors.As(err, &urlErr) { | ||
| return nil, fmt.Errorf("invalid data URL: %w", urlErr.Err) |
There was a problem hiding this comment.
🟡 Suggestion: Unwrapping to urlErr.Err removes the quoted URL, but one underlying error type still carries value bytes: if the failure comes from url.Parse's fragment handling, urlErr.Err is a url.EscapeError, whose Error() is invalid URL escape "%XY" — three characters of the input. Reachability is narrow (the value must start with data: and contain # followed by a bad escape), so this is a strictness gap against the "never reproduced" invariant the new test asserts rather than a practical leak. If you want the invariant to hold unconditionally, map *url.EscapeError (and url.InvalidHostError) to a static message instead of forwarding them. (low confidence)
2 participants
Summary
FileUploadDecodeHook's path-resolution branch (readFromPath=true, used by the CLI, gRPC server, and capabilities commands) wraps three stdlib errors directly:os.Statfailure ->cannot access file: %wos.ReadFilefailure ->error reading file: %wurl.Parsefailure (in thedata:URL branch, reachable from either mode) ->invalid data URL: %w*fs.PathErrorand*url.Errorboth embed the raw input string in theirError()output. Since this field type carries credential files (service-account JSON, certs, private keys), a value supplied in the wrong shape — e.g. the file's raw content passed where a path was expected — gets written verbatim to stderr and whatever collects it.WithIsSecret(true)does not protect this: it only feeds GUI schema rendering (schemaFieldToV1inpkg/field/marshal.go) and is never visible to the decode hook, whosemapstructure.DecodeHookFuncsignature only receives types and the raw value.Fix
redactPathError, which unwraps an*fs.PathErrorto its.Errfield viaerrors.As, dropping.Path. Applied at bothos.Stat/os.ReadFilesites ingetFileContentFromPath.url.Parsefailure inparseJSONBase64DataURL, dropping the quoted URL and keeping only the underlying parse-failure reason.The failure category (not found / permission denied / name too long / invalid control character) is preserved for diagnosability; the field value itself is not.
Test plan
go build ./...go vet ./pkg/field/...golangci-lint run ./pkg/field/...(0 issues)pkg/fieldtest suite passes unmodified (no behavior change on valid input)TestFileUploadDecodeHook_DoesNotLeakValueInErrors, covering all three sites, asserting a secret marker never appears in the resulting error stringgo test ./...passesResolves CE-1284.