New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue #4334 - Improve testing of ErrorHandler behavior #4335
Issue #4334 - Improve testing of ErrorHandler behavior #4335
Conversation
Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
+ Cleanup from PR review Signed-off-by: Joakim Erdfelt <joakim.erdfelt@gmail.com>
|
||
if (contentType.contains("text/html")) | ||
{ | ||
assertThat(content, not(containsString("<script>"))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usefulness of such an attack isn't as obvious as script tags in HTML, but we should assure that (potentially malformed) JSON or XML messages in exceptions are properly escaped as well. Eg. new Exception(""}{"I am injected JSON":"evil"}{"void":"") or new Exception("evil"), or whatever could break out of the JSON/XML structure and create or modify elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's been addressed.
The ErrorHandler only produces text/html
, text/json
, or text/plain
output.
For text/json
all content from exceptions (the stacktrace, messages, causes, and supressed) are filtered via StringUtil.sanitizeXmlString()
For text/json
all values are filtered via the same StringUtil.sanitizeXmlString()
.
For text/plain
they are left alone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add another test for the JSON example you provided.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it's fine.
See commit 217602e
The generated text/json
looks like this ...
{
"cause1":"java.lang.RuntimeException: "}, "glossary": {\n "title": "example"\n }\n {"",
"cause0":"javax.servlet.ServletException: java.lang.RuntimeException: "}, "glossary": {\n "title": "example"\n }\n {"",
"message":"javax.servlet.ServletException: java.lang.RuntimeException: "}, "glossary": {\n "title": "example"\n }\n {"",
"url":"/jsonmessage/",
"status":"500"
}
Signed-off-by: Joakim Erdfelt joakim.erdfelt@gmail.com