From f76f9be4edb35473671e85f54cc1db7f647c8a5e Mon Sep 17 00:00:00 2001 From: Torsten Grote Date: Mon, 18 Feb 2019 15:05:04 -0300 Subject: [PATCH] Reject attachments that exceed the allowed size Closes #1468 --- .../conversation/AttachmentResult.java | 43 +++++++++++++++++++ .../conversation/ConversationViewModel.java | 20 +++++++-- .../briar/android/view/ImagePreviewItem.java | 2 +- .../view/TextAttachmentController.java | 38 +++++++++++----- briar-android/src/main/res/values/strings.xml | 1 + .../api/messaging/FileTooBigException.java | 6 +++ .../api/messaging/MessagingConstants.java | 3 +- .../briar/api/messaging/MessagingManager.java | 2 + .../briar/messaging/MessagingManagerImpl.java | 40 +++++++++++------ 9 files changed, 125 insertions(+), 30 deletions(-) create mode 100644 briar-android/src/main/java/org/briarproject/briar/android/conversation/AttachmentResult.java create mode 100644 briar-api/src/main/java/org/briarproject/briar/api/messaging/FileTooBigException.java diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/AttachmentResult.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/AttachmentResult.java new file mode 100644 index 000000000..926d32fab --- /dev/null +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/AttachmentResult.java @@ -0,0 +1,43 @@ +package org.briarproject.briar.android.conversation; + +import android.net.Uri; + +import org.briarproject.bramble.api.nullsafety.NotNullByDefault; + +import javax.annotation.Nullable; +import javax.annotation.concurrent.Immutable; + +@Immutable +@NotNullByDefault +public class AttachmentResult { + + @Nullable + private final Uri uri; + @Nullable + private final String errorMsg; + + public AttachmentResult(Uri uri) { + this.uri = uri; + this.errorMsg = null; + } + + public AttachmentResult(@Nullable String errorMsg) { + this.uri = null; + this.errorMsg = errorMsg; + } + + @Nullable + public Uri getUri() { + return uri; + } + + public boolean isError() { + return errorMsg != null; + } + + @Nullable + public String getErrorMsg() { + return errorMsg; + } + +} diff --git a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java index b428ee901..304933943 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/conversation/ConversationViewModel.java @@ -27,11 +27,13 @@ import org.briarproject.bramble.api.sync.GroupId; import org.briarproject.bramble.api.sync.Message; import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.bramble.api.system.AndroidExecutor; +import org.briarproject.briar.R; import org.briarproject.briar.android.util.UiUtils; import org.briarproject.briar.android.view.TextAttachmentController.AttachmentManager; import org.briarproject.briar.android.viewmodel.LiveEvent; import org.briarproject.briar.android.viewmodel.MutableLiveEvent; import org.briarproject.briar.api.messaging.AttachmentHeader; +import org.briarproject.briar.api.messaging.FileTooBigException; import org.briarproject.briar.api.messaging.MessagingManager; import org.briarproject.briar.api.messaging.PrivateMessage; import org.briarproject.briar.api.messaging.PrivateMessageFactory; @@ -54,6 +56,7 @@ import static org.briarproject.bramble.util.LogUtils.now; import static org.briarproject.briar.android.conversation.AttachmentDimensions.getAttachmentDimensions; import static org.briarproject.briar.android.settings.SettingsFragment.SETTINGS_NAMESPACE; import static org.briarproject.briar.android.util.UiUtils.observeForeverOnce; +import static org.briarproject.briar.api.messaging.MessagingConstants.MAX_IMAGE_SIZE; @NotNullByDefault public class ConversationViewModel extends AndroidViewModel implements @@ -192,9 +195,11 @@ public class ConversationViewModel extends AndroidViewModel implements } @Override - public void storeAttachment(Uri uri, boolean needsSize, Runnable onSuccess, - Runnable onError) { + public LiveData storeAttachment(Uri uri, + boolean needsSize) { if (messagingGroupId.getValue() == null) loadGroupId(); + // use LiveData to not keep references to view scope + MutableLiveData result = new MutableLiveData<>(); observeForeverOnce(messagingGroupId, groupId -> dbExecutor.execute(() -> { if (groupId == null) throw new IllegalStateException(); @@ -204,13 +209,20 @@ public class ConversationViewModel extends AndroidViewModel implements getApplication().getContentResolver(); attachmentController.createAttachmentHeader(contentResolver, groupId, uri, needsSize); - androidExecutor.runOnUiThread(onSuccess); + result.postValue(new AttachmentResult(uri)); + } catch(FileTooBigException e) { + logException(LOG, WARNING, e); + int mb = MAX_IMAGE_SIZE / 1024 / 1024; + String errorMsg = getApplication() + .getString(R.string.image_attach_error_too_big, mb); + result.postValue(new AttachmentResult(errorMsg)); } catch (DbException | IOException e) { logException(LOG, WARNING, e); - androidExecutor.runOnUiThread(onError); + result.postValue(new AttachmentResult((String) null)); } logDuration(LOG, "Storing attachment", start); })); + return result; } @Override diff --git a/briar-android/src/main/java/org/briarproject/briar/android/view/ImagePreviewItem.java b/briar-android/src/main/java/org/briarproject/briar/android/view/ImagePreviewItem.java index 71576015f..bf1ba71fb 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/view/ImagePreviewItem.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/view/ImagePreviewItem.java @@ -15,7 +15,7 @@ class ImagePreviewItem { private final Uri uri; private boolean waitForLoading = true; - private ImagePreviewItem(Uri uri) { + ImagePreviewItem(Uri uri) { this.uri = uri; } diff --git a/briar-android/src/main/java/org/briarproject/briar/android/view/TextAttachmentController.java b/briar-android/src/main/java/org/briarproject/briar/android/view/TextAttachmentController.java index 0cc1cb32e..db8723caf 100644 --- a/briar-android/src/main/java/org/briarproject/briar/android/view/TextAttachmentController.java +++ b/briar-android/src/main/java/org/briarproject/briar/android/view/TextAttachmentController.java @@ -1,6 +1,8 @@ package org.briarproject.briar.android.view; import android.app.Activity; +import android.arch.lifecycle.LifecycleOwner; +import android.arch.lifecycle.LiveData; import android.content.ClipData; import android.content.Context; import android.content.Intent; @@ -15,6 +17,7 @@ import android.widget.Toast; import org.briarproject.bramble.api.nullsafety.NotNullByDefault; import org.briarproject.briar.R; +import org.briarproject.briar.android.conversation.AttachmentResult; import org.briarproject.briar.android.view.ImagePreview.ImagePreviewListener; import org.briarproject.briar.api.messaging.AttachmentHeader; @@ -149,9 +152,17 @@ public class TextAttachmentController extends TextSendController // store attachments and show preview when successful boolean needsSize = items.size() == 1; for (ImagePreviewItem item : items) { - attachmentManager.storeAttachment(item.getUri(), needsSize, - () -> imagePreview.loadPreviewImage(item), - this::onError); + attachmentManager.storeAttachment(item.getUri(), needsSize) + .observe(imageListener, this::onAttachmentResultReceived); + } + } + + private void onAttachmentResultReceived(AttachmentResult result) { + if (result.isError() || result.getUri() == null) { + onError(result.getErrorMsg()); + } else { + ImagePreviewItem item = new ImagePreviewItem(result.getUri()); + imagePreview.loadPreviewImage(item); } } @@ -194,8 +205,16 @@ public class TextAttachmentController extends TextSendController @Override public void onError() { - Toast.makeText(textInput.getContext(), R.string.image_attach_error, - LENGTH_LONG).show(); + onError(null); + } + + @UiThread + private void onError(@Nullable String errorMsg) { + if (errorMsg == null) { + errorMsg = imagePreview.getContext() + .getString(R.string.image_attach_error); + } + Toast.makeText(textInput.getContext(), errorMsg, LENGTH_LONG).show(); onCancel(); } @@ -268,7 +287,7 @@ public class TextAttachmentController extends TextSendController }; } - public interface AttachImageListener { + public interface AttachImageListener extends LifecycleOwner { void onAttachImage(Intent intent); } @@ -277,11 +296,10 @@ public class TextAttachmentController extends TextSendController * Stores a new attachment in the database. * * @param uri The Uri of the attachment to store. - * @param onSuccess will be run on the UiThread when the attachment was stored successfully. - * @param onError will be run on the UiThread when the attachment could not be stored. + * @param needsSize true if this is the only image in the message + * and therefore needs to know its size. */ - void storeAttachment(Uri uri, boolean needsSize, Runnable onSuccess, - Runnable onError); + LiveData storeAttachment(Uri uri, boolean needsSize); List getAttachmentHeaders(); diff --git a/briar-android/src/main/res/values/strings.xml b/briar-android/src/main/res/values/strings.xml index 4565d2a55..7c397a351 100644 --- a/briar-android/src/main/res/values/strings.xml +++ b/briar-android/src/main/res/values/strings.xml @@ -130,6 +130,7 @@ Add a caption (optional) Attach image Could not attach image(s) + Image too big. Limit is %d MB. Change contact name Contact name Change diff --git a/briar-api/src/main/java/org/briarproject/briar/api/messaging/FileTooBigException.java b/briar-api/src/main/java/org/briarproject/briar/api/messaging/FileTooBigException.java new file mode 100644 index 000000000..f7d9c7c04 --- /dev/null +++ b/briar-api/src/main/java/org/briarproject/briar/api/messaging/FileTooBigException.java @@ -0,0 +1,6 @@ +package org.briarproject.briar.api.messaging; + +import java.io.IOException; + +public class FileTooBigException extends IOException { +} diff --git a/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingConstants.java b/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingConstants.java index c6114dc66..c1be55bdf 100644 --- a/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingConstants.java +++ b/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingConstants.java @@ -20,7 +20,8 @@ public interface MessagingConstants { /** * The maximum allowed size of image attachments. + * TODO: Different limit for GIFs? */ - int MAX_IMAGE_SIZE = 6 * 1024 * 1024; + int MAX_IMAGE_SIZE = MAX_MESSAGE_BODY_LENGTH; // 6 * 1024 * 1024; } diff --git a/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingManager.java b/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingManager.java index d3ba4acf1..d069e337b 100644 --- a/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingManager.java +++ b/briar-api/src/main/java/org/briarproject/briar/api/messaging/MessagingManager.java @@ -37,6 +37,8 @@ public interface MessagingManager extends ConversationClient { /** * Stores a local attachment message. + * + * @throws FileTooBigException */ AttachmentHeader addLocalAttachment(GroupId groupId, long timestamp, String contentType, InputStream is) throws DbException, IOException; diff --git a/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java b/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java index e73dce953..8917be036 100644 --- a/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java +++ b/briar-core/src/main/java/org/briarproject/briar/messaging/MessagingManagerImpl.java @@ -18,14 +18,17 @@ import org.briarproject.bramble.api.sync.Group; import org.briarproject.bramble.api.sync.Group.Visibility; import org.briarproject.bramble.api.sync.GroupId; import org.briarproject.bramble.api.sync.Message; +import org.briarproject.bramble.api.sync.MessageFactory; import org.briarproject.bramble.api.sync.MessageId; import org.briarproject.bramble.api.sync.MessageStatus; import org.briarproject.bramble.api.versioning.ClientVersioningManager; import org.briarproject.bramble.api.versioning.ClientVersioningManager.ClientVersioningHook; +import org.briarproject.bramble.util.IoUtils; import org.briarproject.briar.api.client.MessageTracker; import org.briarproject.briar.api.conversation.ConversationMessageHeader; import org.briarproject.briar.api.messaging.Attachment; import org.briarproject.briar.api.messaging.AttachmentHeader; +import org.briarproject.briar.api.messaging.FileTooBigException; import org.briarproject.briar.api.messaging.MessagingManager; import org.briarproject.briar.api.messaging.PrivateMessage; import org.briarproject.briar.api.messaging.PrivateMessageHeader; @@ -33,18 +36,18 @@ import org.briarproject.briar.api.messaging.event.PrivateMessageReceivedEvent; import org.briarproject.briar.client.ConversationClientImpl; import java.io.ByteArrayInputStream; +import java.io.EOFException; import java.io.IOException; import java.io.InputStream; import java.util.ArrayList; import java.util.Collection; import java.util.Map; -import java.util.Random; import javax.annotation.concurrent.Immutable; import javax.inject.Inject; import static java.util.Collections.emptyList; -import static org.briarproject.bramble.util.StringUtils.fromHexString; +import static org.briarproject.bramble.api.sync.SyncConstants.MAX_MESSAGE_BODY_LENGTH; import static org.briarproject.briar.client.MessageTrackerConstants.MSG_KEY_READ; @Immutable @@ -55,15 +58,18 @@ class MessagingManagerImpl extends ConversationClientImpl private final ClientVersioningManager clientVersioningManager; private final ContactGroupFactory contactGroupFactory; + private final MessageFactory messageFactory; @Inject MessagingManagerImpl(DatabaseComponent db, ClientHelper clientHelper, ClientVersioningManager clientVersioningManager, MetadataParser metadataParser, MessageTracker messageTracker, - ContactGroupFactory contactGroupFactory) { + ContactGroupFactory contactGroupFactory, + MessageFactory messageFactory) { super(db, clientHelper, metadataParser, messageTracker); this.clientVersioningManager = clientVersioningManager; this.contactGroupFactory = contactGroupFactory; + this.messageFactory = messageFactory; } @Override @@ -158,17 +164,25 @@ class MessagingManagerImpl extends ConversationClientImpl @Override public AttachmentHeader addLocalAttachment(GroupId groupId, long timestamp, - String contentType, InputStream is) throws IOException { + String contentType, InputStream is) + throws DbException, IOException { // TODO add real implementation - if (is.available() == 0) throw new IOException(); - byte[] b = new byte[MessageId.LENGTH]; - new Random().nextBytes(b); - return new AttachmentHeader(new MessageId(b), contentType); + byte[] body = new byte[MAX_MESSAGE_BODY_LENGTH]; + try { + IoUtils.read(is, body); + } catch (EOFException ignored) { + } + if (is.available() > 0) throw new FileTooBigException(); + is.close(); + Message m = messageFactory.createMessage(groupId, timestamp, body); + clientHelper.addLocalMessage(m, new BdfDictionary(), false); + return new AttachmentHeader(m.getId(), contentType); } @Override public void removeAttachment(AttachmentHeader header) throws DbException { - // TODO add real implementation + db.transaction(false, + txn -> db.removeMessage(txn, header.getMessageId())); } private ContactId getContactId(Transaction txn, GroupId g) @@ -247,12 +261,10 @@ class MessagingManagerImpl extends ConversationClientImpl } @Override - public Attachment getAttachment(MessageId m) { + public Attachment getAttachment(MessageId mId) throws DbException { // TODO add real implementation - byte[] bytes = fromHexString("89504E470D0A1A0A0000000D49484452" + - "000000010000000108060000001F15C4" + - "890000000A49444154789C6300010000" + - "0500010D0A2DB40000000049454E44AE426082"); + Message m = clientHelper.getMessage(mId); + byte[] bytes = m.getBody(); return new Attachment(new ByteArrayInputStream(bytes)); } -- GitLab