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

fix: crash under csp #1333

Merged
merged 3 commits into from
Dec 20, 2022
Merged

fix: crash under csp #1333

merged 3 commits into from
Dec 20, 2022

Conversation

Jack-Works
Copy link
Contributor

close #1281

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.

Freeze would suffice here

@mhofman
Copy link
Contributor

mhofman commented Oct 22, 2022

Not in scope for this PR, but wondering what kind of test we could have to make sure we don't break no-eval environments. The setup would likely look like replacing the evaluators with functions that throw before importing ses. The question is how to scope a subset of the tests with this environment.

@Jack-Works
Copy link
Contributor Author

since Compartment is not available under no-eval environment, test lockdown is enough.

@kriskowal
Copy link
Member

Not in scope for this PR, but wondering what kind of test we could have to make sure we don't break no-eval environments. The setup would likely look like replacing the evaluators with functions that throw before importing ses. The question is how to scope a subset of the tests with this environment.

As it happens, you wrote one already. https://github.com/endojs/endo/blob/master/packages/ses/test/test-no-eval.js

It’s marked as failing. I’d like this PR to remove the failing bit from that test. I looked into it and this change isn’t sufficient. Thankfully, @mhofman left notes: #903

  no-eval › lockdown must not throw when eval is forbidden

  Error thrown in test:

  TypeError {
    message: 'no unsafe-eval, as if by content-security-policy',
  }

  › globalThis.eval (file://test/no-eval.js:6:9)
  › repairFunction (file://src/tame-function-constructors.js:71:35)
  › tameFunctionConstructors (file://src/tame-function-constructors.js:118:3)
  › repairIntrinsics (file://src/lockdown-shim.js:276:17)
  › lockdown (file://src/lockdown-shim.js:410:28)
  › file://test/test-no-eval.js:9:30

@mhofman
Copy link
Contributor

mhofman commented Oct 26, 2022

Oh lol, thanks past me!
However maybe the failing test I wrote isn't right since @Jack-Works could use the shim before this regression. I'd need to page back in memory what my thinking was back then.

@kriskowal kriskowal enabled auto-merge (squash) December 20, 2022 21:05
@kriskowal
Copy link
Member

I’m landing this as it’s incremental progress. Further work will be necessary to avoid eval with a lockdown flag without compromising lockdown invariants.

@kriskowal kriskowal merged commit e512174 into endojs:master Dec 20, 2022
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.

Uncaught ReferenceError: harden is not defined
3 participants