1915351 - Firefox DLP scans the pasted content multiple times when copied from Google/OneDrive Documents

Status: VERIFIED FIXED

(Firefox :: Data Loss Prevention, defect)

Description

[Bianca Hidecuti, Desktop Test Engineering [:bhidecuti]]

Attachment: scanning multiple times

Found in

• 131.0a1 (2024-08-27)

Affected versions

• 131.0a1

Preconditions

• Download the DLP test assets from https://drive.google.com/file/d/1yjqVRuxdKV3WnO7D2wzMgDXBuYBxUgVw/view
• Create a distribution folder inside the Firefox folder and paste the policies-1.json to it and then rename it to policies.json
• Run the DLP agent in CMD using: .\content_analysis_sdk_agent.exe --user --toblock=.\d{3}-?\d{2}-?\d{4}. --towarn=.warn. --delays=10

Tested platforms

• Affected platforms: Windows 10/11
• Unaffected platforms: Ubuntu, macOS

Steps to reproduce

1. Go to https://drive.google.com/drive/home and sign in
2. Open up a Google Doc and type “ok text” into it and copy it (or any random text)
3. Open a second Google Doc or navigate to a random website (e.g. Wikipedia)
4. Paste the copied text from step 2
5. Observe the behavior

Expected result

• The pasted content is scanned only once

Actual result

• The pasted content is scanned 2-3 times consecutively on website or even 4-5 times on Google Docs

Regression range

• Potentially regressed by 1912384, but I am not 100% sure, since I cannot determine the regression range using Mozregression

Additional notes

• See the attached video
• Also reproducing if pasting a selected text into Google Docs / OndeDrive docs from a random website (the content is scanned twice)
• Also reproducing for the blocked DLP data (e.g. ‘123456789’) / warning DLP data (‘warning text’) - the content is scanned multiple times
• Not reproducing if pasting from Notepad

Comment 1

[Greg Stoll :gstoll]

The problem is that when copying from Google Docs it stores the text in at least three different formats - an custom one, text/html, and text/plain, so if another website is trying to get data from it in multiple formats this will require multiple DLP checks.

Some ideas we had about this:

• perhaps when someone gets one format from the clipboard Firefox could get all the formats and pass them all to DLP, then cache the results for future calls. This has the advantage of only popping up one dialog, but it means we might be doing extra unneeded work, and it won't make things any faster even in the case where the cache is used.

We're probably going to have to live with this for now.

Comment 2

[Greg Stoll :gstoll]

Implementing bug 1915653 would at least reduce this to around 3 calls, as opposed to the 4 or 5 there are now.

Comment 6

[Greg Stoll :gstoll]

I'm going to take another look at this and try to scan all the clipboard contents at once instead of one at a time on demand.

Comment 7

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 1 - make GetNativeClipboardSequenceNumber public r=#geckoview-reviewers,#win-reviewers,handyman,m_kato

This is used in later patches to determine whether we have run
content analysis on the current clipboard contents.

Comment 8

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 2 - make Content Analysis able to analyze all clipboard contents r=#dlp-reviewers,#win-reviewers,handyman

• Make CheckClipboardContentAnalysis() analyze all flavors of data in the
  passed-in transferable, and wrap all the individual requests in an
  AggregatedContentAnalysisResultCallback that fires when all the
  individual requests are done (or early if there's an error or something
  is blocked)
• If the full clipboard contents are analyzed, cache the result so future
  calls to analyze individual entries can get returned immediately.

Comment 9

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 3 - add entry point to get all clipboard to actor r=#dlp-reviewers,#ipc-reviewers,nika,edgar,handyman

Add an entry point to ClipboardContentAnalysisParent to analyze all
the clipboard contents.

Comment 10

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 4 - analyze all clipboard contents before sending out paste event r=#dlp-reviewers,handyman

Comment 11

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 5 - add ability to set promptID on a dialog r=Gijs

This will be used in future patches to identify which dialog to close
when a content analysis request is done.

Comment 12

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 6 - make Content Analysis JS display multiple dialogs seamlessly r=#dlp-reviewers,handyman

This uses the already-existing ability of the TabDialogManager to show
a dialog immediately after the previous one closes. So this code doesn't
need to keep track of what dialog to show next - it just displays them
all (and closes them appropriately), and the TabDialogManager will show
them one at a time.

Comment 13

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 7 - remove old Content Analysis cache r=#dlp-reviewers,handyman

The cache introduced in a previous patch caches all clipboard contents
in a more efficient way, so we no longer need the band-aid cache that
only cached one MIME type at a time.

Comment 14

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 8 - show BLOCK dialog when returning result from cache r=#dlp-reviewers,handyman

If content analysis is returning a cached BLOCK result for the full clipboard,
we should show the BLOCK dialog, so create a fake request and response
and notify based on those.

Comment 18

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

Copying crash signatures from duplicate bugs.

Comment 19

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 9 - fix up existing tests for new behavior r=#dlp-reviewers,handyman

The changes in the rest of this stack change the order that we do CA
in, and also eliminate some redundant calls, so fix up the tests to
reflect this.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a8b2b1f62ae6
part 1 - make GetNativeClipboardSequenceNumber public r=geckoview-reviewers,win-reviewers,handyman,m_kato
https://hg.mozilla.org/integration/autoland/rev/2b900eb520ce
part 2 - make Content Analysis able to analyze all clipboard contents r=dlp-reviewers,win-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/a20adf8d4516
part 3 - add entry point to get all clipboard to actor r=dlp-reviewers,ipc-reviewers,nika,edgar,handyman
https://hg.mozilla.org/integration/autoland/rev/c5279472391f
part 4 - analyze all clipboard contents before sending out paste event r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/fba6942240ea
part 5 - add ability to set promptID on a dialog r=Gijs
https://hg.mozilla.org/integration/autoland/rev/1cf3847d73a7
part 6 - make Content Analysis JS display multiple dialogs seamlessly r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/aa0e2ce8cf21
part 7 - remove old Content Analysis cache r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/4fcb7eea243f
part 8 - show BLOCK dialog when returning result from cache r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/574d567bbbf1
part 9 - fix up existing tests for new behavior r=dlp-reviewers,handyman

Comment 21

[agoloman]

https://hg.mozilla.org/mozilla-central/rev/a8b2b1f62ae6
https://hg.mozilla.org/mozilla-central/rev/2b900eb520ce
https://hg.mozilla.org/mozilla-central/rev/a20adf8d4516
https://hg.mozilla.org/mozilla-central/rev/c5279472391f
https://hg.mozilla.org/mozilla-central/rev/fba6942240ea
https://hg.mozilla.org/mozilla-central/rev/1cf3847d73a7
https://hg.mozilla.org/mozilla-central/rev/aa0e2ce8cf21
https://hg.mozilla.org/mozilla-central/rev/4fcb7eea243f
https://hg.mozilla.org/mozilla-central/rev/574d567bbbf1

Comment 23

[amarc]

Backed out 9 changesets (Bug 1915351) for causing multiple regressions.

• Backout link

Comment 24

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Attachment: 2024-10-25 16-53-55.mkv

Hello! We looked over this today but it seems that the following issues are still reproducing with this patch (screen-recording attached). Leaving some notes here for future reference:

• Pasting from gdoc to another gdoc will scan the content two times (including showing the warning or blocked dialog for two times)
• Pasting from another webpage inside gdoc will scan the content two times (including showing the warning or blocked dialog for two times)
• Sometimes pasting from Gdoc to Gdoc will have an increased time and the content is scanned multiple times, showing the warning dialog if the string is test. (access this link and copy-paste the second test string inside gdoc - see the first issue in the screen recording)
• Additionally, now the content is also scanned twice when pasting it inside Gdoc from an external app (e.g. Notepad). This was not the case before the fix.

We also filled bug 1927129 and bug 1927131 with this patch landed.

Comment 25

[Norisz Fay [:noriszfay]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/19214f0d561f

Comment 26

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 10 - make copy image test more reliable r=#dlp-reviewers

This part of the test fails often on try. I'm not sure what's going on
(sometimes a file gets put on the clipboard as well), but the point of
this test is to test content analysis, not Copy Image Contents, so
just set the clipboard contents directly.

I get passing test-verify builds with this change.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86f4e754ee72
part 1 - make GetNativeClipboardSequenceNumber public r=geckoview-reviewers,win-reviewers,handyman,m_kato
https://hg.mozilla.org/integration/autoland/rev/a33c055119dc
part 2 - make Content Analysis able to analyze all clipboard contents r=dlp-reviewers,win-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/7b3dcb65cb10
part 3 - add entry point to get all clipboard to actor r=dlp-reviewers,ipc-reviewers,nika,edgar,handyman
https://hg.mozilla.org/integration/autoland/rev/b145e536fe64
part 4 - analyze all clipboard contents before sending out paste event r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/e729a9c40766
part 5 - add ability to set promptID on a dialog r=Gijs
https://hg.mozilla.org/integration/autoland/rev/0650e8d79222
part 6 - make Content Analysis JS display multiple dialogs seamlessly r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/7f1779828088
part 7 - remove old Content Analysis cache r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/385fac4e9fc2
part 8 - show BLOCK dialog when returning result from cache r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/800572858d69
part 9 - fix up existing tests for new behavior r=dlp-reviewers,handyman
https://hg.mozilla.org/integration/autoland/rev/d6db8b2ff6eb
part 10 - make copy image test more reliable r=dlp-reviewers,handyman

Comment 28

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/86f4e754ee72
https://hg.mozilla.org/mozilla-central/rev/a33c055119dc
https://hg.mozilla.org/mozilla-central/rev/7b3dcb65cb10
https://hg.mozilla.org/mozilla-central/rev/b145e536fe64
https://hg.mozilla.org/mozilla-central/rev/e729a9c40766
https://hg.mozilla.org/mozilla-central/rev/0650e8d79222
https://hg.mozilla.org/mozilla-central/rev/7f1779828088
https://hg.mozilla.org/mozilla-central/rev/385fac4e9fc2
https://hg.mozilla.org/mozilla-central/rev/800572858d69
https://hg.mozilla.org/mozilla-central/rev/d6db8b2ff6eb

Comment 29

[Bianca Hidecuti, Desktop Test Engineering [:bhidecuti]]

Verified as fixed using Firefox Nightly 134.0a1 (2024-10-29), on Windows 10/11. The content is no longer scanned 4-5 times on Google Docs.
However, the content is still scanned twice when pasting to Gdoc/OneDrive doc from another Gdoc/webpage/external app.
This will be addressed in bug 1927776.

Comment 30

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 1 - make GetNativeClipboardSequenceNumber public

This is used in later patches to determine whether we have run
content analysis on the current clipboard contents.

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

Comment 31

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 2 - make Content Analysis able to analyze all clipboard contents

• Make CheckClipboardContentAnalysis() analyze all flavors of data in the
  passed-in transferable, and wrap all the individual requests in an
  AggregatedClipboardContentAnalysisResultCallback that fires when all the
  individual requests are done (or early if there's an error or something
  is blocked)
• If the full clipboard contents are analyzed, cache the result so future
  calls to analyze individual entries can get returned immediately.

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

Comment 32

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 3 - add entry point to get all clipboard to actor

Add an entry point to ClipboardContentAnalysisParent to get all the
data on the clipboard, which also analyzes all the clipboard contents.

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

Comment 33

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 4 - analyze all clipboard contents before sending out paste event

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

Comment 34

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 5 - add ability to set promptID on a dialog

This will be used in future patches to identify which dialog to close
when a content analysis request is done.

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

Comment 35

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 6 - make Content Analysis JS display multiple dialogs seamlessly

This uses the already-existing ability of the TabDialogManager to show
a dialog immediately after the previous one closes. So this code doesn't
need to keep track of what dialog to show next - it just displays them
all (and closes them appropriately), and the TabDialogManager will show
them one at a time.

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

Comment 36

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 7 - remove old Content Analysis cache

The cache introduced in a previous patch caches all clipboard contents
in a more efficient way, so we no longer need the band-aid cache that
only cached one MIME type at a time.

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

Comment 37

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 8 - show BLOCK dialog when returning result from cache

If content analysis is returning a cached BLOCK result for the full clipboard,
we should show the BLOCK dialog, so create a fake request and response
and notify based on those.

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

Comment 38

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 9 - fix up existing tests for new behavior

The changes in the rest of this stack change the order that we do CA
in, and also eliminate some redundant calls, so fix up the tests to
reflect this.

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

Comment 39

[Greg Stoll :gstoll]

Attachment: Bug 1915351 part 10 - make copy image test more reliable

This part of the test fails often on try. I'm not sure what's going on
(sometimes a file gets put on the clipboard as well), but the point of
this test is to test content analysis, not Copy Image Contents, so
just set the clipboard contents directly.

I get passing test-verify builds with this change.

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

Comment 40

[Phabricator Automation]

esr128 Uplift Approval Request

• User impact if declined: DLP users will experience multiple annoying scans when pasting into Google Docs, etc.
• 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: medium
• Explanation of risk level: changes a little bit of clipboard behavior
• String changes made/needed: no
• Is Android affected?: no