improve parameter-escaping #1627
Conversation
Specifically to get _.escape()
ghost
assigned 
| @@ -86,6 +86,7 @@ BrowserID.Modules.Dialog = (function() { | |||
| if (/^http/.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> | |||
There was a problem hiding this comment.
nice explicit comment re embedded js.
|
r- due to two small but important issues:
once these two things are done, let's get this merged into dev and other branches as well. |
|
good eye! Fixed the global, also pushed this branch to mozilla/browserid in the "issue-1627" branch, I think that'll trigger a travis-ci build. |
|
@warner, @lloyd - I like the approach to this - I did not know underscore had the escape function. Please see comment https://github.com/mozilla/browserid/pull/1627/files#r858967. requiredEmail support needs to be added back in before this is merged. |
|
nice catch @shane-tomlinson! @bwarner - looks like the final addition will be accepting requiredEmail if it looks like a valid email address. @shane-tomlinson - where's a good email validation routine? lloyd |
Since validation is done earlier, invalid addresses are reported as with
renderError() instead of startAction("doError"). So the tests which
watch for doError are now failing (the frontend tests named
"resources/state: start with invalid requiredEmail - print error screen"
and "resources/state: start with empty requiredEmail - prints error
screen". I don't know how to make them watch for renderError() instead.
One upside of this patch is that @stomlinson told me the three-argument
to startAction("doError"..) in state.js was a bug, as it only accepts
two real arguments. Removing it is even easier than fixing it.
|
That patch moves the requiredEmail handling.. @shane-tomlinson, is that what you were thinking of? I don't know how to get the tests to start passing again, but maybe you've got a quick fix for it. Or we could leave requiredEmail where it was, I'm cool with that too. |
…dialog.js. Add more tests. * Add the startExternalDependencies option when initializing the dialog controller, if set to false, no external dependencies are loaded. Used for testing. * Add a new test to check for script containing emails. * Add unit tests for tosURL and privacyURL. * Add a testHelpers.testErrorNotVisible * Removed the actions.js->doError and its tests, it is no longer used.
Move the requiredEmail checks from resources/state.js to controllers/dialog.js.
…cyURL parameters are passed to "start" when expected.
Add additional tests to check whether requiredEmail, tosURL and privacyURL parameters are passed to "start" when expected.
|
@lloyd : how's this look now? (BTW I just pushed a copy to the 'issue-1627' branch, so we should get travis-ci coverage in a few minutes). I verified that the latest added test correctly fails when I comment out the |
|
r+ |
@lloyd: this does the trunk-side cleanup we discussed today, and ought to resolve friday's issue too (but needs specific testing for that). Once this lands we can modify #1569 to use the same style.