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). http://codereview.appspot.com/116047
