| Auto-Submit | +1 |
| Commit-Queue | +1 |
I'm putting both Anders and Dom on this, just to see who gets to it first. No need for two reviews.
promise.Unwrap().Then(script_state, resolved, rejected);Mason FreedDom, I get this here:
```
[FATAL:third_party/blink/renderer/bindings/core/v8/world_safe_v8_reference.h:94] Check failed: world_.Get() == &target_script_state->World() (0x12c00405b68 vs. 0x12c0040b8b0).
```
Done
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Code-Review | +1 |
// EXPECT_EQ(action_results.at(0).result->script_tool_response->result,
// "val");nit: Is this useful? Can't we actually check the respondWidth result now?
active_webmcp_tool_, /*resolved*/ true);nit: `/*resolved=*/` is better, since it's supported by clang-tidy.
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html
rejected = MakeGarbageCollected<RespondWithHandler>(
active_webmcp_tool_, /*resolved*/ false);Isn't this creating exactly the same handler already held by `rejected`? Did you mean:
```suggestion
resolved = rejected;
```
?
We should probably add a test for this branch.
window.submit_event_fired = true;Maybe we should save `e.agentInvoked` on window and check that as well? Right now, `agentInvoked` is completely ignored by tests.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
dcheng@ can you PTAL at my comment below?
ScriptState* script_state = ToScriptStateForMainWorld(frame);While I know that this fixes the crash we observed in the test site, this worries me.
OK, so the flow is:
1. A form is submitted, possibly by an extension.
2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
3. We construct a SubmitEvent, and fire `submit` event handlers
4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
6. The Promise passed into respondWith() is constructed with a script state associated with that same world
7. We store the Promise temporarily on SubmitEvent
8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main world
Unwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).
I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.
active_webmcp_tool_, /*resolved*/ true);nit: `/*resolved=*/` is better, since it's supported by clang-tidy.
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html
+1. It also matches the examples in https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Auto-Submit | +1 |
ScriptState* script_state = ToScriptStateForMainWorld(frame);While I know that this fixes the crash we observed in the test site, this worries me.
OK, so the flow is:
1. A form is submitted, possibly by an extension.
2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
3. We construct a SubmitEvent, and fire `submit` event handlers
4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
6. The Promise passed into respondWith() is constructed with a script state associated with that same world
7. We store the Promise temporarily on SubmitEvent
8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main worldUnwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).
I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.
Thanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.
Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?
Thanks for looping in dcheng@ - curious what he thinks.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Auto-Submit | +1 |
// EXPECT_EQ(action_results.at(0).result->script_tool_response->result,
// "val");nit: Is this useful? Can't we actually check the respondWidth result now?
I'll remove this, mostly to avoid needing another reviewer. But also because we're kind of abandoning this test in favor of the unit test.
ScriptState* script_state = ToScriptStateForMainWorld(frame);Mason FreedWhile I know that this fixes the crash we observed in the test site, this worries me.
OK, so the flow is:
1. A form is submitted, possibly by an extension.
2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
3. We construct a SubmitEvent, and fire `submit` event handlers
4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
6. The Promise passed into respondWith() is constructed with a script state associated with that same world
7. We store the Promise temporarily on SubmitEvent
8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main worldUnwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).
I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.
Thanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.
Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?
Thanks for looping in dcheng@ - curious what he thinks.
I'm hoping we can land this and come back to this problem if dcheng (or you) thinks there is one. But there is a ton of code landing, and this keeps getting merge failures while it sits waiting. I'd like to land it.
Dominic Farolinonit: `/*resolved=*/` is better, since it's supported by clang-tidy.
https://clang.llvm.org/extra/clang-tidy/checks/bugprone/argument-comment.html
+1. It also matches the examples in https://google.github.io/styleguide/cppguide.html#Function_Argument_Comments.
Done
rejected = MakeGarbageCollected<RespondWithHandler>(
active_webmcp_tool_, /*resolved*/ false);Isn't this creating exactly the same handler already held by `rejected`? Did you mean:
```suggestion
resolved = rejected;
```?
We should probably add a test for this branch.
In the next patchset:
Also this branch is actually removed in a subsequent patch.
Maybe we should save `e.agentInvoked` on window and check that as well? Right now, `agentInvoked` is completely ignored by tests.
Tested in the next patchset. Look for `window.agent_invoked`.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Auto-Submit | +1 |
+bengr for timely review. I'd like to land this one since it'd gating 3 others with the rest of the functionality. I'm happy to handle any followups as followup CLs.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ScriptState* script_state = ToScriptStateForMainWorld(frame);Mason FreedWhile I know that this fixes the crash we observed in the test site, this worries me.
OK, so the flow is:
1. A form is submitted, possibly by an extension.
2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
3. We construct a SubmitEvent, and fire `submit` event handlers
4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
6. The Promise passed into respondWith() is constructed with a script state associated with that same world
7. We store the Promise temporarily on SubmitEvent
8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main worldUnwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).
I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.
Mason FreedThanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.
Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?
Thanks for looping in dcheng@ - curious what he thinks.
Daniel ChengI'm hoping we can land this and come back to this problem if dcheng (or you) thinks there is one. But there is a ton of code landing, and this keeps getting merge failures while it sits waiting. I'd like to land it.
I haven't managed to figure out exactly what's going on in this CL yet, but to be clear, a cross-world leak is a security bug.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
ScriptState* script_state = ToScriptStateForMainWorld(frame);Mason FreedWhile I know that this fixes the crash we observed in the test site, this worries me.
OK, so the flow is:
1. A form is submitted, possibly by an extension.
2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
3. We construct a SubmitEvent, and fire `submit` event handlers
4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
6. The Promise passed into respondWith() is constructed with a script state associated with that same world
7. We store the Promise temporarily on SubmitEvent
8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main worldUnwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).
I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.
Thanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.
Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?
Thanks for looping in dcheng@ - curious what he thinks.
Right, it worried me because PS11 stores a world-specific ScriptState on a world-agnostic C++ object that gets accessed by multiple worlds (because it's passed to all `submit` handlers, and those handlers can be created from any world). But maybe it's fine since we don't ever expose the ScriptState itself (or anything derived from it) directly to any handlers in those worlds...
| Auto-Submit | +1 |
| Commit-Queue | +2 |
ScriptState* script_state = ToScriptStateForMainWorld(frame);Mason FreedWhile I know that this fixes the crash we observed in the test site, this worries me.
OK, so the flow is:
1. A form is submitted, possibly by an extension.
2. `PrepareForSubmission()` gets called (by the `ExecuteTool()` changes in this CL)
3. We construct a SubmitEvent, and fire `submit` event handlers
4. Handlers run and get a v8 wrapper view the SubmitEvent associated with the realm that the handler was created in (see [here](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/bindings/core/v8/js_based_event_listener.cc;l=102-106;drc=bf712ec1a13783224debb691ba88ad5c15b93194))
5. The handler invokes SubmitEvent::respondWith(), which receives a ScriptState associated with the wrapper's (and thereofre listener's) world
6. The Promise passed into respondWith() is constructed with a script state associated with that same world
7. We store the Promise temporarily on SubmitEvent
8. After it's fired, we extract the Promise from the SubmitEvent, and `Unwrap()` it exclusively in the main worldUnwrapping exclusively in the main world fixed the crash in the exact configuration we observed it, but I think assuming the Promise was constructed in the main world (as we do here) is problematic. I actually think [PS11 is more correct](https://chromium-review.googlesource.com/c/chromium/src/+/7467862/11/third_party/blink/renderer/core/html/forms/html_form_element.cc), however this kind of violates [the ScriptState documentation](https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/bindings/script_state.h;l=65-66;drc=e187d025a32045c6249b739190382367dfc85985).
I think that's OK, but I'm honestly not sure. I talked with dcheng@ a little about this, let me loop him in here to get his advice directly.
Dominic FarolinoThanks Dom. I was thinking about this over the weekend also - it seems like perhaps the way the latest patchset reads, it's possible to execute code in the wrong world, which would e.g. give the extension world access to main world stuff.
Can you clarify how PS11 violates the docs? It says not to store script_state where it *can be accessed by multiple worlds*. But the point of that code was to ensure only the originating world has access, no?
Thanks for looping in dcheng@ - curious what he thinks.
Right, it worried me because PS11 stores a world-specific ScriptState on a world-agnostic C++ object that gets accessed by multiple worlds (because it's passed to all `submit` handlers, and those handlers can be created from any world). But maybe it's fine since we don't ever expose the ScriptState itself (or anything derived from it) directly to any handlers in those worlds...
Alright, I've gone back to the PS11 approach of storing and returning/using the script_state from the `respondWith()` caller world. From the sounds of it, that should be safe - the promise will be unwrapped in the correct world. The only risk is future users of the promise accessor (of which there aren't likely to be any) might hold it wrong.
I can clean up the approach and make it safer-by-default as a followup. One idea would be to create a stack allocated return value for RespondWithPromise that includes the `ScriptState::Scope` so that the promise can just be directly unwrapped.
For now, I'd really like to land this string of patches. Please keep commenting here with suggestions and issues, and I'll tackle them as followups to this chain.
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Removed Commit-Queue+2 by Mason Freed <mas...@chromium.org>
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
17 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: third_party/blink/renderer/core/html/forms/submit_event.h
Insertions: 11, Deletions: 4.
@@ -29,13 +29,20 @@
bool agentInvoked() const { return agent_invoked_; }
void respondWith(ScriptState*, ScriptPromise<IDLAny>, ExceptionState&);
- MemberScriptPromise<IDLAny> RespondWithPromise() const {
- return respond_with_promise_;
- }
+
+ // NOTE: The promise returned by this getter must be executed using the
+ // script_state also returned by the getter, and not with any other
+ // script_state. Doing so would be a cross-world security leak. E.g.,
+ // auto val = submit_event->RespondWithPromise();
+ // ScriptState::Scope scope(val->second);
+ // val->first.Unwrap()...
+ std::optional<std::pair<MemberScriptPromise<IDLAny>, Member<ScriptState>>>
+ RespondWithPromise() const;
private:
Member<HTMLElement> submitter_;
- MemberScriptPromise<IDLAny> respond_with_promise_;
+ std::pair<MemberScriptPromise<IDLAny>, Member<ScriptState>>
+ respond_with_promise_;
bool agent_invoked_ = false;
};
```
```
The name of the file: third_party/blink/renderer/core/html/forms/submit_event.cc
Insertions: 10, Deletions: 1.
@@ -41,7 +41,16 @@
if (!IsBeingDispatched()) {
LOG(ERROR) << "ERROR - event needs to be being dispatched.";
}
- respond_with_promise_ = script_promise;
+ respond_with_promise_ = {script_promise, script_state};
+}
+
+std::optional<std::pair<MemberScriptPromise<IDLAny>, Member<ScriptState>>>
+SubmitEvent::RespondWithPromise() const {
+ if (respond_with_promise_.first.IsEmpty() || !respond_with_promise_.second ||
+ !respond_with_promise_.second->ContextIsValid()) {
+ return std::nullopt;
+ }
+ return respond_with_promise_;
}
} // namespace blink
```
```
The name of the file: third_party/blink/renderer/core/html/forms/html_form_element.cc
Insertions: 4, Deletions: 6.
@@ -585,12 +585,10 @@
should_submit =
DispatchEvent(*submit_event) == DispatchEventResult::kNotCanceled;
if (declarative_webmcp_call) {
- if (auto promise = submit_event->RespondWithPromise();
- !promise.IsEmpty()) {
- ScriptState* script_state = ToScriptStateForMainWorld(frame);
- if (!script_state || !script_state->ContextIsValid()) {
- return;
- }
+ if (auto promise_and_script_state = submit_event->RespondWithPromise();
+ promise_and_script_state.has_value()) {
+ auto promise = promise_and_script_state->first;
+ auto script_state = promise_and_script_state->second;
auto* resolved = MakeGarbageCollected<RespondWithHandler>(
active_webmcp_tool_, /*resolved=*/true);
auto* rejected = MakeGarbageCollected<RespondWithHandler>(
```
Add agentInvoked and respondWith() to the `submit` event [3/N]
For declarative WebMCP, this CL adds two attributes to the submit
event:
- agentInvoked: true for declarative WebMCP calls
- respondWith(): allows the event listener to pass back a promise
that will resolve to the tool result.
If respondWith is called (and the event is preventDefaulted), then
we will wait for the promise to resolve, and then pass the value
back to the agent.
Fixed: 475973832
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |