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

req.headers is missing propertyIsEnumerable in http2 server #40263

Closed
mccare opened this issue Sep 30, 2021 · 6 comments
Closed

req.headers is missing propertyIsEnumerable in http2 server #40263

mccare opened this issue Sep 30, 2021 · 6 comments
Labels
http2 Issues or PRs related to the http2 subsystem.

Comments

@mccare
Copy link

mccare commented Sep 30, 2021

Version

14.18.0

Platform

linux

Subsystem

http2

What steps will reproduce the bug?

Start a http2 server and query it with curl -v --http2-prior-knowledge http://localhost:8080

var http = require('http2')

var localIP = '127.0.0.1'
var port = 8080

function sayHello(req, res) {
    console.log("We've got a request " + req.url)
    const headers = req.headers
    console.log(
        `headers propertyIsEnumerable is  ${typeof headers.propertyIsEnumerable} in ${typeof headers} ${Object.keys(
            headers
        )}`
    )
    res.writeHead(200, { 'Content-Type': 'text/html' })
    res.write('<html><body>' + '<h1>Hello<h1>' + '</body></html>')
    res.end()
}

var server = http.createServer(sayHello)

server.listen(port, localIP)

Console output should show that propertyIsEnumerable is a function but it is undefined. For http server the function is present.

How often does it reproduce? Is there a required condition?

always

What is the expected behavior?

The req.headers object should have propertyIsEnumerable as a function

What do you see instead?

The req.headers object does not have propertyIsEnumerable function

Additional information

This happens in 14.18.0 but not in 14.17.x

@mcollina
Copy link
Member

This is normal and ok, req.headers do not have a prototype. It's the same in both http/1 and http/2.

@climba03003
Copy link
Contributor

climba03003 commented Sep 30, 2021

This is normal and ok, req.headers do not have a prototype. It's the same in both http/1 and http/2.

@mcollina only http2 is missing propertyIsEnumerable in prototype.
http has it repo

@climba03003
Copy link
Contributor

http is initial with {}

node/lib/_http_incoming.js

Lines 108 to 120 in d3162da

ObjectDefineProperty(IncomingMessage.prototype, 'headers', {
get: function() {
if (!this[kHeaders]) {
this[kHeaders] = {};
const src = this.rawHeaders;
const dst = this[kHeaders];
for (let n = 0; n < this[kHeadersCount]; n += 2) {
this._addHeaderLine(src[n + 0], src[n + 1], dst);
}
}

http2 is initial with Object.create(null)

// Headers in HTTP/1 are not initialized using Object.create(null) which,
// although preferable, would simply break too much code. Ergo header
// initialization using Object.create(null) in HTTP/2 is intentional.
this[kHeaders] = headers;

@mcollina
Copy link
Member

@ronag wdyt? If I recall correctly we set the headers with null prorotype a while ago.

@ronag
Copy link
Member

ronag commented Sep 30, 2021

I think we should not have prototype. I don't think it matters so much but I think we should choose whatever has best perf.

@mccare mccare closed this as completed Sep 30, 2021
@mccare
Copy link
Author

mccare commented Sep 30, 2021

Thanks for the clarification, need to handle this in fastify-multipart then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Development

No branches or pull requests

5 participants