Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: add regenerator runtime taming #2383

Merged
merged 10 commits into from
Aug 13, 2024

Conversation

Jack-Works
Copy link
Contributor

@Jack-Works Jack-Works commented Jul 26, 2024

Closes: #621
Refs: #1950

Description

regenerator-runtime is a widely used package in the ecosystem. It is used to support generators and async functions transpiled to ES5.

This PR adds an option legacyRegeneratorRuntimeTaming to fix regenerator-runtime from 0.10.5 to 0.13.7. Although the newer version of the regenerator runtime package is compatible with lockdown, some libraries bundle old (hence "legacy") regenerator runtime in their code and it's not practical to get them all to upgrade.

  • legacyRegeneratorRuntimeTaming: 'safe' do nothing.
  • legacyRegeneratorRuntimeTaming: 'unsafe-ignore' turns Iterator.prototype[@@iterator] to a funky accessor that drops all assignments to it.

Note: regenerator-runtime is doing this:

Gp[iteratorSymbol] = function () {
    return this;
}

which is effectively

IteratorPrototype[Symbol.iterator] = function () {
    return this;
}

Security Considerations

The replacement function from legacy regenerator runtime is the same as the native code, so it is "safe" to drop this assignment, in the sense that it does not cause any bad effects. However, this option drops the assignment by dropping any assignment to IteratorPrototype[Symbol.iterator], since we have no practical way to ensure that the assignment it drops is exactly the one above. Thus, this option is not actual safe since it causes any other such assignment to be ignored silently. This echoes the unsafety of ES3 and of sloppy mode, where failed assignments were silently ignored. Such behavior is unsafe because it allows control flow to proceed into code that assumes the assignment succeeded. That's why ES5 strict mode changed failed assignments to throw.

To emphasize the hazard, we have named this setting of the option 'unsafe-ignore'.

Scaling Considerations

Nothing

Documentation Considerations

If you're hitting problems with an old version of regenerator-runtime (or any package that bundles it), you might need this.

Testing Considerations

Tests added for the specific effect of this PR. However, to avoid introducing even a devDependency on a legacy version of regenerator runtime, no automated test has been added to test that compatibility. Instead, this PR can be tested like this:

import './lockdown.umd.js'
lockdown({ legacyRegeneratorRuntimeTaming: 'unsafe-ignore', errorTaming: 'unsafe', consoleTaming: 'unsafe' })
const script = document.createElement('script')
script.src = 'https://cdn.jsdelivr.net/npm/regenerator-runtime@0.13.7/runtime.js'
document.head.appendChild(script)

Compatibility Considerations

Note: Some version of regenerator-runtime requires to be run in the sloppy mode. Thus, these are incompat with the ses-shim independent of this option.

Upgrade Considerations

No

Update NEWS.md for user-facing changes.

TODO.

Copy link
Member

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d like to get @erights opinion. Given that the option is safe, I also want to consider automatically detecting the regenerator runtime and applying this mitigation without a feature flag.

packages/ses/types.d.ts Outdated Show resolved Hide resolved
packages/ses/src/permits.js Outdated Show resolved Hide resolved
packages/ses/types.d.ts Outdated Show resolved Hide resolved
packages/ses/src/lockdown.js Outdated Show resolved Hide resolved
packages/ses/src/lockdown.js Outdated Show resolved Hide resolved
packages/ses/src/tame-regenerator-runtime.js Outdated Show resolved Hide resolved
packages/ses/types.d.ts Outdated Show resolved Hide resolved
@Jack-Works
Copy link
Contributor Author

hi, @erights I have updated the option name with a legacy prefix and changed none to safe, and safe to unsafe-ignore. I also changed how the setter behaves. I'd like to add a test after the design is OK.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jack, everything here looks good to me (modulo minor suggestions). Once there's a test case, I'll be happy to approve. Thanks!

packages/ses/src/commons.js Show resolved Hide resolved
packages/ses/src/tame-regenerator-runtime.js Outdated Show resolved Hide resolved
packages/ses/src/tame-regenerator-runtime.js Outdated Show resolved Hide resolved
@Jack-Works
Copy link
Contributor Author

Hi Jack, everything here looks good to me (modulo minor suggestions). Once there's a test case, I'll be happy to approve. Thanks!

Hi! I added tests!

packages/ses/src/tame-regenerator-runtime.js Outdated Show resolved Hide resolved
packages/ses/test/tame-legacy-regenerator-helper.test.js Outdated Show resolved Hide resolved
packages/ses/src/lockdown.js Outdated Show resolved Hide resolved
packages/ses/src/permits.js Outdated Show resolved Hide resolved
packages/ses/src/permits.js Outdated Show resolved Hide resolved
packages/ses/src/tame-regenerator-runtime.js Outdated Show resolved Hide resolved
packages/ses/src/tame-regenerator-runtime.js Outdated Show resolved Hide resolved
packages/ses/test/tame-legacy-regenerator-helper.test.js Outdated Show resolved Hide resolved
@erights erights self-requested a review August 12, 2024 15:16
@Jack-Works
Copy link
Contributor Author

thanks for the review! I think I fixed everything you have mentioned.

Copy link
Contributor

@erights erights left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks!

@erights
Copy link
Contributor

erights commented Aug 13, 2024

Hi @Jack-Works , I've LGTMed and am about to squash-and-merge. But before I do, I'm going to make some minor edits to the PR comment, which will then also be in the commit comment.

Thanks!

@erights
Copy link
Contributor

erights commented Aug 13, 2024

Hi @Jack-Works , I notice you wrote

Update NEWS.md for user-facing changes.

TODO.

Quite right. This reminds me that https://github.com/endojs/endo/blob/master/packages/ses/docs/lockdown.md will also need updating. I'm going to go ahead and merge this without either, but please do take a look at these. (attn @kriskowal )

@erights erights merged commit 6ae7995 into endojs:master Aug 13, 2024
17 checks passed
@kriskowal
Copy link
Member

Yes, please circle back and update lockdown.md and NEWS.md.

@Jack-Works Jack-Works deleted the regenerator-runtime branch August 16, 2024 10:26
erights added a commit that referenced this pull request Aug 22, 2024
Closes: #XXXX
Refs: #2383 

## Description

Of the various property override tests, the ones testing the default
`'moderate'` setting were calling `lockdown` without an `overrideTaming`
option, counting on it to default to `'moderate'`. However, if run
locally in an environment where the environment variable
`LOCKDOWN_OVERRIDE_TAMING` was set to something else, the test would
pointlessly misbehave.

This PR merely sets the option explicitly for those cases, so the tests
for the `'moderate'` case are as insensitive to environment variables as
the tests for the other cases.

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

none
### Testing Considerations

The point. The code before this PR would always work correctly under CI.
But the test may pointless fail locally depending on the developers
environment variable settings.
### Compatibility Considerations

none
### Upgrade Considerations

none
kriskowal pushed a commit that referenced this pull request Aug 22, 2024
Add documentation for `legacyRegeneratorRuntimeTaming`.

Closes: #2423
Refs: #2383
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Compatibility: regenerator-runtime
3 participants