Added request caching to add body to event #811
Conversation
Signed-off-by: Anton Rameykov <arcane561@gmail.com>
| Sentry.capture(ex); | ||
| return null; | ||
| } | ||
| String requestBody = new String(cacheRequest.getContentAsByteArray()); |
There was a problem hiding this comment.
Maybe not use the default charset of the JVM here? What about something like (ignoring error handling):
Charset cs = request.getCharacterEncoding() == null ? StandardCharsets.UTF_8 : Charset.forName(request.getCharacterEncoding());
String requestBody = StreamUtils.copyToString(cacheRequest.getInputStream(), cs);
There was a problem hiding this comment.
Using the charset from the request if available makes sense to me. But also, how do we guarantee this body can be converted to String?
Also, since this potentially allocating a huge string and affecting the server performance this should be opt-in
| ContentCachingRequestWrapper cacheRequest = new ContentCachingRequestWrapper(request); | ||
| filterChain.doFilter(cacheRequest, response); | ||
| } |
There was a problem hiding this comment.
While this is a very nice solution, I'm a little bit unsure about the effects of requiring the content caching wrapper of the request unconditionally. Is there a situation where this might blow up memory consumption?
@metlos Thanks so much for the review! At the expense of memory, an explosion does occur if the request body is binary file and large, as well as if the request was already in the cache. I tried to solve this by checking that the request came in was an instance of the ContentCachingRequestWrapper class, and also doing a check on the size of the request body, however, I can’t decide what value is optimal for caching, the Sentry documentation says something about 10 kilobytes per message, however can you suggest another optimal value
Example code
private final long MAX_BODY_SIZE_CACHING = 31457280; //~30 mb
@Override
protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain filterChain)
throws ServletException, IOException {
if (request instanceof ContentCachingRequestWrapper || request.getContentLengthLong() >= MAX_BODY_SIZE_CACHING){
filterChain.doFilter(request, response);
return;
}
ContentCachingRequestWrapper wrappedRequest = new ContentCachingRequestWrapper(request);
There was a problem hiding this comment.
Most likely, will have to specify the priority of the filter execution, namely, assign the lowest priority so that all user filters are executed first
There was a problem hiding this comment.
I would also think about introducing some kind of blacklist for some Rest paths at the user's choice, we will offer an empty blacklist and an interface that can be implemented, the user will write the desired blacklist configuration with path checking and implement it, and redefine the standard configuration in the case of Spring boot or define it yourself in case of vanilla Spring
What do you think about this?
There was a problem hiding this comment.
Most likely, will have to specify the priority of the filter execution, namely, assign the lowest priority so that all user filters are executed first
On the other hand, it would be cool to have it working for exceptions raised in user-filters as well. What's wrong with executing this filter first?
The idea with checking content length seems fine, it's better to have it working for majority of cases than none. It would be great to have MB threshold configurable in properties file (along with blacklist you mentioned).
What's the status of it? Are we waiting for review? Anyone?
There was a problem hiding this comment.
The other implementations of this have "sizes". And also, are opt-in since in many cases request body is read from a stream and never really cached so doing this cache by default could be suboptimal.
Here's how it was done in C#:
The idea is that unless a size is specified via SentryOptions we're not buffering request bodies.
| Sentry.capture(ex); | ||
| return null; | ||
| } | ||
| String requestBody = new String(cacheRequest.getContentAsByteArray()); |
There was a problem hiding this comment.
Using the charset from the request if available makes sense to me. But also, how do we guarantee this body can be converted to String?
Also, since this potentially allocating a huge string and affecting the server performance this should be opt-in
| ContentCachingRequestWrapper cacheRequest = new ContentCachingRequestWrapper(request); | ||
| filterChain.doFilter(cacheRequest, response); | ||
| } |
There was a problem hiding this comment.
The other implementations of this have "sizes". And also, are opt-in since in many cases request body is read from a stream and never really cached so doing this cache by default could be suboptimal.
Here's how it was done in C#:
The idea is that unless a size is specified via SentryOptions we're not buffering request bodies.
Hi Sentry,
This commit tries to solve the problem described in issues #331 and #273, namely the inability to see the body of the HTTP request in the web interface