Skip to content

Commit

Permalink
feat: Use random filename suffixes for blobstorage (#4309)
Browse files Browse the repository at this point in the history
Recently there was an accident with a chatbot that replaced its avatar set from the command line
with an unrelated avatar of a contact. Both the `selfavatar` setting and the contact avatar `i`
param pointed to `$BLOBDIR/avatar.png` at the time it was detected. How this happened is unclear,
but it is possible that `avatar.png` was removed, unmounted or otherwise not detected by the core,
and the core stored avatar received from the contact as `avatar.png`, while `selfavatar` config
still pointed to `$BLOBDIR/avatar.png`.

Such bugs are unavoidable even if the core itself has no bugs as we cannot rely on blobdir not
reside on the faulty network filesystem, being incorrectly backed up and restored etc., so we should
assume that files may be randomly removed. Then there may be dangling `$BLOBDIR/...` references in
the database which may accidentally point to unrelated files, could even be an `avatar.png` file
sent to the bot in private.

To prevent such bugs, we add random filename suffixes for the blobdir objects. Thanks to the added
Param::Filename these random suffixes aren't sent over the network.
  • Loading branch information
iequidoo committed Apr 25, 2024
1 parent 56b86ad commit b248bbf
Show file tree
Hide file tree
Showing 8 changed files with 119 additions and 97 deletions.
9 changes: 4 additions & 5 deletions python/src/deltachat/message.py
Original file line number Diff line number Diff line change
Expand Up @@ -107,8 +107,8 @@ def set_html(self, html_text):
lib.dc_msg_set_html(self._dc_msg, as_dc_charpointer(html_text))

@props.with_doc
def filename(self):
"""filename if there was an attachment, otherwise empty string."""
def file_path(self):
"""file path if there was an attachment, otherwise empty string."""
return from_dc_charpointer(lib.dc_msg_get_file(self._dc_msg))

def set_file(self, path, mime_type=None):
Expand All @@ -119,9 +119,8 @@ def set_file(self, path, mime_type=None):
lib.dc_msg_set_file(self._dc_msg, as_dc_charpointer(path), mtype)

@props.with_doc
def basename(self) -> str:
"""basename of the attachment if it exists, otherwise empty string."""
# FIXME, it does not return basename
def filename(self) -> str:
"""filename of the attachment if any, otherwise empty string."""
return from_dc_charpointer(lib.dc_msg_get_filename(self._dc_msg))

@props.with_doc
Expand Down
38 changes: 19 additions & 19 deletions python/tests/test_1_online.py
Original file line number Diff line number Diff line change
Expand Up @@ -180,16 +180,16 @@ def send_and_receive_message():

msg = send_and_receive_message()
assert msg.text == "withfile"
assert open(msg.filename).read() == "some data"
msg.filename.index(basename)
assert msg.filename.endswith(ext)
assert open(msg.file_path).read() == "some data"
msg.file_path.index(basename)
assert msg.file_path.endswith(ext)

msg2 = send_and_receive_message()
assert msg2.text == "withfile"
assert open(msg2.filename).read() == "some data"
msg2.filename.index(basename)
assert msg2.filename.endswith(ext)
assert msg.filename != msg2.filename
assert open(msg2.file_path).read() == "some data"
msg2.file_path.index(basename)
assert msg2.file_path.endswith(ext)
assert msg.file_path != msg2.file_path


def test_send_file_html_attachment(tmp_path, acfactory, lp):
Expand All @@ -213,9 +213,9 @@ def test_send_file_html_attachment(tmp_path, acfactory, lp):
assert ev.data2 > dc.const.DC_CHAT_ID_LAST_SPECIAL
msg = ac2.get_message_by_id(ev.data2)

assert open(msg.filename).read() == content
msg.filename.index(basename)
assert msg.filename.endswith(ext)
assert open(msg.file_path).read() == content
msg.file_path.index(basename)
assert msg.file_path.endswith(ext)


def test_html_message(acfactory, lp):
Expand Down Expand Up @@ -309,7 +309,7 @@ def test_webxdc_message(acfactory, data, lp):
msg1.set_file(data.get_path("webxdc/minimal.xdc"))
msg1 = chat.send_msg(msg1)
assert msg1.is_webxdc()
assert msg1.filename
assert msg1.file_path

assert msg1.send_status_update({"payload": "test1"}, "some test data")
assert msg1.send_status_update({"payload": "test2"}, "more test data")
Expand All @@ -322,7 +322,7 @@ def test_webxdc_message(acfactory, data, lp):
msg2 = ac2._evtracker.wait_next_incoming_message()
assert msg2.text == "message1"
assert msg2.is_webxdc()
assert msg2.filename
assert msg2.file_path
ac2._evtracker.get_info_contains("Marked messages [0-9]+ in folder INBOX as seen.")
ac2.direct_imap.select_folder("Inbox")
assert len(list(ac2.direct_imap.conn.fetch(AND(seen=True)))) == 1
Expand All @@ -337,7 +337,7 @@ def test_webxdc_huge_update(acfactory, data, lp):
msg1.set_file(data.get_path("webxdc/minimal.xdc"))
msg1 = chat.send_msg(msg1)
assert msg1.is_webxdc()
assert msg1.filename
assert msg1.file_path

msg2 = ac2._evtracker.wait_next_incoming_message()
assert msg2.is_webxdc()
Expand All @@ -359,7 +359,7 @@ def test_webxdc_download_on_demand(acfactory, data, lp):
msg1.set_file(data.get_path("webxdc/minimal.xdc"))
msg1 = chat.send_msg(msg1)
assert msg1.is_webxdc()
assert msg1.filename
assert msg1.file_path

msg2 = ac2._evtracker.wait_next_incoming_message()
assert msg2.is_webxdc()
Expand Down Expand Up @@ -1393,7 +1393,7 @@ def test_quote_attachment(tmp_path, acfactory, lp):
received_reply = ac1._evtracker.wait_next_incoming_message()
assert received_reply.text == "message reply"
assert received_reply.quoted_text == received_message.text
assert open(received_reply.filename).read() == "data to send"
assert open(received_reply.file_path).read() == "data to send"


def test_saved_mime_on_received_message(acfactory, lp):
Expand Down Expand Up @@ -1486,8 +1486,8 @@ def ac_outgoing_message(self, message):
assert ev.data2 == msg_out.id
msg_in = ac2.get_message_by_id(msg_out.id)
assert msg_in.is_image()
assert os.path.exists(msg_in.filename)
assert os.stat(msg_in.filename).st_size == os.stat(path).st_size
assert os.path.exists(msg_in.file_path)
assert os.stat(msg_in.file_path).st_size == os.stat(path).st_size
m = message_queue.get()
assert m == msg_in

Expand Down Expand Up @@ -1520,7 +1520,7 @@ def assert_account_is_proper(ac):
assert len(messages) == 3
assert messages[0].text == "msg1"
assert messages[1].filemime == "image/png"
assert os.stat(messages[1].filename).st_size == os.stat(original_image_path).st_size
assert os.stat(messages[1].file_path).st_size == os.stat(original_image_path).st_size
ac.set_config("displayname", "new displayname")
assert ac.get_config("displayname") == "new displayname"

Expand Down Expand Up @@ -1593,7 +1593,7 @@ def test_ac_setup_message(acfactory, lp):
with pytest.raises(ValueError):
msg.continue_key_transfer(str(reversed(setup_code)))
lp.sec("try a good setup code")
print("*************** Incoming ASM File at: ", msg.filename)
print("*************** Incoming ASM File at: ", msg.file_path)
print("*************** Setup Code: ", setup_code)
msg.continue_key_transfer(setup_code)
assert ac1.get_info()["fingerprint"] == ac2.get_info()["fingerprint"]
Expand Down
8 changes: 4 additions & 4 deletions python/tests/test_2_increation.py
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ def test_no_increation_copies_to_blobdir(self, tmp_path, acfactory, lp):
src = tmp_path / "file.txt"
src.write_text("hello there\n")
msg = chat.send_file(str(src))
assert msg.filename.startswith(os.path.join(ac1.get_blobdir(), "file"))
assert msg.filename.endswith(".txt")
assert msg.file_path.startswith(os.path.join(ac1.get_blobdir(), "file"))
assert msg.file_path.endswith(".txt")

def test_forward_increation(self, acfactory, data, lp):
ac1, ac2 = acfactory.get_online_accounts(2)
Expand Down Expand Up @@ -99,9 +99,9 @@ def test_forward_increation(self, acfactory, data, lp):

lp.sec("wait1 for original or forwarded messages to arrive")
received_original = ac2._evtracker.wait_next_incoming_message()
assert cmp(received_original.filename, orig, shallow=False)
assert cmp(received_original.file_path, orig, shallow=False)

lp.sec("wait2 for original or forwarded messages to arrive")
received_copy = ac2._evtracker.wait_next_incoming_message()
assert received_copy.id != received_original.id
assert cmp(received_copy.filename, orig, shallow=False)
assert cmp(received_copy.file_path, orig, shallow=False)
35 changes: 19 additions & 16 deletions python/tests/test_3_offline.py
Original file line number Diff line number Diff line change
Expand Up @@ -440,31 +440,34 @@ def test_message_image(self, chat1, data, lp):
assert msg.is_image()
assert msg
assert msg.id > 0
assert os.path.exists(msg.filename)
assert os.path.exists(msg.file_path)
assert msg.filemime == "image/png"

@pytest.mark.parametrize(
("fn", "typein", "typeout"),
("stem", "ext", "typein", "typeout"),
[
("r", None, "application/octet-stream"),
("r.txt", None, "text/plain"),
("r.txt", "text/plain", "text/plain"),
("r.txt", "image/png", "image/png"),
("r", "", None, "application/octet-stream"),
("r", ".txt", None, "text/plain"),
("r", ".txt", "text/plain", "text/plain"),
("r", ".txt", "image/png", "image/png"),
],
)
def test_message_file(self, chat1, data, lp, fn, typein, typeout):
def test_message_file(self, chat1, data, lp, stem, ext, typein, typeout):
lp.sec("sending file")
fn = stem + ext
fp = data.get_path(fn)
msg = chat1.send_file(fp, typein)
assert msg
assert msg.id > 0
assert msg.is_file()
assert os.path.exists(msg.filename)
assert msg.filename.endswith(msg.basename)
assert os.path.exists(msg.file_path)
assert msg.file_path.endswith(ext)
assert msg.filename == fn
assert msg.filemime == typeout
msg2 = chat1.send_file(fp, typein)
assert msg2 != msg
assert msg2.filename != msg.filename
assert msg2.file_path != msg.file_path
assert msg2.filename == fn

def test_create_contact(self, acfactory):
ac1 = acfactory.get_pseudo_configured_account()
Expand Down Expand Up @@ -532,7 +535,7 @@ def test_import_export_on_unencrypted_acct(self, acfactory, tmp_path):
messages = chat2.get_messages()
assert len(messages) == 2
assert messages[0].text == "msg1"
assert os.path.exists(messages[1].filename)
assert os.path.exists(messages[1].file_path)

def test_import_export_on_encrypted_acct(self, acfactory, tmp_path):
passphrase1 = "passphrase1"
Expand Down Expand Up @@ -570,7 +573,7 @@ def test_import_export_on_encrypted_acct(self, acfactory, tmp_path):
messages = chat2.get_messages()
assert len(messages) == 2
assert messages[0].text == "msg1"
assert os.path.exists(messages[1].filename)
assert os.path.exists(messages[1].file_path)

ac2.shutdown()

Expand All @@ -587,7 +590,7 @@ def test_import_export_on_encrypted_acct(self, acfactory, tmp_path):
messages = chat2.get_messages()
assert len(messages) == 2
assert messages[0].text == "msg1"
assert os.path.exists(messages[1].filename)
assert os.path.exists(messages[1].file_path)

def test_import_export_with_passphrase(self, acfactory, tmp_path):
passphrase = "test_passphrase"
Expand Down Expand Up @@ -626,7 +629,7 @@ def test_import_export_with_passphrase(self, acfactory, tmp_path):
messages = chat2.get_messages()
assert len(messages) == 2
assert messages[0].text == "msg1"
assert os.path.exists(messages[1].filename)
assert os.path.exists(messages[1].file_path)

def test_import_encrypted_bak_into_encrypted_acct(self, acfactory, tmp_path):
"""
Expand Down Expand Up @@ -671,7 +674,7 @@ def test_import_encrypted_bak_into_encrypted_acct(self, acfactory, tmp_path):
messages = chat2.get_messages()
assert len(messages) == 2
assert messages[0].text == "msg1"
assert os.path.exists(messages[1].filename)
assert os.path.exists(messages[1].file_path)

ac2.shutdown()

Expand All @@ -688,7 +691,7 @@ def test_import_encrypted_bak_into_encrypted_acct(self, acfactory, tmp_path):
messages = chat2.get_messages()
assert len(messages) == 2
assert messages[0].text == "msg1"
assert os.path.exists(messages[1].filename)
assert os.path.exists(messages[1].file_path)

def test_set_get_draft(self, chat1):
msg = Message.new_empty(chat1.account, "text")
Expand Down
Loading

0 comments on commit b248bbf

Please sign in to comment.