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

Test failures on Node v19 (and likely v20 #2020

Closed
mcollina opened this issue Mar 22, 2023 · 14 comments · Fixed by #2051
Closed

Test failures on Node v19 (and likely v20 #2020

mcollina opened this issue Mar 22, 2023 · 14 comments · Fixed by #2051
Labels
bug Something isn't working

Comments

@mcollina
Copy link
Member

We have a few tests failures on Node v19 which we should work to iron out before the v20 release.

@mcollina mcollina added the bug Something isn't working label Mar 22, 2023
@targos
Copy link
Member

targos commented Mar 22, 2023

@KhafraDev
Copy link
Member

This is the failing test https://github.com/nodejs/undici/blob/main/test/issue-1903.js

@RishabhKodes
Copy link
Contributor

RishabhKodes commented Mar 22, 2023

Failure logs https://github.com/nodejs/undici/actions/runs/4477206842/jobs/7888223614#step:14:7115

# Subtest: test/issue-1903.js
    # Subtest: should parse content-disposition consistencely
        1..5
        not ok 1 - Invalid character in header content ["content-disposition"]
          ---
          stack: |
            Server.<anonymous> (test/issue-1903.js:27:9)
          at:
            line: 572
            column: 5
            file: node:_http_outgoing
            function: storeHeader
          type: TypeError
          code: ERR_INVALID_CHAR
          tapCaught: uncaughtException
          test: should parse content-disposition consistently
                    ...
        
    1..0 # no tests found
        # test count(1) != plan(5)
        # failed 1 test
    1..0 # no tests found
not ok 44 - test/issue-1903.js # time=60011.236ms

@climba03003
Copy link
Contributor

Related to nodejs/node#46528
Seems like either side turns out provide or parsing the header wrongly.

@mcollina
Copy link
Member Author

cc @marco-ippolito

@marco-ippolito
Copy link
Member

marco-ippolito commented Mar 27, 2023

With nodejs/node#46528 if there is explicit content-lenght, the content-disposition header is encoded in latin1 as per https://www.rfc-editor.org/rfc/rfc6266#section-4.3.
Probably there is a mismatch in the implementation, I'll have a look

@marco-ippolito
Copy link
Member

marco-ippolito commented Mar 27, 2023

I dont think this was ever supposed to work:

const server = createServer((req, res) => {
res.writeHead(200, {
'content-length': 2,
'content-disposition': "attachment; filename='år.pdf'"
})
a header value with non-ascii characters is passed without encoding it.
nodejs/node#46528 this PR solved the behaviour mismatch in encoding when using content length.
@ShogunPanda

@KhafraDev
Copy link
Member

Even once the header is encoded it still fails. I noticed that undici always encodes the content-disposition header to latin1, even if there is no content-length header too, but fixing that still doesn't pass the test either ☹️.

@KhafraDev
Copy link
Member

KhafraDev commented Apr 8, 2023

I'm mostly sure this is a bug in node.

The test is triggering this condition and throwing an error:
https://github.com/nodejs/node/blob/57caf1356229bd24c6129574863fa414ff6862b2/lib/_http_outgoing.js#L627
where value is a Buffer:

buffer = Buffer.from(`66 69 6c 65 6e 61 6d 65 3d 27 fd 72 2e 70 64 66 27`.split(' ').map(v => parseInt(v, 16)))

so the regex is being called with a buffer, stringifying it to utf8 (as is the default encoding)

/[^\t\x20-\x7e\x80-\xff]/.exec(buffer) !== null // true

/[^\t\x20-\x7e\x80-\xff]/.exec(buffer.toString()) !== null // true

/[^\t\x20-\x7e\x80-\xff]/.exec(buffer.toString('latin1')) !== null // false, correct case

if I'm missing something please feel free to correct me 😄


adding on, it makes sense for the value to be a buffer because of the line here
https://github.com/nodejs/node/pull/46528/files#diff-48d21edbddb6e855d1ee5716c49bcdc0d913c11ee8a24a98ea7dbc60cd253556R583

@ronag
Copy link
Member

ronag commented Apr 8, 2023

Yea I also think this is a Node bug.

   res.writeHead(200, { 
     'content-length': 2, 
     'content-disposition': "attachment; filename='år.pdf'" 
   }) 

Should work and has nothing to do with undici

@mcollina
Copy link
Member Author

mcollina commented Apr 8, 2023

@marco-ippolito wdyt?

@ronag
Copy link
Member

ronag commented Apr 8, 2023

I suggest, to get our tests working we disable this test for node 19+ until it's been fixed in Node. @mcollina wdyt?

@KhafraDev
Copy link
Member

I have a PR that disables it & matches the parsing of content-disposition with node

@marco-ippolito
Copy link
Member

yes this is a bug, å should be a valid character

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants