Affects: 6.1.12
the content of a parameter annotated with @RequestBody should contain the body of the request as sent to the server but it is encoded wrong
The following example controller:
@RestController
public class AsteriskController {
private static final Logger log = LoggerFactory.getLogger(AsteriskController.class);
@PostMapping(value = "/message", consumes = "application/x-www-form-urlencoded")
public void message(@RequestBody String body) {
log.info("Received body: {}", body);
}
}
when sent the following request
curl -L 'localhost:8080/message' \
-H 'Content-Type: application/x-www-form-urlencoded' \
-d 'payload=%7B%22text%22%3A%22%2ABe%2Bnotified%2A%22%7D'
prints out
Received body: payload=%7B%22text%22%3A%22*Be%2Bnotified*%22%7D
whereas it should print
Received body: payload=%7B%22text%22%3A%22%2ABe%2Bnotified%2A%22%7D
The * encoded as %2A is decoded and encoded wrong somehow in the filter chain when parsing the request
Comment From: bclozel
Can you provide a minimal sample application? You're mentioning the filter chain so I would be interesting to know which Servlet filters are being applied. Thanks!
Comment From: tompson
I created a sample project: https://github.com/tompson/asterisk
Comment From: bclozel
Thanks for the sample @tompson
I traced this behavior back to the fact that this is a FORM request, and that in this case Spring Framework is reading the form using the Servlet request getParameterMap, and reconstructing the body by URL encoding each element. This is done for reasons explained in the implementation.
Now in this case, the * are not re-encoded as JDK's URLEncoder is not encoding such characters.
If you wish to get the exact original body, you should read the body from the request - but then you would lose some abilities to re-read the body as explained just above.
@PostMapping(value = "/other", consumes = "application/x-www-form-urlencoded")
public void other(HttpServletRequest request) throws IOException {
int contentLength = request.getContentLength();
byte[] bytes = request.getInputStream().readNBytes(contentLength);
String body = new String(bytes, StandardCharsets.UTF_8);
log.info("Received body: {}", body);
}
Is this behavior causing some incorrect behavior with HTTP clients, deserialization, or something else?
Comment From: tompson
thanks for looking into it @bclozel !
When I debugged it it tried using ContentCachingRequestWrapper and found the same code reconstructing the body from the getParameterMap in there. (Code duplication?)
I totally understand why this is done because the request body can only be read once, but I think when somebody is using @RequestBody the expection is to get the original body of the request.
In our use case we want to verify the signature of a slack request that is sent by the slack backend to our Spring Boot application that acts as a slack boot as documented here. The signature sent in the x-slack-signature request header is based on the original bytes of the request, so a verification using the reencoded request body does not work.
Comment From: bclozel
When I debugged it it tried using ContentCachingRequestWrapper and found the same code reconstructing the body from the getParameterMap in there. (Code duplication?)
This looks like duplication but it's a small inconvenient; we can't share this part between different parts of our codebase without making it a public contract, which we don't intend to.
This is a well-known behavior of the Servlet specification and I don't think we can do anything about this. It seems you are enforcing this from a Servlet filter (that's what I'm assuming since you're using ContentCachingRequestWrapper). Because this is an integrity check, I would argue that getting the body as a byte stream is the correct way to do this since decoding the request body will lose information in many cases.
The solution I have suggested above also works for a Servlet filter and is in my opinion the correct way to handle this case.
Comment From: tompson
the way we solved it was by implementing a HttpServletRequestWrapper that parses the parameters from the request by still keeping the bytes of the body intact:
public class SlackRequestVerificationRequestWrapper extends HttpServletRequestWrapper {
private final byte[] cacheBody;
private final Map<String, String[]> parameterMap;
public SlackRequestVerificationRequestWrapper(HttpServletRequest request) throws IOException {
super(request);
this.cacheBody = StreamUtils.copyToByteArray(request.getInputStream());
String contentType = super.getContentType();
if (contentType != null &&
contentType.contains(MediaType.APPLICATION_FORM_URLENCODED_VALUE) &&
HttpMethod.POST.matches(getMethod())) {
parameterMap = parseParameters(new String(this.cacheBody), Charset.forName(getCharacterEncoding()));
} else {
parameterMap = super.getParameterMap();
}
}
@Override
public ServletInputStream getInputStream() {
return new CacheBodyServletInputStream(this.cacheBody);
}
@Override
public BufferedReader getReader() {
return new BufferedReader(new InputStreamReader(new ByteArrayInputStream(this.cacheBody)));
}
@Override
public String getCharacterEncoding() {
String enc = super.getCharacterEncoding();
return (enc != null ? enc : WebUtils.DEFAULT_CHARACTER_ENCODING);
}
public byte[] getContentAsBytes() {
return cacheBody;
}
/**
* this is necessary because after reading the input stream of the request the way super.getParameterMap() parses
* the parameters from the request body does not work anymore because the input stream can only be read once
*/
private Map<String, String[]> parseParameters(String requestBody, Charset charset) {
Map<String, String[]> paramMap = new HashMap<>();
for (NameValuePair param : URLEncodedUtils.parse(requestBody, charset)) {
String name = param.getName();
String value = param.getValue();
if (paramMap.containsKey(name)) {
String[] existingValues = paramMap.get(name);
String[] newValues = new String[existingValues.length + 1];
System.arraycopy(existingValues, 0, newValues, 0, existingValues.length);
newValues[existingValues.length] = value;
paramMap.put(name, newValues);
} else {
paramMap.put(name, new String[]{value});
}
}
return paramMap;
}
@Override
public Map<String, String[]> getParameterMap() {
return parameterMap;
}
@Override
public String[] getParameterValues(String name) {
return getParameterMap().get(name);
}
@Override
public String getParameter(String name) {
String[] values = getParameterValues(name);
return (values != null && values.length > 0) ? values[0] : null;
}
@Override
public Enumeration<String> getParameterNames() {
return Collections.enumeration(getParameterMap().keySet());
}
private static class CacheBodyServletInputStream extends ServletInputStream {
private final InputStream cachedBodyInputStream;
public CacheBodyServletInputStream(byte[] cacheBody) {
this.cachedBodyInputStream = new ByteArrayInputStream(cacheBody);
}
@Override
public boolean isFinished() {
try {
return cachedBodyInputStream.available() == 0;
} catch (IOException e) {
// will not happen
}
return false;
}
@Override
public boolean isReady() {
return true;
}
@Override
public void setReadListener(ReadListener listener) {
throw new UnsupportedOperationException();
}
@Override
public int read() throws IOException {
return cachedBodyInputStream.read();
}
}
}