591515 - Jetpack's module loader should restrict module-loading based on privilege-reducing manifest metadata

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Atul Varma [:atul]]

In 0.8, we're going to log warnings when one module requires another that isn't expected; once we iron out the edge-cases and other user concerns, we'll change the warning to a thrown exception.

The privilege-reducing manifest metadata added to harness-options.json by bug 572020 should be used to determine the dependency graph.

Also, we need to implement a way for this metadata to be ignored for special cases, such as the test runner, which looks for "test-*" files and requires them dynamically.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Also see bug 591525.

Comment 2

[Brian Warner [:warner :bwarner]]

Attachment: emit warning when require(X) is called but X is not on the manifest

Here's a preliminary patch, thanks to Atul's awesome helper functions. It passes manual testing, but:

 1: the WARNING it emits (via dump()) is not very noticeable.. is there a better way to announce this sort of problem?
 2: no tests yet, although the existing tests mostly still work
 3: existing tests provoke a lot of WARNINGs as they require() modules that aren't in the manifest, because the manifest scanner doesn't look at test cases. We either need to change the manifest scanner to find them, or suppress these checks when A: unit tests are being run and B: the require() is coming from a unit-test module. We must make sure that regular modules cannot declare themselves to be unit-test modules to escape the manifest checks.

Comment 3

[Brian Warner [:warner :bwarner]]

Attachment: emit warning when require(X) is called but X is not on the manifest

Comment 4

[Brian Warner [:warner :bwarner]]

oops, hit the button before adding comments :).

So that comment#3 patch adds a unit test, which works by attaching some counters to the globals (made available to all modules) that the new manifestChecker increments each time it decides to reject a require(). The unit test then reaches into the Packaging object (which is also available to all modules, for now) to tweak the manifest contents, to allow it to require() certain modules, and checks that the counters are incremented or not.

I feel we should stop letting all modules access Packaging, since if this unit test can manipulate the manifest, other modules could too, letting them bypass the limitations that the manifest was supposed to be enforcing. Atul had a great idea, to make the Loader (and, by extension, Packaging) be part of the bundle you get when you do require("chrome"), or at least to make access to the Loader/Packaging be mediated by a require() statement (which could then be restricted by the manifest).

Fundamentally, unit test modules need to be given slightly more access than normal modules, probably by having the packages/test-harness/lib/harness.js provide different options to the Loader it creates than the usual python-lib/cuddlefish/app-extension/components/harness.js does.

With this patch, all unit tests still emit tons of "undeclared require()" warnings, either when the main "unit-test.js" loads the test file, or when the test files load specific modules under test. Adding manifest-scanning to the test files would address the latter, but wouldn't help the former, since unit-test.js is scanning for modules to require() at runtime (exactly the sort of unreviewable behavior that we're trying to prevent with the manifest).

So I think that part of the "special powers" that test modules should get is the ability to turn on/off the undeclared-require() checking. It could be turned off by default for unit tests, but test-manifest.js needs to turn it back on briefly to do its job.

Comment 5

[Atul Varma [:atul]]

Cool. With respect to manifest-scanning for the test files, I've actually filed this as bug 593177 and submitted a patch for you to review.

Comment 6

[Atul Varma [:atul]]

Comment on attachment 472085 [details] [diff] [review]
emit warning when require(X) is called but X is not on the manifest

Hmm, unless I'm misreading things, I don't think that the manifestChecker will have the desired behavior: allowEval() is only called the first time a particular module is imported in any part of the code--i.e., when its code is evaluated. Once the module has been imported once and its code evaluated, allowEval() is never called again.

This means that if module 'spam' had the following code:

  require('foo');

while module 'eggs' had this:

  require('fo' + 'o');  // cheating!

and module 'main' had this:

  require('spam');
  require('eggs');

Everything would actually work with your patch, even though the expected behavior is for a warning to be logged when 'eggs' attempts to import 'foo'. This is because when 'spam' imports foo, allowEval() is called and the evaluation of 'foo' is granted, but when 'eggs' imports foo, only allowImport() is called, which grants access to the module.

Also, I have reservations about landing this with all the warnings in-place... It'd be great to have those things sorted out before landing, so as to not burden our core developers with lots of spam, right?

Comment 7

[Atul Varma [:atul]]

Oh, by the way, do we even want to do dependency tracking for modules that ask for chrome privileges? The reason I ask is because the modules that ask for chrome privileges will be able to work around the system any way they want to get access to whatever they want, so enforcing security at this level may be giving the illusion that we have more security than we actually do.

But also, chrome-privileged modules are ones that are more likely to do dynamic things when calling require()--for instance, as you mention in comment 4, the test runner auto-require()'s modules it finds in test directories, and the e10s-jetpack augmentor tries to dynamically find "*-e10s-adapter" modules to load. It might be enough just to say that chrome-privileged modules can require whatever they want, than creating another special power that some modules can have.

Comment 8

[Atul Varma [:atul]]

Attachment: refactorings and fixes

Comment 9

[Atul Varma [:atul]]

The patch I just submitted fixes the bug I mentioned in comment 6.

It also changes the test structure around to unit-test instead of functional-test, by creating a cuddlefish.Loader and passing in a fake 'packaging' object and a fake 'fs' object. Instead of using counters to verify whether checks were made successfully, it passes in a fake 'console' object that adds all logged warnings to a list, which is checked by test assertions.

It should be a lot easier to test weird funky edge cases this way.

However, because unexpected require()'s are now logged via console.warn(), they now trigger test failures when test suites run that have unexpected require()'s. So I'll try to fix that next, in part by allowing a module with 'needsChrome' to import whatever it wants.

Comment 10

[Atul Varma [:atul]]

Attachment: modules with needsChrome can now import whatever they want.

Comment 11

[Atul Varma [:atul]]

Attachment: all tests pass, no warnings logged to console.

Comment 12

[Atul Varma [:atul]]

Attachment: removed unused counter logic

Comment 13

[Brian Warner [:warner :bwarner]]

Comment on attachment 473255 [details] [diff] [review]
removed unused counter logic

this patch looks good to me. My only nit is to make the indentation consistent (a couple of sections use one-space instead of two-space indents).

Comment 14

[Brian Warner [:warner :bwarner]]

Pushed, in http://hg.mozilla.org/labs/jetpack-sdk/rev/3cac43806863 .

I think we probably still need the "remove .packaging from globals" fix, to prevent modules from tinkering with the manifest to bypass this check. I'll see if we've got a bug for that already, and add one if not.

Comment 15

[Brian Warner [:warner :bwarner]]

596077 opened for "remove .packaging from globals"

Comment 16

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".