iwa: Introduce the chromeos.isolatedWebApp.setShape blink extension [chromium/src : main]
Edman Anjos (Gerrit)
Edman Anjos added 1 comment![]( "Open in Gerrit")
hi Hidehiko, Simon, PTAL
IPC reviewer PTAL at the mojo changes. The next CL in this chain may have more context on the mojo impl, if you need.
Related details
- Chromium IPC Reviews
- Hidehiko Abe
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: hide...@chromium.org, mk...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): hide...@chromium.org, mk...@chromium.org
Reviewer source(s):
mk...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Hidehiko Abe
- Mike West
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Edman Anjos (Gerrit)
Edman Anjos voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Mike West (Gerrit)
Mike West added 1 comment![]( "Open in Gerrit")
// TODO(crbug.com/480146201): Not yet implemented.I looked briefly at the next two CLs in the chain, but I'm still wondering about how you're enforcing the restrictions set up on this kind of API to defend against a corrupted renderer. It looks like you unconditionally create the pipe in `chrome/browser/chrome_browser_interface_binders.cc`, and leave it up to the renderer from there. Are there browser-side checks you can perform here to ensure that the context calling this API is doing so legitimately?
Related details
- Edman Anjos
- Hidehiko Abe
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Hidehiko Abe (Gerrit)
Hidehiko Abe added 10 comments![]( "Open in Gerrit")
most likely sets of minor comments.
"+content/public/browser",
"+mojo/public/cpp/bindings",
"+third_party/blink/public/mojom/chromeos",
"+ui/views/widget/widget.h",
"+ui/gfx/geometry",
"+ui/aura/window.h",nit: please sort in the lexicographical order.
void Bind(could you explicitly document the behavior here when there already is a bound reciever?
mojo::PendingReceiver<blink::mojom::IsolatedWebAppApiBridge> receiver);and this must not be null either?
content::RenderFrameHost* render_frame_host,could you document this must not be nullptr?
namespace chromeos {often we use ash for the namespace of OS system parts.
std::move(callback).Run(false);nit: NOTIMPLEMENTED() until you actually implement it?
// Sets the shape of the current window to the given `rects`. Only areas of
// the window that are covered by a rectangle become visible and can be
// interacted with.maybe `at least one rectangle` to be more explicit?
// Passing an empty list of `rects` resets the window shape back to normal.could you document the meaning of return value, too?
In case of failure, it returns false? What is the difference from "rejected" promise?
Related details
- Edman Anjos
- Simon Hangl
Paulina Gacek (Gerrit)
Paulina Gacek added 5 comments![]( "Open in Gerrit")
Hey Edman, I had a few thoughts on this CL, so I added some comments, hope you don't mind c:
"BlinkExtensionChromeOS,BlinkExtensionChromeOSIsolatedWebApp");Since this CL introduces a new IWA-specific flag on top of the base flag (`BlinkExtensionChromeOS`), it would be beneficial to split this test fixture into two distinct fixtures to properly verify the feature gating.
Currently, enabling both flags at once loses test coverage for the intermediate state. It would be safer to structure it like this:
- `BlinkExtensionsBaseFlagSetTest` (Enables only BlinkExtensionChromeOS): Verifies that `window.chromeos` exists, but `window.chromeos.iwa` does not.
- `BlinkExtensionsIwaFlagSetTest` (Enables both flags): Verifies that `window.chromeos.iwa` exists and `setShape` works.
What do you think?
Vector<gfx::Rect> converted_rects;
converted_rects.reserve(rects.size());Shouldn't `rects` size be restricted for security reasons? If the developer exceeds it, we could reject the promise with some error.
for (const auto& rect : rects) {
converted_rects.push_back(gfx::Rect(
static_cast<int>(rect->x()), static_cast<int>(rect->y()),
static_cast<int>(rect->width()), static_cast<int>(rect->height())));
}`DOMRectReadOnly` stores its coordinates as `double`, but `gfx::Rect` uses `int`. Your current code uses `static_cast<int>`, which simply truncates the decimal. I think a floating version of gfx::Rect should be used instead: `ui/gfx/geometry/rect_f.h`
```suggestion
for (const auto& rect : rects) {
gfx::RectF float_rect(rect->x(), rect->y(), rect->width(), rect->height());
converted_rects.push_back(gfx::ToEnclosingRect(float_rect));
}
```
// Sets the shape of the current window to the given `rects`. Only areas of
// the window that are covered by a rectangle become visible and can be
// interacted with.WDYT about clarifying that the final window shape is the union of the provided rectangles? The current phrasing ('covered by a rectangle') leaves a little bit of ambiguity regarding how overlapping or disjoint rectangles are handled.
```suggestion
// Sets the shape of the current window to the union of given `rects`.
// Only areas of the window that are covered by at least one of the
// provided rectangles become visible and can be interacted with.
//
```Edman Anjos (Gerrit)
Edman Anjos added 14 comments![]( "Open in Gerrit")
"BlinkExtensionChromeOS,BlinkExtensionChromeOSIsolatedWebApp");Since this CL introduces a new IWA-specific flag on top of the base flag (`BlinkExtensionChromeOS`), it would be beneficial to split this test fixture into two distinct fixtures to properly verify the feature gating.
Currently, enabling both flags at once loses test coverage for the intermediate state. It would be safer to structure it like this:
- `BlinkExtensionsBaseFlagSetTest` (Enables only BlinkExtensionChromeOS): Verifies that `window.chromeos` exists, but `window.chromeos.iwa` does not.
- `BlinkExtensionsIwaFlagSetTest` (Enables both flags): Verifies that `window.chromeos.iwa` exists and `setShape` works.
What do you think?
Acknowledged, there's no scenario in production where one flag is enabled without the other, so I don't see the need to test them independently.
"+content/public/browser",
"+mojo/public/cpp/bindings",
"+third_party/blink/public/mojom/chromeos",
"+ui/views/widget/widget.h",
"+ui/gfx/geometry",
"+ui/aura/window.h",nit: please sort in the lexicographical order.
Done
could you explicitly document the behavior here when there already is a bound reciever?
Done, the current behavior is it will be rebound
mojo::PendingReceiver<blink::mojom::IsolatedWebAppApiBridge> receiver);and this must not be null either?
Done, also added a CHECK for this
could you document this must not be nullptr?
Done
often we use ash for the namespace of OS system parts.
Done
// TODO(crbug.com/480146201): Not yet implemented.I looked briefly at the next two CLs in the chain, but I'm still wondering about how you're enforcing the restrictions set up on this kind of API to defend against a corrupted renderer. It looks like you unconditionally create the pipe in `chrome/browser/chrome_browser_interface_binders.cc`, and leave it up to the renderer from there. Are there browser-side checks you can perform here to ensure that the context calling this API is doing so legitimately?
Thanks for the review Mike.
I added a check for isolation level in `IsolatedWebAppApiBridgeImpl::Create` method in this CL, and added a check for the chrome flag in https://crrev.com/c/7609644.
In the future we will gate this API to an allowlist of IWA IDs (not yet implemented in this chain, should come later in Q1 or Q2).
Does this sound good to you?
I'm also wondering if it's enough to make those checks on `Create` or if we need to e.g. check at every API call. Can you confirm?
nit: NOTIMPLEMENTED() until you actually implement it?
Done
Vector<gfx::Rect> converted_rects;
converted_rects.reserve(rects.size());Shouldn't `rects` size be restricted for security reasons? If the developer exceeds it, we could reject the promise with some error.
Done
for (const auto& rect : rects) {
converted_rects.push_back(gfx::Rect(
static_cast<int>(rect->x()), static_cast<int>(rect->y()),
static_cast<int>(rect->width()), static_cast<int>(rect->height())));
}`DOMRectReadOnly` stores its coordinates as `double`, but `gfx::Rect` uses `int`. Your current code uses `static_cast<int>`, which simply truncates the decimal. I think a floating version of gfx::Rect should be used instead: `ui/gfx/geometry/rect_f.h`
```suggestion
for (const auto& rect : rects) {
gfx::RectF float_rect(rect->x(), rect->y(), rect->width(), rect->height());
converted_rects.push_back(gfx::ToEnclosingRect(float_rect));
}
```
Acknowledge, floating point precision is not required here.
// Sets the shape of the current window to the given `rects`. Only areas of
// the window that are covered by a rectangle become visible and can be
// interacted with.maybe `at least one rectangle` to be more explicit?
Done
// Sets the shape of the current window to the given `rects`. Only areas of
// the window that are covered by a rectangle become visible and can be
// interacted with.WDYT about clarifying that the final window shape is the union of the provided rectangles? The current phrasing ('covered by a rectangle') leaves a little bit of ambiguity regarding how overlapping or disjoint rectangles are handled.
```suggestion
// Sets the shape of the current window to the union of given `rects`.
// Only areas of the window that are covered by at least one of the
// provided rectangles become visible and can be interacted with.
//
```Done
// Passing an empty list of `rects` resets the window shape back to normal.could you document the meaning of return value, too?
In case of failure, it returns false? What is the difference from "rejected" promise?
Done, I improved the docs and made this return `Promise<undefined>` instead.
I also added some basic input validation, like to check that rect width/height must be positive.
Related details
- Hidehiko Abe
- Mike West
- Simon Hangl
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |