Page MenuHomePhabricator

[electrum] add a timeout and max data size when downloading a payment request
ClosedPublic

Authored by PiRK on Jul 3 2024, 15:50.

Details

Summary

Limit the size of payment requests to 1 MB, which should be plenty enough. Add also a 2 x 30 s timeout (first timeout for the initial request, second timeout for downloading the data chunks).

Depends on D16417

Test Plan

python test_runner.py

I tested that a unit test fails if the data is garbage, with the following patch, to ensure coverage for the new way of getting the data.

diff --git a/electrum/electrumabc/paymentrequest.py b/electrum/electrumabc/paymentrequest.py
index 8c90cf8b70..18f0a1cad1 100644
--- a/electrum/electrumabc/paymentrequest.py
+++ b/electrum/electrumabc/paymentrequest.py
@@ -116,7 +116,7 @@ def get_payment_request(url):
     data = b""
     start = time.time()
     for chunk in response.iter_content(1024):
-        data += chunk
+        data += b"garbage"
         if len(data) > 1_000_000:
             return PaymentRequest(data=None, error="oversized payment request data")
         if time.time() - start > timeout:

Run the application, paste this payment URI in the "pay to" field and check for the expected error message "oversized payment request data" after a short time.
ecash:?r=http://212.183.159.230/512MB.zip

Redo this test after bumping the max size from 1_000_000 to 100_000_000 and check for the expected error dialog about the timeout.

Diff Detail

Repository
rABC Bitcoin ABC
Branch
electrum_limit_bip72_wqdl
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 29464
Build 58464: Build Diffelectrum-tests
Build 58463: arc lint + arc unit

Event Timeline

Fabien added inline comments.
electrum/electrumabc/paymentrequest.py
109 ↗(On Diff #48485)

bump it, you don't want spurious timeouts due to network latency

139 ↗(On Diff #48485)

Not requesting changes, but why is there a print_error on success ?

electrum/electrumabc/paymentrequest.py
122 ↗(On Diff #48485)

Note that len(...) has O(1) complexity in python. The len is kept as a counter internally.

139 ↗(On Diff #48485)

print_error is used all over this codebase for logging. Some day I will fix this by backporting https://github.com/spesmilo/electrum/pull/5296, but it always seemed like a low priority task

PiRK edited the summary of this revision. (Show Details)

pass the max_size as an argument for the get_payment_request function to make it testable, add a unit test for the oversized PR, bump the timeout

Unfortunately the timeout behaviour is harder to test in a unit test.

PiRK published this revision for review.Jul 3 2024, 18:43
Fabien requested changes to this revision.Jul 3 2024, 19:02
Fabien added inline comments.
electrum/electrumabc/paymentrequest.py
100

please use 50000 for consistency with the node (see BIP70_MAX_PAYMENTREQUEST_SIZE)

This revision now requires changes to proceed.Jul 3 2024, 19:02

use MAX_PAYMENTREQUEST_SIZE = 50000

This revision is now accepted and ready to land.Jul 3 2024, 21:37