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

undici.request parse header differently to http.request #1903

Closed
climba03003 opened this issue Feb 2, 2023 · 4 comments · Fixed by #1911
Closed

undici.request parse header differently to http.request #1903

climba03003 opened this issue Feb 2, 2023 · 4 comments · Fixed by #1911
Labels
bug Something isn't working good first issue Good for newcomers Status: help-wanted This issue/pr is open for contributions

Comments

@climba03003
Copy link
Contributor

Bug Description

Header handling on latin1 characters that do not have the same byte code in utf8.

Reproducible By

HTTP proxy chaining provide a consistence behavior on header serialization.

const http = require('http')

const header = 'attachment; filename="år.pdf"'
console.log(Buffer.from(header)) // <Buffer 61 74 74 61 63 68 6d 65 6e 74 3b 20 66 69 6c 65 6e 61 6d 65 3d 22 c3 a5 72 2e 70 64 66 22>

http.createServer(function(_, response) {
  response.writeHead(200, {
    'content-length': 2,
    'content-disposition': header
  })
  response.end('OK')
}).listen({ port: 6666 })

http.createServer(async function(_, response) {
  http.request('http://127.0.0.1:6666', {
    method: 'GET',
  }, function(res) {
    const { statusCode, headers } = res
    console.log(headers)
    console.log(Buffer.from(headers['content-disposition'])) // <Buffer 61 74 74 61 63 68 6d 65 6e 74 3b 20 66 69 6c 65 6e 61 6d 65 3d 22 c3 83 c2 a5 72 2e 70 64 66 22>
    delete headers['transfer-encoding']
    response.writeHead(statusCode, headers)
    res.pipe(response)
  }).end()
}).listen({ port: 7777 })

http.createServer(async function(_, response) {
  http.request('http://127.0.0.1:7777', {
    method: 'GET',
  }, function(res) {
    const { statusCode, headers } = res
    console.log(headers)
    console.log(Buffer.from(headers['content-disposition'])) // <Buffer 61 74 74 61 63 68 6d 65 6e 74 3b 20 66 69 6c 65 6e 61 6d 65 3d 22 c3 83 c2 a5 72 2e 70 64 66 22>
    delete headers['transfer-encoding']
    response.writeHead(statusCode, headers)
    res.pipe(response)
  }).end()
}).listen({ port: 8888 })

Undici proxy chaining is broken somehow for å character (the characters that is not match in byte in latin1 and utf8)

const http = require('http')
const { request } = require('undici')

const header = 'attachment; filename="år.pdf"'
console.log(Buffer.from(header)) // <Buffer 61 74 74 61 63 68 6d 65 6e 74 3b 20 66 69 6c 65 6e 61 6d 65 3d 22 c3 a5 72 2e 70 64 66 22>

http.createServer(function(_, response) {
  response.writeHead(200, {
    'content-length': 2,
    'content-disposition': header
  })
  response.end('OK')
}).listen({ port: 6666 })

http.createServer(async function(_, response) {
  const { statusCode, headers, body } = await request('http://localhost:6666', {
    method: "GET"
  })
  console.log(headers)
  console.log(Buffer.from(headers['content-disposition'])) // <Buffer 61 74 74 61 63 68 6d 65 6e 74 3b 20 66 69 6c 65 6e 61 6d 65 3d 22 c3 83 c2 a5 72 2e 70 64 66 22>
  delete headers['transfer-encoding']
  response.writeHead(statusCode, headers)
  body.pipe(response)
}).listen({ port: 7777 })

http.createServer(async function(_, response) {
  const { statusCode, headers, body } = await request('http://localhost:7777', {
    method: "GET"
  })
  console.log(headers)
  console.log(Buffer.from(headers['content-disposition'])) // <Buffer 61 74 74 61 63 68 6d 65 6e 74 3b 20 66 69 6c 65 6e 61 6d 65 3d 22 c3 a5 72 2e 70 64 66 22>
  delete headers['transfer-encoding']
  response.writeHead(statusCode, headers)
  body.pipe(response)
}).listen({ port: 8888 })

Expected Behavior

I assume it is same as http.request or the bug is actually in http.

Logs & Screenshots

Environment

Linux DESKTOP-S1RECSH 4.4.0-19041-Microsoft #2311-Microsoft Tue Nov 08 17:09:00 PST 2022 x86_64 x86_64 x86_64 GNU/Linux
Node.js v18.13.0

Additional context

You can see more detail on the below issue.
Refs fastify/fastify-reply-from#287

@climba03003 climba03003 added the bug Something isn't working label Feb 2, 2023
@climba03003
Copy link
Contributor Author

climba03003 commented Feb 2, 2023

I have no idea which behavior should be the correct one.

But, undici provide a in-consistence header between proxy chaining.
It is more problematic in general.

@mcollina
Copy link
Member

mcollina commented Feb 2, 2023

I think undici should do some special handling of the content-disposition header.

Specifically, we should follow RFC 6266.

In other terms, we should parse content-disposition as latin1. I think this is a bug.

@mcollina
Copy link
Member

mcollina commented Feb 2, 2023

cc @ronag @KhafraDev

@mcollina
Copy link
Member

mcollina commented Feb 6, 2023

@climba03003 would you send a PR?

@ronag ronag added good first issue Good for newcomers Status: help-wanted This issue/pr is open for contributions labels Feb 6, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers Status: help-wanted This issue/pr is open for contributions
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants