Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix PaymentServer::handleURIOrFile warning message #225

Closed
wants to merge 1 commit into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Feb 24, 2021

GUIUtil::parseBitcoinURI does not check address validity. Therefore, an invalid address cannot cause parsing failure.

An invalid address case is covered by its own message.

This PR fixes the warning messages.

@hebasto hebasto changed the title qt: Fix PaymentServer::handleURIOrFile warning message Fix PaymentServer::handleURIOrFile warning message Feb 24, 2021
Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 2434d00, no any testing, just built on Debian Sid with Qt 5.15.2

@hebasto
Copy link
Member Author

hebasto commented May 10, 2021

Updated 2434d00 -> 45ebd9e (pr225.01 -> pr225.02):

@hebasto
Copy link
Member Author

hebasto commented May 10, 2021

@prayank23

Do you mind reviewing this PR?

@hebasto hebasto added the UX All about "how to get things done" label May 10, 2021
@ghost
Copy link

ghost commented May 11, 2021

GUIUtil::parseBitcoinURI does not check address validity. Therefore, an invalid address cannot cause parsing failure.

If address validity is not checked, can it be the cause for parsing failure?

An invalid address case is covered by its own message "Invalid payment address %1".

Invalid address case is covered by IsValidDestination() and updated in #280 however, if code for else statement is executed can we assume address was never checked for validity?

@hebasto
Copy link
Member Author

hebasto commented May 11, 2021

GUIUtil::parseBitcoinURI does not check address validity. Therefore, an invalid address cannot cause parsing failure.

If address validity is not checked, can it be the cause for parsing failure?

Internally the GUIUtil::parseBitcoinURI uses the QUrlQuery class, so if an invalid address does not contain delimiters, it shouldn't cause parsing failure.

An invalid address case is covered by its own message "Invalid payment address %1".

Invalid address case is covered by IsValidDestination() and updated in #280 however, if code for else statement is executed can we assume address was never checked for validity?

I think we can.

GUIUtil::parseBitcoinURI does not check address validity. Therefore, an
invalid address cannot cause parsing failure.
An invalid address case is covered by its own message "Invalid payment
address %1".
@hebasto
Copy link
Member Author

hebasto commented May 29, 2021

Rebased 45ebd9e -> 5b20d46 (pr225.02 -> pr225.03) on top of the recent CI changes.

Copy link

@Talkless Talkless left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-utACK 5b20d46 after rebase.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review ACK 5b20d46 🥃

@ghost
Copy link

ghost commented Jul 6, 2021

@hebasto Can you share any examples that I can try for which GUIUtil::parseBitcoinURI will return false and https://github.com/hebasto/gui/blob/5b20d46f8be6b7fc08012d80a9fc42e9dbc812ff/src/qt/paymentserver.cpp#L254 will be executed ?

@hebasto
Copy link
Member Author

hebasto commented Jul 8, 2021

@hebasto Can you share any examples that I can try for which GUIUtil::parseBitcoinURI will return false and https://github.com/hebasto/gui/blob/5b20d46f8be6b7fc08012d80a9fc42e9dbc812ff/src/qt/paymentserver.cpp#L254 will be executed ?

I can't. It looks like this code is never executed due to checks in earlier stages, e.g. in PaymentServer::ipcParseCommandLine.

Closing for now.

@hebasto hebasto closed this Jul 8, 2021
@bitcoin-core bitcoin-core locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
UX All about "how to get things done"
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants