HTTP request body not being reported for application/json encodings

[dlopuch]

If your HTTP request contains a json-encoded body (eg POST requests on modern API's), the body doesn't show up in sentry.

The lib extracts the body using ServletRequest.getParameterMap(): https://github.com/getsentry/raven-java/blob/v7.8.3/raven/src/main/java/com/getsentry/raven/event/interfaces/HttpInterface.java#L55

This works fine for form url encodings but returns nothing for JSON encodings. eg http://stackoverflow.com/questions/3831680/httpservletrequest-get-json-post-data

The simplest way to get the raw body text is something like servletRequest.getReader().lines().collect(Collectors.joining()) (but getReader() has limitations that it can only be called once, so sentry will have to be clever not to hurt any downstream code). Ideally this could be sent up to sentry.

[bretthoerner]

We discussed this previously here: #223 (comment)

The fact that getReader() can only be called once seemingly kills any chance for Sentry to do anything automatically. Do you have any ideas for getting around that? I think that users will have to set the body manually... 😬

[bretthoerner]

This also comes up some here: #273

[dlopuch]

Yes, there are tons of stackoverflow's asking for workarounds to the fact that an HttpServletRequest's input can be read only once. Someone over at sun once long ago probably had a good reason for it, but today as soon as you add a bunch of logging modules upstream from something like jackson that converts your json into java, it becomes a major pia.

My solution has been to write a request filter that replaces the HttpServletRequest with a custom HttpServletRequestWrapper which caches the body and lets getInputStream() work multiple times for any downstream plugin to not have to worry about what happened before. Basically http://www.myjavarecipes.com/tag/how-to-read-request-twice/. Spring users can easily adopt that for a spring filter bean.

With that caching filter in place, workaround is I just dump the body into the MDC where sentry auto picks it up. An API like in #273 would let me directly inject it.

---

I think sentry automatically replacing the HttpServletRequest with a request cache wrapper is kinda dangerous (lots of concerns around out-of-scope of sentry, eg request size limits, what if someone is already doing their own cache, etc.), but given these limitation of j2ee, I think it would be awesome if you guys at least provided a default implementation that acts as an out-of-the-box opt-in-if-you-accept-the-risks.

#273's API would help a lot, as well as updating the docs to make it clear that only form-encoded request payloads are included by default (to include application/json, opt-in to this this silly request-caching filter kludge).

[bretthoerner]

I agree about having an opt-in solution, I'll check it out. I did the start of #273 (making a field available so it's easier to set a body -- you'd still have to use your own RavenFactory and subclass the HttpEventBuilderHelper for now) if you want to check it out: #334

[bretthoerner]

A String body field was added in 7.8.5. This doesn't automate anything, but it should reduce the amount of customization required for now: 10ac4a5

[dineshsriram]

If this were to be done automagically by Sentry, that would be great.
Are there any plans to have a custom filter integrated as @dlopuch suggested?

[tienhm0202]

@bretthoerner Can you please provide some example how to set request body manual? Can I define it in ServletContextInitializer as a Spring bean? Or I have to set body every time error happen?

[charlie-world]

Is it fixed completely? i can't see body data on sentry error event now with sentry-java 1.7.23

[arpanagr]

Is this fixed or is there any workaround if I am using spring-sentry?

[kaixin0571]

I hava fixed this issue using spring-boot
step1:

public class HttpEventBuilderHelperWithBody extends HttpEventBuilderHelper {

    @Override
    public void helpBuildingEvent(EventBuilder eventBuilder) {
        ServletRequestAttributes requestAttributes = (ServletRequestAttributes)RequestContextHolder.getRequestAttributes();
        ContentCachingRequestWrapper wrapper =
                WebUtils.getNativeRequest(requestAttributes.getRequest(), ContentCachingRequestWrapper.class);
        String body = null;
        if (wrapper != null) {
            byte[] buf = wrapper.getContentAsByteArray();
            if (buf.length > 0) {
                try {
                    body = new String(buf,0,buf.length,wrapper.getCharacterEncoding());
                } catch (UnsupportedEncodingException e) {
                }
            }
        }
        eventBuilder.withSentryInterface(new HttpInterface(requestAttributes.getRequest(),super.getRemoteAddressResolver(),body),false);
        addUserInterface(eventBuilder,requestAttributes.getRequest());
    }

    private void addUserInterface(EventBuilder eventBuilder, HttpServletRequest servletRequest) {
        String username = null;
        if (servletRequest.getUserPrincipal() != null) {
            username = servletRequest.getUserPrincipal().getName();
        }

        UserInterface userInterface = new UserInterface(null, username,
                super.getRemoteAddressResolver().getRemoteAddress(servletRequest), null);
        eventBuilder.withSentryInterface(userInterface, false);
    }
}

step2:

public class SentryClientCustomFactory extends DefaultSentryClientFactory {

	@Override
	public SentryClient createSentryClient(Dsn dsn) {

		try {
			SentryClient sentryClient = new SentryClient(createConnection(dsn), getContextManager(dsn));
			try {
				// `ServletRequestListener` was added in the Servlet 2.4 API, and
				// is used as part of the `HttpEventBuilderHelper`, see:
				// https://tomcat.apache.org/tomcat-5.5-doc/servletapi/
				Class.forName("javax.servlet.ServletRequestListener", false, this.getClass().getClassLoader());
                                // sentryClient.addBuilderHelper(new HttpEventBuilderHelper());
				sentryClient.addBuilderHelper(new HttpEventBuilderHelperWithBody());
			} catch (ClassNotFoundException e) {
				log.debug("The current environment doesn't provide access to servlets,"
						+ " or provides an unsupported version.");
			}
                      
			sentryClient.addBuilderHelper(new ContextBuilderHelper(sentryClient));
			return configureSentryClient(sentryClient, dsn);
		} catch (Exception e) {
			log.error("Failed to initialize sentry, falling back to no-op client", e);
			return new SentryClient(new NoopConnection(), new ThreadLocalContextManager());
		}
	}

}

[bruno-garcia]

For reference what the option looks like in other SDKs:

https://github.com/getsentry/sentry-docs/blob/b54a12d2b2626c112905c890cfa69953e2602972/src/platforms/common/configuration/options.mdx#L177-L186

[marandaneto]

btw logging the whole response body might drop the events by the server - size limits (aka 413), so I'd not recommend that, unless the SDK supports this config max-request-body-size which is not supported by most of them.

[luStopai]

I started using sentry recently and I'm very interested in this feature.

Here are my two cents:

As for the "The fact that getReader() can only be called once seemingly kills any chance for Sentry to do anything automatically", what about caching the body request with a simple cache filter as explained here https://www.baeldung.com/spring-reading-httpservletrequest-multiple-times ?

Since I think caching has already been mentioned before I'm not sure what are exactly the concerns that are preventing this feature to be implemented, so could you please elaborate on that?

For now I'll try to address some of the concerns I've read of.

I'm thinking about having two options:

• Include_body: If sentry should try to fetch body or not, therefore making it optional. It would be the users responsability to cache body by adding a cache filter that allows to call getReader multiple times.
• Enable_cache: This would enable caching by sentry (meaning sentry would provide its own implementation of cache filter). This way, having both options set to true would be the easieast way to send body to sentry while still allowing users to provide their own implementation of a cache filter by setting enable cache to false. This option would also avoid double caching.

What do you think?

In the meantime, if I already have a cache filter, Is there anything I can do, as a workaround, to send body to sentry?

[bruno-garcia]

I started using sentry recently and I'm very interested in this feature.

Here are my two cents:

As for the "The fact that getReader() can only be called once seemingly kills any chance for Sentry to do anything automatically", what about caching the body request with a simple cache filter as explained here baeldung.com/spring-reading-httpservletrequest-multiple-times ?

ASP.NET Core has the same issue where the body is a stream and it's read once and data is gone. So in order to get this feature to work, if opt-in, we do wrap the stream with a memory stream so the payload stays around in case of an error. This obviously can affect the overall application memory footprint since now data can hang around for longer even if not needed (i.e Used to stream into a deserializer and create objects, now both objects plus original binary blob are alive for the duration of the request).

Since I think caching has already been mentioned before I'm not sure what are exactly the concerns that are preventing this feature to be implemented, so could you please elaborate on that?

For now I'll try to address some of the concerns I've read of.

I'm thinking about having two options:

• Include_body: If sentry should try to fetch body or not, therefore making it optional. It would be the users responsability to cache body by adding a cache filter that allows to call getReader multiple times.
• Enable_cache: This would enable caching by sentry (meaning sentry would provide its own implementation of cache filter). This way, having both options set to true would be the easieast way to send body to sentry while still allowing users to provide their own implementation of a cache filter by setting enable cache to false. This option would also avoid double caching.

What do you think?

This feature exists in other SDKs and is configured like this:

An option named maxRequestBodySize.

The values are:

• never: request bodies are never sent (default)
• small: only small request bodies will be captured where the cutoff for small depends on the SDK (typically 4KB)
• medium: medium and small requests will be captured (typically 10KB)
• always: the SDK will always capture the request body for as long as Sentry can make sense of it

If opting in means the request stream need to be buffered, that should be transparent. Docs can mention this caveat but it's fair to assume that if you're looking to get the request body included in errors, you're not running a server that's about to tip over and you can afford having the payload buffered.

In the meantime, if I already have a cache filter, Is there anything I can do, as a workaround, to send body to sentry?

I believe others such as @maciejwalkowiak can help better here. This talk is high in priority though so hopefully we can see this happening in the upcoming weeks.

[luStopai]

That's great news @bruno-garcia, can't wait for that!!

Just a few more suggestions, hopefully they'll be useful:

• Instead of having (just) this 4 "random" body sizes, let the users choose the threshold that better fits their app
• Have an option to log only certain bodies according to request headers such as application/json (or having a similar options such as log text-only-bodies). Useful in cases where we want to log json bodies, but not binary bodies, for example.
  I mean, if an app allows uploading relatively big binary files (few MBs). Would they be buffered too?

Also, don't know if that's possible, but it would also be great if through some configuration, or maybe annotations, we could enable/disable body logging on specific endpoints, therefore having more control on where it makes sense to log or not body requests.
Using annotations seems to be the easiest or cleanest way to achieve that, but configuring this through spring properties would allow to enable/disable body logging temporary on specific endpoints without changing any code.
I know this is asking way too much for a first solution (in case this is possible), I just wanted to suggest this idea in case it makes sense to you to have it in backlog for a far future release. 😉

[marandaneto]

using Spring capabilities is fine and makes total sense, but let's also be sure that the core impl. is also expandable for mobile usage (eg Android uses the sentry package) and should be able to configure such limits as well.

[bruno-garcia]

using Spring capabilities is fine and makes total sense, but let's also be sure that the core impl. is also expandable for mobile usage (eg Android uses the sentry package) and should be able to configure such limits as well.

This is historically a server side feature. For example, the core dotnet sdk doesn't have this feature. Only aspnet and aspnet core. How do you see us using this from android?

[marandaneto]

I thought we could use this feature to populate the request's payload or response's body when doing HTTP calls and raising events as a client

[bruno-garcia]

This was added on Spring MVC through #1595