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 fragment parameter case sensitivity #446

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DanGould
Copy link
Contributor

@DanGould DanGould commented Dec 27, 2024

Bech32 fragment parameters should not be case sensitive. This fixes that by converting params and bech32 HRPs they match against to uppercase.

Close #442

--

Depends on lint fixes from #447

@coveralls
Copy link
Collaborator

coveralls commented Dec 27, 2024

Pull Request Test Coverage Report for Build 12528347962

Details

  • 6 of 6 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 61.844%

Totals Coverage Status
Change from base Build 12528252226: 0.04%
Covered Lines: 2898
Relevant Lines: 4686

💛 - Coveralls

@@ -103,8 +103,8 @@ where
{
if let Some(fragment) = url.fragment() {
for param in fragment.split('+') {
Copy link
Collaborator

@nothingmuch nothingmuch Dec 27, 2024

Choose a reason for hiding this comment

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

fragment.to_oppercase() instead of param? seems a bit more intuitive to me, and only needs to be done once

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've addressed this and chosen a function chain approach rather than nested conditional statements

Copy link
Collaborator

@nothingmuch nothingmuch left a comment

Choose a reason for hiding this comment

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

utack, sorry for not doing this myself when it came up

Bech32 fragment parameters should not be case sensitive. This fixes
that by converting params and bech32 Hrp|'1' prefixes they match against
to uppercase.

Note that this implementation implies all fragment parameters must be
case insentitive, which needs to be specified in BIP 77.

Close payjoin#442
@DanGould
Copy link
Contributor Author

On second pass I realize this change constrains fragment parameters to case insensitivity. BIP 77 is going to need to specify that, and IMO must specify params are bech32 encoded to minimize dependency requirements and QR size

@DanGould DanGould removed the blocked label Dec 28, 2024
@DanGould DanGould marked this pull request as ready for review December 28, 2024 17:45
@DanGould DanGould requested a review from nothingmuch December 28, 2024 17:45
@nothingmuch
Copy link
Collaborator

and IMO must specify params are bech32 encoded to minimize dependency requirements and QR size

in other words uppercase is normative? FWIW that's what i was intending in #417. what motivated creating #442?

@DanGould
Copy link
Contributor Author

DanGould commented Dec 28, 2024

what motivated creating #442?

bbmobile's bitcoin URI logic has bugs, and it lowercases the URI in display and for clipboard, so we end up with a lowercased URL to parse in the pj= parameter. Possible others will pass us lowercase URLs as well.

@nothingmuch
Copy link
Collaborator

we end up with a lowercased URL to parse in the pj= parameter. Possible others will pass us lowercase URLs as well.

ouch... that's incompatible with RFC 3986 FWIW.

RFC 3986 section 6.2.2.1 says case insensitive fields should be lowercased, e.g. scheme, host, but that's not a requirement, so QR alphanumerical encoding seems reasonable, and that % encoding should be uppercased, which they are. and:

The other generic syntax
components are assumed to be case-sensitive unless specifically
defined otherwise by the scheme (see Section 6.2.3).

@nothingmuch
Copy link
Collaborator

ouch... that's incompatible with RFC 3986 FWIW.

and would break if a directory has a path element, see #416

@DanGould
Copy link
Contributor Author

DanGould commented Dec 29, 2024

We're set to resolve this by

I believe this issue itself is wontfix

@DanGould DanGould marked this pull request as draft December 29, 2024 19:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fragment parameter HRPs appear to be case sensitive
3 participants