Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[GTK][WPE][a11y] Allow receiving event listener signals from the a11y bus #29052

Conversation

GeorgesStavracas
Copy link
Contributor

@GeorgesStavracas GeorgesStavracas commented May 24, 2024

e949df8

[GTK][WPE][a11y] Allow receiving event listener signals from the a11y bus
https://bugs.webkit.org/show_bug.cgi?id=240522

Reviewed by Michael Catanzaro.

When a process - usually an AT - registers event listeners, AT-SPI
broadcasts the EventListenerRegistered signal so that clients know
about it. The EventListenerDeregistered signal is broadcasted when
event listeners are removed.

The web process monitors for these signal so that it can reduce the
number of signal emissions on the a11y bus. Now consider that the web
process runs in a sandbox (most of the time anyway), either by using
Bubblewrap directly, or flatpak-spawn. These sandboxing mechanisms
filter D-Bus messages and signals, including on the a11y bus. And
here's the problem: both of them end up preventing the web process to
receive the event listener signals!

As a consequence, the web process never learns when the user turns
on or off the screen reader, or auxiliary accessibility programs.
And without that knowledge, the web process doesn't emit any event
necessary for proper accessibility.

Allow the Bubblewrap sandbox to receive the EventListenerRegistered
and EventListenerDeregistered signals from org.a11y.atspi.Registry.
This is safe, as there is insufficient information in these signals
for any malicious usage, and the bus name in the messages are already
from apps that purposefully advertise themselves.

A similar patch is proposed for Flatpak, and there's nothing to be
done from WebKit side to influence that.

* Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp:
(WebKit::XDGDBusProxy::accessibilityProxy):

Canonical link: https://commits.webkit.org/280770@main

bade5fa

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2 βœ… πŸ§ͺ wincairo-tests
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt βœ… πŸ›  wpe-cairo
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
⏳ πŸ›  vision βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
⏳ πŸ›  vision-sim βœ… πŸ§ͺ mac-wk2-stress βœ… πŸ§ͺ api-gtk
βœ… πŸ›  πŸ§ͺ merge ⏳ πŸ§ͺ vision-wk2
βœ… πŸ›  tv
βœ… πŸ›  tv-sim
βœ… πŸ›  watch
βœ… πŸ›  watch-sim

@GeorgesStavracas GeorgesStavracas requested a review from a team as a code owner May 24, 2024 14:40
@GeorgesStavracas GeorgesStavracas self-assigned this May 24, 2024
@GeorgesStavracas GeorgesStavracas added the Accessibility For bugs related to accessibility. label May 24, 2024
@GeorgesStavracas
Copy link
Contributor Author

I'm confident that neither one of these signals constitute nor can be used for a sandbox escape nor information leak:

signal time=1716560552.341104 sender=:1.1 -> destination=(null destination) serial=13197 path=/org/a11y/atspi/registry; interface=org.a11y.atspi.Registry; member=EventListenerRegistered
   string ":1.164"
   string "Object:StateChanged:Focused"
   array [
   ]

The bus name in the first argument cannot be talked to from sandboxed apps, unless they have full access to the a11y socket (in which case, they don't need to use this signal to cause any harm). The event listener string is harmless, and so is the params array.

@GeorgesStavracas GeorgesStavracas marked this pull request as draft May 24, 2024 14:58
@GeorgesStavracas
Copy link
Contributor Author

Marking as a draft because, contrary to my assumption, xdg-dbus-proxy demands a TALK permission to org.a11y.atspi.Registry to broadcast these signals...

@GeorgesStavracas GeorgesStavracas marked this pull request as ready for review May 28, 2024 13:14
@GeorgesStavracas
Copy link
Contributor Author

This PR will work after flatpak/xdg-dbus-proxy#61 is merged. Technically, flatpak/flatpak#5828 would also make the flatpak-spawn variant work, but there are other bugs in Flatpak that prevent a11y from working. We'll probably need to coordinate with downstreams, and potentially even backport the Flatpak PR to a stable release.

As for this PR, it is safe to land it as is. It won't work until flatpak/xdg-dbus-proxy#61 lands, but nothing will break or behave differently until then.

@GeorgesStavracas
Copy link
Contributor Author

Both flatpak/xdg-dbus-proxy#61 and flatpak/flatpak#5828 were merged.

@GeorgesStavracas GeorgesStavracas added the merge-queue Applied to send a pull request to merge-queue label Jul 9, 2024
… bus

https://bugs.webkit.org/show_bug.cgi?id=240522

Reviewed by Michael Catanzaro.

When a process - usually an AT - registers event listeners, AT-SPI
broadcasts the EventListenerRegistered signal so that clients know
about it. The EventListenerDeregistered signal is broadcasted when
event listeners are removed.

The web process monitors for these signal so that it can reduce the
number of signal emissions on the a11y bus. Now consider that the web
process runs in a sandbox (most of the time anyway), either by using
Bubblewrap directly, or flatpak-spawn. These sandboxing mechanisms
filter D-Bus messages and signals, including on the a11y bus. And
here's the problem: both of them end up preventing the web process to
receive the event listener signals!

As a consequence, the web process never learns when the user turns
on or off the screen reader, or auxiliary accessibility programs.
And without that knowledge, the web process doesn't emit any event
necessary for proper accessibility.

Allow the Bubblewrap sandbox to receive the EventListenerRegistered
and EventListenerDeregistered signals from org.a11y.atspi.Registry.
This is safe, as there is insufficient information in these signals
for any malicious usage, and the bus name in the messages are already
from apps that purposefully advertise themselves.

A similar patch is proposed for Flatpak, and there's nothing to be
done from WebKit side to influence that.

* Source/WebKit/UIProcess/Launcher/glib/XDGDBusProxy.cpp:
(WebKit::XDGDBusProxy::accessibilityProxy):

Canonical link: https://commits.webkit.org/280770@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/GTK-AX-Accessibility-events-missing-for-browser-tabs-contents-when-nothing-is-listening-when-page-loads branch from bade5fa to e949df8 Compare July 9, 2024 12:42
@webkit-commit-queue
Copy link
Collaborator

Committed 280770@main (e949df8): https://commits.webkit.org/280770@main

Reviewed commits have been landed. Closing PR #29052 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit e949df8 into WebKit:main Jul 9, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 9, 2024
@GeorgesStavracas GeorgesStavracas deleted the eng/GTK-AX-Accessibility-events-missing-for-browser-tabs-contents-when-nothing-is-listening-when-page-loads branch July 9, 2024 12:42
@mcatanzaro mcatanzaro added the GLib Suggested Backport Suggest this merge request be backported to current WPE/GTK stable branch label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Accessibility For bugs related to accessibility. GLib Suggested Backport Suggest this merge request be backported to current WPE/GTK stable branch
Projects
None yet
4 participants