apache / apache/gravitino

[Improvement] Authentication filter and ErrorExceptionMapper return raw exception messages

Open
#13,107 3 comments 0 reactions 1 assignee Claimed by @nevzheng View on GitHub
improvement
Dominant language
Java
Stars
3.2k
Forks
939
Avg merge
1d 17h
Merged PRs (30d)
339

Description

### What would you like to be improved?

Two error paths put raw exception text in the `message` field of error responses, with no policy for which text is safe to send to clients:

- **Authentication filter:** every branch of `AuthenticationFilter.sendAuthErrorResponse` builds `message` from `exception.getMessage()`, including the catch-all `ErrorResponse.internalError(exception.getMessage(), exception)` for unexpected exceptions. That path is reachable before a caller is authenticated.
- **Uncaught `java.lang.Error`:** `ErrorExceptionMapper` returns `"Server error while processing request: " + error` (added in #12976, which asserts this message in tests).

For a bug in a custom `Authenticator`, an NPE, or a token-store connection failure, that text can include internal class names, file paths, or connection details, even with `includeErrorStackTrace=false`. This was raised in review of #13057 (https://github.com/apache/gravitino/pull/13057#discussion_r3976730053) and deferred there because it needs further thought: #12728 treats `message` as client-facing, and clients show it to users, so a blanket redaction would hide useful errors.

The desired outcome is an agreed rule for which error text these paths may return to clients, with the underlying cause still available server-side. #13057 adds server-side logging for unexpected authentication failures.

**Scope:** this issue covers the two paths above, which were raised in review. Similar handling likely affects other error paths, including REST `ExceptionHandlers` (the `reason [...]` part of most error messages), `handleTestConnectionException`, and the Iceberg and Lance REST mappers. The comment below lists them from a bounded scan, not a full audit (`catalogs/` and `core/` were not checked). Triage should decide whether to expand this issue or track those paths separately.

### How should we improve?

**Options:**

- **A. Central error mapper:** one place per server maps internal errors to public, client-consumable errors (for example, a generic message for unexpected 5xx), with the cause kept in server logs. It is consistent and covers paths not yet found, but needs a design that fits the existing mappers and filters.
- **B. Patch each place:** change `message` handling in each place that builds an error response (auth filters, `ErrorExceptionMapper`, `ExceptionHandlers`, Iceberg and Lance mappers). The changes are small and local, but paths are easy to miss and can drift, as the Lance and main-server auth filters already have (`LanceAuthenticationFilter` returns `"Authentication failed"` for its 500 branch; the others return the raw message).
- **A + B:** the central mapper as a safety net for unexpected errors, while each place keeps its own authored 4xx messages (for example `"The provided credentials did not support"`).

I lean toward A, possibly combined with B for authored 4xx messages.

Open questions for triage:

- Which option, and should it be its own setting or tied to `includeErrorStackTrace`?
- Expand this issue to the other paths listed under Scope, or track them separately?

Related: #12728, #13057, discussion #11982.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.