Web incompatibility discovered in theathletic.com

[syg]

Chrome discovered a web incompatibility and is unshipping the iterator helpers proposal for now.

cc the other browser folks @dminor @msaboff

The incompat is reproduced below from the issue for ease of reading. Please see https://bugs.chromium.org/p/chromium/issues/detail?id=1474613 for full details.

1. theathletic.com uses a product called airgap.js. This product has a "tamper proof" mode that tries to freeze built-in collections' iterators and iterator prototypes. It has the following snippet:

            let e = (s = (r = R) == null ? void 0 : r.Iterator) == null ? void 0 : s.prototype;
            e && Dt(e);

where R is the global scope, and Dt is a utility function that ultimately called Object.freeze on the argument. The effect of this snippet is that if there's a global called Iterator, Iterator.prototype becomes frozen.

2. theathletic.com, like many sites, ships an old version (< 0.13.8) of the regenerator runtime to polyfill generators. The runtime makes its own version of the Generator function, including setting up the prototype chain. Generator.prototype's [[Prototype]] is Iterator.prototype. Prior to regenerator 0.13.8, setting up the generator code looked like the following:

  var Gp = GeneratorFunctionPrototype.prototype =
    Generator.prototype = Object.create(IteratorPrototype);
  GeneratorFunction.prototype = Gp.constructor = GeneratorFunctionPrototype;
  GeneratorFunctionPrototype.constructor = GeneratorFunction;

The assignment Gp.constructor = GeneratorFunctionPrototype triggers what's commonly called the "override mistake" in JS, where a non-writable property that exists on some object's [[Prototype]] prevents a new data property from being created on that object. In this case, because airgap.js froze Iterator.prototype, Iterator.prototype.constructor is non-writable, causing this assignment to throw.

Regenerator runtime fixed this bug upstream 2 years ago in facebook/regenerator#411

[ljharb]

oof, the second one could maybe be addressed by folks updating regenerator-runtime, but the first one seems like we could never ship a global called Iterator.

Is there any insight into why they would have that snippet in the first place?

[syg]

Is there any insight into why they would have that snippet in the first place?

This product is closed source AFAICT. If someone has the time, feel free to reach out to Transcend?

[gibson042]

IIUC, the issue is that an old version of regenerator is not compatible with the airgap.js "tamper proof" mode when both are loaded in an environment that includes an Iterator global. Am I alone in not considering this interaction bug to be a significant web compatibility issue?

[ljharb]

Freezing builtins is exceedingly rare, and I'd expect the sites that do to be aggressive in keeping up to date.

[michaelficarra]

@gibson042 Due to its popularity, this website alone qualifies as a significant web compatibility issue.

[mhofman]

As @ljharb mentions, I'm confounded by a website using a library that freezes intrinsics, yet does so on top of really outdated polyfills. Is there any chance to reach out to theathletic.com to update their frontend deps?

[syg]

As @ljharb mentions, I'm confounded by a website using a library that freezes intrinsics, yet does so on top of really outdated polyfills. Is there any chance to reach out to theathletics.com to update their frontend deps?

I'm already working internal channels to reach out to them, but non-Google leads would also be appreciated if anyone here has connections.

[syg]

To recap some convo with @bakkot, there are two ways to workaround this issue for theathletic.com:

1. Reach out to theathletic.com and get them to update its bundled regenerator runtime. Babel chooses a version automatically depending on the downlevel version, so maybe this is not as easy as it sounds? I'm not sure.
2. Reach out to transcend.io and get them to update their CDN-served airgap.js to workaround the issue by e.g. delete Iterator.prototype.constructor before doing its freezing. This would be transparent to theathletic.com.

I'm trying to do 1 above. Could I get a volunteer for trying 2 as well?

[bakkot]

cc @michaelfarrell76 @bencmbrook @eligrey @anotherminh as some people at Transcend according to Github. See above thread about a web compatibility issue caused by an interaction between airgap-js, regenerator-runtime, and a new JS language feature (Iterator helpers).

Sorry if y'all are the wrong people to ping; please forward this thread to appropriate people.

[eligrey]

Thank you for bringing this to our attention! Depending on our analysis of this issue we may change airgap.js functionality to better accommodate the iterator helpers proposal.

I haven't gone over this entirely, but as a quick fix for the specific site in question we can manually disable our tamper resistance feature for The Athletic with their approval. We're reaching out to them now.

[eligrey]

For reference, here is a snippet that is representative of the airgap.js tamper resistance code in question that is causing the problematic interaction: https://gist.github.com/eligrey/c5a252f1f48cbdd8bf2b344b4ddaea65

The intention of this code is to assist in preventing malicious code from interfering with our library's network regulation capabilities. Our consent manager library was designed to coexist with and regulate potentially hostile adtech environments. Once the ShadowRealm API is shipping in all major browsers, we intend to explore using that as an alternative technique to achieve tamper resistance.

This snippet was initially written with a version of the iterator helpers proposal in mind. Ignoring the issue caused by the interaction with the old version of regenerator runtime, is this snippet at odds with any common use cases enabled by the iterator helpers proposal? I want to better understand if there are any changes that we should make on our end.

[bakkot]

This snippet was initially written with a version of the iterator helpers proposal in mind. Ignoring the issue caused by the interaction with the old version of regenerator runtime, is this snippet at odds with any common use cases enabled by the iterator helpers proposal?

I don't see any obvious other problems, just the override mistake thing. Of course if it runs before a polyfill of this proposal it will prevent that polyfill from taking effect, but that is a different kind of issue.

(And it's kind of overkill for your purposes; just freezing the Symbol.iterator and iterator's next properties would be sufficient (unless you are worried about someone adding .return or .throw, but in that case you'd also need to freeze Object.prototype to prevent them from being added there). But the overkill shouldn't cause problems except for the override mistake issues.)

Do note that making Iterator.prototype[Symbol.toStringTag] non-writable has exactly the same problem as making Iterator.prototype.constructor non-writable, i.e., it will break old versions of regenerator-runtime. Same probably goes for Iterator.prototype[Symbol.iterator].

[gibson042]

@gibson042 Due to its popularity, this website alone qualifies as a significant web compatibility issue.

Seems to me like we just uncovered a bug at theathletic.com, which is rather easy to fix and likely will be in short order.

[syg]

Seems to me like we just uncovered a bug at theathletic.com, which is rather easy to fix and likely will be in short order.

@gibson042 I don't want to underplay the severity here and suggest that somehow the nature of the bug (i.e. "unlikely interaction") determines the severity. The practical impact remains that, if this hits Chrome stable without fixes upstream or unshipping, we break many customers, some of whom will blame Chrome, some of whom will blame NYT, and none of which is good for the web. With Chrome 117 going to stable next Tuesday, 3 business days is pretty short to not treat it seriously.

We lucked out here with Transcend being (1) so responsive and (2) airgap.js being shipped via CDN. The unlikely interaction nature of the bug isn't why we might get away with not unshipping after all, it's those incidental factors.