fix(exo)!: reject extra args by default

[erights]

closes: #1888 refs: #1889 #1765 #1809 https://github.com/Agoric/agoric-sdk/pull/8514 https://github.com/Agoric/agoric-sdk/pull/8641

Description

Breaking change: Change method guard arguments matching rule to reject extra arguments by default.

Security Considerations

Prior to #1765 we (I) had a bad bug that enabled extra args to bypass intended input validation. This was a definite security risk. At the time of this writing, agoric-sdk does indeed depend on an endo prior to #1765 and so does suffer from this risk.

1765 repairs the security risk per se, in that these extra arguments are silently dropped instead. But experience has shown that this lenient behavior masks bugs that are better caught and fixed. The purpose of this PR is to have the method guard by default reject argument lists with extra args beyond those declared. When we do upgrade agoric-sdk to depend on an endo fixing the original bug, we hope to upgrade it to one incorporating this PR, skipping the lenient behavior of #1765 completely.

This change from #1765 is a tradeoff. See "Upgrade Considerations" below, as well as the discussion in the #1888 issue, for the cost of doing so.

Note that neither #1765 nor this change has any effect on method guards with an explicit .rest. Likewise, such guards to not suffer from #1888 and so are not an issue. For any guard without an explicit .rest, after this PR you can restore the very lenient pre-#1765 behavior by adding an explicit .rest(M.any())

Throwing instead of dropping is consistent with “death before confusion”. In https://github.com/Agoric/agoric-sdk/pull/8514/files#diff-42f4291b619264addc7a7e12a8902cf3c60e5f797e97b5e4fa2a1e0c01d111ffR40, it was necessary to fix the shape of lookupAdmin on a name hub because dropping an argument caused subsequent updates to be applied to a prefix of the name path.

Scaling Considerations

None

Documentation Considerations

We need to document this new, less tolerant, guard behavior.

Testing Considerations

We need to test that we get the rejections this PR should generate, and that the error messages are reasonably understandable.

Upgrade Considerations

The motivation for the leniency of #1765 was to accommodate smoother versioning of APIs, where later versions of an API might add parameters not expected by earlier implementations of the API, and therefore not by guards representing those earlier versions of the API. This mirrors how JS works well enough to be less surprising to some folks. The cost of switching from #1765 to this PR is to lose such accommodations by default. However, when you do expect that a particular method may be extended with additional arguments, you can prepare by adding an explicit .rest(M.any()). But please, only use this lightly, only on specific methods that are likely to need it.

Without the ability to add extra args, the only remaining good choice for API evolution is new methods with new names. This is also familiar from JS as the "feature testing style": If a method with the new name exists, it operates in the new way. Otherwise fall back to the old way.

On the old endo that the current agoric-sdk depends on, there was no good way to do such feature testing remotely. Fortunately https://github.com/endojs/endo/pull/1809 (already merged to endo master) introduces the GET_METHOD_NAMES and GET_INTERFACE_GUARD meta methods enabling such remote feature testing. This is still at the cost of an extra round trip, and so should be done once up front rather than on each invocation. OTOH, if done only up front, then the client may miss the opportunity to use the new functionality when the provider gets upgraded. So best is for the client to check once per incarnation, so that restarting or upgrading the client will let it notice and utilize the opportunity. This is a less surprising time for the client to change behavior anyway.

[erights]

@kriskowal what should I do about the generated yarn.lock changes?

[erights]

@kriskowal No longer staged on #1889 as discussed. But still with the yarn.lock changes, which I still need feedback on.

[kriskowal]

@kriskowal what should I do about the generated yarn.lock changes?

Nothing you’ve done should have caused the changes to yarn.lock. This suggests that if you ran yarn on master, you’d get the same changes. If that’s the case, it would be fine to submit them in a separate PR. But, for purposes of expedience, it’s fine to include a chore: Update yarn.lock commit. I do recommend a separate commit and to create a merge commit to preserve it when merging this PR.

[kriskowal]

Beyond what’s in your description, throwing instead of dropping is consistent with “death before confusion”. In https://github.com/Agoric/agoric-sdk/pull/8514/files#diff-42f4291b619264addc7a7e12a8902cf3c60e5f797e97b5e4fa2a1e0c01d111ffR40, it was necessary to fix the shape of lookupAdmin on a name hub because dropping an argument caused subsequent updates to be applied to a prefix of the name path.

[erights]

..., throwing instead of dropping is consistent with “death before confusion”. In https://github.com/Agoric/agoric-sdk/pull/8514/files#diff-42f4291b619264addc7a7e12a8902cf3c60e5f797e97b5e4fa2a1e0c01d111ffR40, it was necessary to fix the shape of lookupAdmin on a name hub because dropping an argument caused subsequent updates to be applied to a prefix of the name path.

Added to description

[erights]

@kriskowal what should I do about the generated yarn.lock changes?

Nothing you’ve done should have caused the changes to yarn.lock. This suggests that if you ran yarn on master, you’d get the same changes. If that’s the case, it would be fine to submit them in a separate PR. But, for purposes of expedience, it’s fine to include a chore: Update yarn.lock commit. I do recommend a separate commit and to create a merge commit to preserve it when merging this PR.

I went with the separate PR at https://github.com/endojs/endo/pull/1892 . This PR no longer has these yarn.lock changes. I was glad to see that CI was clean anyway.

Which brings up an interesting issue: Should CI test if yarn would change yarn.lock? If it detects that it would, should CI fail? Issue a warning? Attn @kriskowal

[kriskowal]

There is a CI check to ensure yarn does not change yarn.lock.