hapifhir / hapi-fhir

🔥 HAPI FHIR - Java API for HL7 FHIR Clients and Servers
http://hapifhir.io
Apache License 2.0
2.04k stars 1.33k forks source link

Don't send unexpected exceptions' details to the client #5305

Open ghost opened 1 year ago

ghost commented 1 year ago

I misconfigured my database credentials the other day and got an error back like this:

GET http://localhost:8090/fhir-api/R4/Contract =>
HTTP 500 (Internal Server Error) in 0.01s
Response body (382 bytes, application/fhir+json; charset=UTF-8):
{
    "resourceType": "OperationOutcome",
    "issue": [
        {
            "severity": "error",
            "code": "processing",
            "diagnostics": "HAPI-0389: Failed to call access method: org.springframework.jdbc.BadSqlGrammarException: PreparedStatementCallback; bad SQL grammar [SELECT DISTINCT CUSTOMER_ID FROM UNKNOWN_SCHEMA]; nested exception is java.sql.SQLSyntaxErrorException: ORA-00942: table or view does not exist\n"
        }
    ]
}

i.e. specifically this part:

PreparedStatementCallback; bad SQL grammar [SELECT DISTINCT CUSTOMER_ID FROM UNKNOWN_SCHEMA]; nested exception is java.sql.SQLSyntaxErrorException: ORA-00942: table or view does not exist

I don't feel comfortable with such detailed exception details being disclosed to the client. To me, this feels like a case of CWE-209: Generation of Error Message Containing Sensitive Information.

I would rather have HAPI return a canned response a la "An unexpected error occurred" and the details being logged on the error-level to be picked up by Splunk or the like.

I tried implementing my own exception handling interceptor based on ca.uhn.fhir.rest.server.interceptor.ExceptionHandlingInterceptor but couldn't get it to work quite like I'd like. For example, I lost the nice browser formatting when an error was thrown, only getting raw application/fhir+json responses back.

To me personally, returning too much exception details to a client is a potential security issue, which is why I'd personally prefer showing canned error messages by default. If you think there's value in showing unhandled exception details, then it could be a configuration option that users should consciously decide to enable, but that shouldn't be the default (in my opinion).

I'd be happy to customize it myself, too, but couldn't find a straight-forward way to do that.

Is this achievable somehow? Has somebody already achieved something like that?

brsolomon-deloitte commented 1 year ago

Seconding this request. 5xx errors should virtually never reveal unneeded details about the server-side exception.

jamesagnew commented 1 year ago

You could accomplish this by creating an interceptor on the SERVER_PRE_PROCESS_OUTGOING_EXCEPTION pointcut which logs the exception however you want, and rethrows something with a more generic message.

ghost commented 1 year ago

@jamesagnew I tried that but couldn't get it to work like I wanted at first. Before spending more time on it, I wanted to check with you whether the current implementation was intentional.

How do you feel about exposing arbitrary exception messages to the client?

Would it be an option to not send those by default?