URLs containing a / after the address no longer cause parsing errors. #4622
Conversation
|
Tested with: bitcoin-qt -testnet bitcoin:n4dqmiBSWCTLnJRs4FXRxeHVAhZJhyPGCR 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. |
|
What are these cases? |
|
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. |
|
OK. Weird. I've never heard this reported before. |
|
I'll come back with more detail later, under the wrong OS right now. |
|
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 :) |
|
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? :) |
|
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. |
|
@cdecker No, the rewrite to replace "bitcoin://" with "bitcoin:" occurs before this step, at around line 167 of guiutil.cpp |
|
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. |
|
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 |
|
Not sure why we got a bunch of random commits first update... anyway, that should be it just removing any ending / now. |
|
Thanks, that looks a lot better. |
| @@ -117,6 +117,10 @@ bool parseBitcoinURI(const QUrl &uri, SendCoinsRecipient *out) | |||
|
|
|||
| SendCoinsRecipient rv; | |||
| rv.address = uri.path(); | |||
| // Trim any leading forward slash which may have been added by the OS | |||
| if (rv.address.endsWith("/")) { | |||
| rv.address.truncate(rv.address.length() - 1); | |||
|
ACK after nit |
|
Once more, with feeling... |
|
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4622_c7f3876d4af37cfd6e587d0ab7ddbeaedda27857/ for binaries and test log. |
c7f3876 URLs containing a / after the address no longer cause parsing errors. (Ross Nicoll)
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 /.