Strip invalid JS var name chars from JSONP callback #33

Merged
merged 7 commits into from Apr 25, 2011

Conversation

Projects
None yet
3 participants
Member

mtodd commented Apr 6, 2011

This is essentially in order to prevent XSS vulnerabilities.

Would love some feedback.

Right now, I just strip out any non-word characters and .. I think I need to also include $ as well.

Alternatively to stripping out invalid characters is to use all characters up to the first invalid character. This means foo<script>alert(1)</script> would clean to foo instead of fooscriptalert1script.

mtodd added some commits Apr 6, 2011

@mtodd mtodd Clean JSONP callback
* strips any non-word characters: attemps to ensure only valid JavaScript
  function names are returned
* reduces XSS vulnerability
67d5660
@mtodd mtodd Support JSONP callbacks with dots (.)
* in practice, callbacks often are within objects
* for example:
  foo.bar.baz({...})
32f92b5
@mtodd mtodd Support $ as valid JS callback name character
* is a valid JS var/function name
ffa75b4
Member

mtodd commented Apr 6, 2011

Added support for $.

dkubb commented Apr 7, 2011

What I've done in the past is created a regexp that the callback name must match for it to be considered valid:

CALLBACK_IDENTIFIER = /[a-zA-Z_$][\w$]*/
CALLBACK_REGEXP     = /\A#{CALLBACK_IDENTIFIER}(?:\.?#{CALLBACK_IDENTIFIER})*\z/

This allows valid JS identifiers, optionally separated by dots, to be used as a callback. If it doesn't match the regexp what I've done is returned a JS document with an alert saying the callback name is invalid.

Member

mtodd commented Apr 7, 2011

Do you think the alert is, by default, the right choice? Or an error due to a non-existent function call?

Both sides have their benefits and drawbacks: for me, I'd rather it just fail rather than alert, but then again, if someone is sending a bad callback by accident or if someone is doing something evil, an alert might be good but someone that savvy would know to look in the console, I believe.

The goal is to make a change that protects this middleware by default that most everyone would find acceptable. Do we need to make changes to meet this requirement?

dkubb commented Apr 7, 2011

Oh, I don't know if the alert makes sense in the general rack plugin. I was more mentioning it as an alternative.

Although personally I'd prefer an alert to the system silently failing. Even a 400 response would be better than nothing.

Member

mtodd commented Apr 7, 2011

Yeah, that sounds more reasonable. I'll update the branch to return a 400 Bad Request instead. Thank you, sir.

Member

mtodd commented Apr 9, 2011

Sanity check?

dkubb commented Apr 9, 2011

@mtodd: is it necessary to test has_callback?(request) twice? Can you just do: if is_json?(headers) && callback since presumably the guard clause would've taken care of an invalid callback param earlier on.

Other than that it looks great.

Member

mtodd commented Apr 9, 2011

Boom. Green light for master?

dkubb commented Apr 9, 2011

Looks good to me, although I have no authority/access to merge into master. I'm just an interested observer ;)

Contributor

judofyr commented on ffa75b4 Apr 10, 2011

What about underscore?

Member

mtodd replied Apr 10, 2011

\w includes _.

@mtodd mtodd added a commit that referenced this pull request Apr 25, 2011

@mtodd mtodd Merged pull request #33 from rack/jsonp-xss-vulnerability.
Strip invalid JS var name chars from JSONP callback
4849ed7

@mtodd mtodd merged commit 4849ed7 into master Apr 25, 2011

mpalmer deleted the jsonp-xss-vulnerability branch Jun 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment