Page MenuHomePhabricator
Differential D248443

Bug 1960904 - Add IntegrityPolicy as a content-policy r=tschuster
AcceptedPublic
Actions

Authored by fkilic on Thu, May 8, 2:53 PM.

Details

Reviewers
tschuster
zombie
Group Reviewers
extension-reviewers
Bugzilla Bug ID
1960904

Diff Detail

Repository
rFIREFOXAUTOLAND firefox-autoland
Branch
HEAD
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
fkilic added inline comments.Tue, May 27, 2:45 PM
dom/security/IntegrityPolicy.cpp
375

changed it as you suggested, matching more with the spec

tschuster requested changes to this revision.Tue, May 27, 2:50 PM

Very close now, thanks!

dom/security/IntegrityPolicy.h
29

This should not be required.

dom/security/IntegrityPolicyService.cpp
62

Please change this whole method to the above like I said before.

154

We need to file a spec for bug this.

layout/build/components.conf
126

Do we need this?

This revision now requires changes to proceed.Tue, May 27, 2:50 PM
tschuster added inline comments.Tue, May 27, 3:08 PM
dom/security/IntegrityPolicy.h
29

Sorry, actually, I guess like the components.conf change this might be for the next patch?

Harbormaster completed remote builds in B796784: Diff 1047336.Tue, May 27, 3:14 PM
fkilic requested review of this revision.Tue, May 27, 3:25 PM
fkilic updated this revision to Diff 1047393.
fkilic marked 4 inline comments as done.
fkilic added inline comments.Tue, May 27, 3:26 PM
dom/security/IntegrityPolicy.h
29

yeah i think so. i just tried and I get component not registered error when i try to serialize/deserialize.

dom/security/IntegrityPolicyService.cpp
62

Ah I misunderstood the last time. Changed it fully now.

Harbormaster completed remote builds in B796820: Diff 1047393.Tue, May 27, 3:51 PM
fkilic updated this revision to Diff 1047467.Tue, May 27, 4:51 PM
Harbormaster completed remote builds in B796879: Diff 1047467.Tue, May 27, 5:13 PM
fkilic updated this revision to Diff 1047530.Tue, May 27, 6:27 PM
Harbormaster completed remote builds in B796937: Diff 1047530.Tue, May 27, 6:51 PM
tschuster accepted this revision.Wed, May 28, 1:45 PM
tschuster added inline comments.
dom/security/IntegrityPolicyService.cpp
169

I forgot about this, sorry, but even though we don't support the Reporting API, will should still log something in the console.

You probably want use nsContentUtils::ReportToConsoleByWindowID and have an error message for the blocking case, and a warning for the report-only case.

This revision is now accepted and ready to land.Wed, May 28, 1:45 PM
Herald added a project: needs-testing-tag. · View Herald TranscriptWed, May 28, 1:45 PM

This revision requires a Testing Policy Project Tag to be set before landing. Please apply one of testing-approved, testing-exception-unchanged, testing-exception-ui, testing-exception-elsewhere, testing-exception-other. Tip: this Firefox add-on makes it easy!

fkilic updated this revision to Diff 1048486.Wed, May 28, 4:49 PM
Herald added a subscriber: flod. · View Herald TranscriptWed, May 28, 4:49 PM
fkilic marked 4 inline comments as done.Wed, May 28, 4:51 PM
fkilic added inline comments.
dom/security/IntegrityPolicyService.cpp
169

added reporting (to console only for now)

Harbormaster completed remote builds in B797682: Diff 1048486.Wed, May 28, 5:09 PM
fkilic updated this revision to Diff 1051356.Mon, Jun 2, 7:57 PM
fkilic marked an inline comment as done.
fkilic requested review of this revision.Mon, Jun 2, 8:02 PM
fkilic added inline comments.
dom/security/IntegrityPolicyService.cpp
78–85

I added this part, so I'm re-requesting review.

Harbormaster completed remote builds in B799850: Diff 1051356.Mon, Jun 2, 8:23 PM
flod added inline comments.Tue, Jun 3, 5:29 AM
dom/locales/en-US/chrome/security/security.properties
172

Is this correct? Other strings use the format blocked ⇾ would block, also not sure about the colon.

tschuster requested changes to this revision.Tue, Jun 3, 8:24 AM
tschuster added inline comments.
dom/locales/en-US/chrome/security/security.properties
172

Thanks flod! Yes, we should make this error message more verbose and probably more similar to the existing CSP messages.

https://searchfox.org/mozilla-central/source/dom/locales/en-US/chrome/security/csp.properties#100-106

Maybe something like:

The page’s settings blocked a script at %S from being executed because it missing integrity metadata.

And

(Report-Only policy) The page’s settings would block a script ...

dom/security/IntegrityPolicyService.cpp
78–85

We would need a test for this. And I am wondering if we shouldn't use something like BasePrincipal::OverridesCSP here, which is used by LoadInfo::GetCsp. For now this probably ok.

80

aLoadInfo->TriggerPrincipal() as it can never be null.

This revision now requires changes to proceed.Tue, Jun 3, 8:24 AM
tschuster added inline comments.Tue, Jun 3, 9:08 AM
dom/security/IntegrityPolicyService.cpp
78–85

I recently wrote a test for something a bit similar in D246753.

fkilic requested review of this revision.Tue, Jun 3, 2:57 PM
fkilic updated this revision to Diff 1051887.
Herald added a reviewer: extension-reviewers. · View Herald TranscriptTue, Jun 3, 2:57 PM
fkilic marked an inline comment as done.Tue, Jun 3, 2:59 PM
fkilic added inline comments.
dom/locales/en-US/chrome/security/security.properties
172

updated it

dom/security/IntegrityPolicyService.cpp
78–85

added the test

80

changed it

Harbormaster completed remote builds in B800210: Diff 1051887.Tue, Jun 3, 3:22 PM
zombie accepted this revision.Tue, Jun 3, 4:39 PM
zombie added 1 blocking reviewer(s): tschuster.
zombie added a subscriber: zombie.
zombie added inline comments.
toolkit/components/extensions/test/xpcshell/test_ext_contentscript_integritypolicy.js
8 ↗(On Diff #1051887)

Is the expectation that the page itself couldn't load a script tag? If so, probably worth confirming that in the same test, as a sanity check.

36–37 ↗(On Diff #1051887)

I think you can use test.sendMessage directly from here.

56–58 ↗(On Diff #1051887)

you can just do = await extension.awaitMessage()

zombie added inline comments.Tue, Jun 3, 4:41 PM
toolkit/components/extensions/test/xpcshell/test_ext_contentscript_integritypolicy.js
56–58 ↗(On Diff #1051887)

result = await extension.awaitMessage("script-loaded")

fkilic updated this revision to Diff 1052067.Tue, Jun 3, 5:30 PM
fkilic marked an inline comment as done.
fkilic added inline comments.Tue, Jun 3, 5:31 PM
toolkit/components/extensions/test/xpcshell/test_ext_contentscript_integritypolicy.js
8 ↗(On Diff #1051887)

oh you are right added it

36–37 ↗(On Diff #1051887)

huh I couldn't make it work last time but it works now, wonder what I did last time. changed it, thanks!

56–58 ↗(On Diff #1051887)

ah I only did result = extension.awaitMessage("script-loaded") (without await) and thought it didn't work. changed it, thank you!

Harbormaster completed remote builds in B800332: Diff 1052067.Tue, Jun 3, 5:49 PM
tschuster added a comment.Wed, Jun 4, 8:13 AM

Thanks for adding the extension test.

dom/locales/en-US/chrome/security/security.properties
172

Sorry, I made a typo. it is missing

dom/security/IntegrityPolicyService.cpp
187

Let's just make this void. We can't really use the return value anyway.

toolkit/components/extensions/test/xpcshell/test_ext_contentscript_integritypolicy.js
29 ↗(On Diff #1052067)

Uber nit: Can you just call this content_script.js?

tschuster added inline comments.Wed, Jun 4, 8:45 AM
toolkit/components/extensions/test/xpcshell/test_ext_contentscript_integritypolicy.js
19 ↗(On Diff #1052067)

We probably want

registerCleanupFunction(() => {
  Services.prefs.clearUserPref("security.integrity_policy.enabled");
});
tschuster requested changes to this revision.Wed, Jun 4, 9:03 AM
tschuster added inline comments.
dom/security/IntegrityPolicyService.cpp
83

LoadingPrincipal does need a null check.

This revision now requires changes to proceed.Wed, Jun 4, 9:03 AM
fkilic requested review of this revision.Wed, Jun 4, 1:29 PM
fkilic updated this revision to Diff 1052683.
fkilic marked an inline comment as done.
tschuster accepted this revision.Wed, Jun 4, 1:34 PM
This revision is now accepted and ready to land.Wed, Jun 4, 1:34 PM
fkilic marked 11 inline comments as done.Wed, Jun 4, 1:37 PM

Thank you for reviewing!

dom/locales/en-US/chrome/security/security.properties
172

oh right, i should have noticed too, updated it

dom/security/IntegrityPolicyService.cpp
83

yep i noted it above

154

filed https://github.com/w3c/webappsec-subresource-integrity/issues/136

187

changed it

toolkit/components/extensions/test/xpcshell/test_ext_contentscript_integritypolicy.js
19 ↗(On Diff #1052067)

added it thank you

29 ↗(On Diff #1052067)

sure

fkilic marked 5 inline comments as done.Wed, Jun 4, 1:38 PM
fkilic added inline comments.
dom/security/IntegrityPolicyService.cpp
83

oh i guess this was an old comment, sorry.

Harbormaster completed remote builds in B800810: Diff 1052683.Wed, Jun 4, 1:51 PM
fkilic updated this revision to Diff 1052848.Wed, Jun 4, 4:02 PM
fkilic updated this revision to Diff 1052850.Wed, Jun 4, 4:04 PM
Harbormaster completed remote builds in B800949: Diff 1052850.Wed, Jun 4, 4:28 PM
Harbormaster completed remote builds in B800947: Diff 1052848.

Revision Contents
Changeset List

PathSize
dom/
base/
2 lines
locales/
en-US/
chrome/
security/
4 lines
security/
18 lines
12 lines
36 lines
218 lines
5 lines
2 lines
layout/
build/
13 lines

Diff 1051356

View Options

dom/base/Document.h

Loading...
View Options

dom/locales/en-US/chrome/security/security.properties

Loading...
View Options

dom/security/IntegrityPolicy.h

Loading...
View Options

dom/security/IntegrityPolicy.cpp

Loading...
View Options

dom/security/IntegrityPolicyService.h

Loading...
View Options

dom/security/IntegrityPolicyService.cpp

Loading...
View Options

dom/security/SRICheck.cpp

Loading...
View Options

dom/security/moz.build

Loading...
View Options

layout/build/components.conf

Loading...
Privacy · Cookies · Legal