Enforce strict parsing of network names in payment request processing #4275

rnicoll commented at 9:17 PM on June 2, 2014: contributor

Payment request parsing of network names is now strict (does not assume testnet if not main).

laanwj commented at 6:48 AM on June 3, 2014: member

Wasn't it failing later on anyway, when it gets to the real processing? At least that'd allow popping up an error.

Ideally ipcParseCommandLine shouldn't signal errors, as you noticed they can't be displayed to the user.

Diapolo commented at 7:13 AM on June 3, 2014: none

I'm also sure the error is catched later with user visible feedback about what happened. Anyway, I still think we should try to send (via ipcCommandLine()) first to a mainnet instance and if that fails to a testnet instance and remove that whole "try to get or guess the right network" stuff in ipcParseCommandLine() alltogether as it complicates things. Who in real world (normal user) has a testnet AND mainnet instance running?

Edit: See #4278 for a pull.

rnicoll commented at 7:27 AM on June 3, 2014: contributor

I think you're right, it's adding too much error handling at the wrong point, in retrospect. Won't have time to write a new patch until tonight, but how about it switches the client to testnet if it detects a testnet payment request, and otherwise has no interaction with the network field? Thinking more about this, I think it's preferable that a mainnet payment request fails if the client is launched on testnet, than the client is switched over to mainnet.

laanwj commented at 12:53 PM on June 3, 2014: member

@Diapolo I don't agree. It should be possible to run both a testnet and mainnet instance at the same time and have URIs and payment requests arrive at the right instance. @rnicoll The idea is to override the user-provided network (provided on the command line or configuration file) with the network of the payment request. This could happen in both directions.

rnicoll commented at 6:55 PM on June 3, 2014: contributor

Reworked to defer validation until GUI is up, and also to tighten checks in the GUI so a nonsense network name is never accepted (also avoids any potential issues with other coins using unsupported network names and file mixups)

laanwj commented at 7:14 AM on June 4, 2014: member

Untested ACK

in src/qt/paymentserver.cpp:None in b772a43261 outdated

 501 | @@ -499,8 +502,8 @@ bool PaymentServer::processPaymentRequest(PaymentRequestPlus& request, SendCoins
 502 |          const payments::PaymentDetails& details = request.getDetails();
 503 |  
 504 |          // Payment request network matches client network?
 505 | -        if ((details.network() == "main" && TestNet()) ||
 506 | -            (details.network() == "test" && !TestNet()))
 507 | +        if ((TestNet() && details.network() != "test") ||
 508 | +            (!TestNet() && details.network() != "main"))
 509 |          {

leofidus commented at 7:59 PM on June 4, 2014:

This overlaps with a recent commit in #3824 (f0a83fc). In think the version in that PR solves this better.

rnicoll commented at 10:14 PM on June 4, 2014:

Agreed, #3824's changes to paymentserver.cpp (and paymentserver.h) look like a better answer, presuming they're going to be merged.

leofidus commented at 3:32 PM on June 11, 2014:

#3824 is now merged, which makes this part unnessesary. I still like the other change this PR does, even though that one shouldn't make a big difference.

in src/qt/paymentserver.cpp:None in b772a43261 outdated

 211 | -
 212 |              PaymentRequestPlus request;
 213 |              if (readPaymentRequest(arg, request))
 214 |              {
 215 | -                if (request.getDetails().network() == "main")
 216 | +                if (request.getDetails().network().data() == "main")

Diapolo commented at 8:05 PM on June 4, 2014:

Why is .data() needed here now?

rnicoll commented at 10:33 PM on June 4, 2014: contributor

Fixed the superfluous .data() calls, sorry about that.

rnicoll commented at 10:53 PM on June 11, 2014: contributor

Removed changes obsoleted by #3824

in src/qt/paymentserver.cpp:None in 692d2d84a0 outdated

 206 | @@ -207,15 +207,18 @@ bool PaymentServer::ipcParseCommandLine(int argc, char* argv[])
 207 |          }
 208 |          else if (QFile::exists(arg)) // Filename
 209 |          {
 210 | -            savedPaymentRequests.append(arg);

laanwj commented at 10:14 AM on June 25, 2014:

Keep this here - we want the reading to fail later to be able to give an appropriate error message.

rnicoll commented at 5:56 PM on July 6, 2014: contributor

Finally made change as requested by @laanwj , sorry about the delay.

ghost commented at 6:03 PM on July 6, 2014: none

@rnicoll You'll need to rebase this too as there are merge conflicts with master.

laanwj commented at 7:26 AM on July 8, 2014: member

ACK after squashing into one commit (it's going back and forth right now).

Payment request parsing on startup now only changes network if a valid network name is specified. 509f926e80

rnicoll commented at 9:13 PM on July 8, 2014: contributor

Once more, with feeling...

BitcoinPullTester commented at 9:52 PM on July 8, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4275_509f926e80b3d24a6b6c6fbf535e4d4b97156a3c/ 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 commented at 6:47 AM on July 9, 2014: member

Woohoo :)

laanwj merged this on Jul 9, 2014

laanwj closed this on Jul 9, 2014

laanwj referenced this in commit 2ee918d121 on Jul 9, 2014

rnicoll deleted the branch on Aug 29, 2014

MarcoFalke locked this on Sep 8, 2021