feat: Backends return Backends.QueryError struct for errors#3552
feat: Backends return Backends.QueryError struct for errors#3552msmithstubbs wants to merge 3 commits into
Conversation
394c535 to
d322ebf
Compare
| ~s(Field "notthere" does not exist.) | ||
| """ | ||
| @spec query_error_message(QueryError.t()) :: String.t() | nil | ||
| def query_error_message(%QueryError{ |
There was a problem hiding this comment.
Error messages could be in the respective backend adaptors but thought it better to keep this in the UI layer.
| end | ||
|
|
||
| @spec to_query_error(term()) :: QueryError.t() | ||
| defp to_query_error(%Ch.Error{message: message} = error) when is_non_empty_binary(message) do |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
Raw ClickHouse error messages (including DB server version strings like version 26.2.19.43 (official build) and full exception class paths) are stored in QueryError.message and serialized to external API callers via EndpointsController. Previously these paths returned the generic string "Error executing ClickHouse query", so this is a regression in information exposure.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Replace the raw ClickHouse error message with a generic, user-facing string (e.g. "Error executing ClickHouse query") in the QueryError.message field. The full error details are already preserved in raw_error: error for internal logging and debugging, so no diagnostic information is lost internally. This restores the previous non-disclosing behaviour while still surfacing a meaningful status to the caller.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| defp to_query_error(%Ch.Error{message: message} = error) when is_non_empty_binary(message) do | |
| defp to_query_error(%Ch.Error{message: _message} = error) when is_non_empty_binary(_message) do | |
| %QueryError{ | |
| message: "Error executing ClickHouse query", | |
| code: :invalid_query, | |
| raw_error: error, | |
| backend: Logflare.Backends.Adaptor.ClickHouseAdaptor | |
| } | |
| end |
| @spec to_query_error(term()) :: QueryError.t() | ||
| defp to_query_error(%Postgrex.Error{} = error) do | ||
| %QueryError{ | ||
| message: Exception.message(error), |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
Exception.message/1 on Postgrex.Error returns the full PostgreSQL error including the executed SQL query text (e.g. column "x" does not exist\n\n query: select x from <internal_table_name>). This raw message is placed in QueryError.message, which is JSON-serialized and returned to external API callers via EndpointsController, leaking internal table names and query structure.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: Replace Exception.message(error) with an extraction of only the PostgreSQL-level error message from error.postgres[:message]. The Exception.message/1 implementation for Postgrex.Error formats a string that includes the full executed SQL query text (e.g. query: select x from <internal_table_name>), which leaks internal table names and query structure to API callers. The error.postgres map contains a :message key with just the human-readable database error (e.g. "column \"x\" does not exist") without the query text. Use Map.get(error.postgres || %{}, :message, "Invalid query") to safely extract only the sanitized error message, with a generic fallback in case error.postgres is nil.
⚠️ Experimental Feature: This code suggestion is automatically generated. Please review carefully.
| message: Exception.message(error), | |
| message: Map.get(error.postgres || %{}, :message, "Invalid query"), |
Ziinc
left a comment
There was a problem hiding this comment.
needs some adjustment for surfacing of errors.
need some distinction between error messages:
- end consumer errors - needs to be generic and shouldn't leak information like underlying backend errors
- operator user - should surface underlying backend errors, but logged out to Logger and surfaced to user via
system.logswith theuser_idmetadata key set. - admin user - surfaced to admin via Logger, such as connection pool errors, which might not have a
user_idassociated, and needs the most detail, on par with operator user
|
|
||
| defp to_query_error(%Ecto.QueryError{message: message} = error) do | ||
| %QueryError{ | ||
| message: message, |
There was a problem hiding this comment.
i think we should restrict what is surfaced to users, in the event that sensitive values get surfaced. things like invalid columns etc are columns are fine, probably can match on the message prefix and give a more generic message for the rest.
There was a problem hiding this comment.
This applies for all backends, not just for postgres
|
Make sense.
How do we identify an operator user? |
00e4105 to
d4102d9
Compare
|
Revised approach:
|
Co-authored-by: depthfirst-app[bot] <184448029+depthfirst-app[bot]@users.noreply.github.com>
d4102d9 to
443fe54
Compare
This PR improves improves the error message show when a user tries to select a non-existent field.
Endpoints also shows error messages from the backend but just dumps the error body into a flash message. This could be improved but this PR is already larger than I'd like. Could do a follow up?
Closes O11Y-1819