Skip to content

Bug 791681 - Insecure transition from HTTP to HTTPS in form post#928

Merged
jpetto merged 1 commit intomozilla:masterfrom
kyoshino:bug-791681-form-https
Jul 31, 2013
Merged

Bug 791681 - Insecure transition from HTTP to HTTPS in form post#928
jpetto merged 1 commit intomozilla:masterfrom
kyoshino:bug-791681-form-https

Conversation

@kyoshino
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There should be a check for localhost/debug servers somewhere in here. Local installations without SSL need to be able to test form submissions.

@openjck
Copy link
Copy Markdown
Contributor

openjck commented Jun 13, 2013

Notes from our meeting: It might be best to squash these commits into one. The HTML changes look safe, but we might want to pay special attention to the misc.py changes.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PEP-8: "Don't use spaces around the = sign when used to indicate a keyword argument or a default parameter value."

@kyoshino
Copy link
Copy Markdown
Contributor Author

Rebased and removed spaces around the = sign for a default parameter as per @dpoirier's comment.

@pmclanahan
Copy link
Copy Markdown
Contributor

This is looking good, but the new secure_url() function needs some tests.

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Jul 18, 2013

Just a note to self (and @alexgibson for good measure) - will need to fix AJAX call here after merge:

https://github.com/mozilla/bedrock/blob/master/media/js/firefox/os/desktop.js#L214

@kyoshino
Copy link
Copy Markdown
Contributor Author

Will try to write tests in test_helper_misc.py and fix the Ajax code.

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Jul 19, 2013

The fix for the AJAX call should be very simple. I believe line 214 (https://github.com/mozilla/bedrock/blob/master/media/js/firefox/os/desktop.js#L214) will need to look like:

url: $form.attr('action'),

@kyoshino
Copy link
Copy Markdown
Contributor Author

Yeah, $form.attr('action') will be a full URL by using the secure_url() function.

@kyoshino
Copy link
Copy Markdown
Contributor Author

Added tests and fixed the Ajax call.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To more closely mimic a production environment, I think we should have this return ctx['request'].build_absolute_uri(path). This will give us a fully qualified URL locally instead of a possible empty string.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix it later.

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Jul 25, 2013

Aside from my previous comment, this looks like an r+. After that (hopefully) final change, need to test on a demo server before merging.

@kyoshino
Copy link
Copy Markdown
Contributor Author

Fixed the secure_url function (and tests) to output a full URL on a dev site also.

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Jul 29, 2013

This is up on demo1, and all seems to be well. Tested most all of the pages affected and am seeing no ill effects.

@pmac and/or @sgarrity - care to give a demo1 a run through?

@jpetto
Copy link
Copy Markdown
Contributor

jpetto commented Jul 31, 2013

r+!

jpetto added a commit that referenced this pull request Jul 31, 2013
Bug 791681 - Insecure transition from HTTP to HTTPS in form post
@jpetto jpetto merged commit 8223d36 into mozilla:master Jul 31, 2013
@kyoshino kyoshino deleted the bug-791681-form-https branch August 11, 2013 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants