Fixes: #809
Previously it was possible through the GUI to enter an IP address:port into the “Proxy IP” configuration box. After the node was restarted the errant setting would prevent the node starting back up until manually removed from settings.json.
Prevent this with a simple check for “:” in the string. This is acceptable here in the GUI setting because we already fail on a hostname such as “http://x.x.x.x”, so it won’t cause false positives.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | furszy, hebasto |
| Concept ACK | pablomartin4btc |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Concept ACK 94173eac6e89f374f1a98b5d0e449a0a07ed832d
I’d suggest to use a regex validator for the IP address, it’s much simpler I think and the user won’t be allow to enter any other symbols or chars (typo?) that aren’t digits (plus user can’t type more than 3 subnets separated by dots where currently you could and any symbols):
0diff --git a/src/qt/optionsdialog.cpp b/src/qt/optionsdialog.cpp
1index dd654a7ab..1fabee7f2 100644
2--- a/src/qt/optionsdialog.cpp
3+++ b/src/qt/optionsdialog.cpp
4@@ -33,6 +33,11 @@
5 #include <QSystemTrayIcon>
6 #include <QTimer>
7
8+// For IP address validators
9+QRegularExpression ipRegex("^([0-9]{1,3}\\.){3}[0-9]{1,3}$");
10+// For Port number validators
11+QRegularExpression portRegex("^(6553[0-5]|655[0-2]\\d|65[0-4]\\d{2}|6[0-4]\\d{3}|[1-5]\\d{4}|[1-9]\\d{0,3})$");
12+
13 int setFontChoice(QComboBox* cb, const OptionsModel::FontChoice& fc)
14 {
15 int i;
16@@ -112,13 +117,18 @@ OptionsDialog::OptionsDialog(QWidget* parent, bool enableWallet)
17 ui->mapPortNatpmp->setEnabled(false);
18 #endif
19
20+ QRegularExpressionValidator *ipValidator = new QRegularExpressionValidator(ipRegex, this);
21+ QRegularExpressionValidator *portValidator = new QRegularExpressionValidator(portRegex, this);
22+
23 ui->proxyIp->setEnabled(false);
24+ ui->proxyIp->setValidator(ipValidator);
25+
26 ui->proxyPort->setEnabled(false);
27- ui->proxyPort->setValidator(new QIntValidator(1, 65535, this));
28+ ui->proxyPort->setValidator(portValidator);
29
30 ui->proxyIpTor->setEnabled(false);
31+ ui->proxyIpTor->setValidator(ipValidator);
32 ui->proxyPortTor->setEnabled(false);
33- ui->proxyPortTor->setValidator(new QIntValidator(1, 65535, this));
34+ ui->proxyPortTor->setValidator(portValidator);
35
36 connect(ui->connectSocks, &QPushButton::toggled, ui->proxyIp, &QWidget::setEnabled);
37 connect(ui->connectSocks, &QPushButton::toggled, ui->proxyPort, &QWidget::setEnabled);
I’ve also included in the above a “port number validator”, the regex there is a bit more “elaborated” if you like but currently using the int validator, user can enter a dot and then the CService will return that the port is not valid.
Last one since we are here, I’d change the call to ‘CService’ (that performs currently the validation of the port number - eg as a mentioned above that user can enter a dot) to when the value is changed (textChanged?) - this is something that some reviewer noticed testing the corresponding control/ widget in the QML gui repo and I find it more practical - not as how currently behaves that the validation is done on focus changed (you can also see in the code that the tor inputs have different signal connections somehow, and the textChanged there doesn’t work properly either) but this can be done also on a follow-up.
I’d suggest to use a regex validator for the IP address, it’s much simpler I think and the user won’t be allow to enter any other symbols or chars (typo?) that aren’t digits (plus user can’t type more than 3 subnets separated by dots where currently you could and any symbols)
I’m not convinced that a large regex is “simpler” than checking if a “:” is in the string?
Thanks for the other suggestions, but I don’t feel that interested in picking them here with the QML work going on, and them not being much of an issue IMO. In contrast this fix prevents a bug whereby an incorrect setting can stop the node starting up.
The only potential issue I saw with my own solution was that it would outright prevent setting the tor proxy IP to an IPv6 address, however from my (quick) research, it didn’t seem that this was actually something anyone did (and it might not even be possible/practical to do). If I’m wrong on this, I’ll be happy to update the solution further, but from what I can see even setting Tor ORPort to IPV6 is barely supported, and ControlPort is limited to an IPv4 address:port…
481@@ -482,7 +482,10 @@ QValidator(parent)
482 QValidator::State ProxyAddressValidator::validate(QString &input, int &pos) const
483 {
484 Q_UNUSED(pos);
485- // Validate the proxy
486+ // "http://x.x.x.x" already prohibited, so safe to fail early here on a ":"
487+ if (input.contains(':')) {
488+ return QValidator::Invalid;
489+ }
SplitHostPort() from #include <util/strencodings.h> instead?
Would only need to verify that the port isn’t set.
I’m not convinced that a large regex is “simpler” than checking if a “:” is in the string?
The large one was for the port number, the one I referred as “simpler” was for the IP address: QRegularExpression ipRegex("^([0-9]{1,3}\\.){3}[0-9]{1,3}$");. What I meant is that we know in advance that the “:” would be invalid so we could restrict the user to even enter it. As current we could see this (not only “:”):
The regex would keep it cleaner, more user friendly and we don’t have to do an extra validation, but I’ll stop trying to convince you :smile: (btw, also we’d need to set the max length to 15/ follow up).
I don’t feel that interested in picking them here with the QML work going on, and them not being much of an issue IMO.
QML is far for being at the stage where QT currently is, improving user experience could also be useful for both projects feedback.
In contrast this fix prevents a bug whereby an incorrect setting can stop the node starting up.
I agree and there’s a workaround already which is just removing the IP line from the file.
The only potential issue I saw with my own solution was that it would outright prevent setting the tor proxy IP to an IPv6 address, however from my (quick) research, it didn’t seem that this was actually something anyone did (and it might not even be possible/practical to do). If I’m wrong on this, I’ll be happy to update the solution further, but from what I can see even setting Tor ORPort to IPV6 is barely supported, and ControlPort is limited to an IPv4 address:port…
Is it even possible to enable those checkboxes?
Fixes: #809
Previously it was possible through the GUI to enter an IP address:port
into the "Proxy IP" configuration box. After the node was restarted the
errant setting would prevent the node starting back up until manually
removed from settings.json.
Just to point out first, this will only work effectively in the case that editing has finished, as in clicking on another item that will take focus. If you only edit the proxy, leave focus on that box and click ok to save the setting, the invalid proxy will be saved.
Will propose a proper patch soon.
