Describe the bug
On the JWT session-tokenization path, a Jsonnet claims mapper whose output has no object-valued claims field is supposed to produce a controlled ErrBadRequest ("the claims mapper must return a claims object"). Instead the error-reporting code itself dereferences a nil error, panicking the request.
In session/tokenizer.go, after EvaluateAnonymousSnippet succeeds the block-local err is nil. The missing-claims branch then passes that nil err straight into the telemetry helper:
|
evaluatedClaims := gjson.Get(evaluated, "claims") |
|
if !evaluatedClaims.IsObject() { |
|
trace.SpanFromContext(ctx).AddEvent(events.NewJsonnetMappingFailed( |
|
ctx, err, jsonnet.Bytes(), evaluated, "", "", |
|
)) |
|
return errors.WithStack(herodot.ErrBadRequest().WithWrap(err).WithReasonf("Expected tokenizer JsonNet to return a claims object but it did not.")) |
evaluatedClaims := gjson.Get(evaluated, "claims")
if !evaluatedClaims.IsObject() {
trace.SpanFromContext(ctx).AddEvent(events.NewJsonnetMappingFailed(ctx, err, ...)) // err is nil here
...
}
NewJsonnetMappingFailed routes the error through attrErrorReason → reasonForError, which calls err.Error() with no nil guard:
|
func reasonForError(err error) string { |
|
if ve := new(schema.ValidationError); errors.As(err, &ve) { |
|
return ve.Message |
|
} |
|
if r := *new(herodot.ReasonCarrier); errors.As(err, &r) { |
|
return r.Reason() |
|
} |
|
return err.Error() |
func reasonForError(err error) string {
...
return err.Error() // nil error interface -> panic
}
errors.As(nil, ...) returns false safely, so execution falls through to err.Error() on the nil interface and panics. There is no recover() on this path, so Go's net/http per-request recovery aborts the connection instead of returning the intended HTTP 400.
Introduced by: #4409 ("feat: emit events on jsonnet failure when templating a jwt", merged 2025-05-12). That PR added two AddEvent(NewJsonnetMappingFailed(...)) calls; the first passes a genuine non-nil error, the second (missing-claims branch) passes the nil err.
Reproduction
- Configure JWT session tokenization with a Jsonnet claims mapper (
ClaimsMapperURL) whose output is valid JSON but has no object-valued claims field, e.g. { "sub": "x" } (no claims key) or { "claims": "not-an-object" }.
- Exercise the tokenized session flow (e.g.
whoami with a tokenized session template) so evaluation reaches the missing-claims branch in session/tokenizer.go.
- Observe a nil-pointer panic in
reasonForError (x/events/events.go) instead of the documented bad-request response.
Expected behavior
The missing-claims branch returns the documented ErrBadRequest explaining the mapper must return a claims object.
Suggested fix
Construct the missing-claims error before emitting the event and pass that (non-nil) error to NewJsonnetMappingFailed, returning the same error; or make reasonForError nil-safe (if err == nil { return "" }). The first keeps the intended controlled 400 path intact.
Environment: current master (HEAD 783f90e at time of report); code path present since #4409.
Found while testing Ito, an automated code-review tool, against recently-merged PRs. It's free for open source. Sharing this because it looked like a real bug worth fixing, not to sell anything: https://app.ito.ai/share/0cc3f8b0-2faf-488e-a6cb-c1a1e6bf3691?tab=details
Describe the bug
On the JWT session-tokenization path, a Jsonnet claims mapper whose output has no object-valued
claimsfield is supposed to produce a controlledErrBadRequest("the claims mapper must return a claims object"). Instead the error-reporting code itself dereferences a nilerror, panicking the request.In
session/tokenizer.go, afterEvaluateAnonymousSnippetsucceeds the block-localerris nil. The missing-claims branch then passes that nilerrstraight into the telemetry helper:kratos/session/tokenizer.go
Lines 151 to 156 in 783f90e
NewJsonnetMappingFailedroutes the error throughattrErrorReason→reasonForError, which callserr.Error()with no nil guard:kratos/x/events/events.go
Lines 564 to 571 in 783f90e
errors.As(nil, ...)returns false safely, so execution falls through toerr.Error()on the nil interface and panics. There is norecover()on this path, so Go'snet/httpper-request recovery aborts the connection instead of returning the intended HTTP 400.Introduced by: #4409 ("feat: emit events on jsonnet failure when templating a jwt", merged 2025-05-12). That PR added two
AddEvent(NewJsonnetMappingFailed(...))calls; the first passes a genuine non-nil error, the second (missing-claims branch) passes the nilerr.Reproduction
ClaimsMapperURL) whose output is valid JSON but has no object-valuedclaimsfield, e.g.{ "sub": "x" }(noclaimskey) or{ "claims": "not-an-object" }.whoamiwith a tokenized session template) so evaluation reaches the missing-claims branch insession/tokenizer.go.reasonForError(x/events/events.go) instead of the documented bad-request response.Expected behavior
The missing-claims branch returns the documented
ErrBadRequestexplaining the mapper must return aclaimsobject.Suggested fix
Construct the missing-claims error before emitting the event and pass that (non-nil) error to
NewJsonnetMappingFailed, returning the same error; or makereasonForErrornil-safe (if err == nil { return "" }). The first keeps the intended controlled 400 path intact.Environment: current
master(HEAD783f90eat time of report); code path present since #4409.Found while testing Ito, an automated code-review tool, against recently-merged PRs. It's free for open source. Sharing this because it looked like a real bug worth fixing, not to sell anything: https://app.ito.ai/share/0cc3f8b0-2faf-488e-a6cb-c1a1e6bf3691?tab=details