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

CP-50181: Percent decode all Uri paths before using them #5877

Merged

Conversation

last-genius
Copy link
Contributor

@last-genius last-genius commented Jul 23, 2024

Uri.path returns percent-encoded output, therefore it can't be expected to behave correctly in cases where it's used to map to Unix files. Our PR upstream introduced Uri.path_unencoded function, use that instead.

There is one usage of Uri.path left:

$ rg --pcre2 'Uri.path(?!_unencoded)' --no-heading -g 'ocaml/**/*.ml'
ocaml/xe-cli/newcli.ml:138:    let path = Uri.path_and_query uri |> Uri.pct_decode in
ocaml/vhd-tool/cli/sparse_dd.ml:403:          ?port:(Uri.port uri) ~path:(Uri.path uri) ~query:(Uri.query uri)
ocaml/libs/open-uri/open_uri.ml:77:      let filename = Uri.path_and_query uri |> Uri.pct_decode in

But this is not dangeous as it's used as an argument for Uri.make, which is fine without pct_decode:

utop # Uri.make ~path:("/get<>`&") ();;
- : Uri.t = /get%3C%3E%60&
utop # Uri.make ~path:("/get%3C%3E%60&") ();;
- : Uri.t = /get%3C%3E%60&

I'm not sure it's worth adding a quality gate to future-proof this.

===

xapi-project/xs-opam#693 is required to build and merge this, therefore currently opening as a draft - but it's ready for review.

`Uri.path` returns percent-encoded output, therefore it can't
be expected to behave correctly in cases where it's used to
map to Unix files. Our PR upstream introduced 'Uri.path_unencoded'
function, use that instead.

Signed-off-by: Andrii Sultanov <andrii.sultanov@cloud.com>
@last-genius
Copy link
Contributor Author

With the xs-opam changes, this passed BST/BVT (run id 201742)

@psafont
Copy link
Member

psafont commented Jul 23, 2024

Merge needs to wait until the next xs-opam version is tagged. (6.83.0)

@psafont psafont marked this pull request as ready for review July 26, 2024 10:28
@last-genius last-genius merged commit f7d087b into xapi-project:master Jul 31, 2024
15 checks passed
@last-genius last-genius deleted the private/asultanov/uri-path branch July 31, 2024 14:46
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.

4 participants