IssueTracker

kerenzhu: Uh oh! This issue still open and hasn't been updated in the last 550 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add Disable-Nags (case sensitive) to the Chromium Labels custom field.

kerenzhu: Uh oh! This issue still open and hasn't been updated in the last 565 days. This is a serious vulnerability, and we want to ensure that there's progress. Could you please leave an update with the current status and any potential blockers?

If you're not the right owner for this issue, could you please remove yourself as soon as possible or help us find the right one?

If the issue is fixed or you can't reproduce it, please close the bug. If you've started working on a fix, please set the status to Started.

Thanks for your time! To disable nags, add Disable-Nags (case sensitive) to the Chromium Labels custom field.

secondary security shepherd

kerenzhu@chromium.org: Could you please provide an update?

I observe the focus is moved away from the document to the chrome UI. This result in "TAB ENTER ENTER" to click on some buttons of the omnibox. Locally this wasn't clicking on accepting the localisation button. It might have changed. Still, we probably don't want document to force focus to be moved away.

Still, we probably don't want document to force focus to be moved away.

Preventing focus changes initiated by the document could regress the user experience. A common case is when the location permission prompt intentionally takes focus to enable immediate keyboard interaction (e.g., dismissing the prompt with the Esc key). If we were to force the prompt to appear without focus, the user would need an extra step (clicking the prompt) to interact with it, which feels less intuitive.

The current implementation in extensions::WindowsCreateFunction::Run() already attempts to prevent the newly created unfocused window from occluding the existing one. It does so by reactivating the previously active browser window if it was active:

  if (focused) {
    new_window->window()->Show();
  } else {
    // The new window isn't supposed to be focused. Here, instead of showing an
    // unfocused window on top (possible on some operating systems), we show
    // the window and then bring the old focused window back on top.
    // We still use ShowInactive() (instead of doing a Show() followed
    // immediately by Deactivate()) because the process of showing the window is
    // somewhat asynchronous. This causes the immediate Deactivate() call to not
    // work.
    BrowserList* const browser_list = BrowserList::GetInstance();
    Browser* active_browser = browser_list->GetLastActive();
    bool reset_active = false;
    // Check if there's a currently-active window that should re-take focus.
    // NOTE: This browser *may* be from another profile. We don't access any
    // data from it.
    if (active_browser && active_browser->window()->IsActive())
      reset_active = true;
    new_window->window()->ShowInactive();
    // NOTE: It's possible that showing the new browser synchronously caused
    // the old one to close. Ensure it's still valid before activating it.
    if (reset_active && base::Contains(*browser_list, active_browser))
      active_browser->window()->Activate();
  }

The issue is that the activation test is unreliable. Specifically, active_browser->window()->IsActive() may return false when the permission prompt is open, because technically the activation is on the prompt window, not on the browser window.

The solution I proposed in 2022 was to add a IsTreeActive() to BaseWindow. This functions returns true if a window or any of its descendants is active. The idea is to re-activate the old browser window if the window tree was active. Then I encountered difficulty in writing tests due to issue 40062245, that there can be multiple active widgets at the same time. Technically this does not break the fix, but we wanted good coverage of framework-level API like IsTreeActive(), so this became a blocker.

alesandro@ suggested in code review that we may simply remove the active_browser->window()->IsActive() test. Back then, I was opposed to this idea because I think it may break user's input flow. For example, if a user is typing in another application (non-chrome), unconditionally activating the browser window would steal focus.

Nevertheless, I am going to give Alesandro's proposal a try. While it does make the window creation process slightly more intrusive, extensions already disrupt the user experience to some degree when creating new windows. We should perhaps hold the extensions responsible for choosing appropriate timings for window creation.

Shepherd and kerenzhu@: Thanks for taking a look again.

For future reference, the CL comment referenced in #comment54 is https://chromium-review.googlesource.com/c/chromium/src/+/3996184/comment/0f8664a9_0bb5d552/ (line 819 comment thread)

Project: chromium/src
Branch: main
Author: Keren Zhu <kerenzhu@chromium.org>
Link:      https://chromium-review.googlesource.com/6087847

extensions: fix an unfocused new window obscuring the old window

extensions: fix an unfocused new window obscuring the old window 
 
This CL unconditionally re-activate the last active browser window when 
opening a new unfocused browser window using chrome.windows.create with 
focused: false. 
 
Previously the code re-activates the old window only if the old window 
is active. This is unreliable because the old window could be inactive 
while one of its child window (e.g. permission bubble) is active. We want the old window to be at the front for this case. Otherwise, the new 
unfocused window will obscure the old window which still has the input 
focus. The malicious content in the new window can then misleads the 
user into interacting with the old window without their awareness. 
 
Bug: 40061026 
Change-Id: I29468b9429009bcbb07072b1b1c5ef4ec0953f46 
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6087847 
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com> 
Reviewed-by: Devlin Cronin <rdevlin.cronin@chromium.org> 
Commit-Queue: Keren Zhu <kerenzhu@chromium.org> 
Cr-Commit-Position: refs/heads/main@{#1397024}

Thanks for landing patch kerenzhu@, I'll verify once there's a snapshot build with patch available. (I checked with original reporter Vitor, but he's busy this week.)

Thank you alesandro@. I look forward to your verification.

Verified as fixed on 133.0.6905.0 Canary using PoC from #comment18 and 19.

Also checked that this still reproduces on 131.0.6778.205 Stable, to rule out that any other changes would have otherwise broken repro.

Thanks again for fixing!

For reporter credit of this bug, please use: Vitor Torres and Alesandro Ortiz

Congratulations Vitor and Alesandro! Thank you for your efforts and reporting this issue to us.

Thanks for the reward!

(For my future reference and public record, we split the reward 70/30, with most going to Vitor; p2p-vrp@ already processed the split request)

This bug has been closed for more than 14 weeks. Removing issue access restrictions.