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