Validate exposedTo before registering AbortSignal algorithm - #240
Conversation
Move the validation of `options.exposedTo` origins before attaching the unregister algorithm to `options.signal` in `registerTool()`. Previously, the abort algorithm was attached to the signal before origin validation. If `exposedTo` contained an invalid or untrustworthy origin, the promise was rejected with a `SecurityError`, but the abort algorithm remained attached to the signal and could inadvertently unregister a subsequently registered tool of the same name upon aborting. Also reorder `ModelContextRegisterToolOptions` dictionary members and domintro definitions to match the processing order of options.
|
So the only observable with this change is the buggy ability to unregister a later-registered, valid tool by the abort algorithm hanging around, right? (I think this will be fixed regardless, once we end up with a UUID-keyed tool registry and |
Yes. |
SHA: 7aa8235 Reason: push, by domfarolino Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| 1. [=model context/Unregister a tool=] given [=this=] and |tool name|. | ||
|
|
||
| 1. [=Reject=] |promise| with |signal|'s [=AbortSignal/abort reason=]. | ||
| 1. If |options|'s {{ModelContextRegisterToolOptions/signal}} [=map/exists=] and is |
There was a problem hiding this comment.
I'd argue we could make it nicer by moving this below the exposed origins check so that we check first all things related to exposed origins, then and only then signal aborted + registering algo. Here, we're mixing both.
I'm happy to send a spec PR and Chromium CL if accepted.
There was a problem hiding this comment.
I think while this is a little strange, we should avoid doing as much work as possible (aside from environment checks like at the very beginning of this method) if the developer passes in an aborted signal. IMO if the signal is already aborted, the Promise shouldn't abort due to any other invalid input (that WebIDL doesn't already catch), since we shouldn't even process them. If you can think of a way to preserve that invariant while rearranging this, I'm all ears. Otherwise, I think the semantics here are at least worth preserving.
Move the validation of
options.exposedToorigins before attaching the unregister algorithm tooptions.signalinregisterTool().Previously, the abort algorithm was attached to the signal before origin validation. If
exposedTocontained an invalid or untrustworthy origin, the promise was rejected with aSecurityError, but the abort algorithm remained attached to the signal and could inadvertently unregister a subsequently registered tool of the same name upon aborting.Also reorder
ModelContextRegisterToolOptionsdictionary members and domintro definitions to match the processing order of options.Fixed in Chromium: https://chromium-review.googlesource.com/c/chromium/src/+/8236351
Preview | Diff