Fixes several problems in our support for safe debugging. (issue 226970043 by erights@gmail.com)

[eri...@gmail.com]

Reviewers: kpreid2,

Description:
Fixes several problems in our support for safe debugging.

Fixes https://code.google.com/p/google-caja/issues/detail?id=1516
The UnsafeError object was exposed to privileged code as a property on
ses. Since the ses is accessible only to privileged code, this is not
a vulnerability, but it does violate a stated invariant. Caught by
trying to verify SES using S5
http://blog.brownplt.org/2011/11/11/s5-javascript-semantics.html

Fixes https://code.google.com/p/google-caja/issues/detail?id=1963
Rewires the Error inheritance hierarchy to stay compatible with ES6
while staying safe.

Fixes https://code.google.com/p/google-caja/issues/detail?id=1964
On non-v8, debug.js detects of Error.prototype.stack is an accessor
property. If so, grab its getter for its own internal use. This now
provides proper encapsulation of stack information on FF40
Nightly in addition to the encapsulation we have long had on v8.

Fixes https://code.google.com/p/google-caja/issues/detail?id=1965
When detecting a url into the rawgit service that matches a common
pattern, useHTMLLogger renders this as a link that takes you to the
corresponding page on github with the correct line highlighted.

Please review this at https://codereview.appspot.com/226970043/

Affected files (+68, -41 lines):
M src/com/google/caja/ses/debug.js
M src/com/google/caja/ses/useHTMLLogger.js

[eri...@gmail.com]

Fixes several problems in our support for safe debugging.

Fixes https://code.google.com/p/google-caja/issues/detail?id=1516
The UnsafeError object was exposed to privileged code as a property on
ses. Since the ses is accessible only to privileged code, this is not
a vulnerability, but it does violate a stated invariant. Caught by
trying to verify SES using S5
http://blog.brownplt.org/2011/11/11/s5-javascript-semantics.html

Fixes https://code.google.com/p/google-caja/issues/detail?id=1963
Rewires the Error inheritance hierarchy to stay compatible with ES6
while staying safe.

Fixes https://code.google.com/p/google-caja/issues/detail?id=1964
On non-v8, debug.js detects of Error.prototype.stack is an accessor
property. If so, grab its getter for its own internal use. This now
provides proper encapsulation of stack information on FF40
Nightly in addition to the encapsulation we have long had on v8.

Fixes https://code.google.com/p/google-caja/issues/detail?id=1965
When detecting a url into the rawgit service that matches a common
pattern, useHTMLLogger renders this as a link that takes you to the
corresponding page on github with the correct line highlighted.

https://codereview.appspot.com/226970043/

[eri...@gmail.com]

New snapshot

https://codereview.appspot.com/226970043/

[kpr...@google.com]

https://codereview.appspot.com/226970043/diff/40001/src/com/google/caja/ses/debug.js
File src/com/google/caja/ses/debug.js (right):

https://codereview.appspot.com/226970043/diff/40001/src/com/google/caja/ses/debug.js#newcode71
src/com/google/caja/ses/debug.js:71: Object.setPrototypeOf(err,
FakeError);
The above code as well as existing code appears to be part of SES
security.

Yet, file comments say this file is optional. Are the comments wrong?

https://codereview.appspot.com/226970043/diff/40001/src/com/google/caja/ses/debug.js#newcode241
src/com/google/caja/ses/debug.js:241: if (!stack || typeof stack !==
'string') { return void 0; }
For audit-style-readability, I'd rather see this written as:

if (typeof stack !== 'string' || stack === '')

https://codereview.appspot.com/226970043/diff/40001/src/com/google/caja/ses/debug.js#newcode340
src/com/google/caja/ses/debug.js:340: typeof (result =
primStackGetter.call(err)) === 'string') {
This can return '' where it is otherwise careful to return undefined.
Deliberate?

https://codereview.appspot.com/226970043/

[eri...@gmail.com]

https://codereview.appspot.com/226970043/diff/40001/src/com/google/caja/ses/debug.js
File src/com/google/caja/ses/debug.js (right):

https://codereview.appspot.com/226970043/diff/40001/src/com/google/caja/ses/debug.js#newcode71
src/com/google/caja/ses/debug.js:71: Object.setPrototypeOf(err,
FakeError);

On 2015/04/15 22:45:39, kpreid_google wrote:
> The above code as well as existing code appears to be part of SES
security.

> Yet, file comments say this file is optional. Are the comments wrong?

The comments are correct but obscure. Added clarifying comments. PTAL.

https://codereview.appspot.com/226970043/diff/40001/src/com/google/caja/ses/debug.js#newcode241
src/com/google/caja/ses/debug.js:241: if (!stack || typeof stack !==
'string') { return void 0; }

On 2015/04/15 22:45:39, kpreid_google wrote:
> For audit-style-readability, I'd rather see this written as:

> if (typeof stack !== 'string' || stack === '')

Done.

https://codereview.appspot.com/226970043/diff/40001/src/com/google/caja/ses/debug.js#newcode340
src/com/google/caja/ses/debug.js:340: typeof (result =
primStackGetter.call(err)) === 'string') {

On 2015/04/15 22:45:39, kpreid_google wrote:
> This can return '' where it is otherwise careful to return undefined.
> Deliberate?

Done.

https://codereview.appspot.com/226970043/diff/60001/src/com/google/caja/ses/debug.js
File src/com/google/caja/ses/debug.js (right):

https://codereview.appspot.com/226970043/diff/60001/src/com/google/caja/ses/debug.js#newcode28
src/com/google/caja/ses/debug.js:28: * <p>TODO(erights): Explore
alternatives to using "instanceof Error"
Note the added TODO.

https://codereview.appspot.com/226970043/