date-time format doesn't escape values properly

[Ry0taK]

Prerequisites

• I have written a descriptive issue title
• I have searched existing issues to ensure the bug has not already been reported

Fastify version

4.9.2

Plugin version

No response

Node.js version

v16.18.0

Operating system

Linux

Operating system version (i.e. 20.04, 11.3, 10)

N/A

Description

In serializer.js, the following code is used to serialize date-like objects.

  asDateTime (date) {
    if (date === null) return '""'
    if (date instanceof Date) {
      return '"' + date.toISOString() + '"'
    }
    if (typeof date === 'string') {
      return '"' + date + '"'
    }
    throw new Error(`The value "${date}" cannot be converted to a date-time.`)
  }

  asDate (date) {
    if (date === null) return '""'
    if (date instanceof Date) {
      return '"' + new Date(date.getTime() - (date.getTimezoneOffset() * 60000)).toISOString().slice(0, 10) + '"'
    }
    if (typeof date === 'string') {
      return '"' + date + '"'
    }
    throw new Error(`The value "${date}" cannot be converted to a date.`)
  }

  asTime (date) {
    if (date === null) return '""'
    if (date instanceof Date) {
      return '"' + new Date(date.getTime() - (date.getTimezoneOffset() * 60000)).toISOString().slice(11, 19) + '"'
    }
    if (typeof date === 'string') {
      return '"' + date + '"'
    }
    throw new Error(`The value "${date}" cannot be converted to a time.`)
  }

As you can see, if the date variable is a string, it appends two double quotes to the string without proper sanitization.
This means serializing an object like {"test": "\",\"new_key\":\"new_value"} will produce the following string if the format of test is date-time, which creates a new key called new_key when parsed by JSON.parse.

{"test": "","new_key":"new_value"}

(Please note that this issue was explicitly marked as a bug, rather than a security issue.)

Steps to Reproduce

fsj.js:

const fastJson = require('fast-json-stringify');
const stringify = fastJson({
    type: 'object',
    properties: {
        test: {
            type: "string",
            format: "date-time"
        }
    }
});

const obj = { "test": "\",\"new_key\":\"new_value" };
console.log(`Before stringify: ${JSON.stringify(obj)}`);
const str = stringify(obj);
console.log(`After stringify: ${str}`);

Confirm that fast-json-stringify didn't escape the value of test, and created a new key called new_key.

Before stringify: {"test":"\",\"new_key\":\"new_value"}
After stringify: {"test":"","new_key":"new_value"}

Expected Behavior

It shouldn't change the structure of the object.

[jsumners]

What would you like to see done here? A JSON string must be wrapped in " characters. When the value is typeof === string, then we should be wrapping it in ". If the user of the library passes in a string that deserializes in an unexpected way by JSON.parse, how is that FJS's issue?

[Ry0taK]

The problem here is that stringify(JSON.parse('{"test": "\",\"new_key\":\"new_value"}')) will become {"test": "","new_key":"new_value"} since asDateTime function doesn't escape values like asString does in serializer.js.
Please check HackerOne report #1763835 for more context.

[ivan-tymoshenko]

@Ry0taK fast-json-stringify uses json schema to speed up the serialization process. It uses validation only in cases when it can't decide how to serialize your data. If you pass your example in the default json serializer, you also get "malformed" data.

[jsumners]

FJS has nothing to do with the issue being reported from what I can tell:

https://runkit.com/62878ccef3acb000084759de/636907233a2c5f0008a93984

[Ry0taK]

@jsumners @ivan-tymoshenko Sorry, I posted a wrong script above. Correct one should be the following: stringify(JSON.parse('{"test": "\\",\\"new_key\\":\\"new_value"}'))

[ivan-tymoshenko]

JSON.parse('{"test": "\",\"new_key\":\"new_value"}')

Yes, this has more sense. You want us to call sanitize function here.

[Ry0taK]

@ivan-tymoshenko Yes, that's what I was thinking while reporting an original issue on HackerOne. However, as this package is trying optimize things as much as possible, it may make sense to choose performance over the security. So, instead of sanitizing inputs, adding a security notice about it like discussed in #558 may be an another option. (I'm just reporting it here as told by @mcollina so I'm not sure about the best way to address it.)