Serializing configuration to client should do the proper encoding #11

Closed
imalberto opened this Issue Sep 24, 2013 · 9 comments

Comments

Projects
None yet
4 participants

See Trello card 745 for more info

Collaborator

caridy commented Sep 24, 2013

Here is the implementation used in Mojito today:
https://github.com/yahoo/mojito/blob/develop/lib/app/autoload/util.common.js#L105

And here is where we clean up YUI_config before sending it to the client side:
https://github.com/yahoo/mojito/blob/develop/lib/app/addons/ac/deploy.server.js#L127

ericf was assigned Sep 25, 2013

Collaborator

ericf commented Sep 25, 2013

Can you guys point to some documentation that explains the attack vector? An authoritative blog post would do — I want to have something to link to in the code and HISTORY.md, and also have an example to test.

Collaborator

caridy commented Oct 3, 2013

There are also other issues related with the encoding. Like this one:

yahoo/mojito#1257

Member

drewfish commented Oct 3, 2013

The encoding was originally put in place in response to bugzilla ticket 5590319 (marked as S1).

Comment 10 of that bug has a valid (in my mind) question which no one seemed to answer (in the bug log):
"Where does this config data come from, is it locally sourced form the machine, or does it come form the browser?"

Member

drewfish commented Oct 3, 2013

Encoding was added in commit yahoo/mojito@83a68da.

Collaborator

caridy commented Oct 3, 2013

Yeah, the reality was that cofnig was coming from a merge between the locally sourced from the machine + any custom config produced by controllers (which could include request data). That's not longer the case for configs, but it is still a valid point for any data pushed thru the data channels in mojito. Which means that this encoding is still a very valid concern. I will probably recommend to add encoding by default in express-state for any res.expose() call, unless the specific call is flagged somehow. While app.expose() is probably fine with the encoding disabled by default since it happens before requests arrive (in most cases).

Collaborator

ericf commented Oct 15, 2013

While app.expose() is probably fine with the encoding disabled by default since it happens before requests arrive (in most cases).

I won't differentiate between the app vs. res expose() calls because it all gets grouped together and it's easy to call req.app.expose() (not sure why you would, but very easy), also I'll do the encoding at serialization time which means both the app- and response- level data will get encoded.

Collaborator

ericf commented Oct 15, 2013

@caridy I wonder if the solution to fixing escaped unicode chars in URLs (yahoo/mojito#1257) is to use HTML entities like YUI's Y.Escape.html() function uses: http://yuilibrary.com/yui/docs/api/files/escape_js_escape.js.html#l10

ericf closed this in 8759765 Oct 16, 2013

Collaborator

caridy commented Oct 17, 2013

Alright, once we start using express-state in mojito, we can remove a lot of code :)

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