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

fast-json-stringify is not a validator #558

Merged
merged 2 commits into from
Nov 8, 2022
Merged

fast-json-stringify is not a validator #558

merged 2 commits into from
Nov 8, 2022

Conversation

mcollina
Copy link
Member

@mcollina mcollina commented Nov 7, 2022

Let's make it clear that fast-json-stringify is not a validator and the caller takes all the responsibilities on the safety of the data being rendered.

Checklist

@ivan-tymoshenko
Copy link
Member

Does it mean that we can drop all unnecessary validation checks in the next fastify major release? Because if not, then it's better to specify which validation check we do.

@mcollina
Copy link
Member Author

mcollina commented Nov 7, 2022

It means that we offer no guarantees we will do any checks. If we want to do checks it's up to us.

@ivan-tymoshenko
Copy link
Member

Why do we want to make some validation checks that are not used in serialization (they are just validation checks) and don't tell users about them?

@ivan-tymoshenko
Copy link
Member

ivan-tymoshenko commented Nov 7, 2022

I want to say that validation in the fast-json-stringify is the main and one of the most difficult questions for me right now. We have a thread where we discuss how it should work in the next major.

I agree that FJS doesn't guarantee that your input or output will match 100% of the schema, and I'm ok with such a comment. But the question is much deeper, and if we say that "fast-json-stringify makes no claim to validate the incoming data against the schema" it's a little bit strange for me that we throw an error if your data doesn't match the schema in some cases.

README.md Outdated
Comment on lines 653 to 654
The _data_ that `fast-json-stringify` serializes is trusted too. In other terms,
fast-json-stringify makes no claim to validate the incoming data against the schema.
Copy link
Member

@climba03003 climba03003 Nov 7, 2022

Choose a reason for hiding this comment

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

The _data_ that `fast-json-stringify` serializes should be a trusted source. 
Developer should take their responsibility to sanitize the _data_ before passing 
to `fast-json-stringify`. The validation in this package is only against the `schema` 
but not `malicious code`.

Copy link
Member

Choose a reason for hiding this comment

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

Would it be better? @ivan-tymoshenko

Copy link
Member Author

Choose a reason for hiding this comment

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

That would work for me.

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean by malicious code? Whatever it means I agree with this point, because FJS doesn't check your data for any malicious code. But I think you talking about something different. Sanitizing is not validation.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, I saw the original security report. I would say that: fast-json-stringify guarantees that you will get a valid output only if your input matches the schema or can be coerced to the schema. If your input doesn't match the schema, you will get undefined behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

PTAL

@Fdawgs
Copy link
Member

Fdawgs commented Nov 7, 2022

Is it also worth using GitHub's info or warning highlighting syntax, to draw more attention to it?

@jsumners
Copy link
Member

jsumners commented Nov 7, 2022

Is it also worth using GitHub's info or warning highlighting syntax, to draw more attention to it?

I would rather not rely on GitHub specific rendering syntax. Callouts with generic Markdown are easy enough -- https://github.com/fastify/fastify/blob/675b00d6ca7a7eeaad073e6469c70b50fcd532eb/docs/Guides/Getting-Started.md?plain=1#L94-L121

Signed-off-by: Matteo Collina <hello@matteocollina.com>
@ivan-tymoshenko
Copy link
Member

LGTM

@mcollina
Copy link
Member Author

mcollina commented Nov 8, 2022

@climba03003 PTAL

@Uzlopak
Copy link
Contributor

Uzlopak commented Nov 8, 2022

Ah the good old undefined behaviour :)

@Uzlopak Uzlopak merged commit b6d6811 into master Nov 8, 2022
@Uzlopak Uzlopak deleted the mcollina-patch-1 branch November 8, 2022 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants