[RPC] require valid URL scheme on budget commands

[CaveSpectre11]

Release Notes

[RPC] Require valid URL schemes (URLs starting with "http" or "https") on preparebudget and submitbudget.

Details

The budget proposal system did not have any checks on the URL aside of string length. This allowed for the scheme to be left off the URL; which on some browsers, resulted in the link to fail when clicked:

gio: file:///home/CaveSpectre/forum.pivx.org/index.php%3Fthreads/bizdev-proposal-john-m.285: 
Error when getting information for file “/home/CaveSpectre/forum.pivx.org/index.php?
threads/bizdev-proposal-john-m.285”: No such file or directory

The included PR changes the submitbudget and preparebudget rpc commands to prevent inadvertent omissions of the URL scheme (http:// or https://).

Qt changes to fixup the existing & historical budget URLs will be submitted as a separate PR after the merging of #954 .

(This PR originally included the Qt changes, and as a result some of the below discussion relates to that, before the decision was made to back out the Qt changes until a later date)

[Warrows]

Concept ACK, however the use of std regex seems to be making a travis job fail.

[CaveSpectre11]

I originally thought that regex failure in the zerocoin test was something unrelated; or related to the zerocoin changes. But now that I look at it....

@Warrows , do you happen to know why that test uses gcc 4.8? regex should be stable in 4.9, and most of the builds appear to be using 7.4. Are we restricted to supporting 4.8?

[Warrows]

That's a question better suited for @Fuzzbawls

[CaveSpectre11]

I should be able to conditionalize it so that regex_match() just returns true on builds prior to gcc 4.9, which would effectively fix the travis test, but have no impact on the releases. I will wait for @Fuzzbawls to suggest direction.

#include <regex>

#if __cplusplus >= 201103L &&                             \
    (!defined(__GLIBCXX__) || (__cplusplus >= 201402L) || \
        (defined(_GLIBCXX_REGEX_DFS_QUANTIFIERS_LIMIT) || \
         defined(_GLIBCXX_REGEX_STATE_LIMIT)           || \
             (defined(_GLIBCXX_RELEASE)                && \
             _GLIBCXX_RELEASE > 4)))
#  define HAVE_WORKING_REGEX 1
#else
#  define HAVE_WORKING_REGEX 0
#endif

It'll take a little toying to get it right

[Fuzzbawls]

Concept ACK, but would need to do this without relying on std::regex so as to avoid bumping the minimum supported ABI.

maybe have a look at using std::string::find instead.

[CaveSpectre11]

std::string::find

It wouldn't be as robust; but yes, searching for "://" would at least solve the inadvertent and common omission.

[CaveSpectre11]

Quick test log of the rpc restrictions. logically, "h://" and "H://" pass, but we at least force [hH]+:// which should make it better.

[Fuzzbawls]

ACK d92b438

[random-zebra]

I don't particularly like this solution.
Think it would be better to use tolower() (even if locale dependent and requiring some additional changes in the lint script) on everything before :// and compare with the allowed prefixes.

Anyway, would code the logic in a separate utility function and call that from submitbudget and preparebudget to avoid more code duplication (those two functions already share way too much code in my opinion).

[CaveSpectre11]

Anyway, would code the logic in a separate utility function and call that from submitbudget and preparebudget to avoid more code duplication (those two functions already share way too much code in my opinion).

The original design had a utility function for adding the scheme; but the code check in the rpc code was less complicated. I think I agree that a utility function is the way to go; especially given the premise of the last round of changes (adding more URL scheme) validation. Pulling it together into a utility function centralizes where future code can go, if the need to have further validation of the URL is desired later.

Think it would be better to use tolower() (even if locale dependent and requiring some additional changes in the lint script) on everything before :// and compare with the allowed prefixes.

I will go ahead and pull the logic into a function; and you and @Fuzzbawls can hash out the pros and cons of tolower() and finish it out when that direction is determined.

[CaveSpectre11]

@Fuzzbawls @random-zebra; I have submitted a much broader PR (#964 ) that encompasses this PR, with modifications to the logic to hopefully account for both your concerns.

That PR does supersede this one, and if that solution is preferred, this one should be closed.

[CaveSpectre11]

Ok; I just totally jacked this one up trying to merge in #965. I'll try to figure it out tomorrow.

[Mrs-X]

Just some random thoughts:

• checkBudgetInputs() was already there, so why did you copy that code into the methods?
• the canonical form of a scheme is lowercase, so I second forcing http:// or https://

[CaveSpectre11]

• checkBudgetInputs() was already there, so why did you copy that code into the methods?

#964 was meant as a replacement to this one originally when I submitted it. But it was decided they should be separate. This one to deal with the scheme check, 964 for the refactor only; and 965 for the fixes on the existing code. The refactoring was what combined the common code in the prepare and submit into checkBudgetInputs().

Then to make it more confusing; 964's change was cherry picked into 965; and both were picked into this one... presuming the merge order to mainline would be 964 first; then 965, then this one... and thus the rebasing legwork was already done.

[CaveSpectre11]

rebased due to conflict from on budget.cpp

[random-zebra]

Needs rebase after #965 merge.

[CaveSpectre11]

Needs rebase after #965 merge.

Merged through github; will squash that commit when I get home.

[Fuzzbawls]

functionally good, just a comment move

[Fuzzbawls]

ACK 009bd63

[random-zebra]

utACK 009bd63 and merging...