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.

Sign up for GitHub

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

Jump to bottom

Fix escaping of '#' in parameter values and handling of unescaped '#' in uri #3

Merged
merged 4 commits into from
Dec 2, 2024
Merged

Fix escaping of '#' in parameter values and handling of unescaped '#' in uri #3

merged 4 commits into from
Dec 2, 2024

Conversation

DanGould
Copy link

@DanGould DanGould commented Dec 2, 2024

The first two commits add test assertions for existing behavior.

The next commit adds a failing test for parameter values containing '#', which should be escaped. The commit after that addresses this by specifying a more precise character set, causing the test to pass and should therefore be squashed into it before merge to preserve bisectability. For ease of review they were added separately.

The last three commits add two additional failing tests for correct handling of fragment (unescaped #) followed by a fix commit, and should also be squashed. This set of changes might be considered incomplete without a capability to parse RFC 3986 fragments analogous to the Extras mechanism, but since this is not specified in BIP 21 arguably this data should be extracted using the url crate.

-- @nothingmuch: Kixunil#26

@nothingmuch
test that unknown parameters aren't retained
c38295c
@nothingmuch
test escaping in parameters
eb32638
@nothingmuch
Specify escaped characters more conservatively
0585806 
'#'' is not in the set qchar indirectly defined in BIP 21, and therefore
should be escaped.

[BIP 21](https://github.com/bitcoin/bips/blob/master/bip-0021.mediawiki#abnf-grammar):

>     labelparam     = "label=" *qchar
>     messageparam   = "message=" *qchar
>     otherparam     = qchar *qchar [ "=" *qchar ]
...
> Here, "qchar" corresponds to valid characters of an RFC 3986 URI query
> component, excluding the "=" and "&" characters, which this BIP takes
> as separators.

[RFC 3986 § 3.4](https://www.rfc-editor.org/rfc/rfc3986#section-3.4):

> The query component is indicated by the first question mark ("?")
> character and terminated by a number sign ("#") character or by the
> end of the URI.

[RFC 3986 Appendix A](https://www.rfc-editor.org/rfc/rfc3986#appendix-A):

>     pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"
>     query         = *( pchar / "/" / "?" )
...
>     pct-encoded   = "%" HEXDIG HEXDIG
>     unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
...
>     sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
>                   / "*" / "+" / "," / ";" / "="
@nothingmuch
Ignore RFC 3986 fragments in BIP 21 URIs
646de25 
Although behavior for when encountering RFC 3986 fragments in BIP 21
URIs is not specified, according to RFC 3986 it is unambiguously not
query data and therefore should be excluded from BIP 21 query
parameters.
DanGould
DanGould commented Dec 2, 2024
View reviewed changes
Copy link
Author

@DanGould DanGould left a comment

Choose a reason for hiding this comment

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

tACK 646de25

This is my PR so I can't explicitly approve but that's OK with me

@DanGould DanGould merged commit a02bdb5 into payjoin:master Dec 2, 2024
@DanGould DanGould mentioned this pull request Dec 2, 2024
Merged
@nothingmuch nothingmuch mentioned this pull request Dec 3, 2024
Merged
DanGould added a commit to payjoin/rust-payjoin that referenced this pull request Dec 3, 2024
@DanGould
Work around '#' escaping bug in bip21 crate (#373)
a1fbac5 
Upstream fix PR: payjoin/bitcoin_uri#3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@DanGould @nothingmuch