URI-handling code update: added safety checks and notifications #1002

Diapolo commented at 1:21 PM on March 28, 2012: none

validate bitcoin address in the URI before switching to sendCoinsPage and pasting into the form, when a bitcoin: link is clicked
validate bitcoin address in the URI before switching to sendCoinsPage and pasting into the form, when a bitcoin: link is dropped on the Bitcoin-Qt window (Drag&Drop feature)
show a tray-notification if an URI could not be parsed to alert the user

laanwj commented at 2:22 PM on March 28, 2012: member

Good idea.

I think there should also be a notification if an invalid URI is sent, to aid web developers etc in testing (hey, where did my url silently go) Op 28 mrt. 2012 15:21 schreef "Philip Kaufmann" < reply@reply.github.com> het volgende:

Current state of this request:

added:

validate bitcoin address in the URL before switching to sendCoinsPage and pasting into the form, when a bitcoin: link is clicked

validate bitcoin address in the URL before switching to sendCoinsPage and pasting into the form, when a bitcoin: link is dropped on the Bitcoin-Qt window (Drag&Drop feature)

I verified both cases in my own build on Windows, which is based on RC5 master.

Dia

You can merge this Pull Request by running:

git pull https://github.com/Diapolo/bitcoin URL-handling_2

Or you can view, comment on it, or merge it online at:

#1002

-- Commit Summary --

initial re-work on URL-handling code

-- File Changes --

M src/qt/bitcoingui.cpp (22) M src/qt/guiutil.cpp (5) M src/qt/sendcoinsdialog.cpp (12) M src/qt/sendcoinsdialog.h (2)

-- Patch Links --

#1002.patch #1002.diff

Reply to this email directly or view it on GitHub: #1002

Diapolo commented at 7:41 PM on March 28, 2012: none

I'm going to add what you suggested :), sounds good.

Diapolo commented at 9:28 PM on March 28, 2012: none

I added message boxes. A hint would be nice, if I should call "it" URL or URI ;)? I'm also working on somewhat "hardening" the IPC code and I would prefer one of the two for every source comment, var name, function and so on.

Edit: I will take URI in the message boxes and where a user can see it and retain the current naming and not change code, where not needed! Will update the strings tomorrow...

laanwj commented at 5:23 AM on March 29, 2012: member

Nitpick: use notificator, not a message box

Re: URI or URL, I'm fine with either. Though URI is the most correct, more users know what URL means.

Diapolo commented at 5:30 AM on March 29, 2012: none

Cool idea with the notificator, will see how that looks :).

Diapolo commented at 5:43 AM on March 29, 2012: none

Changed to notificator, which is much better/smoother in terms of usability and usage flow.

laanwj commented at 7:04 PM on April 2, 2012: member

Can you please rebase this into one commit?

Diapolo commented at 7:27 PM on April 2, 2012: none

Done :).

in src/qt/bitcoingui.cpp:None in fbbdb2653e outdated

 730 | +
 731 | +        // if valid URLs were found
 732 | +        if (!nInvalidUrlsFound)
 733 | +            gotoSendCoinsPage();
 734 | +        else
 735 | +            notificator->notify(Notificator::Warning, tr("URI handling - drag & drop"), tr("%n URI(s) can not be parsed! This can be caused by (an) invalid address(es) or malformed parameters.", "", nInvalidUrlsFound));

laanwj commented at 6:32 AM on April 4, 2012:

I'd use the exact same message here as on line 752 as there is no real need to distinguish between "URI handling and URI handling - drag&drop", and no one drags more URLs at a time. Saves the translators some work.

Diapolo commented at 6:39 AM on April 4, 2012:

Agreed :). Thanks for your valuable suggestions.

Edit: Is it possible to paste more than 1 URI at all? I'm asking with the foreach in my mind.

in src/qt/bitcoingui.cpp:None in 6c4d77d3c9 outdated

 717 | @@ -718,12 +718,19 @@ void BitcoinGUI::dropEvent(QDropEvent *event)
 718 |  {
 719 |      if(event->mimeData()->hasUrls())
 720 |      {
 721 | -        gotoSendCoinsPage();
 722 | +        int nInvalidUrlsFound = 0;

luke-jr commented at 4:34 PM on April 6, 2012:

Uri!

in src/qt/bitcoingui.cpp:None in 6c4d77d3c9 outdated

 726 | -            sendCoinsPage->handleURL(url.toString());
 727 | +            if (!sendCoinsPage->handleURL(url.toString()))
 728 | +                nInvalidUrlsFound++;
 729 |          }
 730 | +
 731 | +        // if valid URLs were found

luke-jr commented at 4:35 PM on April 6, 2012:

URI!

Diapolo commented at 8:40 PM on April 6, 2012: none

Rebase to current master and all remaining URL / url were re-named to URI / uri ;).

in src/qt/guiutil.cpp:None in 8578a5d3de outdated

  56 | @@ -57,6 +57,11 @@ bool GUIUtil::parseBitcoinURI(const QUrl &uri, SendCoinsRecipient *out)
  57 |      if(uri.scheme() != QString("bitcoin"))
  58 |          return false;
  59 |  
  60 | +    // check if the address is valid
  61 | +    CBitcoinAddress addressFromUrl(url.path().toStdString());
  62 | +    if (!addressFromUrl.IsValid())

luke-jr commented at 1:51 PM on April 10, 2012:

These two need to be changed to URI to compile.

Diapolo commented at 5:39 PM on April 10, 2012: none

Rebased once more and fixed the 2 glitches luke observed :).

sipa commented at 2:14 PM on April 20, 2012: member

What does @TheBlueMatt think about this?

in src/qt/bitcoingui.cpp:None in f0a3e5c1ea outdated

 756 | +            if (!sendCoinsPage->handleURI(uri.toString()))
 757 | +                nInvalidUrisFound++;
 758 |          }
 759 | +
 760 | +        // if valid URIs were found
 761 | +        if (!nInvalidUrisFound)

TheBlueMatt commented at 8:03 PM on April 20, 2012:

I would think it would be better to handle any valid URIs and ignore invalid ones if there are valid ones present, but I dont really think many OSs will give a list of URIs unless you are dragging a folder full of files, and in that case, none of them will be bitcoin:, so it probably doesnt matter.

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

  58 | @@ -59,6 +59,11 @@ bool parseBitcoinURI(const QUrl &uri, SendCoinsRecipient *out)
  59 |      if(uri.scheme() != QString("bitcoin"))
  60 |          return false;
  61 |  
  62 | +    // check if the address is valid
  63 | +    CBitcoinAddress addressFromUri(uri.path().toStdString());
  64 | +    if (!addressFromUri.IsValid())
  65 | +        return false;
  66 | +

TheBlueMatt commented at 8:03 PM on April 20, 2012:

Though any current addresses in bitcoin: URIs must be valid, I could see a point where the address field doesnt matter (when transferring other data in more complicated payment schemes). Maybe move this check to handleURI before pasteEntry?

Diapolo commented at 11:33 AM on April 22, 2012:

If I would move this to handleURI, where I have an unprocessed QString instead of a pre-processed QUrl with "//" removed from "bitcoin://" this would bloat the code a little, so I don't like that idea.

laanwj commented at 2:26 PM on June 13, 2012:

@TheBlueMatt we could eventually go to representing all URIs as QStrings until the last moment before parsing. QUrls were great until the requirement came along that "bitcoin://" should work too... Qt mangles the address in this case, which kind of breaks the abstraction already...

luke-jr commented at 2:28 PM on June 13, 2012:

IMO, that's an "irrational user expectation", not a requirement :p

TheBlueMatt commented at 11:34 PM on April 20, 2012: member

Those are minor gripes anyway, ACK

Diapolo commented at 5:25 PM on April 21, 2012: none

@TheBlueMatt I'll look into your suggestions, even small glitches can be changed / fixed :).

Diapolo commented at 5:58 PM on April 29, 2012: none

Rebased once more, merged the 3 commits into a single one and re-worked a few days ago to include one of @TheBlueMatt suggestions.

Diapolo commented at 8:50 PM on May 8, 2012: none

Rebased and reworded the commit message!

Diapolo commented at 10:20 PM on May 12, 2012: none

Rebased and fixed merge-conflict. Can this get in please, if there are no further wishes?

laanwj commented at 7:39 AM on May 13, 2012: member

I think this one's waiting for URI support to be resurrected. Otherwise, it's kind of hard (and insensible) to test.

Diapolo commented at 10:33 AM on May 13, 2012: none

At least for Linux URI-support is in the client ;). And it's working fine with my build ^^, just wanted to bring this back into devs-mind.

TheBlueMatt commented at 1:57 PM on May 20, 2012: member

I agree with @Diapolo here, we have a huge list of pulls piling up (dont we always...) and I see little reason to not pull this because URIs are supported, just not on Windows or Mac.

Diapolo commented at 5:57 PM on May 24, 2012: none

If this would get in, I didn't need to rebase and keep this current, so ... :).

Diapolo commented at 3:01 PM on May 27, 2012: none

Updated to use toggleHidden() function for showing the window after a bitcoin-URI was clicked.

Diapolo commented at 1:28 PM on June 2, 2012: none

Rebased to fix a merge-conflict.

Diapolo commented at 2:02 PM on June 13, 2012: none

Rebased and changed back the use of toggleHidden() into showNormalIfMinimized() as another pull takes care of this. Can we please merge this with #1437!

URI-handling code update: added safety checks and tray-notifications 93b7af3099

Diapolo commented at 6:01 AM on June 14, 2012: none

Updated to fix a merge conflict.

laanwj referenced this in commit 64d46e7c6a on Jun 14, 2012

laanwj merged this on Jun 14, 2012

laanwj closed this on Jun 14, 2012

coblee referenced this in commit 470797bae1 on Jul 17, 2012

ptschip referenced this in commit 8430e4fedb on Apr 4, 2018

lateminer referenced this in commit 5666184cc5 on Dec 25, 2019

lateminer referenced this in commit d6573e70c7 on Dec 25, 2019

lateminer referenced this in commit 3f31b4fc38 on May 6, 2020

lateminer referenced this in commit 7fb724ffe4 on May 6, 2020

DrahtBot locked this on Sep 8, 2021