docs: Unsafe example for converting a message.url to an URL

[mlegenhausen]

Affected URL(s)

https://nodejs.org/api/http.html#messageurl

Description of the problem

Following the proposed way of converting request.url to an URL object you can easily come up with the following implementation:

import * as http from 'node:http';

const server = http.createServer((req, res) => {
  console.log(new URL(req.url, `http://${req.headers.host}`));
  res.end();
});

server.listen(3000);

This simple implementation lacks two issues.

1. You can crash the server via

curl http://localhost:3000//

with following message

TypeError: Invalid URL
    at new URL (node:internal/url:775:36)
    at Server.<anonymous> (file:///server.js:3:26)
    at Server.emit (node:events:518:28)
    at parserOnIncoming (node:_http_server:1151:12)
    at HTTPParser.parserOnHeadersComplete (node:_http_common:119:17) {
  code: 'ERR_INVALID_URL',
  input: '//',
  base: 'http://localhost:3000'
}

2. You can change the host to whatever you want

curl http://localhost:3000//evil.com/foo/bar

this generates following URL

URL {
  href: 'http://evil.com/foo/bar',
  origin: 'http://evil.com',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'evil.com',
  hostname: 'evil.com',
  port: '',
  pathname: '/foo/bar',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

This might have security implications if you e. g. base your CSRF protection on the parsed URL host value.

A better approach would it be to concatenate the host with the req.url instead of using the baseUrl parameter from the URL constructor.

import * as http from 'node:http';

const server = http.createServer((req, res) => {
  console.log(new URL(`http://${req.headers.host}${req.url}`));
  res.end();
});

server.listen(3000);

If you now repeat both curls. You get

1. No crash

URL {
  href: 'http://localhost:3000//',
  origin: 'http://localhost:3000',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'localhost:3000',
  hostname: 'localhost',
  port: '3000',
  pathname: '//',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

2. No host change

URL {
  href: 'http://localhost:3000//evil.com/foo/bar',
  origin: 'http://localhost:3000',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'localhost:3000',
  hostname: 'localhost',
  port: '3000',
  pathname: '//evil.com/foo/bar',
  search: '',
  searchParams: URLSearchParams {},
  hash: ''
}

[avivkeller]

Great issue! Your issue suggested a fix (which is excellent!). If you'd like, you can also make a PR for it, or a community member can do it.

[lpinca]

A better approach would it be to concatenate the host with the req.url instead of using the baseUrl parameter from the URL constructor.

I don't think it is a good idea. It is not better than the current example.

'use strict';

const http = require('http');

const server = http.createServer();

server.on('request', function (request, response) {
  console.log(`http://${request.headers.host}${request.url}`);
  response.end();
});

server.listen(function () {
  const { port } = server.address();
  const request = http.request({
    headers: {
      host: 'evil.com'
    },
    path: '/foo/bar',
    port
  });

  request.on('response', function (response) {
    response.resume();
  });

  request.end()
});

[thisalihassan]

@lpinca your approach showcases to modify HOST header on server side but atleast the above solution is good enough for client side solutions since HOST header is controlled by the browsers and also "//" as path will crash the server.

the above solution provided by @mlegenhausen does seem to be a good practice in most cases atleast better than current examples

[lpinca]

You can't make assumption about the client. There is not crash here

'use strict';

const http = require('http');

const server = http.createServer();

server.on('request', function (request, response) {
  console.log(`http://${request.headers.host}${request.url}`);
  response.end();
});

server.listen(function () {
  const { port } = server.address();
  const request = http.request({
    path: '//',
    port
  });

  request.on('response', function (response) {
    response.resume();
  });

  request.end()
});

[thisalihassan]

@lpinca you are missing the point you need, it's about parsing the URL with
new URL(request.url, http://${request.headers.host}) // this is shown in docs
vs
new URL(http://${req.headers.host}${req.url})) // this is purposed solution

the later approach does not crash the code, do test all scenarios yourself

[lpinca]

The proposed solution crashes just like the original one and is not safer either.

'use strict';

const http = require('http');
const net = require('net');

const server = http.createServer();

server.on('request', function (request, response) {
  new URL(`http://${request.headers.host}${request.url}`);
  response.end();
});

server.listen(function () {
  const { port } = server.address();
  const socket = net.createConnection({ port });

  socket.on('connect', function () {
    socket.write(
      [
        'GET / HTTP/1.1',
        'Host:',
        '\r\n'
      ].join('\r\n')
    );
  });
});

[thisalihassan]

@lpinca I did not say it is perfect URL() constructor always throw error if parsing fails which crashes the server. this is same as before where you pointed out and rewrote HOST from server side.

I am not trying to say this is perfect solution but this is better than the current example as it does handle more scenarios.

Moreover thanks for pointing out different cases.

[lpinca]

I am not trying to say this is perfect solution but this is better than the current example as it does handle more scenarios.

What are those scenarios? It fails exactly like original one. You simply cannot trust the Host header as it is provided by the user. It does not matter if new URL(request.url, http://${request.headers.host}) or new URL(`http://${req.headers.host}${req.url})`) is used. They both fails in the same way if the Host header is not validated.

[mlegenhausen]

Thanks for your feedback. It seems in general that the concatenation has no serious flaws over the baseUrl approach.

Thanks @lpinca for pointing out the Host header I didn't had that on my watch. You are right as we can manipulate both values so both approaches are equal unsafe in theory.

In the real world I would still think that the manipulation of IncomingMessage#url leads to errors that are much more common in practice than the one of a manipulated Host header as you break the routing to the target system when the application is deployed like in a cloud environment. This will of course not fit for all scenarios.

If we would replace the Host header with a static baseUrl like http://localhost we would still have the same issues. Then my approach is superior. Maybe we could improve the docs in the following way:

new URL(`http://localhost${request.url}`)
new URL(`http://${process.env.HOST ?? 'localhost'}${request.url}`)

So we remove the whole manipulated Host header scenario and fix the relative url resolution.

WDYT?

[avivkeller]

Uses process.env isn't a bad idea, but it's probably better heard over at #52536 (which is the PR where it could be implemented)

[avivkeller]

Congrats on the merged PR!

I'm closing this issue as completed, but if you disagree, please let me know

[mlegenhausen]

Congrats on the merged PR!

Thanks guys. I must say it was a pleasant process to contribute to nodeJS.