spesmilo / electrum

Electrum Bitcoin Wallet
https://electrum.org
MIT License
7.37k stars 3.07k forks source link

Suggestions for removing the embedded JavaScript files and depending on system installed versions #8023

Closed sorenstoutner closed 1 year ago

sorenstoutner commented 1 year ago

I am in the process of packaging the current release for Debian. As part of this process I would like to remove the embedded jQuery files and instead depend on system installed versions.

  1. The version of electrum/www/jquery-3.4.1.min.js is 3.4.1. The current version in Debian is 3.6.1. Are you aware of any problems that would arise by using a newer version with Electrum?
  2. The version of electrum/www/jquery-ui.min.js is 1.12.1. The current version in Debian is 1.13.2. Are you aware of any problems that would arise by using a newer version with Electrum?
  3. There are several packages in Debian that provide qrcode.js files. One is node-qrcode-generator, which is currently at version 1.4.4. Do you know if this is the source of your qrcode.js (your copy is minified and the one that ships with node-qrcode-generator is not, making it a little difficult to compare)?
  4. Do you know if there is any part of Electrum that uses jQuery besides electrum/www/index.html and electrum/www/pay?
  5. electrum/www/index.html and electrum/www/pay appear to be two identical copies of the same file. Are they used in different ways by the system?
  6. Probably most importantly, what would be the best way I could test the functionality of replacing these files? I have been an Electrum user for a while but I am not aware of how one gets to the part of the program that uses these files in the electrum/www directory.
sorenstoutner commented 1 year ago

Maybe this should have been the question I lead with, but are these files even used at all with the Linux version of Electrum? I know that at least some file are Android only and it occurred to me that might be the case for these.

SomberNight commented 1 year ago
  1. The version of electrum/www/jquery-3.4.1.min.js is 3.4.1. The current version in Debian is 3.6.1. Are you aware of any problems that would arise by using a newer version with Electrum?
  2. The version of electrum/www/jquery-ui.min.js is 1.12.1. The current version in Debian is 1.13.2. Are you aware of any problems that would arise by using a newer version with Electrum?

I expect newer versions should work.

  1. There are several packages in Debian that provide qrcode.js files. One is node-qrcode-generator, which is currently at version 1.4.4. Do you know if this is the source of your qrcode.js (your copy is minified and the one that ships with node-qrcode-generator is not, making it a little difficult to compare)?

No idea, sorry. Maybe @ecdsa remembers where the files are from. Feel free to open a PR against https://github.com/spesmilo/electrum-http and replace the files with ones that have known origin and are not minified. (how large is the size diff btw between non-minified and minified?)

  1. Do you know if there is any part of Electrum that uses jQuery besides electrum/www/index.html and electrum/www/pay?

The usage is local to that git submodule.

  1. electrum/www/index.html and electrum/www/pay appear to be two identical copies of the same file. Are they used in different ways by the system?

pay is a symlink to index.html

  1. Probably most importantly, what would be the best way I could test the functionality of replacing these files? I have been an Electrum user for a while but I am not aware of how one gets to the part of the program that uses these files in the electrum/www directory.

https://electrum.readthedocs.io/en/latest/merchant.html#create-and-use-your-merchant-wallet https://github.com/spesmilo/electrum-docs/blob/213fd697c0c0cdeb396ed13186416e3c020b93f0/merchant.rst#create-and-use-your-merchant-wallet

$ ./run_electrum -o setconfig payserver_address 127.0.0.1:9922
$ ./run_electrum daemon --testnet -d
$ ./run_electrum load_wallet -w ~/.electrum/testnet/wallets/test_segwit_2 --testnet
$ ./run_electrum add_request 0.00001 -m "test" -w ~/.electrum/testnet/wallets/test_segwit_2 --testnet
{
    "URI": "bitcoin:tb1q327fh2mle04q0ecrmd23j40tfdt7d4wy3vzcgn?amount=0.00001&message=test&time=1666188788&exp=3600",
    "address": "tb1q327fh2mle04q0ecrmd23j40tfdt7d4wy3vzcgn",
    "amount_BTC": "0.00001",
    "amount_msat": 1000000,
    "amount_sat": 1000,
    "can_receive": true,
    "expiration": 1666192388,
    "is_lightning": true,
    "lightning_invoice": "lntb10u1p34qp05pp5d9uh62zhf0w9453plmj2lmrv8rdnktarlte943csez4crvtv4c8ssp5r4affwn7q4h4sttcsfzct3z66g08waju6hpug9ghlmanc8pa93psdq8w3jhxaqcqzynxqrrss9q7sqqqqqqqqqqqqqqqqqqqqqqqqq9qsqfppq327fh2mle04q0ecrmd23j40tfdt7d4wyrzjq2lc9c30n8wd0tqauj4d29fvuj8sd9xydmzcy4nlx70q4klcrcks7gkm7yqqq2gqqyqqqqlgqqqqqeqqjqtz9q2lc9c30n8wd0tqauj4d29fvuj8sd9xydmzcy4nlx70q4klcrcks7qqqq05qqqqqvsqfqgn60fer9m2ws95lp22aypkwm4tuhf7fgx7t8esce2n0nqh7y58erw6z58ha05nt630j546s4sfrwsf6dmtvj3chsmxqrpk2ehw4eydqp4uvhx4",
    "message": "test",
    "request_id": "69797d28574bdc5ad221fee4afec6c38db3b2fa3faf25ac710c8ab81b16cae0f",
    "rhash": "69797d28574bdc5ad221fee4afec6c38db3b2fa3faf25ac710c8ab81b16cae0f",
    "status": 0,
    "status_str": "Expires in about 1 hour",
    "timestamp": 1666188788,
    "view_url": "http://127.0.0.1:9922/r/pay?id=69797d28574bdc5ad221fee4afec6c38db3b2fa3faf25ac710c8ab81b16cae0f"
}

Maybe this should have been the question I lead with, but are these files even used at all with the Linux version of Electrum? I know that at least some file are Android only and it occurred to me that might be the case for these.

They are only used if the payserver_address config key is set.


Note that while testing this just now, I have realised we mostly broke this functionality in electrum 4.3. Pushed a fix in https://github.com/spesmilo/electrum/commit/0fc90e07c95243c7ea0c646b394f4d3ed92c5ff2 I've also changed the behaviour just now that even if payserver_address is set, if the www dir is missing, instead of a critical error, the process simply logs an error (https://github.com/spesmilo/electrum/commit/60769e47692d06adc6351d33aa584bcfd359c6bb).

sorenstoutner commented 1 year ago

Thank you for the information. It is very helpful.

In the tarball, pay and index.html are individual files. My guess is that something in the tarball generation process converted the symlink into a copy of the original.

The minified qrcode.js is 19.3 KiB. The non-minified one from node-qrcode-generator is 55.4 KiB. However, I do not yet know if they are functionally equivalent.

I will spend some time digging into this further and post an update here and possibly on electrum-http.

ecdsa commented 1 year ago

sorry, no, I do not remember where the files are from

sorenstoutner commented 1 year ago

I am trying to test my Debian packages. First, I am trying to test a build that has the bundled JavaScript files and then I will test one that replaces them with system files. Following the instructions above don't produce an error, but they also don't produce a URL I can use to test the JavaScript and QR code generation.

soren@soren-laptop:~$ electrum -o setconfig payserver_address 127.0.0.1:9922
true
soren@soren-laptop:~$ electrum daemon --testnet -d
starting daemon (PID 12654)
soren@soren-laptop:~$ electrum load_wallet -w ~/.electrum/testnet/wallets/default_wallet --testnet
true
soren@soren-laptop:~$ electrum add_request 0.00001 -m "test" -w ~/.electrum/testnet/wallets/default_wallet --testnet
{
    "URI": "bitcoin:tb1q0666d4tde7ny554av6974qexc3lkr6qv7dwgaw?amount=0.00001&message=test&time=1666291739&exp=3600",
    "address": "tb1q0666d4tde7ny554av6974qexc3lkr6qv7dwgaw",
    "amount_BTC": "0.00001",
    "amount_msat": 1000000,
    "amount_sat": 1000,
    "can_receive": false,
    "expiration": 1666295339,
    "is_lightning": true,
    "lightning_invoice": "lntb10u1p34rxqmpp5nrhzhcaes2eyw4vhc9lsr07fg8fz4s54nyv4z7z24dkg5mcqrs6qsp55qcm0sh3ysd67xq2g3e33n42kg5m7dkdz3yqwycwu948aramuj0sdq8w3jhxaqcqzynxqrrss9q7sqqqqqqqqqqqqqqqqqqqqqqqqq9qsqfppq0666d4tde7ny554av6974qexc3lkr6qv2jtqzllxzvq7fh4mvjfd9wf9dpfdfrvleywt6k7tsgaqaafal9d3ga59rfyssln3stgy09lkk6fqwfashtp6fq6yxlzy9uh6ex608jcp9qwsp5",
    "message": "test",
    "request_id": "98ee2be3b982b2475597c17f01bfc941d22ac295991951784aab6c8a6f001c34",
    "rhash": "98ee2be3b982b2475597c17f01bfc941d22ac295991951784aab6c8a6f001c34",
    "status": 0,
    "status_str": "Expires in about 1 hour",
    "timestamp": 1666291739
}
SomberNight commented 1 year ago

oops, $ ./run_electrum -o setconfig payserver_address 127.0.0.1:9922 also need the --testnet flag, otherwise it is modifying the mainnet config file

so e.g.:

$ ./run_electrum --testnet -o setconfig payserver_address 127.0.0.1:9922

sorry, I did not fully test the whole snippet I wrote, evidently :)

sorenstoutner commented 1 year ago

Thank you. After fixing the pay symlink problem described at https://github.com/spesmilo/electrum/issues/8030 I was able to display the URL. However, the contents of the page were mostly blank. Is this what you meant by this functionality being mostly broken in 4.3?

Empty Request

SomberNight commented 1 year ago

However, the contents of the page were mostly blank. Is this what you meant by this functionality being mostly broken in 4.3?

yes. should be fixed by https://github.com/spesmilo/electrum-http/commit/ebcff4f6bd695f0c664e2bce6d99eb3824b0d234

sorenstoutner commented 1 year ago

I am trying to use symbolic links to redirect the Electrum web server to system resources. For example, I am trying to replace electrum/www/jquery-ui.min.js with a symbolic link to ../../../../../share/javascript/jquery-ui/jquery-ui.min.js. However, Firefox's network inspector shows that this returns a 404 error. My guess is that Electrum's webserver is programmed to not follow links. Does anyone know how difficult it would be to modify this behavior so that it does follow symbolic links?

SomberNight commented 1 year ago

Well, as said before, normally pay is a symlink to index.html, so I don't think the issue is due to not following symlinks in general.

Perhaps it does not follow symlinks that traverse out from the basedir of index.html? Maybe test with a symlink that points to a file inside electrum/www first, to try to narrow down the cause.

sorenstoutner commented 1 year ago

That is a very good point. You are correct. Testing shows that it will follow symlinks as long as the target is inside electrum/www. But if the target is in a parent directory it will not follow them. For example, if I put jquery-ui.min.js directly in electrum and then create a symlink from electrum/www/jquery-ui.min.js that points to ../jquery-ui.min.js it will fail with a 404 error.

Do you know if there is an easy way to set Electrum's webserver to follow symlinks outside of the www directory? I would really prefer to not place copies of these JavaScript files inside of Electrum's package. Not only does that waste space by having duplicate files on a Debian system, but it also creates an extra location for JQuery files that has to be monitored for security updates.

For a little bit of reference on Debian's philosophy regarding embedded copies of system files you can see https://wiki.debian.org/EmbeddedCopies

SomberNight commented 1 year ago

Looking at aiohttp docs, I guess this might fix it:

diff --git a/electrum/daemon.py b/electrum/daemon.py
index 46b0729064..4701bd80dd 100644
--- a/electrum/daemon.py
+++ b/electrum/daemon.py
@@ -393,7 +393,7 @@ class PayServer(Logger, EventListener):
         app.add_routes([web.get('/api/get_invoice', self.get_request)])
         app.add_routes([web.get('/api/get_status', self.get_status)])
         app.add_routes([web.get('/bip70/{key}.bip70', self.get_bip70_request)])
-        app.add_routes([web.static(root, self.WWW_DIR)])
+        app.add_routes([web.static(root, self.WWW_DIR, follow_symlinks=True)])
         if self.config.get('payserver_allow_create_invoice'):
             app.add_routes([web.post('/api/create_invoice', self.create_request)])
         runner = web.AppRunner(app)
sorenstoutner commented 1 year ago

I can confirm that fixes the problem, thank you. Is this something you are comfortable adding to your source code or would you prefer that I modify the Debian copy?

SomberNight commented 1 year ago

see https://github.com/spesmilo/electrum-http/commit/a9cdb5436f79c0577045274194c0473944cdd2a8 and https://github.com/spesmilo/electrum/commit/32ce64faa52fc03b8a3c93289d43c30bd94c2a84

sorenstoutner commented 1 year ago

Thank you.

SomberNight commented 1 year ago

btw I've found the ~origin of the vendored libs: https://github.com/spesmilo/electrum/commit/3da148f4063f2910aedf443f06a50946903e91de (plus I guess later commits changed the versions) the hint is that this www stuff was originally in this repo (electrum), then moved to electrum-merchant, and then to electrum-http

sorenstoutner commented 1 year ago

If I may make a suggestion, it is generally considered good practice to include non-minified JavaScript in your source code next to the minified copies. So, for example, if you include jquery-ui.min.js you should place jquery-ui.js in the same directory. Your web server can use the minified version to save on bandwidth, but that way the original source code is available in case there is ever a question about what the JavaScript is doing.

Regarding qrcode.js, the version you have included does not seem to be actively maintained. Perhaps that is because not much maintenance is needed, but the last commit was 7 years ago.

https://github.com/davidshimjs/qrcodejs

You might consider switching to a different implementation, like https://github.com/kazuhikoarase/qrcode-generator The last commit to this repo was two years ago, and it appears to be a more developed project, including test suites and implementations in various programming languages. From my perspective, it also has the advantage of already being packaged in Debian, which means that any security fixes would be automatic on my end.

sorenstoutner commented 1 year ago

I am in the process of packaging and testing 4.3.3 for Debian, and am having a problem similar to one I had last October, but the fix then (adding --testnet to the first command) doesn't resolve the issue. Specifically, when attempting to test the payserver it does not produce a URL.

soren@soren-laptop:~$ electrum --testnet -o setconfig payserver_address 127.0.0.1:9922
true
soren@soren-laptop:~$ electrum daemon --testnet -d
starting daemon (PID 12479)
soren@soren-laptop:~$ electrum load_wallet -w ~/.electrum/testnet/wallets/default_wallet --testnet
true
soren@soren-laptop:~$ electrum add_request 0.00001 -m "test" -w ~/.electrum/testnet/wallets/default_wallet --testnet
{
    "URI": "bitcoin:tb1q0666d4tde7ny554av6974qexc3lkr6qv7dwgaw?amount=0.00001&message=test&time=1673562195&exp=3600",
    "address": "tb1q0666d4tde7ny554av6974qexc3lkr6qv7dwgaw",
    "amount_BTC": "0.00001",
    "amount_msat": 1000000,
    "amount_sat": 1000,
    "can_receive": false,
    "expiration": 1673565795,
    "is_lightning": true,
    "lightning_invoice": "lntb10u1p3upzznpp575kht5v03lhykv7aqs95ps9dcv3e2kyrp9qd5cp3dzydvmtajd6qsp54y0j4mxq650873qgxucahpaqsnydhvl5v9lzs22dkl9qu0asqdyqdq8w3jhxaqcqzynxqrrss9q7sqqqqqqqqqqqqqqqqqqqqqqqqq9qsqfppq0666d4tde7ny554av6974qexc3lkr6qvrrw8ryp6gg4ke0rq5exwjejtu4ljcl5lcsq8nqahm7xcz3xd88xp0htnwrhvz4wgskekn55xk2z9qq79csuv0xgn7p4xy90gfpywmlgp50jwfu",
    "message": "test",
    "request_id": "f52d75d18f8fee4b33dd040b40c0adc3239558830940da60316888d66d7d9374",
    "rhash": "f52d75d18f8fee4b33dd040b40c0adc3239558830940da60316888d66d7d9374",
    "status": 0,
    "status_str": "Expires in about 1 hour",
    "timestamp": 1673562195,
    "tx_hashes": []
}
SomberNight commented 1 year ago

The payserver was turned into a plugin in 4.3.3, so it is not loaded by default anymore. To enable it, you now need to also set the use_payserver config key (or use the GUI and tick the checkbox in the Plugins dialog).

$ ./run_electrum --testnet -o setconfig use_payserver true
$ ./run_electrum --testnet -o setconfig payserver_address 127.0.0.1:9922
$ ./run_electrum daemon --testnet -d
$ ./run_electrum load_wallet -w ~/.electrum/testnet/wallets/test_segwit_2 --testnet
$ ./run_electrum add_request 0.00001 -m "test" -w ~/.electrum/testnet/wallets/test_segwit_2 --testnet
sorenstoutner commented 1 year ago

There still seems to be some piece I am missing.


soren@soren-laptop:~$ electrum --testnet -o setconfig use_payserver true
true
soren@soren-laptop:~$ electrum --testnet -o setconfig payserver_address 127.0.0.1:9922
true
soren@soren-laptop:~$ electrum daemon --testnet -d
starting daemon (PID 26762)
soren@soren-laptop:~$ electrum load_wallet -w ~/.electrum/testnet/wallets/default_wallet --testnet
true
soren@soren-laptop:~$ electrum add_request 0.00001 -m "test" -w ~/.electrum/testnet/wallets/default_wallet --testnet
{
    "URI": "bitcoin:tb1qqqejnd8t5h0vhywjxgshtzmun93dqrmc8npd2c?amount=0.00001&message=test&time=1673631152&exp=3600",
    "address": "tb1qqqejnd8t5h0vhywjxgshtzmun93dqrmc8npd2c",
    "amount_BTC": "0.00001",
    "amount_msat": 1000000,
    "amount_sat": 1000,
    "can_receive": false,
    "expiration": 1673634752,
    "is_lightning": true,
    "lightning_invoice": "lntb10u1p3ur9dspp57ykt2w05mezn8lz94hlpz6v6ey5xclpmdqcgr92jfapf9sukfk0qsp5t8t5t4dkvngfkaz20ffdnch3hcmfp28zxxup7wklmr53lwxcvv8sdq8w3jhxaqcqzynxqrrss9q7sqqqqqqqqqqqqqqqqqqqqqqqqq9qsqfppqqqejnd8t5h0vhywjxgshtzmun93dqrmcten57uv5ykr5vx634hjr76qnxz8t7459fwpspdlqx0a7zuxgnz24ffrwprh4a5nk7hmd4nyqa9h9eauhgz3p7cmp7pc6djj9qrhe68qp8vzgnc",
    "message": "test",
    "request_id": "f12cb539f4de4533fc45adfe11699ac9286c7c3b68308195524f4292c3964d9e",
    "rhash": "f12cb539f4de4533fc45adfe11699ac9286c7c3b68308195524f4292c3964d9e",
    "status": 0,
    "status_str": "Expires in about 1 hour",
    "timestamp": 1673631152,
    "tx_hashes": []
}
ecdsa commented 1 year ago

This should work, though. -Make sure you stopped the previous daemon: electrum --testnet stop

The current CLI fails to display "Daemon already running" if there is already an instance running.

sorenstoutner commented 1 year ago

That was it. Thank you.

ecdsa commented 1 year ago

Great. We should definitely fix that CLI issue...

sorenstoutner commented 1 year ago

I have successfully dealt with all the minified JavaScript in the upcoming 4.3.3 Debian package except for the qrcode.js file.

In the case of JQuery, JQuery UI, and the JQuery Trontastic theme I remove the files you ship and depend on the existing Debian packages for those projects, which include both the minified JavaScript and the original source.

qrcode.js is more problematic. Despite the file name not including "min", the version shipped in Electrum is minified.

The upstream source contains both a minified and an ostensibly non-minified version.

https://github.com/davidshimjs/qrcodejs

However, the non-minified version of the file contains a large segment of minified code on lines 87-152. As such, it is not appropriate for inclusion in Debian unless there is a non-minified version available.

Debian does contain a different QR Code JavaScript library:

https://github.com/kazuhikoarase/qrcode-generator

This library shares a common origin with the one you are currently using, as both of them contain the following text at the beginning of the file:

//---------------------------------------------------------------------
//
// QR Code Generator for JavaScript
//
// Copyright (c) 2009 Kazuhiko Arase
//
// URL: http://www.d-project.com/
//
// Licensed under the MIT license:
//  http://www.opensource.org/licenses/mit-license.php
//
// The word 'QR Code' is registered trademark of
// DENSO WAVE INCORPORATED
//  http://www.denso-wave.com/qrcode/faqpatent-e.html
//
//---------------------------------------------------------------------

However, the API is different enough that it cannot be used as a drop-in replacement for your current qrcode.js file.

Is there any way you could a) rebase your payserver to use the qrcode.js from https://github.com/kazuhikoarase/qrcode-generator, or b) find a non-minified version of the qrcode.js file you are currently using?

Debian doesn't mind using minified JavaScript in production to cut down on network bandwidth, but they consider it to be a compiled file and require that the source version be available as part of the package creation.

ecdsa commented 1 year ago

I have fixed the lack of "Daemon already running" detection: fbf79b148b29ecf99a704947d999190cdbb4e517 It might have some false positives (if an orphaned lockfile remains after the daemon was killed), but I think that is better than the previous situation.

ecdsa commented 1 year ago

@sorenstoutner I will try to use the qrcode.jsfile you suggested.

ecdsa commented 1 year ago

I replaced that file, see: https://github.com/spesmilo/electrum-http/commit/fc112140271033131ab09927aa02d523d330d568 The submodule update is in the electrum repo is 2c0dd0deb0a9fdfde4efdfc6e6afa74ccc80d9ee

sorenstoutner commented 1 year ago

That you for doing this. I would like you to know that I appreciate how easy it has been to work with Electrum's upstream.

sorenstoutner commented 1 year ago

I just successfully tested the payserver with the Debian 4.3.4 package, meaning that this will probably be the first Debian release that successfully runs the payserver. Thank you.