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

HTML-escaped security bug fix [ issue #188 ] #424

Merged
merged 8 commits into from
Jun 16, 2020

Conversation

aswinijena100
Copy link
Collaborator

PR contains issues fix for User data should be HTML-escaped for web display .

@googlebot googlebot added the cla: yes Do not use - reserved for devops label May 28, 2020
Copy link
Contributor

@nikklassen nikklassen left a comment

Choose a reason for hiding this comment

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

I don't feel very comfortable with this solution, it relies too much on programmers remembering to clean their inputs. I would prefer using @Valid annotation, like this blog post, or a global filter like this blog post.

I prefer the @Valid annotation because this pattern can be used for lots of other types of validation too, which I mentioned in #409 as a way to clean up our controller code.

}

@SuppressWarnings("rawtypes")
public static void replaceAll(Object obj, String... excludeProps) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to use a standard sanitizer here, like the one from jsoup, instead of writing our own.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have removed the custom logic to handle XSS attacked and implemented standard sanitizer (jsoup) by extending PropertyEditorSupport class. Please review and let us know.

Copy link
Contributor

@nikklassen nikklassen left a comment

Choose a reason for hiding this comment

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

This looks pretty good, but can you add the @InitBinder to a @ControllerAdvice so that it is automatically applied to all Controllers?

@aswinijena100
Copy link
Collaborator Author

This looks pretty good, but can you add the @InitBinder to a @ControllerAdvice so that it is automatically applied to all Controllers?

Yes, done the changes now to use @ControllerAdvice and added @Initibinder in it instead of adding in each controller classes.

@aswinijena100 aswinijena100 merged commit 5f56cb8 into early-access Jun 16, 2020
@aswinijena100 aswinijena100 deleted the early-access-html-escaped-issue-fix branch June 16, 2020 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Do not use - reserved for devops
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants