URLs containing a / after the address no longer cause parsing errors. #4622

rnicoll commented at 6:57 PM on August 2, 2014: contributor

In some cases under Windows a / is appended to bitcoin: URIs sent from browser to the client. This patch catches that case and removes the /.

rnicoll commented at 1:42 PM on August 3, 2014: contributor

Tested with:

bitcoin-qt -testnet bitcoin:n4dqmiBSWCTLnJRs4FXRxeHVAhZJhyPGCR bitcoin-qt -testnet bitcoin:n4dqmiBSWCTLnJRs4FXRxeHVAhZJhyPGCR/ bitcoin-qt -testnet bitcoin:n4dqmiBSWCTLnJRs4FXRxeHVAhZJhyPGCR/?amount=100

With 0.9.2 the latter two fail, with the revised patch all three are parsed as valid URIs and the prompt to complete payment is shown after startup.

laanwj commented at 1:46 PM on August 3, 2014: member

What are these cases? I'd like to know why this corruption happens in the first place. Do we do something wrong with escaping?

rnicoll commented at 1:50 PM on August 3, 2014: contributor

It's only something I'm seeing arising in Windows, but for some reason bitcoin: URIs are arriving with the client with a / on the end, as if they're being treated as URLs by some intermediary which rewrites them. Bitcoin is fine in theory, as the URI shouldn't have a / injected, but as it happens it seems practical to handle it.

The last test case was suggested as an extreme example, I haven't seen a / injected mid-URI, but as the code handles the possibility it's good to verify it.

laanwj commented at 1:52 PM on August 3, 2014: member

OK. Weird. I've never heard this reported before.

rnicoll commented at 1:54 PM on August 3, 2014: contributor

I'll come back with more detail later, under the wrong OS right now.

laanwj commented at 1:55 PM on August 3, 2014: member

Something 'intermediating' between the browser and Bitcoin Core does sound vaguely scary, by the way. I hope it's not a man in the middle attack :)

rnicoll commented at 1:57 PM on August 3, 2014: contributor

I meant it looks like the OS takes the URI and modifies it. Still, this is fairly much why the client verifies payments before they're sent, right? :)

cdecker commented at 3:52 PM on August 3, 2014: contributor

Does this have an impact on people (wrongfully) using URIs with the bitcoin:// prefix, or are these broken anyway? Splitting by '/' feels awkward when a more explicit if-else checking for the last char would have done the trick.

rnicoll commented at 12:25 AM on August 4, 2014: contributor

@cdecker No, the rewrite to replace "bitcoin://" with "bitcoin:" occurs before this step, at around line 167 of guiutil.cpp

rnicoll commented at 10:18 PM on August 4, 2014: contributor

Reconfirmed the problem under Windows 7 with Chrome, and I think found why it occurs...

Looking at https://chain.so/address/BTC/14nsgXjL7xCEXFf8UkGCm9KnSTTFBDKqcn (just a random address that's been used recently), the "Send Bitcoin" link has bitcoin:// in it, which somehow triggers a / being appended to the URI as well. Fixing the URI in the page does also resolve this.

laanwj commented at 12:54 PM on August 5, 2014: member

OK - they shouldn't be using bitcoin:// in the first place. But we remove the //, so it's fine with me to remove the trailing slash too.

I agree with @cdecker that splitting and then taking the first feels awkward though. I'd prefer a more specific if(uri.path().endsWith("/")) check.

laanwj added the label GUI on Aug 6, 2014

laanwj added the label Improvement on Aug 6, 2014

rnicoll commented at 9:39 PM on August 7, 2014: contributor

Not sure why we got a bunch of random commits first update... anyway, that should be it just removing any ending / now.

cdecker commented at 9:41 PM on August 7, 2014: contributor

Thanks, that looks a lot better.

in src/qt/guiutil.cpp:None in dec6c09805 outdated

 116 | @@ -117,6 +117,10 @@ bool parseBitcoinURI(const QUrl &uri, SendCoinsRecipient *out)
 117 |  
 118 |      SendCoinsRecipient rv;
 119 |      rv.address = uri.path();
 120 | +    // Trim any leading forward slash which may have been added by the OS
 121 | +    if (rv.address.endsWith("/")) {
 122 | +      rv.address.truncate(rv.address.length() - 1);

Diapolo commented at 5:50 AM on August 8, 2014:

Nit: Indentation (use 4 spaces).

in src/qt/guiutil.cpp:None in dec6c09805 outdated

 116 | @@ -117,6 +117,10 @@ bool parseBitcoinURI(const QUrl &uri, SendCoinsRecipient *out)
 117 |  
 118 |      SendCoinsRecipient rv;
 119 |      rv.address = uri.path();
 120 | +    // Trim any leading forward slash which may have been added by the OS

laanwj commented at 6:24 AM on August 8, 2014:

is this a leading or trailing forward slash? :imp:

laanwj commented at 6:26 AM on August 8, 2014: member

ACK after nit

URLs containing a / after the address no longer cause parsing errors. c7f3876d4a

rnicoll commented at 6:14 PM on August 8, 2014: contributor

Once more, with feeling...

BitcoinPullTester commented at 6:28 PM on August 8, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4622_c7f3876d4af37cfd6e587d0ab7ddbeaedda27857/ for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

laanwj merged this on Aug 11, 2014

laanwj closed this on Aug 11, 2014

laanwj referenced this in commit 85af3856e7 on Aug 11, 2014

rnicoll deleted the branch on Aug 29, 2014

MarcoFalke locked this on Sep 8, 2021