Filter out whitespace and zero-width non-breaking spaces in address field validator #1329

pull laanwj wants to merge 2 commits into bitcoin:master from laanwj:2012_05_addrremovewhitespace changing 1 files +19 −9

laanwj commented at 10:13 AM on May 17, 2012: member
- Fixes issues with copy/pasting from web or html emails (#1325)
laanwj commented at 2:33 PM on May 17, 2012: member

If everyone thinks the other correction substitutions are a bad idea, those can be removed, let me know....
gmaxwell commented at 3:07 AM on May 18, 2012: contributor

I'd really rather do something like highlight them in red in the interface rather than silently correct them. But maybe thats just me?

I don't think we should correct them for the same reason that we don't try all 1596 one character variations when an address is invalid to see if any give a proper hash even though it would be computationally cheap: Because its not that unlikely to correct to the wrong thing and/or the detection of a single typo may indicate multiple typos. ... and once coin is sent to a bogus address it is absolutely irreparably unrecoverable.

I figured that the confusion resistant base was to reduce the risk of badguys creating lookalike addresses and to make typos more likely to be detected (it's not uncommon for our weak 'checksum' to permit typos), not for silent automatic correction.
laanwj commented at 6:59 AM on May 18, 2012: member

I've removed the correction. After the last commit, the O and 0 and such can no longer be typed the address field and it will only accept true base58 characters (and whitespaces, which are deleted).
in src/qt/bitcoinaddressvalidator.cpp:None in 402aa33f94 outdated
```
  45 |              break;
  46 |          }
  47 | +        // Remove whitespace
  48 | +        if(ch.isSpace())
  49 | +            removeChar = true;
  50 | +        // To next character
```
luke-jr commented at 7:36 PM on May 18, 2012:

This should be moved into the for loop, IMO:

for(unsigned int idx = 0; idx < input.size(); removeChar ? input.remove(idx, 1) : ++idx)

(also changed idx type to unsigned since it is compared with .size())
laanwj commented at 8:05 PM on May 18, 2012:

QString::size() returns an int, not an unsigned int: http://qt-project.org/doc/qt-4.8/qstring.html#size

IMO it is more readable this way. I don't like complex expressions in the third clause of a for
luke-jr commented at 7:37 PM on May 18, 2012: member

Maybe it should pop up an error message explaining that it's ignoring it?
laanwj commented at 8:08 PM on May 18, 2012: member

Maybe. It'd be nice in general to be more verbose with validation, and give useful hints. But that's not the goal for this pull.

In the case of zero-width spaces, the users don't even know the characters are in there, so there is no reason to tell them about it either. It should just work.
Filter out whitespace and zero-width non-breaking spaces in validator
- Fixes issues with copy/pasting from web or html emails (#1325)
25047eb3e9
Remove autocorrection of 0/i in addresses in UI ce7896070c
in src/qt/bitcoinaddressvalidator.cpp:None in 402aa33f94 outdated
```
  20 | @@ -21,21 +21,28 @@
  21 |  QValidator::State BitcoinAddressValidator::validate(QString &input, int &pos) const
  22 |  {
  23 |      // Correction
```
gavinandresen commented at 11:28 PM on May 19, 2012:

A comment here expressing the security concerns would be nice; maybe:

// Corrections made are very conservative on purpose, to avoid // opening up the possibility for attackers to replace valid addresses // with look-alike addresses.

laanwj commented at 7:34 AM on May 20, 2012:

I'll add a comment. But the reason is not that it allows attackers anything. The corrections were already very conservative and limited to what base58 considers equivalent, so there was already no risk of look-alike addresses.

More like:

// Corrections made are very conservative on purpose, to avoid // users unexpectedly getting away with typos that would normally // be detected, and thus sending to the wrong address.
laanwj commented at 4:53 PM on May 21, 2012: member

Added comment to validator function
laanwj referenced this in commit bc5053d93e on May 21, 2012
laanwj merged this on May 21, 2012
laanwj closed this on May 21, 2012
coblee referenced this in commit 63c7dbd9c0 on Jul 17, 2012
laanwj deleted the branch on Apr 9, 2014
suprnurd referenced this in commit 6483e6d06f on Dec 5, 2017
lateminer referenced this in commit 8d520b5a94 on Jan 22, 2019
DrahtBot locked this on Sep 8, 2021

Contributors

Linked (view graph)

#1325 Better feedback when unprintable characters in Bitcoin address