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

decode header value with utf-8 #375

Merged
merged 6 commits into from
Jun 22, 2024
Merged

Conversation

zuisong
Copy link
Contributor

@zuisong zuisong commented Jun 21, 2024

  • add test case

For reference, this behavior is the same as the redirection behavior of reqwest.

see https://github.com/seanmonstar/reqwest/blob/29d4cff234b37065632512f002b9785700c51aa8/src/async_impl/client.rs#L2624


related issue: #323

Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Nice catch!

src/redirect.rs Outdated
Comment on lines 54 to 55
.and_then(|location| String::from_utf8(location.as_bytes().to_vec()).ok())
.and_then(|location| request.url().join(&location).ok())
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm realizing now that if decoding or joining fails we should log::warn!() (after #371). We can pick that up in another PR though.

src/printer.rs Outdated Show resolved Hide resolved
@zuisong
Copy link
Contributor Author

zuisong commented Jun 21, 2024

I'm wondering if we should change all calls to HeaderValue.to_str() to HeaderValue.to_utf8_str().

I think it's a good idea.

For example, this will allow us to download filenames that support UTF-8 encoding.

let header = response.headers().get(CONTENT_DISPOSITION)?.to_str().ok()?;

@blyxxyz
Copy link
Collaborator

blyxxyz commented Jun 21, 2024

Officially you're not supposed to do that so we should decide on a case-by-case basis.

For filenames we can actually pass the bytes through directly depending on the platform. But we should check what different servers and clients do.

download file support unicode file name
@zuisong
Copy link
Contributor Author

zuisong commented Jun 21, 2024

Yes, you're right.

I decided to use UTF-8 encoding to decode the CONTENT_DISPOSITION response header, which is very useful for downloading files with Unicode names.
And Chrome implements it the same way.

before

❯ xh  httpbingo.org/response-headers 'content-disposition==attachment; filename="🥰.json"' -d
HTTP/1.1 200 OK
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *
Connection: close
Content-Disposition: "attachment; filename=\"\xf0\x9f\xa5\xb0.json\""
Content-Length: 76
Content-Type: application/json; charset=utf-8
Date: Fri, 21 Jun 2024 16:07:28 GMT
Fly-Request-Id: 01J0XRV0MHF1VBYPJY576KHWSM-sjc
Server: Fly/ebd3372a (2024-06-19)
Via: 1.1 fly.io

Downloading 76 B to "response-headers.json-3"
Done. 76 B in 0.00108s (68.80 KiB/s)

after

❯ /Users/chen/.rust-target/debug/xh httpbingo.org/response-headers 'content-disposition==attachment; filename="🥰.json"' -d
HTTP/1.1 200 OK
Access-Control-Allow-Credentials: true
Access-Control-Allow-Origin: *
Connection: close
Content-Disposition: attachment; filename="🥰.json"
Content-Length: 76
Content-Type: application/json; charset=utf-8
Date: Fri, 21 Jun 2024 16:06:44 GMT
Fly-Request-Id: 01J0XRSP2WEE21V3XAHNGBFCBY-sjc
Server: Fly/ebd3372a (2024-06-19)
Via: 1.1 fly.io

Downloading 76 B to "🥰.json"
Done. 76 B in 0.00144s (51.63 KiB/s)

@zuisong zuisong marked this pull request as ready for review June 21, 2024 16:27
Copy link
Collaborator

@blyxxyz blyxxyz left a comment

Choose a reason for hiding this comment

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

Thank you!

src/printer.rs Outdated Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
tests/cli.rs Outdated Show resolved Hide resolved
@ducaale ducaale merged commit 1b0f019 into ducaale:master Jun 22, 2024
9 checks passed
@zuisong zuisong deleted the utf8-hader-value branch June 23, 2024 03:27
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.

3 participants