unhandledrejection should fire even for muted scripts

[jridgewell]

Previous context in:

• https://bugs.chromium.org/p/chromium/issues/detail?id=655694
• Censor events for cross-origin scripts domenic/unhandled-rejections-browser-spec#20

The AMP project was recently surprised that promise rejections that are unhandled did not fire an unhandledrejection event when the script is cross-origin.

I understand the PII risk that lead to muting script errors to begin with, but we still expected to receive a "Script error." error. We have an alert setup to monitor for that error message and ping us whenever there is a spike.

So, I want to contest the "it's useless" reasoning for not firing any error when a promise rejection is unhandled. The raw number of muted errors that we receive is useful for telling us if something has gone horribly wrong with a release.

/cc @domenic

[annevk]

Why can these not be CORS scripts?

[domenic]

A similar (but not the same) issue was discussed in #2440.

We currently have no plans to enable more security bypasses for non-CORS scripts, via onerror or onunhandledrejection or any other cross-origin information leakage vectors. Instead we're doing a lot of work on things like COOP/COEP to push more and more pages into a "CORS only" mode.

So yeah, as @annevk indicates, the correct approach here is to mark your scripts as CORS (using the crossorigin="" attribute).

[jridgewell]

Why can these not be CORS scripts?

AMP is a library included on publisher's pages. Part of being "valid AMP" is following a set of restrictions on the HTML. Unfortunately, we didn't when we designed the original validator, we didn't understand that we needed crossorigin attribute on the <script>, so it's actually forbidden. We're in the process of allowing it, but it requires publishers to update their pages, meaning it's going to take forever.

Additionally, we do experimental opt-ins via cookies (you can opt into next week's production build). Adding crossorigin means we'll have to figure out some other way to allow developers to opt into experimental builds.

Then there's also #2440 (comment).

We currently have no plans to enable more security bypasses for non-CORS scripts, via onerror or onunhandledrejection or any other cross-origin information leakage vectors.

I'm not arguing that we should take #2440's approach. Just the same "Script error." message without a stack trace, now passed to unhandledrejection. That seems extremely minimal, and still gives us a signal if we have a broken release.

COOP/COEP

What is the acronym here? Googling is very unhelpful…

[annevk]

https://docs.google.com/document/d/1zDlfvfTJ_9e8Jdc8ehuV4zMEu9ySMCiTGMS9y0GU92k/edit explains COOP and COEP, though they won't help as much since they now require CORP and not CORS (I know).

The problem with adding side channels is that everyone thinks their side channel is minor enough, but in aggregate all these bits become problematic. So yeah, I think a conservative stance on them is a better approach.

[jridgewell]

Soo many 4-letter CO** acronyms. But I don't think these help me at all.

The problem with adding side channels is that everyone thinks their side channel is minor enough, but in aggregate all these bits become problematic. So yeah, I think a conservative stance on them is a better approach.

Let's examine the side channel, then.

I (the attacker? I'm at least interested in listening to unhandledrejection) make a webpage. I control everything about it, including the ability to patch the window context however I see fit.

I load some cross-origin script. It's a non-cors request, so the script has the muted errors flag on. I want to listen for unhandledrejections generated by the script, so obviously, it must have loaded (the other CO** headers didn't stop it from loading). Just a regular old script that executes.

Somewhere, the script created a rejected Promise, and it's unhandled. It's muted, so the natvie unhandledrejection isn't going to fire. But that Promise class that the script created could be anything, because I controlled the window before loading the script.

So:

const originalPromise = Promise;
const originalThen = Promise.prototype.then;
function Wrapper(resolver) {
  const p = new originalPromise((resolve, reject) => {
    let called = false;
    function wrappedResolve(value) {
      if (called) return;
      called = true;
      resolve(value);
    }

    function wrappedReject(err) {
      if (called) return;
      called = true;
      reject(err);
      maybeReport(err, p);

    }

    try {
      resolver(wrappedResolve, wrappedReject);
    } catch (e) {
      wrappedReject(e);
    }
  });
  p._chainEnd = true;

  Object.setPrototypeOf(p, Wrapper.prototype);
  return p;
}
Object.setPrototypeOf(Wrapper, Promise);
Object.setPrototypeOf(Wrapper.prototype, Promise.prototype);

Wrapper.prototype.then = function(f, r) {
  this._chainEnd = false;
  const p = originalThen.call(this, f, r);

  const wrapper = Promise[Symbol.species];
  Promise[Symbol.species] = null;
  originalThen.call(p, undefined, (err) => maybeReport(err, p));
  Promise[Symbol.species] = wrapper;

  return p;
};

function maybeReport(err, p) {
  setTimeout(() => {
    if (p._chainEnd) {
      console.error('unhandledrejection', err, p);
      p._chainEnd = false;
    }
  }, 1);
}

Promise[Symbol.species] = function() {
  return Wrapper;
};
Promise.prototype.constructor = Wrapper;
window.Promise = Wrapper;

By patching Promise[Symbol.species] and Promise.prototype.constructor, I've made it so that any time somePromise.then() is called, I'll be able to intercept an unredacted rejection error.

It's admittedly not perfect. It'll be slow, and anything that has native access a few native things will create native promises instead of my species wrapper. async functions, import expressions and DOM APIs that use the WebIDL promise definitions, and I think that's it.

I could wrap all the DOM APIs, but as soon as you .then on these native promises I can get the errors either way. The only way to remain completely safe here is to exclusively use async functions and await expressions, never use a DOM API and never call .then.

---

Now back to me as the library author, who installed a unhandledrejection listener in the same script that's generating the rejected promises that are unhandled. I have to now follow a few rules so that I can report the errors:

• Don't use async functions
• Don't use import expressions
• Always use the promise return value (in any way, any then call will do it) from DOM APIs

As a library author, I'm able to forbid async functions in my codebase. Either use Promise.p.then, or turn them into promise wrapped asyncToGenerators. This is rather easy.

As for the DOM APIs, I'm not really sure I care about someone who called fetch() and didn't use f.then() to do something with the response. So this is rather easy, too.

So, now I have to choose detecting promise rejections (verify the correctness of my releases) and performance/async functions.

I can still get the errors, it just would have been nice to not have the browser fight me along the way. I think this is the same frustration Malte had in #2440 (comment), but the surface area is easier to wrap for unhandledrejection.

[domenic]

You can do all that. Or you can add crossorigin="" to the script. That is the recommended path.

(I realize the deployment challenges are different.)

[jridgewell]

You can do all that. Or you can add crossorigin="" to the script. That is the recommended path.

Yes, we're going to try updating to crossorigin.

But my point was that the protection (not exposing a redacted "Script error.") here is really ineffective, when I can attack the script in 50 lines (getting unredacted errors in the process).

Things that would be more useful:

1. Fire a redacted error
2. Check the handler's script
   • if the handler is from the same script as the error, don't mute it
3. Don't mute at all, since it's easily attacked

3. is probably too much. 2. would be nice, but I don't know if there's any existing way to do this in the specs. 1. seems extremely minimal, and it'd allow me to verify releases.