[APICHANGE] Replace NO_KNOWN_EXPLOIT_SPEC_VIOLATION with configurable list.

The notion of NO_KNOWN_EXPLOIT_SPEC_VIOLATION (NKESV) depends on the use
being made of SES (for example, the cross-frame freeze bug is
problematic only if you use multiple frames). Therefore, it should not
be hardcoded in SES itself, but supplied by the system loading SES.

* Remove NKESV and replace it with ses.acceptableProblems, which
  allows specifying 'permit' and 'doNotRepair' flags for problem IDs.

  'permit' causes a problem's severity to not be counted in the max
  severity. 'doNotRepair' causes a problem to not be repaired at all;
  this new feature may be used to improve performance by disabling slow
  repairs that are not necessary for the application.

* caja.js specially recognizes NKESV in its config and causes acceptance
  of the same list of problems to be accepted as NKESV did. For safety,
  and pending design, there is no way to configure the list of problems
  using caja.js.

* Replace ses-iframe-init.js with a hook which runs on an iframe before
  the ses-single-frame.js is loaded; this also allows us to throw out
  the kludge where we set SES's max severity to NEW_SYMPTOM and check
  separately after (which incidentally reduces the hazard of
  issue 1758), and might make autoswitching faster.

* Resurrect the "too slow" repair_PUSH_IGNORES_SEALED from r5238 and
  disable it using doNotRepair. Note that the repair will now be applied
  in Caja unless NKESV is requested.

Impact:
* Applications using SES directly must specify acceptableProblems rather
  than NKESV. Applications using caja.js are unaffected except that they
  will now get repair_PUSH_IGNORES_SEALED unless specifying NKESV.

@r5442

[felix8a, 2013-06-06 21:33:59]

the code looks fine, but I'm wary that repairES5 has now lost the distinction
between "never ever ignore this UNSAFE_SPEC_VIOLATION" and "it's sometimes ok to
ignore this UNSAFE_SPEC_VIOLATION".

for example, after this change, it's not particularly obvious that
PUSH_IGNORES_SEALED is ok but PUSH_IGNORES_FROZEN is not.

I'd be ok with brief comments saying why something is unsafe, or when it's ok to
ignore the unsafeness.

[kpreid2, 2013-06-06 21:41:14]

On 2013/06/06 21:33:59, felix8a wrote:
> the code looks fine, but I'm wary that repairES5 has now lost the distinction
> between "never ever ignore this UNSAFE_SPEC_VIOLATION" and "it's sometimes
> ok to ignore this UNSAFE_SPEC_VIOLATION".
> 
> for example, after this change, it's not particularly obvious that
> PUSH_IGNORES_SEALED is ok but PUSH_IGNORES_FROZEN is not.

I'm not sure that line is all that sharp, but I agree that it might be good to
write it down — preferably, enforced in code. Perhaps each kludge record should
have a flag which specifies whether it may be ignored.

Waiting for MarkM's comments on the design.

[MarkM, 2013-06-06 21:59:52]

ses/* stuff LGTM

https://codereview.appspot.com/9979047/diff/1/src/com/google/caja/ses/repairE...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/9979047/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:240: * An object whose keys are problem
names and whose values are one of
"whose enumerable keys", since you're finding them with a for/in loop.

https://codereview.appspot.com/9979047/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:3729: repair: undefined, // workaround is
too slow
Given the rest of this CL, why not reinstate this repair?

[MarkM, 2013-06-06 22:02:54]

On 2013/06/06 21:41:14, kpreid2 wrote:
> On 2013/06/06 21:33:59, felix8a wrote:
> > the code looks fine, but I'm wary that repairES5 has now lost the
distinction
> > between "never ever ignore this UNSAFE_SPEC_VIOLATION" and "it's sometimes
> > ok to ignore this UNSAFE_SPEC_VIOLATION".
> > 
> > for example, after this change, it's not particularly obvious that
> > PUSH_IGNORES_SEALED is ok but PUSH_IGNORES_FROZEN is not.
> 
> I'm not sure that line is all that sharp, but I agree that it might be good to
> write it down — preferably, enforced in code. Perhaps each kludge record
should
> have a flag which specifies whether it may be ignored.
> 
> Waiting for MarkM's comments on the design.

I think distinguishing these in prose is adequate for now. If this turns out to
be a maintenance hazard we can revisit.

[MarkM, 2013-06-06 22:14:54]

On 2013/06/06 22:02:54, MarkM wrote:
> On 2013/06/06 21:41:14, kpreid2 wrote:
> > On 2013/06/06 21:33:59, felix8a wrote:
> > > the code looks fine, but I'm wary that repairES5 has now lost the
> distinction
> > > between "never ever ignore this UNSAFE_SPEC_VIOLATION" and "it's sometimes
> > > ok to ignore this UNSAFE_SPEC_VIOLATION".
> > > 
> > > for example, after this change, it's not particularly obvious that
> > > PUSH_IGNORES_SEALED is ok but PUSH_IGNORES_FROZEN is not.
> > 
> > I'm not sure that line is all that sharp, but I agree that it might be good
to
> > write it down — preferably, enforced in code. Perhaps each kludge record
> should
> > have a flag which specifies whether it may be ignored.
> > 
> > Waiting for MarkM's comments on the design.
> 
> I think distinguishing these in prose is adequate for now. If this turns out
to
> be a maintenance hazard we can revisit.

Each of these description records should have links to issues in some issue
tracker. Have the first of these be to a google-caja tracking issue. These
issues would be a good place to put the text explaining the hazard of proceeding
with the issue unrepaired, and, for those that the default Caja configuration
permits, why it does so. These would then be easy to navigate to from
explicit.html.

[kpreid2, 2013-06-06 22:19:01]

On 2013/06/06 22:14:54, MarkM wrote:
> Each of these description records should have links to issues in some
> issue tracker. Have the first of these be to a google-caja tracking issue.
> These issues would be a good place to put the text explaining the hazard
> of proceeding with the issue unrepaired, and, for those that the default
> Caja configuration permits, why it does so. These would then be easy to
> navigate to from explicit.html.

This is a nice possible enhancement, but I don't think it is a prerequisite for
this change — it specifically does not expose the ability to ignore problems as
via caja.js configuration. I would prefer that this change simply be seen as
refactoring for Caja.

[MarkM, 2013-06-06 22:48:04]

On 2013/06/06 22:19:01, kpreid2 wrote:
> On 2013/06/06 22:14:54, MarkM wrote:
> > Each of these description records should have links to issues in some
> > issue tracker. Have the first of these be to a google-caja tracking issue.
> > These issues would be a good place to put the text explaining the hazard
> > of proceeding with the issue unrepaired, and, for those that the default
> > Caja configuration permits, why it does so. These would then be easy to
> > navigate to from explicit.html.
> 
> This is a nice possible enhancement, but I don't think it is a prerequisite
for
> this change — it specifically does not expose the ability to ignore problems
as
> via caja.js configuration. I would prefer that this change simply be seen as
> refactoring for Caja.

Fine with me. Still LGTM

[kpreid2, 2013-06-07 19:43:05]

The notion of NO_KNOWN_EXPLOIT_SPEC_VIOLATION (NKESV) depends on the use
being made of SES (for example, the cross-frame freeze bug is
problematic only if you use multiple frames). Therefore, it should not
be hardcoded in SES itself, but supplied by the system loading SES.

* Remove NKESV and replace it with ses.acceptableProblems, which
  allows specifying 'permit' and 'doNotRepair' flags for problem IDs.

  'permit' causes a problem's severity to not be counted in the max
  severity. 'doNotRepair' causes a problem to not be repaired at all;
  this new feature may be used to improve performance by disabling slow
  repairs that are not necessary for the application.

* caja.js specially recognizes NKESV in its config and causes acceptance
  of the same list of problems to be accepted as NKESV did. For safety,
  and pending design, there is no way to configure the list of problems
  using caja.js.

* Replace ses-iframe-init.js with a hook which runs on an iframe before
  the ses-single-frame.js is loaded; this also allows us to throw out
  the kludge where we set SES's max severity to NEW_SYMPTOM and check
  separately after (which incidentally reduces the hazard of
  issue 1758), and might make autoswitching faster.

* Resurrect the "too slow" repair_PUSH_IGNORES_SEALED from r5238 and
  disable it using doNotRepair. Note that the repair will now be applied
  in Caja unless NKESV is requested.

Impact:
* Applications using SES directly must specify acceptableProblems rather
  than NKESV. Applications using caja.js are unaffected.

[kpreid2, 2013-06-07 19:43:55]

Significant changes to acceptableProblems; change description updated. See below
for why.

https://codereview.appspot.com/9979047/diff/1/src/com/google/caja/ses/repairE...
File src/com/google/caja/ses/repairES5.js (right):

https://codereview.appspot.com/9979047/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:240: * An object whose keys are problem
names and whose values are one of
On 2013/06/06 21:59:52, MarkM wrote:
> "whose enumerable keys", since you're finding them with a for/in loop.

Done.

https://codereview.appspot.com/9979047/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:256: */
I've added a warning here that permitting unrepaired problems is potentially
dangerous to SES itself.

https://codereview.appspot.com/9979047/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:3729: repair: undefined, // workaround is
too slow
On 2013/06/06 21:59:52, MarkM wrote:
> Given the rest of this CL, why not reinstate this repair?

Done. However, this has significant consequences:

1. That same repair also applies to PUSH_DOES_NOT_THROW_ON_FROZEN_ARRAY and
PUSH_IGNORES_FROZEN, so I have to add it to those as well or they will be
reported as “Accidentally repaired”.

2. But we don't want to run the slow repair in Caja, and we can't express "don't
repair, but abort" for PUSH_IGNORES_FROZEN using my current scheme.

Therefore, I have changed the format of acceptableProblems to specify 'permit'
and 'doNotRepair' as independent flags; doNotRepair no longer implies permit.

Also, repair_PUSH_IGNORES_SEALED is now done by default when using caja.js: in
order to get the old behavior of never repairing you have to specify NKESV
(which Playground and Apps Script do, so most users will see no difference). I
could change this, but it seems to be the right thing on principle.

https://codereview.appspot.com/9979047/diff/1/src/com/google/caja/ses/repairE...
src/com/google/caja/ses/repairES5.js:4017: }
I have added logging of ignored problems.

[MarkM, 2013-06-07 20:46:18]

A nice improvement. I also like the addition you made to the logging. The logged
output would look confusing otherwise, as the alleged max wouldn't have been the
max of the previously enumerated results.

LGTM