Skip to content
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

Avoiding possible XSS attack through the default error handler #1368

Merged
merged 2 commits into from Aug 15, 2019

Conversation

imeszaros
Copy link
Member

@jknack Please review, especially the test case. I'm not familiar with it, i'm not sure if this line is correct:

expect(env.xss("html")).andReturn(HtmlEscapers.htmlEscaper()::escape);

Function<Object, String> xssFilter = env.xss("html").compose(Objects::toString);
BiFunction<String, Object, String> escaper = (k, v) -> xssFilter.apply(v);

Supplier<Map<String, Object>> detailsProvider = () -> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's remove the supplier and just use the Map

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question about this: in the original code, the html and all cases got two different instances of the map, because toMap() was invoked in the Supplier lambda passed to the when calls:

Results
    .when(MediaType.html, () -> Results.html(VIEW).put("err", ex.toMap(stackstrace)))
    .when(MediaType.all, () -> ex.toMap(stackstrace)));

This is the reason I introduced the supplier. If I pass the same map instance to the suppliers in when calls, test cases will fail because org.jooby.DefaultErrHandlerTest#checkErr is invoked when validating the html response type view, and it removes all entries from the map, but later there is another assertation for the all response data which expects the size of the map to be 4.

I could either modify the test case or pass a copy of the map to the suppliers like this:

      Map<String, Object> details = ex.toMap(stackstrace);
      details.compute("message", escaper);
      details.compute("reason", escaper);

      rsp.send(
          Results
              .when(MediaType.html, () -> Results.html(VIEW).put("err", new LinkedHashMap<>(details)))
              .when(MediaType.all, () -> new LinkedHashMap<>(details)));

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. If you feel strongly go and modify the existing test, otherwise let's keep it

Thanks

@jknack jknack added the bug label Aug 14, 2019
@jknack jknack added this to the 1.6.4 milestone Aug 14, 2019
@imeszaros
Copy link
Member Author

I modified the test case very slightly so org.jooby.DefaultErrHandlerTest#checkErr now works with a local copy of the map.

@jknack jknack merged commit 27b4af2 into jooby-project:1.x Aug 15, 2019
@imeszaros imeszaros deleted the xss-fix branch August 16, 2019 07:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants