1912340 - In extensions, dataTransfer.getData returns empty String instead of data when dragging across windows

Status: RESOLVED FIXED

(Core :: DOM: Copy & Paste and Drag & Drop, defect)

Description

[bobo1239]

Attachment: Patch for Sidebery which adds some relevant logging.

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:130.0) Gecko/20100101 Firefox/130.0

Steps to reproduce:

• Install the Sidebery extension (https://addons.mozilla.org/en-US/firefox/addon/sidebery/)
• Create a 2nd window and open the Sidebery sidebar on both windows.
• Drag a tab from the one window's sidebar to the other window's sidebar.

Actual results:

The tab is cloned to the 2nd window.

Expected results:

The tab should be moved to the 2nd window. (removed from the original window)

Comment:

Note that this is a different bug than Bug 1911486. Firefox 128 behaves correctly but neither Firefox 129 nor a freshly built Firefox with https://phabricator.services.mozilla.com/D218667 applied (which fixes Bug 1911486) do. The relevant line in Sidebery is this one. The returned string from .getData('application/x-sidebery-dnd') is an empty string instead of something like {"type":1,"items":[{"id":2,"url":"about:newtab","title":"New Tab","parentId":-1,"container":"firefox-default"}],"windowId":36,"incognito":false,"panelId":"9tquD7XMOskZ","pinnedTabs":false,"x":58,"y":19} as when using Firefox 128. (The data is set here for completeness sake.)

The strange thing is that the correct string is returned in onDrop() and onDragEnd(). Just in onDragEnter() it is the empty string. I'm attaching a patch for Sidebery which adds some relevant logging in case that's helpful. (See here for dev instructions.) Remember to apply the fix for Bug 1911486 first.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

:handyman, since you are the author of the regressor, bug 1893119, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

[David Parks [:handyman]]

I can reproduce this as well, thanks. It looks like the it is in part a dual to the other issue, but there seems to be more to this than just that.

Comment 3

[David Parks [:handyman]]

Attachment: Bug 1912340: Use correct principal for remotely dragged data-transfer contents r=m_kato!

This comment assumes dom.events.datatransfer.protected.enabled=true. when
dom.events.datatransfer.protected.enabled=false (which matches the spec and is
not the default), the behavior is fine as-is.

Since bug 1871222 (by one perspective) moved drag sessions from PContents to
PBrowsers, we need to update permission handling for in-process "remote"
sessions (ones that started on one PBrowser and currently target another). When
the target and source are in the same process, they don't properly handle
privileged DataTransfer access. We are currently denying privileged access to
some in-process principals because we incorrectly use the system principal. The
system principal always happens to give the right result for out-of-process
remote sessions, which were the only kind that existed before bug 1871222.

This solution creates the "remote" DataTransfer with the correct (not system)
principal. However, this misses the case where two different browsers are at
the same location (same principal) -- each would incorrectly get privileged
access to the other's DataTransfer in a drag. We avoid that case by tracking
if the DataTransfer came from a drag that started outside of the target browser
and forbidding access to content (but not e.g. extended) principals, regardless
of subsumption.

Comment 4

[David Parks [:handyman]]

Attachment: Bug 1912340: Test DataTransfer access in synthesizeMockDragAndDrop r=m_kato!

Verify that DataTransfer is properly granted/restricted during DND operations
between different browsers.

Comment 5

[David Parks [:handyman]]

Attachment: Bug 1912340: Run drag-drop test with dom.events.dataTransfer.protected.enabled true and false r=m_kato!

This patch is separated from the previous to ease reading -- the code changes in
this patch are minor so the file split is mostly transparent. We also run the
test for dom.events.dataTransfer.protected.enabled = true. The test is split
into two to avoid timeouts.

Comment 6

[bobo1239]

Thank you for your work so far already. Just some quick comments:

• In your first patch you mention that dom.events.datatransfer.protected.enabled=false is not the default but it actually seems to be? (both nightly and stable) Unless I'm missing something which is quite possible...
• Are the current patches already supposed to fix the sidebery issue? Afaict I'm still observing it with a fresh Firefox build. (the issue from Bug 1911486 is resolved though)

Comment 7

[David Parks [:handyman]]

(In reply to bobo1239 from comment #6)

Thank you for your work so far already. Just some quick comments:

• In your first patch you mention that dom.events.datatransfer.protected.enabled=false is not the default but it actually seems to be? (both nightly and stable) Unless I'm missing something which is quite possible...

Thanks for catching that -- it was a typo.

• Are the current patches already supposed to fix the sidebery issue? Afaict I'm still observing it with a fresh Firefox build. (the issue from Bug 1911486 is resolved though)

Yeah, I posted this prematurely. It seems the extension is requesting the data under a content principal instead of an extended one.

Comment 8

[David Parks [:handyman]]

Attachment: Bug 1912340: Further split up browser_dragdrop_[un]protected tests r=m_kato!

No substantive changes. This avoids timeouts in automation.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6f49e7d3bbf
Use correct principal for remotely dragged data-transfer contents r=m_kato
https://hg.mozilla.org/integration/autoland/rev/17fa2e5dccdb
Test DataTransfer access in synthesizeMockDragAndDrop r=m_kato
https://hg.mozilla.org/integration/autoland/rev/238109e9e436
Run drag-drop test with dom.events.dataTransfer.protected.enabled true and false r=m_kato
https://hg.mozilla.org/integration/autoland/rev/aa243ae27748
Further split up browser_dragdrop_[un]protected tests r=m_kato

Comment 10

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/a6f49e7d3bbf
https://hg.mozilla.org/mozilla-central/rev/17fa2e5dccdb
https://hg.mozilla.org/mozilla-central/rev/238109e9e436
https://hg.mozilla.org/mozilla-central/rev/aa243ae27748

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:handyman, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox130 to wontfix.

For more information, please visit BugBot documentation.

Comment 12

[David Parks [:handyman]]

Comment on attachment 9418895 [details]
Bug 1912340: Use correct principal for remotely dragged data-transfer contents r=m_kato!

Beta/Release Uplift Approval Request

• User impact if declined: Some broken drag-drop behavior in extensions. The bug was that extensions could not access some data that they previously could, when dragging from one window to another.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This change should only affect the extension process. It's also contained -- all but one of the patches are test-only.
• String changes made/needed: N/A
• Is Android affected?: No

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9418895 [details]
Bug 1912340: Use correct principal for remotely dragged data-transfer contents r=m_kato!

Approved for 130.0rc1. Thanks for all the added test coverage.

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/67af105e5502
https://hg.mozilla.org/releases/mozilla-beta/rev/6ead1f4e8247
https://hg.mozilla.org/releases/mozilla-beta/rev/3e929b1f31f8
https://hg.mozilla.org/releases/mozilla-beta/rev/f087ce7110d8

Comment 15

[Greg Stoll :gstoll]

Attachment: Bug 1912340: Use correct principal for remotely dragged data-transfer contents

This comment assumes dom.events.datatransfer.protected.enabled=false. when
dom.events.datatransfer.protected.enabled=true (which matches the spec and is
not the default), the behavior is fine as-is.

Since bug 1871222 (by one perspective) moved drag sessions from PContents to
PBrowsers, we need to update permission handling for in-process "remote"
sessions (ones that started on one PBrowser and currently target another). When
the target and source are in the same process, they don't properly handle
privileged DataTransfer access. We are currently denying privileged access to
some in-process principals because we incorrectly use the system principal. The
system principal always happens to give the right result for out-of-process
remote sessions, which were the only kind that existed before bug 1871222.

This solution creates the "remote" DataTransfer with the "correct" (not system)
principal of its source content. However, this misses the case where two
different browsers are at the same location (same principal) -- each would
incorrectly get privileged access to the other's DataTransfer in a drag. We
avoid that case by limiting the change to extension processes. This means that
remote DataTransfers created in the extension process will check for protected
access using a principal check against the drag-source-element's principal but,
otherwise, DataTransfers will continue to use the system principal for the
check.

Original Revision: https://phabricator.services.mozilla.com/D219074

Comment 16

[Greg Stoll :gstoll]

Attachment: Bug 1912340: Test DataTransfer access in synthesizeMockDragAndDrop

Verify that DataTransfer is properly granted/restricted during DND operations
between different browsers.

Original Revision: https://phabricator.services.mozilla.com/D219075

Comment 17

[Greg Stoll :gstoll]

Attachment: Bug 1912340: Run drag-drop test with dom.events.dataTransfer.protected.enabled true and false

This patch is separated from the previous to ease reading -- the code changes in
this patch are minor so the file split is mostly transparent. We also run the
test for dom.events.dataTransfer.protected.enabled = true. The test is split
into two to avoid timeouts.

Original Revision: https://phabricator.services.mozilla.com/D219076

Comment 18

[Greg Stoll :gstoll]

Attachment: Bug 1912340: Further split up browser_dragdrop_[un]protected tests

No substantive changes. This avoids timeouts in automation.

Original Revision: https://phabricator.services.mozilla.com/D219548

Comment 19

[Greg Stoll :gstoll]

esr128 Uplift Approval Request

• User impact if declined: breaks drag and drop in some extensions
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: n/a
• Risk associated with taking this patch: low
• Explanation of risk level: small change to principal passed for drag and drop
• String changes made/needed: no
• Is Android affected?: no

Comment 20

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/0e8678acd81f
https://hg.mozilla.org/releases/mozilla-esr128/rev/8d1c74665a7d
https://hg.mozilla.org/releases/mozilla-esr128/rev/fa02775f2ec7
https://hg.mozilla.org/releases/mozilla-esr128/rev/e0514f56702c