Skip to content
This repository has been archived by the owner on Jul 6, 2018. It is now read-only.

http2: add test for invalid pseudo headers #170

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions test/parallel/test-http2-server-set-invalid-header.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// Flags: --expose-http2
'use strict';

const common = require('../common');
const assert = require('assert');
const http2 = require('http2');
const body =
'Cannot set HTTP/2 pseudo-headers';

const invalidHeaders = [
{ key: ':status', value: 200 },
{ key: ':method', value: 'GET' },
{ key: ':path', value: '/' },
{ key: ':authority', value: 'example.com' },
{ key: ':scheme', value: 'http' }
];

const checkServer = (invalidHeader, value) => {
const server = http2.createServer((req, res) => {
res.setHeader('foobar', 'baz');
res.setHeader('X-POWERED-BY', 'node-test');
try {
res.setHeader(invalidHeader, value);
} catch (e) {
res.statusCode = 500;
res.end(e.message);
}
});

server.listen(0, common.mustCall(() => {
const client = http2.connect(`http://localhost:${server.address().port}`);
const headers = { ':path': '/' };
const req = client.request(headers);
req.on('response', common.mustCall((headers) => {
assert.strictEqual(headers[':status'], 500);
assert.strictEqual(headers['foobar'], 'baz');
assert.strictEqual(headers['x-powered-by'], 'node-test');
}));

let data = '';
req.on('data', (d) => data += d);
Copy link
Member

Choose a reason for hiding this comment

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

this should require a setEncoding('utf8') as it should come out as Buffer here. I assume this test is passing, so maybe there is something else going on.

cc @sebdeckers @jasnell

Copy link
Member

Choose a reason for hiding this comment

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

that should not be. We should be moving buffers.

Copy link
Member

Choose a reason for hiding this comment

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

nodejs/node@d515857 is very old. So, no there should be something else into play here.

Copy link
Contributor

Choose a reason for hiding this comment

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

My bad, I read that all wrong. (deleted previous confused messages)

Copy link
Contributor

Choose a reason for hiding this comment

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

@mcollina String concatenation => type coercion?

'foo' + new Buffer('bar')
// 'foobar'

Copy link
Member

Choose a reason for hiding this comment

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

oh that's fine then.

Copy link
Contributor

Choose a reason for hiding this comment

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

Just checked and d is in fact a Buffer. We're still emitting buffers, not strings, from the data event. 😅

Copy link
Member

@addaleax addaleax Jul 7, 2017

Choose a reason for hiding this comment

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

@sebdeckers The request from @mcollina is the right way to handle this, that’s what setEncoding is there for. In your example you are lucky it works, but if the chunks are e.g. Buffer([0x46, 0xc3]) and Buffer([0x96, 0x4f]), the result is F��O (edit: and not FÖO, as it should be).

Copy link
Contributor

Choose a reason for hiding this comment

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

@addaleax Yep, absolutely agreed. I'm just saying there is no deeper bug; nothing else is broken in the codebase. Apologies for adding my own confusion to this discussion. 😖

@yosuke-furukawa Can you please add res.setEncoding('utf8') here?

req.on('end', common.mustCall(() => {
assert.strictEqual(body, data);
server.close();
client.destroy();
}));
req.end();
}));
server.on('error', common.mustNotCall());
};

invalidHeaders.forEach(({ key, value }) => {
checkServer(key, value);
});