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

[UP-3315] Fix some XSS issue around inputting usernames. The issue was ... #379

Merged

Conversation

jhelmer-unicon
Copy link
Contributor

...occurring because "</script>" always terminates the

JS block -- even if that's inside a string. The JS interpreter would terminate the block and then misinterpret the unterminated
contents of the string as executable code. The right solution for this is probably to start whitelisting the
input characters for every input field. Any field that will be part of a RESTful URL should not be allowed to contain the '/'
character. The "solution" in this commit is intended as a stop-gap. It solves this biggest issue -- the XSS vulnerability --
but it doesn't fully solve the issues caused by invalid chars in the input. IMO, this is better than what we had, but
still not great.
(cherry picked from commit b034dec)

…s occurring because "</script>" always terminates the

JS block -- even if that's inside a string.  The JS interpreter would terminate the block and then misinterpret the unterminated
contents of the string as executable code.  The *right* solution for this is probably to start whitelisting the
input characters for every input field. Any field that will be part of a RESTful URL should not be allowed to contain the '/'
character.  The "solution" in this commit is intended as a stop-gap.  It solves this biggest issue -- the XSS vulnerability --
but it doesn't fully solve the issues caused by invalid chars in the input.  IMO, this is better than what we had, but
still not great.
(cherry picked from commit b034dec)
@jhelmer-unicon
Copy link
Contributor Author

Note: I'm creating this PR for now just to get the PR setup. I'm 99% sure that it's fine, but so far have been unable to test in 4.0.15 due to issues I am having with initdb in the 4.0.15-SNAPSHOT branch. Will update when I get a chance to test better...

-- k. Was able to test the basic XSS issue. There is still an issue w/ tomcat not allowing %2f ('/') but that is expected.

@jameswennmacher
Copy link

I think this contribution would be better placed in the portlet-utils project in the portlet-mvc-util module, file org.jasig.web.jsp.JstlUtil so it could be used by portlets as well.

@jameswennmacher
Copy link

Also, please revise the commit message (and pull request message) from [up-3315] ... to UP-3315 ... to follow conventions.

@jhelmer-unicon jhelmer-unicon changed the title [up-3315] Fix some XSS issue around inputting usernames. The issue was ... [UP-3315] Fix some XSS issue around inputting usernames. The issue was ... Jul 9, 2014
…s occurring because "</script>" always terminates the

JS block -- even if that's inside a string.  The JS interpreter would terminate the block and then misinterpret the unterminated
contents of the string as executable code.  The *right* solution for this is probably to start whitelisting the
input characters for every input field. Any field that will be part of a RESTful URL should not be allowed to contain the '/'
character.  The "solution" in this commit is intended as a stop-gap.  It solves this biggest issue -- the XSS vulnerability --
but it doesn't fully solve the issues caused by invalid chars in the input.  IMO, this is better than what we had, but
still not great.
(cherry picked from commit b034dec)
@apetro
Copy link
Member

apetro commented Oct 17, 2014

👍

@jameswennmacher
Copy link

Created issue uPortal-Project/portlet-utils#2 to track adding this functinality to portlet-util project.

@drewwills
Copy link
Contributor

Looks good to me too. Merging.

drewwills added a commit that referenced this pull request Feb 17, 2015
[UP-3315] Fix some XSS issue around inputting usernames.  The issue was ...
@drewwills drewwills merged commit a2dfd16 into uPortal-Project:rel-4-0-patches Feb 17, 2015
@drewwills
Copy link
Contributor

Should probably have been opened on master, however.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants