search

endojs / endo

Endo is a distributed secure JavaScript sandbox, based on SES
Apache License 2.0
834 stars 72 forks source link

fix(marshal)!: Update compareRank to terminate comparability at the first remotable #2597

Closed gibson042 closed 1 month ago

gibson042 commented 1 month ago

Fixes #2588 Fixes #2594

Description

To be refinable into a total order that distinguishes remotables, compareRank must consider [r1, 'x'] vs. [r2, 'y'] as a tie rather than as equivalent to [0, 'x'] vs. [0, 'y'], in case r1 vs. r2 ends up comparing differently than 'x' vs. 'y'.

Security Considerations

None known.

Scaling Considerations

None known.

Documentation Considerations

This includes implicit fixup in types.js, but there may be some Markdown that needs updating as well.

Testing Considerations

This expands property-based testing, but only narrowly. Expansion is expected in the future.

Compatibility Considerations

This affects storage of compound Keys in e.g. CopySets/CopyBags/CopyMaps, but any ordering written by the old compareRank should be accepted by the new one (which now reclassifies as ties what its predecessor considered to be distinct ranks). As such, we should be fine, but I welcome challenges to this assertion.

Upgrade Considerations

This is breaking in the sense of changing compareRank output, but as far as I know, nothing depends upon that except in combination with compareKeys (which is fixed by this PR). I am not aware of any concerns that should hinder upgrade to a version of endo including this PR.

But I am wondering what if anything to put in NEWS.md file(s).

erights commented 1 month ago

Just skimming, but saw the q at end. rankCompare is a full order. It cannot answer NaN. (Unless this PR is making a more radical change than I thought.

For all promises p,q, compareRank(p,q) === 0

gibson042 commented 1 month ago

Just skimming, but saw the q at end. rankCompare is a full order. It cannot answer NaN. (Unless this PR is making a more radical change than I thought.

@erights Right, and that remains true... the default compareRemotables (i.e., the one used by compareRank) now returns NaN to avoid misclassification of e.g. [r1, 'x'] vs. [r2, 'y'] as being in different ranks (without disrupting such correct classification of e.g. [0, 'x'] vs. [0, 'y']), but that NaN is replaced with 0 on the way out—compareRank and indeed any function returned from makeComparatorKit continues to implement a total preorder in which all valid inputs are comparable but ties are possible.

Do you have further comments or reason(s) to delay merging now that @kriskowal has approved?

erights commented 1 month ago

Do you have further comments or reason(s) to delay merging now that @kriskowal has approved?

I have not found time to review this in enough depth to approve it. But I saw no red flags and am satisfied from the conversations above that all the issues I thought to worry about have been extremely well thought through and reviewed. So please consider your existing approval adequate and do not wait for me. In this case, I'm happy to complete my review post merge.

Really looks great. A clever simple solution that I would not have thought of. Thanks much!!