improve parameter-escaping #1627
Changes from all commits
c618519
b22a4f8
91b7e6e
5afa0b4
80d8383
4ddbd3f
b860170
62043b0
5f6e89f
e2fb1cf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,7 @@ BrowserID.Modules.Dialog = (function() { | |
errors = bid.Errors, | ||
dom = bid.DOM, | ||
win = window, | ||
startExternalDependencies = true, | ||
channel, | ||
sc; | ||
|
||
|
@@ -83,9 +84,12 @@ BrowserID.Modules.Dialog = (function() { | |
|
||
function fixupURL(origin, url) { | ||
var u; | ||
if (/^http/.test(url)) u = URLParse(url); | ||
if (typeof(url) !== "string") | ||
throw "urls must be strings: (" + url + ")"; | ||
if (/^http(s)?:\/\//.test(url)) u = URLParse(url); | ||
else if (/^\//.test(url)) u = URLParse(origin + url); | ||
else throw "relative urls not allowed: (" + url + ")"; | ||
// encodeURI limits our return value to [a-z0-9:/?%], excluding <script> | ||
return encodeURI(u.validate().normalize().toString()); | ||
} | ||
|
||
|
@@ -97,8 +101,23 @@ BrowserID.Modules.Dialog = (function() { | |
|
||
win = options.window || window; | ||
|
||
// startExternalDependencies is used in unit testing and can only be set | ||
// by the creator/starter of this module. If startExternalDependencies | ||
// is set to false, the channel, state machine, and actions controller | ||
// are not started. These dependencies can interfere with the ability to | ||
// unit test this module because they can throw exceptions and show error | ||
// messages. | ||
startExternalDependencies = true; | ||
if (typeof options.startExternalDependencies === "boolean") { | ||
startExternalDependencies = options.startExternalDependencies; | ||
} | ||
|
||
sc.start.call(self, options); | ||
startChannel.call(self); | ||
|
||
if (startExternalDependencies) { | ||
startChannel.call(self); | ||
} | ||
|
||
options.ready && _.defer(options.ready); | ||
}, | ||
|
||
|
@@ -111,32 +130,51 @@ BrowserID.Modules.Dialog = (function() { | |
return this.get(origin_url, {}, success, error); | ||
}, | ||
|
||
get: function(origin_url, params, success, error) { | ||
get: function(origin_url, paramsFromRP, success, error) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. great to make it clear that the second argument can contain anything. evil was more dramatic, your word choice here is more clear. |
||
var self=this, | ||
hash = win.location.hash; | ||
|
||
setOrigin(origin_url); | ||
|
||
var actions = startActions.call(self, success, error); | ||
startStateMachine.call(self, actions); | ||
|
||
params = params || {}; | ||
if (startExternalDependencies) { | ||
var actions = startActions.call(self, success, error); | ||
startStateMachine.call(self, actions); | ||
} | ||
|
||
// Security Note: paramsFromRP is the output of a JSON.parse on an | ||
// RP-controlled string. Most of these fields are expected to be simple | ||
// printable strings (hostnames, usernames, and URLs), but we cannot | ||
// rely upon the RP to do that. In particular we must guard against | ||
// these strings containing <script> tags. We will populate a new | ||
// object ("params") with suitably type-checked properties. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. fantastic prose. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This newly created whitelist of fields does not include requiredEmail - this must be added to the whitelist or else we will regress in requiredEmail support. Input checking was initially done in dialog/resources/state.js->"start" handler, the requiredEmail field was the first input we added. When the code was added for TOS/PP, the implementer put the TOS/PP checks here but the original requiredEmail check was never moved. To keep all these checks in one place, the requiredEmail check should be moved here as well. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whoops! I just pushed another patch that passes requiredEmail through (unchecked), and shuffles the try/catch section a little bit to anticipate some sort of validator being added to the requiredEmail assignment. One question though: why didn't my tests fail when I regressed requiredEmail? Does that imply that we're missing test coverage for that feature? (I have phantomjs installed, but not mysql, so it's possible that requiredEmail is covered by a test that I didn't run) In general, the "copy only what you expect to use" pattern might protect us from other surprises, especially since it looks like internal code adds properties to 'params' that didn't come from the RP, in which case the RP might sneakily supply them itself. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @warner - Test coverage is far from complete in several areas, it looks like dialog.js is one of those areas where tests are lacking. |
||
var params = {}; | ||
params.hostname = user.getHostname(); | ||
|
||
// verify params | ||
if (params.tosURL && params.privacyURL) { | ||
try { | ||
params.tosURL = fixupURL(origin_url, params.tosURL); | ||
params.privacyURL = fixupURL(origin_url, params.privacyURL); | ||
} catch(e) { | ||
return self.renderError("error", { | ||
action: { | ||
title: "error in " + origin_url, | ||
message: "improper usage of API: " + e | ||
} | ||
}); | ||
try { | ||
if (paramsFromRP.requiredEmail) { | ||
if (!bid.verifyEmail(paramsFromRP.requiredEmail)) | ||
throw "invalid requiredEmail: (" + paramsFromRP.requiredEmail + ")"; | ||
params.requiredEmail = paramsFromRP.requiredEmail; | ||
} | ||
if (paramsFromRP.tosURL && paramsFromRP.privacyURL) { | ||
params.tosURL = fixupURL(origin_url, paramsFromRP.tosURL); | ||
params.privacyURL = fixupURL(origin_url, paramsFromRP.privacyURL); | ||
} | ||
} catch(e) { | ||
// note: renderError accepts HTML and cheerfully injects it into a | ||
// frame with a powerful origin. So convert 'e' first. | ||
self.renderError("error", { | ||
action: { | ||
title: "error in " + _.escape(origin_url), | ||
message: "improper usage of API: " + _.escape(e) | ||
} | ||
}); | ||
|
||
return e; | ||
} | ||
// after this point, "params" can be relied upon to contain safe data | ||
|
||
// XXX Perhaps put this into the state machine. | ||
self.bind(win, "unload", onWindowUnload); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice explicit comment re embedded js.