Repository navigation
resources: mask debug_redact fields in validation error text - #3071
Jeff Luo (JeffLuoo) merged 1 commit into
Conversation
818c1cd to
8993209
Compare
Jeff Luo (JeffLuoo)
left a comment
Jeff Luo (JeffLuoo)
left a comment
There was a problem hiding this comment.
Thanks for the fix. It masks the duplicate EnvVar from #3070 in both ToAPIError and ToGRPCStatusError, and the new test passes for me at this commit. I have two comments: a third renderer of validation errors that does not mask, and a nil-entry check in the test that cannot fail.
| // (an EnvVar with its value), and the error text renders it as JSON; the RPC | ||
| // interceptors log that text as a string, which the logging handler's proto | ||
| // redaction cannot see into. errs is left unmodified. | ||
| func redactBadValues(errs field.ErrorList) field.ErrorList { |
There was a problem hiding this comment.
toGRPCInternalError in cmd/ateapi/internal/controlapi/validate.go:23 is a third place that renders a field.ErrorList, and it does not call redactBadValues. It renders the result of the second validation, on the stored object, in CreateActorTemplate (actor_template.go:70), CreateActor and UpdateActor (actor.go:152, :331), and worker.go:141, :210. Request validation runs first and goes through ToAPIError, so a duplicate from a request does not reach this path today. But an object that already holds a duplicate entry, or a server-side change that adds one, sends the full EnvVar value to the log through this path. Please export the helper, or add a resources.ToInternalError, so that every renderer masks the bad values.
|
|
||
| // A nil entry must not panic: the error is built on the RPC path, and | ||
| // fmt would otherwise have rendered it. | ||
| if err := ToAPIError(append(field.ErrorList{nil}, errs...)); strings.Contains(err.Error(), secret) { |
There was a problem hiding this comment.
This check passes for the wrong reason. ToAggregate keeps the typed-nil *field.Error, and aggregate.Error() calls Error() on it, which panics. In ToAPIError, fmt recovers the panic, so the whole message becomes %!v(PANIC=Error method: runtime error: invalid memory address or nil pointer dereference). That message can never contain the secret. ToGRPCStatusError calls .Error() directly, so a nil entry still panics there. Please remove this check and its comment. Or make redactBadValues drop nil entries, and assert for both functions that the message still names API_KEY.
8993209 to
ffb4b29
Compare
A duplicate entry error from validate.PtrSliceUnique carries the whole
entry as its bad value, and apimachinery renders it as JSON in the error
text. For a template or Run request with two env vars of the same name
that is
env[1]: Duplicate value: {"name":"API_KEY","value":"sk-..."}
The RPC interceptors log the error with slog.Any("err", err). It is a
string by then, so the logging handler's proto redaction cannot mask it,
and the env value reaches the Info "Handle RPC" line even though the
request in the same record shows value:"[REDACTED]".
ToAPIError and ToGRPCStatusError now replace a proto bad value with its
protoredact copy before building the text, so the error still names the
duplicate but shows value:"[REDACTED]". The caller's ErrorList is left
unmodified.
The controlapi handlers rendered validation errors on the stored object
with their own toGRPCInternalError, which did not mask. They now call a
new resources.ToInternalError, so every renderer of a field.ErrorList
masks bad values.
Nil entries are dropped before rendering. Rendering one panics, which
fmt recovered into an unreadable message in ToAPIError and which crashed
ToGRPCStatusError outright.
ffb4b29 to
16da9c3
Compare
Jeff Luo (JeffLuoo)
added
area/observability
kind/bug
A duplicate entry error from validate.PtrSliceUnique carries the whole entry as its bad value, and apimachinery renders it as JSON in the error text. For a template or Run request with two env vars of the same name that is
The RPC interceptors log the error with slog.Any("err", err). It is a string by then, so the logging handler's proto redaction cannot mask it, and the env value reaches the Info "Handle RPC" line even though the request in the same record shows value:"[REDACTED]".
ToAPIError and ToGRPCStatusError now replace a proto bad value with its protoredact copy before building the text, so the error still names the duplicate but shows value:"[REDACTED]". The caller's ErrorList is left unmodified.
Why is this needed? (Or link the issue. Open one first for features, API, install or CI changes.)
Fixes #3070
AI use: