Improve attachment upload failure handling (#2135)

lib/src/main/java/org/asamk/signal/manager/helper/AttachmentHelper.java

@@ -2,11 +2,13 @@ package org.asamk.signal.manager.helper;
 
 import org.asamk.signal.manager.api.AttachmentInvalidException;
 import org.asamk.signal.manager.api.Message.AttachmentDimensions;
+import org.asamk.signal.manager.api.Pair;
 import org.asamk.signal.manager.config.ServiceConfig;
 import org.asamk.signal.manager.internal.SignalDependencies;
 import org.asamk.signal.manager.storage.AttachmentStore;
 import org.asamk.signal.manager.util.AttachmentUtils;
 import org.asamk.signal.manager.util.IOUtils;
+import org.asamk.signal.manager.util.MimeUtils;
 import org.asamk.signal.manager.util.Utils;
 import org.signal.libsignal.protocol.InvalidMessageException;
 import org.slf4j.Logger;
@@ -21,6 +23,7 @@ import org.whispersystems.signalservice.api.util.StreamDetails;
 import org.whispersystems.signalservice.internal.crypto.PaddingInputStream;
 import org.whispersystems.signalservice.internal.push.http.ResumableUploadSpec;
 
+import java.io.ByteArrayInputStream;
 import java.io.File;
 import java.io.IOException;
 import java.io.InputStream;
@@ -28,6 +31,8 @@ import java.io.OutputStream;
 import java.nio.file.Files;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.Optional;
+import java.util.function.Supplier;
 
 public class AttachmentHelper {
 
@@ -35,12 +40,21 @@ public class AttachmentHelper {
 
     private final SignalDependencies dependencies;
     private final AttachmentStore attachmentStore;
-    private final Context context;
+    private final Uploader uploader;
+    private final Supplier<File> dataPath;
 
     public AttachmentHelper(final Context context) {
-        this.context = context;
         this.dependencies = context.getDependencies();
         this.attachmentStore = context.getAttachmentStore();
+        this.uploader = new DependenciesUploader(dependencies);
+        this.dataPath = () -> context.getAccount().getDataPath();
+    }
+
+    AttachmentHelper(final Uploader uploader, final Supplier<File> dataPath) {
+        this.dependencies = null;
+        this.attachmentStore = null;
+        this.uploader = uploader;
+        this.dataPath = dataPath;
     }
 
     public File getAttachmentFile(SignalServiceAttachmentPointer pointer) {
@@ -51,29 +65,36 @@ public class AttachmentHelper {
         return attachmentStore.retrieveAttachment(id);
     }
 
+    /**
+     * Prepares and uploads all attachments. I/O failures are reported as {@link AttachmentInvalidException}, so
+     * callers know that nothing has been sent yet. Attachments after the failing one are not uploaded.
+     */
     public List<SignalServiceAttachment> uploadAttachments(
             final List<String> attachments,
             final List<AttachmentDimensions> dimensions,
             final List<String> blurHashes,
             boolean voiceNote
-    ) throws AttachmentInvalidException, IOException {
+    ) throws AttachmentInvalidException {
         final var attachmentStreams = createAttachmentStreams(attachments, dimensions, blurHashes, voiceNote);
 
         try {
             // Upload attachments here, so we only upload once even for multiple recipients
             final var attachmentPointers = new ArrayList<SignalServiceAttachment>(attachmentStreams.size());
-            for (final var attachmentStream : attachmentStreams) {
-                attachmentPointers.add(uploadAttachment(attachmentStream));
+            for (var i = 0; i < attachmentStreams.size(); i++) {
+                final var attachment = attachments.get(i);
+                try {
+                    attachmentPointers.add(uploader.upload(attachmentStreams.get(i)));
+                } catch (IOException e) {
+                    throw invalid(describe(attachment, inlineAttachmentLabel(i)), e);
+                }
             }
             return attachmentPointers;
         } finally {
-            for (final var attachmentStream : attachmentStreams) {
-                attachmentStream.close();
-            }
+            closeAll(attachmentStreams);
         }
     }
 
-    public List<SignalServiceAttachment> uploadAttachments(final List<String> attachments) throws AttachmentInvalidException, IOException {
+    public List<SignalServiceAttachment> uploadAttachments(final List<String> attachments) throws AttachmentInvalidException {
         return uploadAttachments(attachments, List.of(), List.of(), false);
     }
 
@@ -82,45 +103,55 @@ public class AttachmentHelper {
             List<AttachmentDimensions> dimensions,
             List<String> blurHashes,
             boolean voiceNote
-    ) throws AttachmentInvalidException, IOException {
+    ) throws AttachmentInvalidException {
         if (attachments == null) {
-            return null;
+            return List.of();
         }
         final var signalServiceAttachments = new ArrayList<SignalServiceAttachmentStream>(attachments.size());
         for (var i = 0; i < attachments.size(); i++) {
             final var size = i < dimensions.size() ? dimensions.get(i) : null;
             final var blurHash = i < blurHashes.size() && !blurHashes.get(i).isEmpty() ? blurHashes.get(i) : null;
-            signalServiceAttachments.add(getAttachmentStream(attachments.get(i), size, blurHash, voiceNote));
+            signalServiceAttachments.add(getAttachmentStream(attachments.get(i),
+                    inlineAttachmentLabel(i),
+                    size,
+                    blurHash,
+                    voiceNote));
         }
         return signalServiceAttachments;
     }
 
     private SignalServiceAttachmentStream getAttachmentStream(
             final String attachment,
+            final String inlineLabel,
             final AttachmentDimensions dimensions,
             final String blurHash,
             final boolean voiceNote
     ) throws AttachmentInvalidException {
+        final var label = describe(attachment, inlineLabel);
         try {
-            // Reject local files that point into the signal-cli data directory
-            if (attachment != null && !attachment.startsWith("data:")) {
-                try {
-                    final var file = new File(attachment);
-                    final var canonical = file.getCanonicalFile();
-                    final var dataPath = context.getAccount().getDataPath().getCanonicalFile();
-                    if (canonical.toPath().startsWith(dataPath.toPath())) {
-                        throw new AttachmentInvalidException(attachment,
-                                new IOException("Attaching files from the signal-cli data directory is not allowed"));
-                    }
-                } catch (IOException e) {
-                    throw new AttachmentInvalidException(attachment, e);
+            final Pair<StreamDetails, Optional<String>> streamDetailsAndFileName;
+            if (isInline(attachment)) {
+                // Never open inline data as a file, whose errors could echo its payload. Check for the comma
+                // before parsing to avoid regex backtracking over long malformed values.
+                if (attachment.indexOf(',') < 0) {
+                    throw new IOException("Invalid data URI");
                 }
+                try {
+                    streamDetailsAndFileName = Utils.createStreamDetailsFromDataURI(attachment);
+                } catch (IllegalArgumentException e) {
+                    throw new IOException("Invalid data URI");
+                }
+            } else {
+                // Reject local files that point into the signal-cli data directory
+                final var canonical = new File(attachment).getCanonicalFile();
+                final var dataPath = this.dataPath.get().getCanonicalFile();
+                if (canonical.toPath().startsWith(dataPath.toPath())) {
+                    throw new IOException("Attaching files from the signal-cli data directory is not allowed");
+                }
+                streamDetailsAndFileName = Utils.createStreamDetails(attachment);
             }
-
-            final var streamDetailsAndFileName = Utils.createStreamDetails(attachment);
             final var streamDetails = streamDetailsAndFileName.first();
             final var uploadSpec = getResumableUploadSpec(streamDetails);
-
             return AttachmentUtils.createAttachmentStream(streamDetails,
                     streamDetailsAndFileName.second(),
                     voiceNote,
@@ -128,7 +159,7 @@ public class AttachmentHelper {
                     blurHash,
                     uploadSpec);
         } catch (IOException e) {
-            throw new AttachmentInvalidException(attachment, e);
+            throw invalid(label, e);
         }
     }
 
@@ -136,17 +167,89 @@ public class AttachmentHelper {
         final var streamLength = streamDetails.getLength();
         final var ciphertextLength = AttachmentCipherStreamUtil.getCiphertextLength(PaddingInputStream.getPaddedSize(
                 streamLength));
-        return dependencies.getCdnService().getResumableUploadSpecBlocking(ciphertextLength);
+        return uploader.getResumableUploadSpec(ciphertextLength);
     }
 
-    public SignalServiceAttachmentPointer uploadAttachment(String attachment) throws IOException, AttachmentInvalidException {
-        final var attachmentStream = getAttachmentStream(attachment, null, null, false);
-        return uploadAttachment(attachmentStream);
+    public SignalServiceAttachmentPointer uploadAttachment(String attachment) throws AttachmentInvalidException {
+        return uploadAttachment(attachment, "inline attachment");
     }
 
     public SignalServiceAttachmentPointer uploadAttachment(SignalServiceAttachmentStream attachment) throws IOException {
-        var messageSender = dependencies.getMessageSender();
-        return messageSender.uploadAttachment(attachment);
+        return uploader.upload(attachment);
+    }
+
+    /**
+     * Prepares and uploads a single attachment, such as a quote thumbnail or a link preview image. Any
+     * failure is reported as {@link AttachmentInvalidException} with the given label for inline data.
+     */
+    public SignalServiceAttachmentPointer uploadAttachment(
+            final String attachment,
+            final String inlineLabel
+    ) throws AttachmentInvalidException {
+        try (final var attachmentStream = getAttachmentStream(attachment, inlineLabel, null, null, false)) {
+            return uploader.upload(attachmentStream);
+        } catch (IOException e) {
+            throw invalid(describe(attachment, inlineLabel), e);
+        }
+    }
+
+    /**
+     * Uploads the full text of a message that is too long to be sent inline. Failures never include the text.
+     */
+    public SignalServiceAttachmentPointer uploadLongTextAttachment(final byte[] messageBytes) throws AttachmentInvalidException {
+        final var streamDetails = new StreamDetails(new ByteArrayInputStream(messageBytes),
+                MimeUtils.LONG_TEXT,
+                messageBytes.length);
+        try (final var textAttachment = createAttachmentStream(streamDetails)) {
+            return uploader.upload(textAttachment);
+        } catch (IOException e) {
+            throw invalid("long text attachment", e);
+        }
+    }
+
+    /**
+     * Prepares a sticker for sending. The sticker itself is uploaded later by the message sender.
+     */
+    public SignalServiceAttachmentStream createStickerAttachmentStream(final StreamDetails streamDetails) throws AttachmentInvalidException {
+        try {
+            return createAttachmentStream(streamDetails);
+        } catch (IOException e) {
+            throw invalid("sticker", e);
+        }
+    }
+
+    private SignalServiceAttachmentStream createAttachmentStream(final StreamDetails streamDetails) throws IOException {
+        final var uploadSpec = getResumableUploadSpec(streamDetails);
+        return AttachmentUtils.createAttachmentStream(streamDetails, Optional.empty(), uploadSpec);
+    }
+
+    private static String describe(final String attachment, final String inlineLabel) {
+        return isInline(attachment) ? inlineLabel : attachment;
+    }
+
+    static AttachmentInvalidException invalid(final String label, final Exception e) {
+        final var message = e.getMessage() != null ? e.getMessage() : e.getClass().getName();
+        final var exception = new AttachmentInvalidException(label + ": " + message);
+        exception.initCause(e);
+        return exception;
+    }
+
+    private static boolean isInline(final String attachment) {
+        return attachment != null && attachment.regionMatches(true, 0, "data:", 0, 5);
+    }
+
+    private static String inlineAttachmentLabel(final int index) {
+        return "inline attachment #" + (index + 1);
+    }
+
+    private static void closeAll(final List<SignalServiceAttachmentStream> streams) {
+        for (final var stream : streams) {
+            try {
+                stream.close();
+            } catch (IOException e) {
+                logger.warn("Failed to close attachment stream, ignoring: {}", e.getMessage());
+            }
+        }
     }
 
     public void downloadAttachment(final SignalServiceAttachment attachment) {
@@ -223,4 +326,24 @@ public class AttachmentHelper {
 
         void handle(InputStream inputStream) throws IOException;
     }
+
+    interface Uploader {
+
+        ResumableUploadSpec getResumableUploadSpec(long ciphertextLength) throws IOException;
+
+        SignalServiceAttachmentPointer upload(SignalServiceAttachmentStream stream) throws IOException;
+    }
+
+    private record DependenciesUploader(SignalDependencies dependencies) implements Uploader {
+
+        @Override
+        public ResumableUploadSpec getResumableUploadSpec(final long ciphertextLength) throws IOException {
+            return dependencies.getCdnService().getResumableUploadSpecBlocking(ciphertextLength);
+        }
+
+        @Override
+        public SignalServiceAttachmentPointer upload(final SignalServiceAttachmentStream stream) throws IOException {
+            return dependencies.getMessageSender().uploadAttachment(stream);
+        }
+    }
 }

lib/src/main/java/org/asamk/signal/manager/internal/ManagerImpl.java

@@ -90,7 +90,6 @@ import org.asamk.signal.manager.storage.recipients.RecipientId;
 import org.asamk.signal.manager.storage.stickerPacks.JsonStickerPack;
 import org.asamk.signal.manager.storage.stickerPacks.StickerPackStore;
 import org.asamk.signal.manager.storage.stickers.StickerPack;
-import org.asamk.signal.manager.util.AttachmentUtils;
 import org.asamk.signal.manager.util.KeyUtils;
 import org.asamk.signal.manager.util.MimeUtils;
 import org.asamk.signal.manager.util.PhoneNumberFormatter;
@@ -124,10 +123,8 @@ import org.whispersystems.signalservice.api.messages.calls.OfferMessage;
 import org.whispersystems.signalservice.api.messages.calls.SignalServiceCallMessage;
 import org.whispersystems.signalservice.api.push.ServiceIdType;
 import org.whispersystems.signalservice.api.push.exceptions.CdsiResourceExhaustedException;
-import org.whispersystems.signalservice.api.util.StreamDetails;
 import org.whispersystems.signalservice.internal.util.Util;
 
-import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
 import java.io.File;
 import java.io.FileNotFoundException;
@@ -953,15 +950,8 @@ public class ManagerImpl implements Manager {
             final var remainder = result.getSecond();
             if (remainder != null) {
                 final var messageBytes = message.messageText().getBytes(StandardCharsets.UTF_8);
-                final var streamDetails = new StreamDetails(new ByteArrayInputStream(messageBytes),
-                        MimeUtils.LONG_TEXT,
-                        messageBytes.length);
-                final var uploadSpec = context.getAttachmentHelper().getResumableUploadSpec(streamDetails);
-                final var textAttachment = AttachmentUtils.createAttachmentStream(streamDetails,
-                        Optional.empty(),
-                        uploadSpec);
                 messageBuilder.withBody(trimmed);
-                additionalAttachments.add(context.getAttachmentHelper().uploadAttachment(textAttachment));
+                additionalAttachments.add(context.getAttachmentHelper().uploadLongTextAttachment(messageBytes));
             } else {
                 messageBuilder.withBody(message.messageText());
             }
@@ -994,9 +984,12 @@ public class ManagerImpl implements Manager {
             final var quote = message.quote().get();
             final var quotedAttachments = new ArrayList<SignalServiceDataMessage.Quote.QuotedAttachment>();
             for (final var a : quote.attachments()) {
+                final var label = "quote thumbnail #" + (quotedAttachments.size() + 1);
                 final var quotedAttachment = new SignalServiceDataMessage.Quote.QuotedAttachment(a.contentType(),
                         a.filename(),
-                        a.preview() == null ? null : context.getAttachmentHelper().uploadAttachment(a.preview()));
+                        a.preview() == null
+                                ? null
+                                : context.getAttachmentHelper().uploadAttachment(a.preview(), label));
                 quotedAttachments.add(quotedAttachment);
             }
             messageBuilder.withQuote(new SignalServiceDataMessage.Quote(quote.timestamp(),
@@ -1027,10 +1020,7 @@ public class ManagerImpl implements Manager {
             if (streamDetails == null) {
                 throw new InvalidStickerException("Missing local sticker file");
             }
-            final var uploadSpec = context.getAttachmentHelper().getResumableUploadSpec(streamDetails);
-            final var stickerAttachment = AttachmentUtils.createAttachmentStream(streamDetails,
-                    Optional.empty(),
-                    uploadSpec);
+            final var stickerAttachment = context.getAttachmentHelper().createStickerAttachmentStream(streamDetails);
             messageBuilder.withSticker(new SignalServiceDataMessage.Sticker(packId.serialize(),
                     stickerPack.packKey(),
                     stickerId,
@@ -1040,8 +1030,9 @@ public class ManagerImpl implements Manager {
         if (!message.previews().isEmpty()) {
             final var previews = new ArrayList<SignalServicePreview>(message.previews().size());
             for (final var p : message.previews()) {
+                final var label = "link preview image #" + (previews.size() + 1);
                 final var image = p.image().isPresent() ? context.getAttachmentHelper()
-                        .uploadAttachment(p.image().get()) : null;
+                        .uploadAttachment(p.image().get(), label) : null;
                 previews.add(new SignalServicePreview(p.url(),
                         p.title(),
                         p.description(),

lib/src/test/java/org/asamk/signal/manager/helper/AttachmentHelperTest.java

@@ -0,0 +1,289 @@
+package org.asamk.signal.manager.helper;
+
+import org.asamk.signal.manager.api.AttachmentInvalidException;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
+import org.whispersystems.signalservice.api.messages.SignalServiceAttachment;
+import org.whispersystems.signalservice.api.messages.SignalServiceAttachmentPointer;
+import org.whispersystems.signalservice.api.messages.SignalServiceAttachmentRemoteId;
+import org.whispersystems.signalservice.api.messages.SignalServiceAttachmentStream;
+import org.whispersystems.signalservice.api.util.StreamDetails;
+import org.whispersystems.signalservice.internal.push.http.ResumableUploadSpec;
+
+import java.io.ByteArrayInputStream;
+import java.io.File;
+import java.io.IOException;
+import java.io.PrintWriter;
+import java.io.StringWriter;
+import java.net.SocketException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Base64;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.UUID;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertInstanceOf;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class AttachmentHelperTest {
+
+    private static final String PAYLOAD = Base64.getEncoder()
+            .encodeToString("private attachment marker ".repeat(200).getBytes(StandardCharsets.UTF_8));
+
+    @TempDir
+    Path tempDir;
+
+    @Test
+    void preparationFailureIsAttachmentInvalidAndNothingIsUploaded() {
+        final var uploader = new FakeUploader();
+        uploader.failSpecOnCall = 1;
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(uploader).uploadAttachments(List.of(inline("one"), inline("two"))));
+
+        assertEquals("inline attachment #1: spec failed", error.getMessage());
+        assertRedacted(error);
+        assertEquals(0, uploader.uploads.size());
+    }
+
+    @Test
+    void uploadFailureOnSecondInlineAttachmentStopsBeforeTheThird() {
+        final var uploader = new FakeUploader();
+        uploader.failUploadOnCall = 2;
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(uploader).uploadAttachments(List.of(inline("one"), inline("two"), inline("three"))));
+
+        assertEquals("inline attachment #2: upload failed", error.getMessage());
+        assertSame(uploader.uploadFailure, error.getCause());
+        assertRedacted(error);
+        assertEquals(List.of("one", "two"), uploader.uploads);
+    }
+
+    @Test
+    void uploadFailureOnSecondFileAttachmentNamesThePath() throws IOException {
+        final var files = List.of(file("a.txt"), file("b.txt"), file("c.txt"));
+        final var uploader = new FakeUploader();
+        uploader.failUploadOnCall = 2;
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(uploader).uploadAttachments(files));
+
+        assertEquals(files.get(1) + ": upload failed", error.getMessage());
+        assertInstanceOf(SocketException.class, error.getCause());
+        assertEquals(List.of("a.txt", "b.txt"), uploader.uploads);
+    }
+
+    @Test
+    void successfulUploadsKeepInputOrder() throws Exception {
+        final var uploader = new FakeUploader();
+        final var pointers = helper(uploader).uploadAttachments(List.of(inline("one"),
+                file("two.txt"),
+                inline("three")));
+
+        assertEquals(List.of("one", "two.txt", "three"),
+                pointers.stream()
+                        .map(SignalServiceAttachment::asPointer)
+                        .map(p -> p.getFileName().orElseThrow())
+                        .toList());
+        assertEquals(3, uploader.specs);
+    }
+
+    @Test
+    void malformedInlineAttachmentDoesNotExposePayload() {
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(new FakeUploader()).uploadAttachments(List.of("data:application/octet-stream;base64,"
+                        + PAYLOAD
+                        + "!")));
+
+        assertEquals("inline attachment #1: Invalid data URI", error.getMessage());
+        assertRedacted(error);
+    }
+
+    @Test
+    void upperCaseInlineAttachmentIsNotTreatedAsAFilePath() throws Exception {
+        // Longer than PATH_MAX, so canonicalizing it as a file path would fail
+        final var uploader = new FakeUploader();
+        final var pointers = helper(uploader).uploadAttachments(List.of(
+                "DATA:application/octet-stream;filename=upper;base64," + PAYLOAD));
+
+        assertEquals(1, pointers.size());
+        assertEquals(List.of("upper"), uploader.uploads);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"data:text/plain;base64", "data://", "data:;base64//", "DATA://"})
+    void inlineAttachmentWithoutCommaIsNotExposed(final String prefix) {
+        // Short enough to reach file opening if inline input is incorrectly treated as a path.
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(new FakeUploader()).uploadAttachments(List.of(prefix + PAYLOAD.substring(0, 80))));
+
+        assertEquals("inline attachment #1: Invalid data URI", error.getMessage());
+        assertRedacted(error);
+    }
+
+    @Test
+    void longInlineAttachmentWithoutCommaIsRejected() {
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(new FakeUploader()).uploadAttachments(List.of("data:" + PAYLOAD.repeat(160))));
+
+        assertEquals("inline attachment #1: Invalid data URI", error.getMessage());
+        assertRedacted(error);
+    }
+
+    @Test
+    void defaultUploadUsesTheSameErrorContract() {
+        final var uploader = new FakeUploader();
+        uploader.failUploadOnCall = 1;
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(uploader).uploadAttachment(inline("image")));
+
+        assertEquals("inline attachment: upload failed", error.getMessage());
+        assertSame(uploader.uploadFailure, error.getCause());
+        assertRedacted(error);
+    }
+
+    @Test
+    void stickerPreparationFailureIsLabelled() {
+        final var uploader = new FakeUploader();
+        uploader.failSpecOnCall = 1;
+        final var stream = new ByteArrayInputStream(new byte[4]);
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(uploader).createStickerAttachmentStream(new StreamDetails(stream, "image/webp", 4)));
+
+        assertEquals("sticker: spec failed", error.getMessage());
+    }
+
+    @ParameterizedTest
+    @ValueSource(booleans = {true, false})
+    void longTextFailureNeverEchoesTheText(final boolean preparation) {
+        final var uploader = new FakeUploader();
+        uploader.failSpecOnCall = preparation ? 1 : 0;
+        uploader.failUploadOnCall = preparation ? 0 : 1;
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> helper(uploader).uploadLongTextAttachment(PAYLOAD.getBytes(StandardCharsets.UTF_8)));
+
+        assertEquals("long text attachment: " + (preparation ? "spec" : "upload") + " failed", error.getMessage());
+        assertRedacted(error);
+        assertInstanceOf(SocketException.class, error.getCause());
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"quote thumbnail #1", "link preview image #1"})
+    void imageFailuresUseTheirLabel(final String label) {
+        final var uploader = new FakeUploader();
+        uploader.failSpecOnCall = 1;
+        final var helper = helper(uploader);
+        final var preparationError = assertThrows(AttachmentInvalidException.class,
+                () -> helper.uploadAttachment(inline("image"), label));
+        assertEquals(label + ": spec failed", preparationError.getMessage());
+        assertRedacted(preparationError);
+        assertEquals(0, uploader.uploads.size());
+
+        uploader.failUploadOnCall = 1;
+        final var uploadError = assertThrows(AttachmentInvalidException.class,
+                () -> helper.uploadAttachment(inline("image"), label));
+        assertEquals(label + ": upload failed", uploadError.getMessage());
+        assertRedacted(uploadError);
+    }
+
+    @Test
+    void filesInTheDataDirectoryAreStillRejected() throws IOException {
+        final var dataDir = Files.createDirectory(tempDir.resolve("data")).toFile();
+        final var secret = new File(dataDir, "account.db");
+        Files.writeString(secret.toPath(), "secret");
+        final var uploader = new FakeUploader();
+        final var error = assertThrows(AttachmentInvalidException.class,
+                () -> new AttachmentHelper(uploader, () -> dataDir).uploadAttachments(List.of(secret.getPath())));
+
+        assertEquals(secret.getPath() + ": Attaching files from the signal-cli data directory is not allowed",
+                error.getMessage());
+        assertEquals(0, uploader.specs);
+    }
+
+    @Test
+    void invalidWithoutCauseMessageFallsBackToClassName() {
+        final var cause = new SocketException();
+        final var error = AttachmentHelper.invalid("inline attachment #1", cause);
+
+        assertEquals("inline attachment #1: java.net.SocketException", error.getMessage());
+        assertSame(cause, error.getCause());
+    }
+
+    private AttachmentHelper helper(final FakeUploader uploader) {
+        return new AttachmentHelper(uploader, () -> tempDir.resolve("data").toFile());
+    }
+
+    private String file(final String name) throws IOException {
+        final var path = tempDir.resolve(name);
+        Files.writeString(path, PAYLOAD);
+        return path.toString();
+    }
+
+    private static String inline(final String filename) {
+        return "data:application/octet-stream;filename=" + filename + ";base64," + PAYLOAD;
+    }
+
+    private static void assertRedacted(final AttachmentInvalidException error) {
+        final var trace = new StringWriter();
+        error.printStackTrace(new PrintWriter(trace));
+        assertFalse(trace.toString().contains(PAYLOAD.substring(0, 32)));
+    }
+
+    private static final class FakeUploader implements AttachmentHelper.Uploader {
+
+        final IOException uploadFailure = new SocketException("upload failed");
+        int failSpecOnCall;
+        int failUploadOnCall;
+        int specs;
+        final List<String> uploads = new ArrayList<>();
+
+        @Override
+        public ResumableUploadSpec getResumableUploadSpec(final long ciphertextLength) throws IOException {
+            if (++specs == failSpecOnCall) {
+                throw new SocketException("spec failed");
+            }
+            return new ResumableUploadSpec(new byte[64],
+                    new byte[16],
+                    "cdn-key",
+                    3,
+                    "https://cdn.invalid/upload",
+                    Long.MAX_VALUE,
+                    Map.of());
+        }
+
+        @Override
+        public SignalServiceAttachmentPointer upload(final SignalServiceAttachmentStream stream) throws IOException {
+            final var fileName = stream.getFileName().orElse("");
+            uploads.add(fileName);
+            if (uploads.size() == failUploadOnCall) {
+                throw uploadFailure;
+            }
+            return new SignalServiceAttachmentPointer(3,
+                    SignalServiceAttachmentRemoteId.from("cdn-key", 3),
+                    stream.getContentType(),
+                    new byte[64],
+                    Optional.of((int) stream.getLength()),
+                    Optional.empty(),
+                    0,
+                    0,
+                    Optional.empty(),
+                    Optional.empty(),
+                    0,
+                    Optional.of(fileName),
+                    false,
+                    false,
+                    false,
+                    Optional.empty(),
+                    Optional.empty(),
+                    0,
+                    UUID.randomUUID());
+        }
+    }
+}

man/signal-cli-dbus.5.adoc

@@ -50,6 +50,8 @@ The final parenthetical value (such as "boolean:") is the type indicator used by
 Exceptions are the names of the Java Exceptions returned in the body field.
 They typically contain an additional message with details.
 All Exceptions begin with "org.asamk.Signal.Error." which is omitted here for better readability.
+When sending a message, AttachmentInvalid means an attachment could not be read or uploaded, and the message
+was not sent to anyone; Failure may mean that it reached some recipients.
 
 Phone numbers always have the format +<countrycode><regional number>
 

src/test/java/org/asamk/signal/commands/SendCommandErrorTest.java

@@ -0,0 +1,59 @@
+package org.asamk.signal.commands;
+
+import org.asamk.signal.commands.exceptions.UnexpectedErrorException;
+import org.asamk.signal.manager.Manager;
+import org.asamk.signal.manager.api.AttachmentInvalidException;
+import org.asamk.signal.output.JsonWriter;
+import org.asamk.signal.testutil.ManagerMock;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import java.io.IOException;
+import java.lang.reflect.Proxy;
+import java.net.SocketException;
+import java.util.List;
+import java.util.Map;
+import java.util.stream.Stream;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertSame;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+
+class SendCommandErrorTest {
+
+    @ParameterizedTest
+    @MethodSource("failures")
+    void uploadAndTransmissionFailuresKeepTheirClassification(final Exception cause) {
+        final var error = send(cause);
+
+        assertEquals("Failed to send message: " + cause.getMessage() + " (" + cause.getClass().getSimpleName() + ")",
+                error.getMessage());
+        assertSame(cause, error.getCause());
+    }
+
+    private static Stream<Exception> failures() {
+        return Stream.of(new AttachmentInvalidException("inline attachment #2: Connection reset"),
+                new SocketException("Connection reset"),
+                new IOException("Server error: 500"));
+    }
+
+    private static UnexpectedErrorException send(final Exception failure) {
+        final var delegate = ManagerMock.create("+15551234567");
+        final var manager = (Manager) Proxy.newProxyInstance(Manager.class.getClassLoader(),
+                new Class<?>[]{Manager.class}, (proxy, method, args) -> {
+                    if (method.getName().equals("sendMessage")) {
+                        throw failure;
+                    }
+                    return method.invoke(delegate, args);
+                });
+        // Upload and transmission failures must map to the same exception type, so the exit code and the
+        // JSON-RPC error code stay the same.
+        return assertThrows(UnexpectedErrorException.class,
+                () -> new SendCommand().handleCommand(new JsonRpcNamespace(Map.of("noteToSelf",
+                        true,
+                        "message",
+                        "hi",
+                        "attachments",
+                        List.of("photo.jpg"))), manager, (JsonWriter) ignored -> {}));
+    }
+}