Closed Bug 1225715 Opened 9 years ago Closed 8 years ago

Typecheck WebExtension manifests at startup

Categories

(WebExtensions :: Untriaged, defect)

defect
Not set
normal

Tracking

(firefox47 fixed)

RESOLVED FIXED
mozilla47
Iteration:
46.3 - Jan 25
Tracking Status
firefox47 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

Attachments

(6 files)

      No description provided.
Flags: blocking-webextensions?
Iteration: 45.2 - Nov 30 → 45.3 - Dec 14
Flags: blocking-webextensions? → blocking-webextensions+
Iteration: 45.3 - Dec 14 → 46.2 - Jan 11
Iteration: 46.2 - Jan 11 → 46.3 - Jan 25
This one's a bit weird. I was trying to avoid it for a while, but when we
start to support different sets of APIs on different apps, it's going make it
complicated to maintain a single, centralized manifest schema without some way
for them to directly extend it.

Review commit: https://reviewboard.mozilla.org/r/32043/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32043/
Attachment #8711262 - Flags: review?(wmccloskey)
This currently forbids unknown top-level schema properties, and unknown
permissions. In the future, I'd like to make those warnings rather than
errors, for compatibility purposes, but I think errors are fine for now.

Review commit: https://reviewboard.mozilla.org/r/32047/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32047/
Attachment #8711264 - Flags: review?(wmccloskey)
https://reviewboard.mozilla.org/r/32045/#review28791

::: toolkit/components/extensions/test/xpcshell/test_ext_schemas.js:1
(Diff revision 1)
> -"use strict";
> +https://reviewboard.mozilla.org/r/32037

Er, this was obviously a mistake. But since I can't push a fixed version to reviewboard without sending a mess of extra spam, I'll just say I fixed it locally.
Can you give me a summary of the goal here and why we want to do it?
Flags: needinfo?(kmaglione+bmo)
(In reply to Dave Townsend [:mossop] from comment #8)
> Can you give me a summary of the goal here and why we want to do it?

Right now, we don't do any type checking of manifests at startup outside of XPIProvider. This causes problems, because a lot of our code assumes the manifests are sane, so mistakes wind up causing strange, inscrutable errors.

The first few parts of this patch add type checking for the entire manifest structure. The last patch removes the type checking code in XPIProvider, because it's now redundant with the schema verification checks.
Flags: needinfo?(kmaglione+bmo)
Comment on attachment 8711259 [details]
MozReview Request: Bug 1225715: Part 1 - Add support for patterned strings and properties in schemas. r?billm

https://reviewboard.mozilla.org/r/32039/#review29291

The semantics of patternProperties seem a little weird to me. "properties" are requires unless you specify "optional". "patternProperties" are optional in the sense that you are allowed to omit any of them. But if you do specify them, then the "optional" attribute specifies whether null/undefined are accepted.

I feel like the actual usage of this is more like a limited form of "additionalProperties". Could we perhaps get rid of "patternProperties" and instead allow someone to specify an extra "propertyPattern" on "additionalProperties"? Then we would just check the name of each additionalProperty against the pattern and reject if no match is found.

Another nice thing about this is that you don't have to worry about overlapping patterns since there can be only one.

However, maybe you have other uses in mind for this where additionalProperties+pattern would be insufficient?

::: toolkit/components/extensions/Schemas.jsm:302
(Diff revision 1)
> -    for (let prop of Object.keys(this.properties)) {
> +    let checkProperty = (prop, propType) => {

Could we pass result as an extra argument? That way it's clearer that it's being modified.
Sorry, didn't mean to cancel review. I just wanted to see what you had to say about this.
Attachment #8711260 - Flags: review?(wmccloskey) → review+
Comment on attachment 8711260 [details]
MozReview Request: Bug 1225715: Part 2 - Add string format support to schemas. r?billm

https://reviewboard.mozilla.org/r/32041/#review29305

::: toolkit/components/extensions/Schemas.jsm:79
(Diff revision 1)
> +const FORMATS = {

Please comment that these methods either return a normalized value or else throw an exception.

Also, I feel like it might be cleaner to implement these checks outside of Schemas.jsm, but I'm not sure.

::: toolkit/components/extensions/Schemas.jsm:130
(Diff revision 1)
>    normalize(value) {

Maybe add "context" here to illustrate the possibility for it and to document it.
(In reply to Bill McCloskey (:billm) from comment #10)
> Comment on attachment 8711259 [details]
> MozReview Request: Bug 1225715: Part 1 - Add support for patterned strings
> and properties in schemas. r?billm
> 
> https://reviewboard.mozilla.org/r/32039/#review29291
> 
> 
> I feel like the actual usage of this is more like a limited form of
> "additionalProperties". Could we perhaps get rid of "patternProperties" and
> instead allow someone to specify an extra "propertyPattern" on
> "additionalProperties"? Then we would just check the name of each
> additionalProperty against the pattern and reject if no match is found.

That's actually pretty close to my original plan. I checked the JSON Schema
docs before I got very far implementing it, though, and found that
patternProperties is part of the spec:
http://json-schema.org/latest/json-schema-validation.html#anchor64

I'm not really opposed to doing something different from the spec here, but
I'd generally rather stay close to it unless we have a good reason not to.

> The semantics of patternProperties seem a little weird to me. "properties"
> are requires unless you specify "optional". "patternProperties" are optional
> in the sense that you are allowed to omit any of them. But if you do specify
> them, then the "optional" attribute specifies whether null/undefined are
> accepted.

Yeah, I agree the semantics of the "optional" are weird. I started out with
"optional" forced to false for patternProperties. Eventually I realized that
every other "optional" property or argument accepts null, and winds up defined
as null if it was omitted, so I decided to allow that for patternProperties
too. I don't really have a good use case for it, though, so I'm happy to
remove it.

> ::: toolkit/components/extensions/Schemas.jsm:302
> (Diff revision 1)
> > -    for (let prop of Object.keys(this.properties)) {
> > +    let checkProperty = (prop, propType) => {
> 
> Could we pass result as an extra argument? That way it's clearer that it's
> being modified.

Yeah, that sounds like a good idea.
(In reply to Bill McCloskey (:billm) from comment #12)
> Also, I feel like it might be cleaner to implement these checks outside of
> Schemas.jsm, but I'm not sure.

Yeah, I'm not sure either. I was originally planning to do it as some sort of
callback, until I read through the spec and found that "format" was part of
it. I think at least the syntax checking part belongs here. I'm less sure
about the checkLoadURI part. It just feels a bit weird to do it in a different
way than the format checks.

> ::: toolkit/components/extensions/Schemas.jsm:130
> (Diff revision 1)
> >    normalize(value) {
> 
> Maybe add "context" here to illustrate the possibility for it and to
> document it.

I wound up doing that in the next patch.
I finally actually *read* the JSON schema spec and I was a bit surprised :-). It looks like Schemas.jsm, as well as Chromium's validator, differ quite a bit from the draft spec. As far as I can tell, Chrome's implementation sorta matches draft-02 of the spec, while draft-04 is the latest version. The biggest difference I see is that draft-04 doesn't have an "optional" attribute for object properties. Instead, you provide a "required" list of properties at the object level. If we wanted to implement this, we'd have to change a lot of our schemas.

I also wasn't aware that additionalProperties could be a boolean. We don't support that, but it looks like part 5 in your patch series uses it. Did you add support somewhere else in the patch series?

Anyway, it's hard to decide what to do here. In draft-02, properties are required by default. In draft-04, they're optional by default. That's why patternProperties thing seemed weird to me.

I think we've probably gotten all the mileage we ever will from using the same format as Chromium. A more important consideration is that we'll probably be using JSON schemas for native.js, and people will have a lot easier time with them if we match the latest spec. So we should probably start moving in that direction.

So let's implement patternProperties as it's specified. I'll file a bug on bringing our schemas up to spec.
https://reviewboard.mozilla.org/r/32039/#review29437

Sorry for the pedantry here. I guess I mostly just want to understand how this is supposed to work.

::: toolkit/components/extensions/Schemas.jsm:325
(Diff revision 1)
> +    for (let prop of Object.keys(this.properties)) {
> +      let error = checkProperty(prop, this.properties[prop]);
> +      if (error) {
> +        return error;
> +      }
>      }
>  
> +    outer:
>      for (let prop of Object.keys(properties)) {
> -      if (!(prop in this.properties)) {
> +      if (prop in this.properties) {
> +        continue;
> +      }
> +
> +      for (let {pattern, type} of this.patternProperties) {
> +        if (pattern.test(prop)) {
> +          let error = checkProperty(prop, type);
> +          if (error) {
> +            return error;
> +          }
> +
> +          continue outer;
> +        }
> +      }

This doesn't appear to match my reading of the spec:
http://json-schema.org/latest/json-schema-validation.html#anchor134
http://json-schema.org/latest/json-schema-validation.html#anchor64

I think the basic pseudocode should look like this:

validated = {}
for each prop in type.properties:
  result[prop] = validate obj[prop] against type.properties[prop]
  validated[prop] = true
  
for each pattern in type.patternProperties:
  for each prop in obj:
    if prop matches pattern:
      result[prop] = validate obj[prop] against type.patternProperties[pattern]
      validated[prop] = true

for each prop in obj:
  if !validated[prop]:
    check against additionalProperties
    throw error if additionalProperties not present or false
    
This is still not really spec-compliant since it makes the default value of additionalProperties be false. The spec says it should default to a schema that allows any JSON. However, to make that change, we would have to add "additionalProperties":false to every object-type schema we have, which is a lot of work. So let's assume it's false for now. And please comment that we'll need to fix this for bug 1243516.
(In reply to Bill McCloskey (:billm) from comment #15)
> I finally actually *read* the JSON schema spec and I was a bit surprised
> :-). It looks like Schemas.jsm, as well as Chromium's validator, differ
> quite a bit from the draft spec. As far as I can tell, Chrome's
> implementation sorta matches draft-02 of the spec, while draft-04 is the
> latest version. The biggest difference I see is that draft-04 doesn't have
> an "optional" attribute for object properties. Instead, you provide a
> "required" list of properties at the object level. If we wanted to implement
> this, we'd have to change a lot of our schemas.

Hm. Yeah. I didn't realize they were using an older spec. The validator
apparently also put together a spec for this, and I noticed that they used
"required" rather than "optional", which makes sense now.

> I also wasn't aware that additionalProperties could be a boolean. We don't
> support that, but it looks like part 5 in your patch series uses it. Did you
> add support somewhere else in the patch series?

Yeah, I added it to one of my schema files without thinking. I think I added
support in the same patch when my tests failed.

> Anyway, it's hard to decide what to do here. In draft-02, properties are
> required by default. In draft-04, they're optional by default. That's why
> patternProperties thing seemed weird to me.
>
> I think we've probably gotten all the mileage we ever will from using the
> same format as Chromium. A more important consideration is that we'll
> probably be using JSON schemas for native.js, and people will have a lot
> easier time with them if we match the latest spec. So we should probably
> start moving in that direction.
>
> So let's implement patternProperties as it's specified. I'll file a bug on
> bringing our schemas up to spec.

That sounds good to me.
(In reply to Bill McCloskey (:billm) from comment #16)
> https://reviewboard.mozilla.org/r/32039/#review29437
> 
> Sorry for the pedantry here. I guess I mostly just want to understand how
> this is supposed to work.

No complaints here.

> This doesn't appear to match my reading of the spec:
> http://json-schema.org/latest/json-schema-validation.html#anchor134
> http://json-schema.org/latest/json-schema-validation.html#anchor64

Hm... This spec is a bit hard to parse, so I'm honestly not sure. But I think
my code matches the behavior of your pseudocode if we assume that |validate x
against y| is supposed to throw if the property doesn't validate (which I
think it is).

> This is still not really spec-compliant since it makes the default value of
> additionalProperties be false. The spec says it should default to a schema
> that allows any JSON. However, to make that change, we would have to add
> "additionalProperties":false to every object-type schema we have, which is a
> lot of work. So let's assume it's false for now. And please comment that
> we'll need to fix this for bug 1243516.

Yeah, that makes sense. It sounds like a good bug for one of our new hires. :)
(In reply to Kris Maglione [:kmag] from comment #18)
> > This doesn't appear to match my reading of the spec:
> > http://json-schema.org/latest/json-schema-validation.html#anchor134
> > http://json-schema.org/latest/json-schema-validation.html#anchor64
> 
> Hm... This spec is a bit hard to parse, so I'm honestly not sure. But I think
> my code matches the behavior of your pseudocode if we assume that |validate x
> against y| is supposed to throw if the property doesn't validate (which I
> think it is).

Here's an example that I think shows the difference:

Schema:
{ "type": "object",
  "properties": { "foo": X },
  "patternProperties": { "foo": Y, "fo": Z }
}

If we try to validate the object {"foo": v} against this schema, the draft-04 spec wants us to validate v against X, Y, and Z. Your code only validates it against X and ignores Y, Z.

I tried this at http://jsonschemalint.com/draft4/, and it seems to match my reading of the spec.

This does pose some issues with our normalization code. If X, Y, and Z all normalize in different ways, which one do we pick? I don't particularly care myself.

As I said, this is all pretty pedantic. But I guess I could see someone using a fairly generic check for most properties via patternProperties and then something more specific in "properties". I dunno.
Hm. Yeah, good point.

I guess as far as normalization goes, the second normalize function should get the result of the first as input.

But I think we should probably just ignore this until we have a use case for it.
(In reply to Kris Maglione [:kmag] from comment #20)
> But I think we should probably just ignore this until we have a use case for
> it.

I don't think it would be much work to implement the spec, so I'd rather do that. It's fine to do something arbitrary for normalization though.
Attachment #8711262 - Flags: review?(wmccloskey) → review+
Comment on attachment 8711262 [details]
MozReview Request: Bug 1225715: Part 3 - Allow extending existing schema types. r?billm

https://reviewboard.mozilla.org/r/32043/#review29497

Thanks, this seems great.
Comment on attachment 8711265 [details]
MozReview Request: Bug 1225715: Part 6 - Remove redundant manifest type checking from XPIProvider. r?Mossop

https://reviewboard.mozilla.org/r/32049/#review29503

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:869
(Diff revision 1)
> -    throw new Error("Expected manifest_version to be 2 but was " + mVersion);
> +    throw new Error("Extension is invalid");

This should be above the locale init. In fact I'm a little surprised that readManifest doesn't just throw when there are errors.

I assume the actual errors have been logged previously?
Attachment #8711265 - Flags: review?(dtownsend) → review+
(In reply to Bill McCloskey (:billm) from comment #21)
> I don't think it would be much work to implement the spec, so I'd rather do
> that. It's fine to do something arbitrary for normalization though.

Fair enough.
(In reply to Dave Townsend [:mossop] from comment #23)
> Comment on attachment 8711265 [details]
> MozReview Request: Bug 1225715: Part 6 - Remove redundant manifest type
> checking from XPIProvider. r?Mossop
> 
> https://reviewboard.mozilla.org/r/32049/#review29503
> 
> ::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:869
> (Diff revision 1)
> > -    throw new Error("Expected manifest_version to be 2 but was " + mVersion);
> > +    throw new Error("Extension is invalid");
> 
> This should be above the locale init. In fact I'm a little surprised that
> readManifest doesn't just throw when there are errors.
> 
> I assume the actual errors have been logged previously?

The locale validation checks don't rely on the manifest being valid, so both load operations continue as far as they can before bailing out so the user gets to see all of the errors at once.

The errors are reported as they're encountered, yes.
Attachment #8711263 - Flags: review?(wmccloskey) → review+
Comment on attachment 8711263 [details]
MozReview Request: Bug 1225715: Part 4 - Improve reporting of schema errors. r?billm

https://reviewboard.mozilla.org/r/32045/#review29507

::: toolkit/components/extensions/Schemas.jsm:96
(Diff revision 1)
> +  get cloneScope() {
> +    return this.params.cloneScope;
> +  }

What's this for?

::: toolkit/components/extensions/Schemas.jsm:550
(Diff revision 1)
> -      element = this.itemType.normalize(element, context);
> +      element = context.withPath(i, () => this.itemType.normalize(element, context));

I know this works, but I'd feel better if you convert i to a string before passing it in.

::: toolkit/components/extensions/Schemas.jsm:687
(Diff revision 1)
>          let r = parameter.type.normalize(arg, context);

It might be nice to add the parameter name to the path here.
Attachment #8711264 - Flags: review?(wmccloskey) → review+
Comment on attachment 8711264 [details]
MozReview Request: Bug 1225715: Part 5 - Add schema for extension manifests. r?billm

https://reviewboard.mozilla.org/r/32047/#review29517

::: toolkit/components/extensions/Extension.jsm:113
(Diff revision 1)
> +    // Load order maders here. The base manifest defines types which are

s/maders/matters/

::: toolkit/components/extensions/Extension.jsm:1041
(Diff revision 1)
> +        // b2g add-ons generate manifest errors that we've silently
> +        // ignoring prior to adding this check.

silently been ignoring?

What errors are these?

::: toolkit/components/extensions/schemas/manifest.json:3
(Diff revision 1)
> +    "namespace": "manifest",

We need to make sure that nothing from this namespace gets injected into an extension. Can you write a test that ensures that browser.manifest is undefined? This could maybe fit in with a test for bug 1243602, which I just noticed.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_icons.js
(Diff revision 1)
> -    icons: {
> -      32: "icon32.png",
> -      banana: "bananana.png",
> -      "20.5": "icon20.5.png",
> -      "20.0": "also invalid",
> -      "123banana": "123banana.png",
> -      64: "icon64.png"

I didn't really follow the icon sizes bugs. How is our behavior changing here?
(In reply to Bill McCloskey (:billm) from comment #26)
> ::: toolkit/components/extensions/Schemas.jsm:96
> (Diff revision 1)
> > +  get cloneScope() {
> > +    return this.params.cloneScope;
> > +  }
> 
> What's this for?

Hm. I was just thinking out loud. I thought it might be useful for the places we throw errors or check types from the content scope, but I didn't wind up using it. I meant to remove it.

> ::: toolkit/components/extensions/Schemas.jsm:687
> (Diff revision 1)
> >          let r = parameter.type.normalize(arg, context);
> 
> It might be nice to add the parameter name to the path here.

I thought about it, but we already include the parameter name in the errors we throw from the arg checks, with a much more helpful message.
(In reply to Bill McCloskey (:billm) from comment #27)
> ::: toolkit/components/extensions/Extension.jsm:1041
> (Diff revision 1)
> > +        // b2g add-ons generate manifest errors that we've silently
> > +        // ignoring prior to adding this check.
> 
> silently been ignoring?
> 
> What errors are these?

Their tests don't include an "application" object, for one thing, so we've
been printing the errors, but their extension loader doesn't bail out. I'm not
sure if their add-ons are meant to include an ID or not, or if their manifests
are valid by our standards anywhere else.

At this point, I'm just trying to be nice and not break them entirely. I'm not
supposed to be spending time fixing things for b2g.

> ::: toolkit/components/extensions/schemas/manifest.json:3
> (Diff revision 1)
> > +    "namespace": "manifest",
> 
> We need to make sure that nothing from this namespace gets injected into an
> extension. Can you write a test that ensures that browser.manifest is
> undefined? This could maybe fit in with a test for bug 1243602, which I just
> noticed.

I think it does get created, at the moment, but I don't think it causes any
problems. I honestly think the better solution is to move all of these types
into the extensionTypes namespace, but I was trying to avoid polluting it for
the time being.

> ::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_icons.js
> (Diff revision 1)
> > -    icons: {
> > -      32: "icon32.png",
> > -      banana: "bananana.png",
> > -      "20.5": "icon20.5.png",
> > -      "20.0": "also invalid",
> > -      "123banana": "123banana.png",
> > -      64: "icon64.png"
> 
> I didn't really follow the icon sizes bugs. How is our behavior changing
> here?

The only difference is that we now throw for invalid properties rather than
ignoring them. I think that should be the preferred behavior anyway. I removed
these tests rather than changing them because they're covered by the
webextension tests now.
https://hg.mozilla.org/integration/fx-team/rev/01030087329d5f22a9ab0b4b36349e1e5e1403c1
Bug 1225715: Part 1 - Add support for patterned strings and properties in schemas. r=billm

https://hg.mozilla.org/integration/fx-team/rev/3d4c135d40fbb699e92ba7a009edf4396b2c4cd4
Bug 1225715: Part 2 - Add string format support to schemas. r=billm

https://hg.mozilla.org/integration/fx-team/rev/35b68cd78980aa1a5fb10cb0fc36520b2a032a4a
Bug 1225715: Part 3 - Allow extending existing schema types. r=billm

https://hg.mozilla.org/integration/fx-team/rev/33dc36ce5f03969f5e7296c942cef0956880616d
Bug 1225715: Part 4 - Improve reporting of schema errors. r=billm

https://hg.mozilla.org/integration/fx-team/rev/614127a442fa0adffabaded612cb2820eb250821
Bug 1225715: Part 5 - Add schema for extension manifests. r=billm

https://hg.mozilla.org/integration/fx-team/rev/43d3300fa0a9163592bdadb421e2fabfeb92880b
Bug 1225715: Part 6 - Remove redundant manifest type checking from XPIProvider. r=Mossop
It just occurred to me that we need a way to avoid rejecting Chrome extensions that have properties we don't recognize in their manifest.
Yeah, I was going to file a follow-up to make extra properties a warning rather than an error. I want to see how things go with it as a hard error first, though, since it immediately caught some bugs in a couple of tests.
Blocks: 1244474
Also, part 1 landed without an r+ :-(. It looks fine, but please be more careful next time.
(In reply to Bill McCloskey (:billm) from comment #33)
> Also, part 1 landed without an r+ :-(. It looks fine, but please be more
> careful next time.

Ah, sorry, I thought I'd checked all of them
Blocks: 1244805
Depends on: 1245671
https://hg.mozilla.org/integration/fx-team/rev/281de9bf9ff6f22882f614d8ea443dfed007e9c4
Bug 1225715: Follow-up: Ignore unknown permissions and top-level manifest keys. r=me
Depends on: 1263560
Depends on: 1272323
Depends on: 1279130
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: