Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[Android] Check for text to paste before trying to retrieve data from URI #48166

Merged
auto-submit merged 19 commits into from
Dec 1, 2023
Merged

[Android] Check for text to paste before trying to retrieve data from URI #48166

Changes from 5 commits
Commits
Show all changes
19 commits
Select commit Hold shift + click to select a range
edb08a5
Add fix
camsim99 Nov 17, 2023
5f69541
Merge remote-tracking branch 'upstream/main' into paste_bug
camsim99 Nov 17, 2023
35a71a0
Merge remote-tracking branch 'upstream/main' into paste_bug
camsim99 Nov 17, 2023
99bdb12
Re-add text check
camsim99 Nov 18, 2023
29bfca2
Formatting
camsim99 Nov 18, 2023
b2a514a
Add branching to clipboard logic
camsim99 Nov 20, 2023
f26c5b3
Working on tests
camsim99 Nov 20, 2023
165fc05
Merge remote-tracking branch 'upstream/main' into paste_bug
camsim99 Nov 20, 2023
3f84b32
modify code, add tests
camsim99 Nov 21, 2023
2df7f8c
Formatting
camsim99 Nov 21, 2023
bf8bf57
Merge remote-tracking branch 'upstream/main' into paste_bug
camsim99 Nov 21, 2023
a339df3
Reduce branching, reorganize tests
camsim99 Nov 21, 2023
d1343b1
Extract URI logic
camsim99 Nov 27, 2023
b7aa9d3
Reduce redundant setup in test
camsim99 Nov 27, 2023
72d5f30
Merge remote-tracking branch 'upstream/main' into paste_bug
camsim99 Nov 27, 2023
483ea4d
Undo overkill
camsim99 Nov 29, 2023
8d93d73
Merge remote-tracking branch 'upstream/main' into paste_bug
camsim99 Nov 29, 2023
190c9ad
fix merge
camsim99 Nov 30, 2023
b508241
Merge remote-tracking branch 'upstream/main' into paste_bug
camsim99 Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 14 additions & 2 deletions shell/platform/android/io/flutter/plugin/platform/PlatformPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,8 +517,20 @@ private CharSequence getClipboardData(PlatformChannel.ClipboardContentFormat for
if (clip == null) return null;
if (format == null || format == PlatformChannel.ClipboardContentFormat.PLAIN_TEXT) {
ClipData.Item item = clip.getItemAt(0);
if (item.getUri() != null)
activity.getContentResolver().openTypedAssetFileDescriptor(item.getUri(), "text/*", null);
// First, try getting clipboard as text.
CharSequence itemText = item.getText();
if (itemText == null) {
Copy link
Contributor

@reidbaker reidbaker Nov 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nonblocking nit: consider extracting uri logic into a method or methods.

Copy link
Contributor Author

@camsim99 camsim99 Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved logic trying to extract text from URI to a new method!

Copy link
Contributor Author

@camsim99 camsim99 Dec 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Undid this due to some recent changes here that actually made the new method more confusing.

// Clipboard does not contain text, so check whether or not we will be
// able to retrieve text from URI. FileNotFoundException will be thrown
// if not, and then null will be returned.
if (item.getUri() != null) {
Copy link
Contributor

@stuartmorgan-g stuartmorgan-g Nov 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even with the restructuring, shouldn't we be checking that the URI is non-null and also content:? We know from the docs that it's invalid to call openTypedAssetFileDescriptor and we know from the issue that calling this with at least file: will throw an exception that can potentially bring down the whole app.

Hopefully moving getText up will avoid that in most cases, but we've had a lot of weird plugin bugs over the years that are related to other apps sending us bad data (e.g., intent responses that have unexpected nulls), so it seems like we should have a defense-in-depth approach here to make sure that if someone sends us a file: URI with no text it won't throw.

Copy link
Contributor Author

@camsim99 camsim99 Nov 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that explicitly returning null in the case where the URI is null makes sense to make sure no weird errors come up like you're referencing.

In terms of adding a check for content: before calling openTypedAssetFileDescriptor, that makes sense as well; I initially assumed a FileNotFoundException would be thrown (which we catch) which does not seem to be the case (edit: I think this is the case for the text/* check actually).

I think it would also be helpful to add some logs to these new branches to make it clear why null is returned.

Copy link
Contributor Author

@camsim99 camsim99 Nov 27, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed these comments, would you be willing to give it a second look? @stuartmorgan

activity
.getContentResolver()
.openTypedAssetFileDescriptor(item.getUri(), "text/*", null);
}
}
// Safely return clipbaord item into text by returning itemText or text retrieved
// from its URI.
return item.coerceToText(activity);
}
} catch (SecurityException e) {
Expand Down