Closed
Bug 1251043
Opened 8 years ago
Closed 8 years ago
Content Security Policy form-action directive is ignored
Categories
(Core :: DOM: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: ilya.nesterov, Assigned: ckerschb)
References
(Blocks 1 open bug)
Details
(Keywords: regression, Whiteboard: [domsecurity-active])
Attachments
(4 files)
482 bytes,
text/html
|
Details | |
1.30 KB,
patch
|
francois
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
5.51 KB,
patch
|
francois
:
review+
ritu
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
5.47 KB,
patch
|
ckerschb
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/48.0.2564.109 Safari/537.36 Steps to reproduce: Load attached "form-action-src-blocked.html" in a browser and click "Submit" button. You can also reproduce the issue if Content Security Policy delivered via content-security-policy http header. (You need to remove "<meta http-equiv="Content-Security-Policy" content="form-action 'none'"> from attached file, and adjust your http server settings to add "Content-Security-Policy: form-action 'none'" header to response) Actual results: Form successfully submitted Expected results: Form submit should be blocked. (Open the same file in Chrome)
Reporter | ||
Comment 1•8 years ago
|
||
Firefox 44.0.2 on OS X 10.11.3
Reporter | ||
Updated•8 years ago
|
OS: Unspecified → All
Hardware: Unspecified → All
Assignee | ||
Comment 2•8 years ago
|
||
We need to investigate. I see a CSP error in the console but it seems the page still loads.
Assignee | ||
Comment 3•8 years ago
|
||
Kate, any chance I can assign this one to you as well?
Flags: needinfo?(kmckinley)
Whiteboard: [domsecurity-backlog]
Updated•8 years ago
|
Assignee: nobody → kmckinley
Flags: needinfo?(kmckinley)
Assignee | ||
Updated•8 years ago
|
Whiteboard: [domsecurity-backlog] → [domsecurity-active]
Assignee | ||
Comment 4•8 years ago
|
||
I just stumbled across that piece of code which would explain the current behavior. Francois, are we missing a return statement here: http://hg.mozilla.org/mozilla-central/rev/5a9dc311e76f#l2.40 I suppose we should only perform > actionURL.forget(aActionURL); if allowed by the CSP, right? Most likely this will fix the failing wpt form-action tests as well, right? See: https://bugzilla.mozilla.org/show_bug.cgi?id=937025#c18
Flags: needinfo?(francois)
Assignee | ||
Comment 5•8 years ago
|
||
Ah I suppose upgrade-insecure-requests broke that: http://hg.mozilla.org/mozilla-central/diff/6081d8a4054e/dom/html/HTMLFormElement.cpp#l1.49 That's why we should use early returns - rrgh!
Comment 7•8 years ago
|
||
(In reply to Christoph Kerschbaumer [:ckerschb] from comment #4) > I just stumbled across that piece of code which would explain the current > behavior. Francois, are we missing a return statement here: > http://hg.mozilla.org/mozilla-central/rev/5a9dc311e76f#l2.40 > > I suppose we should only perform > > actionURL.forget(aActionURL); > if allowed by the CSP, right? Yeah, it does look like we should have an early return in there.
Flags: needinfo?(francois)
Assignee | ||
Comment 8•8 years ago
|
||
So, after evaluating this bug I realized this is in fact a regression of Bug 1139297, we should fix and uplift.
Assignee | ||
Comment 9•8 years ago
|
||
Attachment #8734078 -
Flags: review?(francois)
Assignee | ||
Comment 10•8 years ago
|
||
It's actually not that easy to write an automated test for this feature, because there is no error thrown if we block the form submission. If CSP blocks the load then we just log an error to the console. We test that we correctly log that error to the console within |test_form-action.html|. Within this patch I am adding a test to make sure the form is actually not submitted.
Attachment #8734080 -
Flags: review?(francois)
Updated•8 years ago
|
Attachment #8734078 -
Flags: review?(francois) → review+
Comment 11•8 years ago
|
||
Comment on attachment 8734080 [details] [diff] [review] bug_1251043_fix_csp_form_action_test_updates.patch Review of attachment 8734080 [details] [diff] [review]: ----------------------------------------------------------------- I wonder whether we should be throwing an error when the form submission fails for CSP reasons. It would make the test simpler, but more importantly, it would give some feedback to webdevs that something went wrong in case they rely on that form submission to work.
Attachment #8734080 -
Flags: review?(francois) → review+
Assignee | ||
Comment 12•8 years ago
|
||
(In reply to François Marier [:francois] from comment #11) > It would make the test simpler, but more importantly, it would give some > feedback to webdevs that something went wrong in case they rely on that form > submission to work. I agree, I'll consult people what they think about that. But let's not pollute this bug (since we need to uplift the patches within this bug). Thanks!
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbc5d3de958a https://hg.mozilla.org/integration/mozilla-inbound/rev/d8705d309f1c
Keywords: checkin-needed
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fbc5d3de958a https://hg.mozilla.org/mozilla-central/rev/d8705d309f1c
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8734078 [details] [diff] [review] bug_1251043_fix_csp_form_action.patch Approval Request Comment Regression introduced within FF42, but I suppose the form-action CSP directive is not widely used. I suggest we uplift to aurora and beta anyway. It's only a one line fix. [Feature/regressing bug #]: Bug 1139297 - Implement CSP upgrade-insecure-requests directive [User impact if declined]: forms can be submitted even though CSP might restrict submitting forms. Please note that a CSP error message would still be logged to the console, which might confuse people. [Describe test coverage new/current, TreeHerder]: Manual testing and automated mochitest attached to this bug. [Risks and why]: very low, it's only affecting sites that submit forms using the CSP directive 'form-action'. [String/UUID change made/needed]: no
Attachment #8734078 -
Flags: approval-mozilla-beta?
Attachment #8734078 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 16•8 years ago
|
||
Comment on attachment 8734080 [details] [diff] [review] bug_1251043_fix_csp_form_action_test_updates.patch Approval Request Comment See above comment.
Attachment #8734080 -
Flags: approval-mozilla-beta?
Attachment #8734080 -
Flags: approval-mozilla-aurora?
status-firefox46:
--- → affected
status-firefox47:
--- → affected
Comment on attachment 8734078 [details] [diff] [review] bug_1251043_fix_csp_form_action.patch Seems low risk, Aurora47+
Attachment #8734078 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8734080 [details] [diff] [review] bug_1251043_fix_csp_form_action_test_updates.patch Test patches don't need explicit approval, they are auto-approved.
Attachment #8734080 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
The test patch doesn't apply cleanly on aurora, can you rebase that patch?
Flags: needinfo?(mozilla)
Assignee | ||
Comment 20•8 years ago
|
||
Rebased tests for uplifting to aurora.
Flags: needinfo?(mozilla)
Attachment #8735696 -
Flags: review+
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/52315caa0dae https://hg.mozilla.org/releases/mozilla-aurora/rev/45b21b36621f
If this were to be uplifted to beta, the rebased test patch for aurora does not apply cleanly to beta, so would need a newly rebased patch.
Comment 23•8 years ago
|
||
Christoph is out for a couple of weeks. Steve can you help us find someone to rebase the patch for beta?
Flags: needinfo?(sworkman)
Comment 24•8 years ago
|
||
Comment on attachment 8734078 [details] [diff] [review] bug_1251043_fix_csp_form_action.patch Too late for beta, not a recent regression. This will be fixed in 47.
Attachment #8734078 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 25•8 years ago
|
||
Comment on attachment 8734080 [details] [diff] [review] bug_1251043_fix_csp_form_action_test_updates.patch Merge issues unresolved, too late for beta 46. This will ship with 47
Attachment #8734080 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 26•8 years ago
|
||
Hi Liz - many apologies! I was partially out last week, and missed this. Christoph should be back this week, but it sounds like this has missed beta46 anyway. Sorry about that!
Flags: needinfo?(sworkman)
Updated•8 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•