Unbundled synchronous loading for Cajita and ServerJS modules

Synchronous modules are retrieved before the container is instantiated.
The load object is created given the current identifier and an 
identifier resolver.

[maoziqing, 2009-09-09 22:48:41]

addressed the following comments

http://codereview.appspot.com/116047/diff/1/14
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/116047/diff/1/14#newcode121
Line 121: r.resolve(cache[mid]);
cache should maintain promises to the module

http://codereview.appspot.com/116047/diff/1/14#newcode135
Line 135: var dependency = resolveDependency(
pass load to resolveDependency

http://codereview.appspot.com/116047/diff/1/14#newcode183
Line 183: }
whether we need this?

http://codereview.appspot.com/116047/diff/1/14#newcode206
Line 206: module, curMid, midResolver, scriptAsyncLoad);
pass the load to resolveDependency

http://codereview.appspot.com/116047/diff/1/16
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/116047/diff/1/16#newcode91
Line 91: private final List<StringLiteral> moduleNameList
it should be a set.

http://codereview.appspot.com/116047/diff/1/16#newcode92
Line 92: = new ArrayList<StringLiteral>();
add a todo of putting this into scope

http://codereview.appspot.com/116047/diff/1/18
File src/com/google/caja/serverjs-sandbox.js (right):

http://codereview.appspot.com/116047/diff/1/18#newcode76
Line 76: };
add id to the cajita load object;
pass the securedmodule to require

[ihab.awad, 2009-09-10 21:07:21]

LGTM with comments.

If you have the time, it would be nice to add copyright headers to the test
files as well.

Congratulations! :)

http://codereview.appspot.com/116047/diff/21/35
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/116047/diff/21/35#newcode95
Line 95: for (var i=0; i<size; i++) {
Code style: Spaces around "=" and "<"

http://codereview.appspot.com/116047/diff/21/35#newcode140
Line 140: + "module "+ mid + " failed."));
Code style: Spaces around "+"

http://codereview.appspot.com/116047/diff/21/37
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/116047/diff/21/37#newcode243
Line 243: {
Code style: Move opening brace 1 line up.

http://codereview.appspot.com/116047/diff/21/37#newcode263
Line 263: 
Extra blank line?

http://codereview.appspot.com/116047/diff/21/39
File src/com/google/caja/serverjs-sandbox.js (right):

http://codereview.appspot.com/116047/diff/21/39#newcode20
Line 20: var serverJsSandboxMaker = (function(env, valijaMaker, loadMaker) {
So ... since you wrote this code, ServerJS is now called CommonJS. :) I think
it's perfectly ok to check in this CL as-is and rename later though.

http://codereview.appspot.com/116047/diff/21/39#newcode26
Line 26: function resolveModuleId(mid, src) {
As we discussed, this is here only to provide "require.moduleId" for CommonJS
compliance.

I vote we make "moduleId" a public field of the Cajita-level secured module
object and simplify things at this level. If we decide this is not secure
enough, we can always just say that, for security reasons, the Caja
implementation of CommonJS does not support "require.moduleId".

http://codereview.appspot.com/116047/diff/21/39#newcode82
Line 82: };
Code style: Extra semicolon?

http://codereview.appspot.com/116047/diff/21/39#newcode97
Line 97: var load = loadMaker(mid, resolveModuleId);
Would you be able to eliminate the call to loadMaker if the Cajita-level secured
module exposed its "load" as a field?

If so, I vote we expose "load" from the Cajita-level secured module, simplify
the code here, then start to work how to make it more secure.

The problem is that module ID resolvers are not unique -- there can be many --
and it would be a very confusing event if the CommonJS-level one were different
from the underlying Cajita one.

http://codereview.appspot.com/116047/diff/21/39#newcode108
Line 108: "Loading module " + newMid + "failed, " + reason));
Space in "failed..."?

http://codereview.appspot.com/116047/diff/21/30
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/116047/diff/21/30#newcode99
Line 99: <div id="untrusted_content" class="xyz___"
The original (left hand side) is newer.

http://codereview.appspot.com/116047/diff/21/30#newcode189
Line 189: (new Function(htmlAndScript[2]))();
The original (left hand side) is newer.

http://codereview.appspot.com/116047/diff/21/31
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/116047/diff/21/31#newcode2987
Line 2987: function testStaticServerJsModule() {
Indent this line with the preceding single quote (for consistency with rest of
file).

[maoziqing, 2009-09-11 01:33:11]

committed version: 3724

http://codereview.appspot.com/116047/diff/21/35
File src/com/google/caja/cajita-module.js (right):

http://codereview.appspot.com/116047/diff/21/35#newcode95
Line 95: for (var i=0; i<size; i++) {
On 2009/09/10 21:07:23, ihab.awad wrote:
> Code style: Spaces around "=" and "<"

Done.

http://codereview.appspot.com/116047/diff/21/35#newcode140
Line 140: + "module "+ mid + " failed."));
On 2009/09/10 21:07:23, ihab.awad wrote:
> Code style: Spaces around "+"

Done.

http://codereview.appspot.com/116047/diff/21/37
File src/com/google/caja/parser/quasiliteral/CajitaRewriter.java (right):

http://codereview.appspot.com/116047/diff/21/37#newcode243
Line 243: {
On 2009/09/10 21:07:23, ihab.awad wrote:
> Code style: Move opening brace 1 line up.

Done.

http://codereview.appspot.com/116047/diff/21/37#newcode263
Line 263: 
On 2009/09/10 21:07:23, ihab.awad wrote:
> Extra blank line?

Done.

http://codereview.appspot.com/116047/diff/21/39
File src/com/google/caja/serverjs-sandbox.js (right):

http://codereview.appspot.com/116047/diff/21/39#newcode20
Line 20: var serverJsSandboxMaker = (function(env, valijaMaker, loadMaker) {
On 2009/09/10 21:07:23, ihab.awad wrote:
> So ... since you wrote this code, ServerJS is now called CommonJS. :) I think
> it's perfectly ok to check in this CL as-is and rename later though.

Done.

http://codereview.appspot.com/116047/diff/21/39#newcode26
Line 26: function resolveModuleId(mid, src) {
On 2009/09/10 21:07:23, ihab.awad wrote:
> As we discussed, this is here only to provide "require.moduleId" for CommonJS
> compliance.
> 
> I vote we make "moduleId" a public field of the Cajita-level secured module
> object and simplify things at this level. If we decide this is not secure
> enough, we can always just say that, for security reasons, the Caja
> implementation of CommonJS does not support "require.moduleId".

Done.

http://codereview.appspot.com/116047/diff/21/39#newcode82
Line 82: };
On 2009/09/10 21:07:23, ihab.awad wrote:
> Code style: Extra semicolon?

Done.

http://codereview.appspot.com/116047/diff/21/39#newcode97
Line 97: var load = loadMaker(mid, resolveModuleId);
On 2009/09/10 21:07:23, ihab.awad wrote:
> Would you be able to eliminate the call to loadMaker if the Cajita-level
secured
> module exposed its "load" as a field?
> 
> If so, I vote we expose "load" from the Cajita-level secured module, simplify
> the code here, then start to work how to make it more secure.
> 
> The problem is that module ID resolvers are not unique -- there can be many --
> and it would be a very confusing event if the CommonJS-level one were
different
> from the underlying Cajita one.

Done.

http://codereview.appspot.com/116047/diff/21/39#newcode108
Line 108: "Loading module " + newMid + "failed, " + reason));
On 2009/09/10 21:07:23, ihab.awad wrote:
> Space in "failed..."?

Done.

http://codereview.appspot.com/116047/diff/21/30
File tests/com/google/caja/plugin/domita_test.html (right):

http://codereview.appspot.com/116047/diff/21/30#newcode99
Line 99: <div id="untrusted_content" class="xyz___"
On 2009/09/10 21:07:23, ihab.awad wrote:
> The original (left hand side) is newer.

Done.

http://codereview.appspot.com/116047/diff/21/30#newcode189
Line 189: (new Function(htmlAndScript[2]))();
On 2009/09/10 21:07:23, ihab.awad wrote:
> The original (left hand side) is newer.

Done.

http://codereview.appspot.com/116047/diff/21/31
File tests/com/google/caja/plugin/domita_test_untrusted.html (right):

http://codereview.appspot.com/116047/diff/21/31#newcode2987
Line 2987: function testStaticServerJsModule() {
On 2009/09/10 21:07:23, ihab.awad wrote:
> Indent this line with the preceding single quote (for consistency with rest of
> file).

Done.