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

store: refuse keys that are empty pass-by-copy objects #2585

Merged
merged 2 commits into from
Mar 11, 2021

Conversation

warner
Copy link
Member

@warner warner commented Mar 5, 2021

To protect against surprises when we change harden({}) from pass-by-reference to pass-by-copy (#2018), @michaelfig and I figured that the simplest safety measure would be to have store/weakstore refuse such keys.

This branch adds these checks, as well as a bunch of unit tests (it appeared to have none before).

I could use some help from a TypeScript guru, I can't seem to find a way to satisfy the signature of the helper function.

@warner warner changed the title 2018 store no empty data keys store: refuse keys that are empty pass-by-copy objects Mar 10, 2021
@warner warner force-pushed the 2018-store-no-empty-data-keys branch from 0d6dc4f to 21ab960 Compare March 10, 2021 20:25
@warner warner marked this pull request as ready for review March 10, 2021 20:26
@warner warner requested a review from erights March 10, 2021 20:26
Copy link
Member

@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

packages/store/src/store.js Outdated Show resolved Hide resolved
@warner
Copy link
Member Author

warner commented Mar 10, 2021

@erights can you help me with the typescript problem? The PR fails lint and I don't know how to give the helper function a type that will satisfy it.

@erights
Copy link
Member

erights commented Mar 10, 2021

You deleted helper.js but didn't push helpers.js

@warner warner force-pushed the 2018-store-no-empty-data-keys branch from bc001b9 to 1288439 Compare March 10, 2021 21:22
@warner
Copy link
Member Author

warner commented Mar 10, 2021

oops, fixed

packages/store/src/helpers.js Outdated Show resolved Hide resolved
@warner
Copy link
Member Author

warner commented Mar 10, 2021

ah, right, this needs to land on top of a cleanup in cosmic-swingset (#2567) which violates the rule we're imposing

@erights
Copy link
Member

erights commented Mar 10, 2021

Btw, in Zenhub you declare that this PR is blocked on #2567

When #2018 changes the interpretation of `harden({})` to become pass-by-copy,
any code which was previously using that to make a "handle" will break,
because the things they send will be treated as pass-by-copy. To catch cases
where this is happening, we forbid such keys from being used in
store/weakstore. Clients should use `Far('interfacename')` to create handles,
and these will be accepted by store/weakstore as keys.
@warner warner force-pushed the 2018-store-no-empty-data-keys branch from 83286e3 to c38a4dc Compare March 11, 2021 06:55
@warner warner merged commit 6076586 into master Mar 11, 2021
@warner warner deleted the 2018-store-no-empty-data-keys branch March 11, 2021 07:42
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.

2 participants