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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: Setting content-type on reply object does not work correctly and/or hangs server #3283

Closed
petef19 opened this issue Aug 28, 2021 · 8 comments
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@petef19
Copy link

petef19 commented Aug 28, 2021

馃挰 Bug report

It seems that fastify tries to force guess the content-type of the response, and does ONLY allow to override it on the reply object if it matches what it has guessed/determined, but even that does not work correctly all the time.

example 1

reply
    .code(200)
    .headers({'content-type': 'text/plain; charset=utf-32'})
    .send(JSON.stringify({bar: 'foo', baz: 'foobar'}));

this works b/c .send is sending a text string, hence fastify will take the override in .headers(). Btw, the override also works in this example w/o adding the charset (contrary to example 3 below).

example 2

reply
    .code(200)
    .headers({'content-type': 'text/plain; charset=utf-8'})
    .send({bar: 'foo', baz: 'foobar'});

this does NOT work - .send is sending an object, hence fastify determines application/json; charset=utf-8 and will simply ignore the override in .headers()... more importantly: this now hangs the server forever, the response will never come through

example 3

reply
    .code(200)
    .headers({'content-type': 'application/json'})
    .send({bar: 'foo', baz: 'foobar'});

looking at the code you'd think this would work, we're sending an object and fastify will guess application/json and hence use the override in .headers(). It doesn't. It sends application/json; charset=utf-8, which does NOT match what was specified in .headers().

Summary:
example 1: it uses the override
example 2: it does NOT use the override and hangs
example 3: it does NOT use the override although the logic is very similar to example 1

Your Environment

  • node version: 14.6.0
  • fastify version: 3.20.2
  • os: Win 10 x64
@climba03003
Copy link
Member

I think it will not override the type when ever a user set the content-type header.

fastify/lib/reply.js

Lines 136 to 137 in a04f4c9

const contentType = this.getHeader('content-type')
const hasContentType = contentType !== undefined

@climba03003
Copy link
Member

climba03003 commented Aug 28, 2021

Example 1:
It is correct behavior.

Example 2:
You need to ensure the payload is one of the following type which stated in the document clearly.
https://github.com/fastify/fastify/blob/main/docs/Reply.md#type-of-the-final-payload

When using a custom content-type it skip all the serializing code

fastify/lib/reply.js

Lines 139 to 191 in a04f4c9

if (payload !== null) {
if (Buffer.isBuffer(payload) || typeof payload.pipe === 'function') {
if (hasContentType === false) {
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.OCTET
}
onSendHook(this, payload)
return this
}
if (hasContentType === false && typeof payload === 'string') {
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.PLAIN
onSendHook(this, payload)
return this
}
}
if (this[kReplySerializer] !== null) {
if (typeof payload !== 'string') {
preserializeHook(this, payload)
return this
} else {
payload = this[kReplySerializer](payload)
}
// The indexOf below also matches custom json mimetypes such as 'application/hal+json' or 'application/ld+json'
} else if (hasContentType === false || contentType.indexOf('json') > -1) {
if (hasContentType === false) {
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.JSON
} else {
// If hasContentType === true, we have a JSON mimetype
if (contentType.indexOf('charset') === -1) {
// If we have simply application/json instead of a custom json mimetype
if (contentType.indexOf('/json') > -1) {
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.JSON
} else {
const currContentType = this[kReplyHeaders]['content-type']
// We extract the custom mimetype part (e.g. 'hal+' from 'application/hal+json')
const customJsonType = currContentType.substring(
currContentType.indexOf('/'),
currContentType.indexOf('json') + 4
)
// We ensure we set the header to the proper JSON content-type if necessary
// (e.g. 'application/hal+json' instead of 'application/json')
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.JSON.replace('/json', customJsonType)
}
}
}
if (typeof payload !== 'string') {
preserializeHook(this, payload)
return this
}
}

So, it directly pass object to the res.end and it throw an error.
I can confirm it is a bug due to onSendHook set kReplySent to true.

reply[kReplySent] = true

Then onSendEnd cannot properly throw and send the error message.

fastify/lib/reply.js

Lines 429 to 431 in a04f4c9

if (typeof payload !== 'string' && !Buffer.isBuffer(payload)) {
throw new FST_ERR_REP_INVALID_PAYLOAD_TYPE(typeof payload)
}

Example 3:
It is intended behavior to always set charset when we see application/json only content-type.

fastify/lib/reply.js

Lines 170 to 173 in a04f4c9

// If we have simply application/json instead of a custom json mimetype
if (contentType.indexOf('/json') > -1) {
this[kReplyHeaders]['content-type'] = CONTENT_TYPE.JSON
} else {

@climba03003 climba03003 transferred this issue from fastify/help Aug 28, 2021
@climba03003 climba03003 added bug Confirmed bug good first issue Good for newcomers labels Aug 28, 2021
@climba03003
Copy link
Member

We do have 2 test-case against the invalid payload, however it is checked inside the handler which do not ensure the client always get the error response.

// Issue 595 https://github.com/fastify/fastify/issues/595
test('\'*\' should throw an error due to serializer can not handle the payload type', t => {
t.plan(3)
const fastify = Fastify()
fastify.get('/', (req, reply) => {
reply.type('text/html')
try {
reply.send({})
} catch (err) {
t.type(err, TypeError)
t.equal(err.code, 'FST_ERR_REP_INVALID_PAYLOAD_TYPE')
t.equal(err.message, "Attempted to send payload of invalid type 'object'. Expected a string or Buffer.")
}
})
fastify.inject({
url: '/',
method: 'GET'
}, (e, res) => {
t.fail('should not be called')
})
})
test('should throw an error if the custom serializer does not serialize the payload to a valid type', t => {
t.plan(3)
const fastify = Fastify()
fastify.get('/', (req, reply) => {
try {
reply
.type('text/html')
.serializer(payload => payload)
.send({})
} catch (err) {
t.type(err, TypeError)
t.equal(err.code, 'FST_ERR_REP_INVALID_PAYLOAD_TYPE')
t.equal(err.message, "Attempted to send payload of invalid type 'object'. Expected a string or Buffer.")
}
})
fastify.inject({
url: '/',
method: 'GET'
}, (e, res) => {
t.fail('should not be called')
})
})

@petef19
Copy link
Author

petef19 commented Aug 28, 2021

Example 2:
You need to ensure the payload is one of the following type which stated in the document clearly.
https://github.com/fastify/fastify/blob/main/docs/Reply.md#type-of-the-final-payload

what is confusing about this is that fastify usually converts objects automatically to json - in example 2, if I omit setting the header, it automatically stringifies the object and then sets application/json - but I guess it doesn't do it when I set 'plain/text';

Example 3:
It is intended behavior to always set charset when we see application/json only content-type.

ok, hopefully that is in the docs somewhere.
But, what is confusing is, that with plain/text content-type (set by user) fastify does NOT automatically add the charset, although when it auto detects/guesses plain/text (and user did not explicitly set a content type) it auto adds the charset to plain/text resulting in to plain/text; charset=utf-8....

Thanks !

@iamdevelopergirl
Copy link

I understand the problem and I want to work on this issue. Can I take it?

@mcollina
Copy link
Member

I understand the problem and I want to work on this issue. Can I take it?

go for it!

@iamdevelopergirl
Copy link

Thanks.

@jsumners
Copy link
Member

I think this can be closed as resolved by #3285.

sergejostir added a commit to sergejostir/fastify that referenced this issue Nov 14, 2021
sergejostir added a commit to sergejostir/fastify that referenced this issue Nov 14, 2021
mcollina pushed a commit that referenced this issue Nov 23, 2021
* fixup! Encapsulated error handling (#3261)

* fixup! fixed unresponsive server  #3283 (#3285)

* fixup! fix: reply object not marked as sent for stream payloads (#3318)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

5 participants