smartsheet / smartsheet-java-sdk

Library that uses Java to connect to Smartsheet services.
Apache License 2.0
5 stars 13 forks source link

DefaultHttpClient logs entire response on Error #67

Closed zromano closed 1 year ago

zromano commented 1 year ago

In DefaultHttpClient, we allow clients to configure what level of logging they would like to have. This helps clients from leaking sensitive PII into their logs.

However, if there is an HTTP error (non 200 code) when communicating with Smartsheet, then we log the entire response.

See the logging code below. We should not be using REQUEST_RESPONSE here.

public void logRequest(HttpRequestBase request, HttpEntitySnapshot requestEntity,
                       HttpResponse response, HttpEntitySnapshot responseEntity, long durationMillis) throws IOException {
    logger.info("{} {}, Response Code:{}, Request completed in {} ms", request.getMethod(), request.getURI(),
            response.getStatusCode(), durationMillis);
    if (response.getStatusCode() != 200) {
        // log the request and response on error
        logger.warn(LOG_ARG, RequestAndResponseData.of(request, requestEntity, response, responseEntity, REQUEST_RESPONSE));
    } else {
        // log the summary request and response on success
        logger.debug(LOG_ARG, RequestAndResponseData.of(request, requestEntity, response, responseEntity, REQUEST_RESPONSE_SUMMARY));
    }
}

Asks:

zromano commented 1 year ago

In my MR, I made it so we don't log any additional request/response info on a non 200 response.

The client can enable debug logging or look at the request URI that was in the info level log that still happens.

In the future if clients ask for better error logging, we can add some configurable logic in there.