HTTP Date header time faking fails on Node 15.2.0 and later

[gorankarlic]

trafficstars

HTTP Date header time is faked just fine up to Node v15.1.0 but fails with Node v15.2.0 or later.

• FakeTimers version : 6.0.1
• Environment : Node v15.5.0
• Other libraries you are using: assert, http

What did you expect to happen?

Should return HTTP Date header with fake time date: "Tue, 31 Dec 2013 23:59:59 GMT".

What actually happens

Instead returns HTTP Date header with current time, e.g. date: "Sun, 03 Jan 2021 17:22:52 GMT".

How to reproduce

Following is a minimal and complete mocha test case that reproduces the issue

import assert from "assert";
import http from "http";
import fake_timers from "@sinonjs/fake-timers";

describe("fake HTTP Date header time", function()
{
    let clock;
    let server;

    before(function(done)
    {
        clock = fake_timers.install({now: Date.parse("2013-12-31 23:59:59.123Z")});
        server = http.createServer((req, res) =>
        {
            req.on("data", () => void null);
            req.on("end", () =>
            {
                res.writeHead(200);
                res.end();
            });
        });
        server.listen(8080, done);
    });

    after(function(done)
    {
        clock.uninstall();
        server.close(done);
    });

    it("should fake HTTP Date header time", function(done)
    {
        const expectedHeaders =
        {
            "connection": "close",
            "date": "Tue, 31 Dec 2013 23:59:59 GMT",
            "transfer-encoding": "chunked"
        };
        const opts =
        {
            hostname: "localhost",
            method: "GET",
            path: "/",
            port: 8080
        };
        const req = http.request(opts, (res) =>
        {
            const {headers, statusCode, statusMessage} = res;
            res.on("data", () => void null);
            res.on("end", () =>
            {
                assert.strictEqual(statusCode, 200);
                assert.strictEqual(statusMessage, "OK");
                assert.deepStrictEqual(headers, expectedHeaders);
                done();
            });
        });
        req.end();
    });
});

Note

Starting with Node 15.2.0 http.js imports Date from primordials while Node 15.1.0 http.js just uses the global Date object.

[benjamingr]

That sounds reasonable, I think the current behaviour is expected but the use-case wasn't considered and the breakage wasn't expected.

Alternatives I can think of:

• Expose a --no-primordials flag or similar making primordials not return frozen instances.
• Expose an API for manipulating the base date/time inside Node.js
• Not support the case and expect users to mock time at a system level or to set the header manually themselves regardless of whether or not thy're in a test.
• Expose an API directly for http that sets the server time used in headers

[gorankarlic]

Looking at the alternatives:

1. Expose a --no-primordials flag or similar making primordials not return frozen instances.
   • Pro: simple fix that would allow stubbing of primordials across the board
   • Con: extra complexity in Node core, different production / test environments, may introduce subtleties
2. Expose an API for manipulating the base date/time inside Node.js
   • Pro: addresses the issue specifically
   • Con: does not enable stubbing of primordials in general
3. Not support the case and expect users to mock time at a system level or to set the header manually themselves regardless of whether or not thy're in a test.
   • Pro: no change Node, only need to amend Sinon documentation for clarity
   • Con: does not address actual issue; system level time mocking is not practical (messes up calendar reminders, leaves incorrect system time in case of test breakages, NTP clock-sync interference, timezone issues etc.)
4. Expose an API directly for http that sets the server time used in headers
   • Pro: Node already provides response.sendDate
   • Con: need to carry test-related code in implementation, e.g. response.sendDate = process.env.NODE_ENV === "production" or similar

I would be in favour of (1) or (2) because (3) is not practical / does not address the issue and (4) is not quite good as implementation needs to carry test code.

Looking forward to reading your opinion on this.

[benjamingr]

Hmm, @aduh95 what do you think?

[aduh95]

I quite like the flag idea as it gives to users the more power over how Node.js internals behave. What about a --expose-primordials flag? I think it would be easier to implement than a --no-primordials one, and would allow users to define a specific behaviour for Node.js internals without even changing the globals.

[benjamingr]

@gorankarlic Antoine's PR landed in Node - can you please take a look :)?

[fatso83]

The PR was merged as part of Node 15.7. What's the next step to get this closed?

[fatso83]

@gorankarlic Could you test this on Node 15.7 and see if we are getting closer?

[fatso83]

Problem not seen in:

• Node 10
• Node 12
• Node 14.15.4

Problem seen in:

• Node 14.18.0
• Node 15.14.0

I now tried to see if I could understand how to expose the internals, but I was not successful:

node --expose-internals -r internal/test/binding  node_modules/.bin/mocha breaks.js

The breaks.js file is the reproducible snippet given in the original issue. The -r internal/test/binding bit is just something I took from the https://github.com/nodejs/node/pull/36872 PR, which I hoped would somehow make all of this work 🤷

[benjamingr]

I think the point of -r internal/test/binding is that it exposes a primordials global object with access to all the primordials (the locked object) - in our case we would need to check (and mock) globalThis.primordials.Date presumably.

[fatso83]

That was more promising. Thanks, Ben.

$ node --expose-internals -r internal/test/binding  -p -e 'Object.getOwnPropertyDescriptor(global.primordials, "Date")'
{
  value: [Function: Date],
  writable: false,
  enumerable: true,
  configurable: false
}
(node:1726) internal/test/binding: These APIs are for internal testing only. Do not use them.
(Use `node --trace-warnings ...` to show where the warning was created)

But ... we cannot really do much about this prop, can we? Seeing that it's neither writable nor configurable.

[benjamingr]

The primordial itself is not but its properties are. Nothing is stopping us from doing this for example:

➜  Code git:(main) node --expose-internals -r internal/test/binding -e "primordials.Date.now = () => 3; console.log(Date.now())" 
3
(node:27563) internal/test/binding: These APIs are for internal testing only. Do not use them.
(Use `node --trace-warnings ...` to show where the warning was created)

[fatso83]

Yeah, but in the case of this exact issue, it seems that it is the Date constructor the code supplied by @gorankarlic depends upon, so we are kind of screwed. Not a huge loss IMHO: we can never guarantee that modules outside of our control does not hold an internal reference to the original Date object, which is what is going on here anyway.

[benjamingr]

But I think we can override the methods called after new Date() which are called (like toUTCString() and valueOf()) instead can't we?

[fatso83]

Aha ... true. There are more ways to skin a cat, obviously 😄 We cannot have our cake, but we can probably eat it anyway, etc, etc..

With regards to the current http implementation, just primordials.Date.prototype.toUTCString = function(){ return 'foo' } probably won't fly. Node http caches a direct reference to DatePrototypeToUTCString, which I think defeats any attempt we could come up with that touches Date.

This target seems to move faster than we can come up with workarounds, so I am suspecting that even if we should start replacing all methods on the Date.prototype the amount of usecases we actually are able to support is rather slim. Don't mind trying, though. Or rather, I don't mind someone trying 😃

[benjamingr]

I can just make a PR to node to not do this when the --expose-primordials flag is set probably?

[benjamingr]

Let's see how it goes https://github.com/nodejs/node/pull/40733

[fatso83]

This seems like something we cannot fix ourselves, @gorankarlic. Ref https://github.com/nodejs/node/pull/40733#issuecomment-1016522091

[gorankarlic]

Thank you for trying hard to fix this & sorry for the late reply.

It seems the only way forward is to ignore the HTTP Date header.

IMHO it's very sad that NodeJS seems to have sacrificed it's original, simple and beautiful idea - i.e. expose the POSIX API to ECMAScript - to such an extent that AFAIK it now blatantly violates ECMAScript itself, in that modifying a prototype is not possible and/or does not result in the correct/expected behaviour; it instead has seemingly become a micro-optimization-driven mess at it's very core.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[fatso83]

This is not moving anywhere and it was clear to years ago that it will not be possible to hook into Node in any way. Tests are best written to not rely on this header, basically.