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

Serializing configuration to client should do the proper encoding #11

Closed
imalberto opened this issue Sep 24, 2013 · 9 comments · Fixed by #13
Closed

Serializing configuration to client should do the proper encoding #11

imalberto opened this issue Sep 24, 2013 · 9 comments · Fixed by #13
Assignees
Labels
Milestone

Comments

@imalberto
Copy link

See Trello card 745 for more info

@caridy
Copy link
Contributor

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

@ghost ghost assigned ericf Sep 25, 2013
@ericf
Copy link
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.

@caridy
Copy link
Contributor

caridy commented Oct 3, 2013

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

YahooArchive/mojito#1257

@drewfish
Copy link

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?"

@drewfish
Copy link

drewfish commented Oct 3, 2013

Encoding was added in commit YahooArchive/mojito@83a68da453.

@caridy
Copy link
Contributor

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).

@ericf
Copy link
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.

@ericf
Copy link
Collaborator

ericf commented Oct 15, 2013

@caridy I wonder if the solution to fixing escaped unicode chars in URLs (YahooArchive/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

@caridy
Copy link
Contributor

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
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants