Skip to content

fix(ses): align with XS property censorship agreement #1718

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

Merged
merged 2 commits into from
Aug 25, 2023

Conversation

erights
Copy link
Contributor

@erights erights commented Aug 10, 2023

Fixes #910

@phoddie, I seem unable to add you as an official reviewer, but please consider yourself one anyway. I'm eager for your comments.

Technically this is a compat break, which I currently mark with a fix(ses)!:. Reviewers, please let me know whether you consider this change a compat break, or whether I should demote it to fix(ses):. Thanks.

@erights erights requested a review from kriskowal August 10, 2023 22:44
@erights erights self-assigned this Aug 10, 2023
@erights erights force-pushed the markm-910-align-w-xs-prop-censor branch 3 times, most recently from 1a67747 to 825bdc6 Compare August 21, 2023 23:22
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.

This is a breaking change to the extent that anyone relied on the old behavior. My feeling is that the old behavior sufficiently deterred relying upon the old behavior that this is not a breaking change. But, if you wish to play it safe, we can bump the 0.minor version and you’ll need a conventional commit to that effect.

Otherwise, this just needs an entry in NEWS.md under # Next release and an update to the docs, particularly the doc that outlines the differences in the Hardened JavaScript realm and compartment.

@erights erights force-pushed the markm-910-align-w-xs-prop-censor branch from 825bdc6 to 48fc4c9 Compare August 24, 2023 23:21
@erights erights changed the title fix(ses)!: align with XS property censorship agreement fix(ses): align with XS property censorship agreement Aug 24, 2023
@erights
Copy link
Contributor Author

erights commented Aug 24, 2023

This is a breaking change to the extent that anyone relied on the old behavior. My feeling is that the old behavior sufficiently deterred relying upon the old behavior that this is not a breaking change. But, if you wish to play it safe, we can bump the 0.minor version and you’ll need a conventional commit to that effect.

Based on this, I just changed from fix(ses)!: to fix(ses):. Do I understand correctly that this is consistent with your answer above?

@erights
Copy link
Contributor Author

erights commented Aug 24, 2023

Otherwise, this just needs an entry in NEWS.md under # Next release and an update to the docs, particularly the doc that outlines the differences in the Hardened JavaScript realm and compartment.

There are indeed stale *.md files that need updating. Working on it. By "docs", do you mean anything besides *.md files in the endo repo?

@kriskowal
Copy link
Member

Otherwise, this just needs an entry in NEWS.md under # Next release and an update to the docs, particularly the doc that outlines the differences in the Hardened JavaScript realm and compartment.

There are indeed stale *.md files that need updating. Working on it. By "docs", do you mean anything besides *.md files in the endo repo?

Correct, just *.md in the ses package. I think just docs/reference.md and docs/guide.md. There are other stale docs that are out of scope.

@kriskowal
Copy link
Member

This is a breaking change to the extent that anyone relied on the old behavior. My feeling is that the old behavior sufficiently deterred relying upon the old behavior that this is not a breaking change. But, if you wish to play it safe, we can bump the 0.minor version and you’ll need a conventional commit to that effect.

Based on this, I just changed from fix(ses)!: to fix(ses):. Do I understand correctly that this is consistent with your answer above?

Yes, we can treat this as non-breaking, unless you can think of a realistic working program that would break because of these changes.

@erights erights force-pushed the markm-910-align-w-xs-prop-censor branch 2 times, most recently from a754d0a to 8910e93 Compare August 25, 2023 22:14
@erights
Copy link
Contributor Author

erights commented Aug 25, 2023

Correct, just *.md in the ses package. I think just docs/reference.md and docs/guide.md. There are other stale docs that are out of scope.

Done. The second commit has these. A lot more than either of us expected. PTAL.

@erights erights force-pushed the markm-910-align-w-xs-prop-censor branch from 8910e93 to b02d145 Compare August 25, 2023 22:19
@erights erights force-pushed the markm-910-align-w-xs-prop-censor branch from b02d145 to e4be709 Compare August 25, 2023 22:26
@erights erights requested a review from kriskowal August 25, 2023 22:58
@erights erights merged commit 9b17577 into master Aug 25, 2023
@erights erights deleted the markm-910-align-w-xs-prop-censor branch August 25, 2023 23:29
erights added a commit that referenced this pull request Jul 16, 2024
Closes: #XXXX
Refs: #910 (comment)
#1718
#2354
#910

## Description

While investigating #2354 , I just
tried it locally by visiting

    file:///.../endojs/endo/packages/ses/demos/console/index.html

and

    file:///.../endojs/endo/packages/ses/demos/challenge/index.html

in my browser. The first is the SES demo console, which worked just
fine. The second is the SES Escape Room, which still relied on the
disabled `Date.now()` not throwing. Indeed, before
#1718 a disabled (secure mode)
`Date.now()` returned `NaN`. But
#1718 changed it to throw.

What's strange is that in #1718 I
revise endojs/endo/packages/ses/demos/challenge/index.html to adjust the
text to say that `Date.now()` is "disabled" rather than "NaN". But I
didn't fix the escape room code.

Although I found this while investigating
#2354 , this is a completely
distinct bug that is unrelated to
#2354 . This PR itself does nothing
to fix #2354 .

### Security Considerations

none
### Scaling Considerations

none
### Documentation Considerations

All our docs that link to an explain the Escape Room challenge need to
be revisited, especially once #2354
is fixed. For example,
https://agoric.com/blog/technology/a-taxonomy-of-security-issues , which
I just verified links to the broken page reported at
#2354

### Testing Considerations

It is frustrating that the Escape Room is broken at least since
#1718 , and also broken by
#2354 for an undetermined period of
time, without anyone noticing until now. It would be good to bring that
site under some kind of automated testing.

### Compatibility Considerations

none
### Upgrade Considerations

none
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.

{[inert function]} from toString() for Math.random etc.
2 participants