BitcoinQT downcases bitcoin://… address on OSX #11645

issue Sjors openend this issue on November 9, 2017
  1. Sjors commented at 4:21 pm on November 9, 2017: member

    When entering bitcoin://1F1tAaz5x1HUXrCNLbtMDqcw6o5GNn4xqX in the Safari address bar, it opens BitcoinQT, but then says:

    Dev hint: try RCDefault in order to make OSX open these links using a custom build.

    Tested with v0.15.1rc1 as well as master. Using QT 5.8.0.

    This doesn’t happen when I start QT from the command line: Bitcoin-Qt bitcoin://1F1tAaz5x1HUXrCNLbtMDqcw6o5GNn4xqX

    It does happen if I use: open bitcoin://1F1tAaz5x1HUXrCNLbtMDqcw6o5GNn4xqX

    So it’s not a Safari issue.

    Using log statements I found that this downcase happens somewhere before bool parseBitcoinURI in guiutil.cpp. This function actually strips // from the URI because:

    0//    Cannot handle this later, because bitcoin:// will cause Qt to see the part after // as host,
    

    See also #854.

    This function is called from PaymentServer::handleURIOrFile in paymentserver.cpp.

    Before that bool PaymentServer::eventFilter uses:

    0      QFileOpenEvent *fileEvent = static_cast<QFileOpenEvent*>(event);
    1      ...
    2          handleURIOrFile(fileEvent->url().toString());
    

    Lower case already happened here.

    If I use a single slash, there’s no downcase, altough the UI then complains about the initial slash.

    So perhaps something changed in QT and we need to do this earlier. Or it’s OSX that’s broken.

    This does behave, suggesting that indeed something is treating the address as a host name: open bitcoin:1F1tAaz5x1HUXrCNLbtMDqcw6o5GNn4xqX

    QT documentation for QUrl suggests this is expected behavior:

    Note that the case folding rules in Nameprep, which QUrl conforms to, require host names to always be converted to lower case, regardless of the Qt::FormattingOptions used.

    That remark has been in their documentation more or less forever.

    I can’t find a workaround, unless there’s some way to get additional information out of QFileOpenEvent.

  2. Sjors commented at 8:06 pm on November 9, 2017: member
    On the bright side, bech32 addresses are lower case…
  3. fanquake added the label MacOSX on Nov 9, 2017
  4. laanwj added the label GUI on Nov 13, 2017
  5. laanwj commented at 1:13 pm on November 13, 2017: member
    I don’t know what to be more surprised about, that this issue exists or that it has been the case forever and no one stumbled on this, or at least reported this before.
  6. Sjors commented at 1:31 pm on November 13, 2017: member
    Perhaps a subtle change in OSX or a recent update to QT made it behave as documented? Comments in the source code suggest it did work back in the day, otherwise why bother with all the OSX specific code?
  7. luke-jr commented at 1:36 pm on November 13, 2017: member
    Note that bitcoin://address is not a valid BIP21 URI. It should be bitcoin:address, which I suspect you’ll find works okay since it’s not being treated as a hostname…
  8. Sjors commented at 2:05 pm on November 13, 2017: member

    @luke-jr when I typedbitcoin:address into Safari it didn’t switch to the BitcoinQT, it just started googling it. Only adding // triggered the app switching. This is probably not a real use case though.

    When bitcoin:address is used as a link in a proper website, the // isn’t needed.

    open bitcoin:address indeed works.

    The wallet could perhaps throw a more informative error if // was used, but I’m not sure if that’s worth fixing. Given that BIP-21 doesn’t even allow // I’ll just close this.

  9. Sjors closed this on Nov 13, 2017

  10. jonasschnelli commented at 6:50 pm on November 13, 2017: contributor
    I think we should keep that open… it looks like that we have to fix something here.
  11. luke-jr commented at 7:01 pm on November 13, 2017: member
    Shrug… invalid URIs don’t work. No big deal either way IMO.
  12. sipa commented at 7:04 pm on November 13, 2017: member
    If somehow an invalid URI is passed (whether it looks like a URL or not) due to an overly broad OS level filter, we should at least give an error message, no?
  13. jonasschnelli commented at 7:23 pm on November 13, 2017: contributor
    Agree with @sipa. I could imagine that the way how we collect the URI from the event (In Qt) may be deprecated… I’m currently analysing.
  14. jonasschnelli commented at 7:51 pm on November 13, 2017: contributor

    RFC 3986 states:

    0   The host subcomponent of authority is identified by an IP literal
    1   encapsulated within square brackets, an IPv4 address in dotted-
    2   decimal form, or a registered name.  The host subcomponent is case-
    3   insensitive.
    

    and

    0Some schemes define additional subcomponents that consist of case-
    1   insensitive data, giving an implicit license to normalizers to
    2   convert this data to a common case (e.g., all lowercase).  For
    3   example, URI schemes that define a subcomponent of path to contain an
    4   Internet hostname, such as the "mailto" URI scheme, cause that
    5   subcomponent to be case-insensitive and thus subject to case
    6   normalization (e.g., "mailto:Joe@Example.COM" is equivalent to
    7   "mailto:Joe@example.com", even though the generic syntax considers
    8   the path component to be case-sensitive).
    

    and

    0When a URI uses components of the generic syntax, the component
    1   syntax equivalence rules always apply; namely, that the scheme and
    2   host are case-insensitive and therefore should be normalized to
    3   lowercase.  For example, the URI <HTTP://www.EXAMPLE.com/> is
    4   equivalent to <http://www.example.com/>.  The other generic syntax
    5   components are assumed to be case-sensitive unless specifically
    6   defined otherwise by the scheme (see Section 6.2.3).
    

    Would that mean the concept flaw is in BIP21? AFAIK there is no guarantee by RFC3986 that the address (either it’s a host or an additional subcomponent) needs to be handles case sensitive.

    Not sure if there is a simple workaround in our code nor would it make sense to try to fix this upstream (because the code there seems to respect RFC3986)

  15. jonasschnelli commented at 8:04 pm on November 13, 2017: contributor

    I think BIP21 is correct because the additional subcomponent bitcoin:<subcomp> is case-sensitive. The problem just comes with bitcoin://<host> (with the double slashes). There, RFC3986 says it’s case-insensitive and we need to expect a to-lowercase-normalised string.

    The main problem in our source code is probably that we auto-correct bitcoin:// to bitcoin: which does not work (reliable) at our level.

  16. Sjors reopened this on Nov 13, 2017

  17. sipa commented at 8:09 pm on November 13, 2017: member
    Indeed, we should just treat incoming ‘bitcoin://’ as invalid-BIP21. No need to deal with lowercase or whatever; it’s just not a valid URI.
  18. Sjors commented at 8:11 pm on November 13, 2017: member
    Only thing I’d like to check though is how iOs app switching works. I vaguely remember that it requires the // to work. As in, a Bitcoin app needs to register bitcoin://. If that’s true then some websites might put in there. In that case it would be nice to support it if it’s doable. Unfortunately I’m without iPhone for a few more days…
  19. Sjors commented at 8:46 pm on November 13, 2017: member
    I just tried this in the iOs Simulator. It’s the same behavior as on a Mac; in Safari address bar you need to use bitcoin://, but a link can just use bitcoin: (e.g. try the BitPay demo page). I can double check on a device later, but I suspect it will work.
  20. luke-jr commented at 0:39 am on November 14, 2017: member

    What @jonasschnelli said.

    Some schemes define additional subcomponents that consist of case- insensitive data, giving an implicit license to normalizers to convert this data to a common case (e.g., all lowercase).

    The bitcoin: scheme does not define this property for the address.

  21. laanwj commented at 8:24 am on November 14, 2017: member

    Indeed, we should just treat incoming ‘bitcoin://’ as invalid-BIP21. No need to deal with lowercase or whatever; it’s just not a valid URI.

    Yes. The bitcoin:// workaround seems a bad idea. I’m not sure why it was done at the time (maybe some popular site was doing that?), but after all this time I would hold people to follow BIP21 to the letter.

  22. Sjors commented at 1:00 pm on November 14, 2017: member
    Should we remove that workaround (and/or the incorrect comment about //) from the code in that case?
  23. jonasschnelli commented at 7:02 am on November 15, 2017: contributor
    @Sjors: Yes. A PR would be welcome. Ideally an error message gets spit in case of a bitcoin:// (maybe a specific one because we did support it partially in the past).
  24. laanwj referenced this in commit 310dc61ea3 on Mar 21, 2018
  25. jnewbery commented at 8:29 pm on March 27, 2018: member
    Can we close this now that #12723 is merged?
  26. MarcoFalke closed this on Mar 27, 2018

  27. PastaPastaPasta referenced this in commit 898423ccad on Dec 16, 2020
  28. PastaPastaPasta referenced this in commit 8f6228bb2c on Dec 18, 2020
  29. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 12:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me