-
-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix: potential bug in content type parser #4477
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a test that fails without this change.
First I want to see if the CI/CD pipeline fails or not. I have locally issues with running the tests, some unrelated tests fail. |
Ok, it seems like my expectation was met and the codechange did not make it fail the ci pipeline. I guess the unit test passes because it falls back to the json parser. Will provide a unit test asap. |
Interesting, it is not failed because var a = { toString() { return 'a' } }
'abc'.indexOf(a) // it is actually 0 One failing test for your changes would be |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for opening a PR! Can you please add a unit test?
Added 3 Unit tests. |
Why we would like to support a content type with leading and trailing space? Just a reminder, it is breaking change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it is breaking because the headers are trimmed and the spaces are preserved only if the user set the header value as " application/json"
(with double quotes)
It is a breaking change because here change from Also, if we accept it. It means that #4450 will not be able to land since it will becomes a breaking change again.
|
Consider the below case currently supported, but not documented. test('allow partial content-type', async t => {
t.plan(1)
const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser('json', function (request, body, done) {
t.pass('should be called')
done(null, body)
})
fastify.post('/', async () => {
return 'ok'
})
await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'application/json; foo=bar; charset=utf8'
},
body: ''
})
}) |
I tested only the spacing cases, not the |
I doubt that that works, because content-type parser will filter out media-types without / in it as invalid. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Don't mix-up supported content-type and incoming content-type. I still see no point on support |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Blocking, to be sure everyone know it is a breaking change before land.
One possible plugin that break would be @fastify/multipart
Or, use this.name = contentType.trim()
which does the same without breaking change.
hmm. it seems to me, that the implementation in fastify/multipart is wrong in the first place. It should be multipart/form-data and not multipart. |
Not really, all the https://www.rfc-editor.org/rfc/rfc2046#section-5.1.1
Hmm, seems like it blocked for other |
Well, should we target the next branch for this PR? |
@climba03003 @Uzlopak could you reach an agreement on the couple of PRs open on this? I don't understand the conflict and what will break. |
You can see the example in this comment. From what I can tell, |
Could you send a PR with that test #4477 (comment)? So we know it's supported. |
I added the unit test of @climba03003 to this PR. tests pass |
Please update the test to the following one. You are actually added a parser that would match all test('allow partial content-type', async t => {
t.plan(1)
const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser('json', function (request, body, done) {
t.pass('should be called')
done(null, body)
})
fastify.post('/', async () => {
return 'ok'
})
await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'application/json; foo=bar; charset=utf8'
},
body: ''
})
await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'image/jpeg'
},
body: ''
})
}) |
What about now? |
Look better, but still I believe more problem would cause than a fix when we allow trimming. Consider the following is allowed but the developer have no idea which one should be matched (Currently, the third one). fastify.addContentTypeParser(' application/json ', function (request, body, done) {
done(null, body)
})
fastify.addContentTypeParser('application/json ', function (request, body, done) {
done(null, body)
})
fastify.addContentTypeParser(' application/json', function (request, body, done) {
done(null, body)
}) And which one should be removed? Currently, none of them. fastify.addContentTypeParser(' application/json ', function (request, body, done) {
done(null, body)
})
fastify.addContentTypeParser('application/json ', function (request, body, done) {
done(null, body)
})
fastify.addContentTypeParser(' application/json', function (request, body, done) {
done(null, body)
})
fastify.removeContentTypeParser('application/json') When you consider |
@Uzlopak are you still pursuing this? |
As we never agreed on how to solve this issue, i can not finalize this PR. |
Is this PR finally obsolete? Of are there still some things we can derive from this? Can we close it? |
Is #4509 answered your problem trying to fix in this PR.
The next thing we need to consider is #4477 (comment) |
See #5329, this is no longer possible 🙏 I want to actually look at increasing matches though, right now it seems like a Content-Type header like |
It partially addressed, you need to do the same to |
Checklist
npm run test
andnpm run benchmark
and the Code of conduct