chore: Updated content-type header parsing - #6414

It like a light weight version of content-type parser?
Removed the parameters parts, it is ok on our case because we never use it.

How the performance after the change?

It like a light weight version of content-type parser? Removed the parameters parts, it is ok on our case because we never use it.

In our code base, "content-type parser" refers to the thing that collects and manages the functions used to parse HTTP POST bodies. Prior to this PR, the indicator used to denote what format those POST bodies is in was inspected in various ways that are very fragile. This PR introduces a dedicated identifier parser.

How the performance after the change?

I don't know. Likely slower than the previous fragile checks, but likely not the slowest possible. Unfortunately, proper handling of the content-type header value is an imperative. We cannot continue using the previous fragile inspections.

Are you sure that there are no content types without a /? I think a user could create one like that, and I feel we should kind of support it.

Please review https://httpwg.org/specs/rfc9110.html#media.type

The required format is type/subtype. Anything written as type is not spec compliant. We opt to adhere to the specs: https://github.com/fastify/fastify/blob/main/docs/Reference/Principles.md#specification-adherence

@mcollina I have written the benchmark below to test the performance of splitting out only the type and subtype parts. I agree that the IIFE is too expensive. It looks like the regular expression version becomes worth it as we start dealing with the parameters.

'use strict'

const assert = require('node:assert')
const { Suite } = require('bench-node')
const suite = new Suite({
  reporter
})

const basicMediaType = 'application/json'
const paramsMediaType = 'application/json; charset=utf-8; foo=bar'

suite.add('iife split (basic)', () => {
  let parts = (() => {
    const [first, ...rest] = basicMediaType.split('/')
    if (rest.length === 0) return [first]
    return [first, rest.join('/')]
  })()
  assert.equal(parts[0], 'application')
  assert.equal(parts[1], 'json')
})

suite.add('iife split (params)', () => {
  let parts = (() => {
    const [first, ...rest] = paramsMediaType.split('/')
    if (rest.length === 0) return [first]
    return [first, rest.join('/')]
  })()

  const type = parts[0]
  parts = parts[1].split(';')
  const subtype = parts[0]
  
  assert.equal(type, 'application')
  assert.equal(subtype, 'json')
})

suite.add('long split (basic)', () => {
  let parts = basicMediaType.split('/')
  assert.equal(parts[0], 'application')
  assert.equal(parts[1], 'json')
})

suite.add('long split (params)', () => {
  let parts = paramsMediaType.split('/')
  const type = parts[0]

  parts = parts[1].split(';')
  const subtype = parts[0]

  assert.equal(type, 'application')
  assert.equal(subtype, 'json')
})

const tokensReg = /^([\w!#$%&'*+.^`|~-]+)\/([\w!#$%&'*+.^`|~-]+)\s*;?/
suite.add('regex split (basic)', () => {
  const matches = tokensReg.exec(basicMediaType)
  const type = matches[1]
  const subtype = matches[2]
  assert.equal(type, 'application')
  assert.equal(subtype, 'json')
})

suite.add('regex split (params)', () => {
  const matches = tokensReg.exec(paramsMediaType)
  const type = matches[1]
  const subtype = matches[2]
  assert.equal(type, 'application')
  assert.equal(subtype, 'json')
})

suite.run()

const { format } = new Intl.NumberFormat('en-US', {
  useGrouping: true
})
function reporter (results) {

  for (const result of results) {
    const ops = format(result.opsSec)
    console.log(`${result.name}: ${ops} ops/sec`)
  }
}

❯ node --allow-natives-syntax index.js
iife split (basic): 9,655,587.18 ops/sec
iife split (params): 6,261,203.487 ops/sec
long split (basic): 15,462,605.433 ops/sec
long split (params): 8,142,902.648 ops/sec
regex split (basic): 15,020,238.273 ops/sec
regex split (params): 14,836,393.537 ops/sec

I'll be getting back to this soon.

Hi @jsumners and @mcollina ,

I've analyzed the blocker regarding the performance vs. accuracy of the header parsing. Since Regex is proving difficult for strictness and split has overhead, the most performant way to handle this in V8 is usually a direct indexOf slice.

Here is a proposed solution that fixes the vulnerability (adding the missing else check) and includes a high-performance parser that passes the tests without Regex complexity.

1. Optimized Parser (Replaces getEssenceMediaType) This handles application/json; charset=utf-8 correctly without allocating arrays.

function getEssenceMediaType (header) {
  if (!header) return ''
  let end = header.indexOf(';')
  if (end === -1) end = header.length
  return header.slice(0, end).trim().toLowerCase()
}

The Logic Fix (Inside validate) This ensures we don't "Fail Open" if the content type is missing from the schema map.

// ... inside validate() ...
    } else if (context[bodySchema]) {
      const contentType = getEssenceMediaType(request.headers['content-type'])
      const contentSchema = context[bodySchema][contentType]
      
      if (contentSchema) {
        validatorFunction = contentSchema
      } else {
        // Fix: Fail closed if Content-Type is present but not defined in schema
        const error = new Error('Content-Type not supported')
        error.statusCode = 415
        error.code = 'FST_ERR_VALIDATION'
        return error
      }
    }

This approach should satisfy the 415 test expectation and resolve the performance regression.

Are you sure that there are no content types without a /? I think a user could create one like that, and I feel we should kind of support it.

Please review https://httpwg.org/specs/rfc9110.html#media.type

The required format is type/subtype. Anything written as type is not spec compliant. We opt to adhere to the specs: https://github.com/fastify/fastify/blob/main/docs/Reference/Principles.md#specification-adherence

Whats the appropriate workaround/approach to take when a client is serving up invalid headers?

Whats the appropriate workaround/approach to take when a client is serving up invalid headers?

It is not clear to me client means request or reply?
For request, the behavior should be similar as before.
Parsing do not happen when the Content-Type do not match the media type part.
If you want to ensure do something on the invalid media type, you can

fastify.post('/', async function(request, reply) {
  if(request.mediaType == undefined) {
    // You can use hook to do the same thing.
    throw Error('Invalid Content-Type')
  } else {
    reply.send({ ok: true })
  }
})

For response, developer should always ensure they send a valid header.

I'm pretty confused, and I think maybe this isn't quite right yet.

RFC 9110 states:

“Unless noted otherwise, a recipient MAY attempt to recover a usable protocol element from an invalid construct.”

In a Fastify service that receives webhooks from Jira, for example, we observe Content-Type headers like:

"application/json,application/json"                  (rejected after this change)
"application/json; charset=utf-8,application/json"   (still accepted after this change)

Best I can tell, a request ends up with duplicate Content-Type headers and then some intermediary collapses them into a single header. By the time they reach Fastify, they are in this weird state.

We do not appear to enable joinDuplicateHeaders, so I suspect something in front of the service (either in the thing sending it or our infra). Either way, these are headers our service receives and cannot control. It might even be outside the client’s control if the request passes through other layers that introduce this behavior.

Previously, for application/json,application/json, I could add an encapsulated RegExp addContentTypeParser on the affected routes. Fastify 5.5 passed the raw header value through parser matching, our RegExp recognized the known malformed form, and we deliberately selected the JSON body parser. That seems like a good example of the application-specific recovery permitted by RFC 9110.

After this change, however, application/json,application/json is rejected before my custom parser is consulted. I instead have to handle it in an onRequest hook by parsing and mutating the header into a valid value before Fastify performs its validation.

I understand that addContentTypeParser manages body-parser selection rather than being a general-purpose header parser. However, registering a RegExp parser was previously a clear way to say, “For this known external header form, parse the body as FOO”

Is onRequest header mutation really the intended layer for recovering correctable external input? This seems like both a spec-conformance fix and a Fastify policy and extension-point change. I am open to being convinced otherwise, but this feels like behavior that should have been introduced behind an option or in a breaking release.

@climba03003, if my understanding is correct, checking request.mediaType in the handler would not work. The request handler never runs because Fastify responds with 415 Unsupported Media Type before entering it. When inspected from an onRequest hook, request.mediaType also appears to be "/" for this invalid value rather than undefined.

My main concerns are:

• This broke a previously working use of an explicitly registered RegExp content-type parser, but it was released as a bug fix.
• Stricter default validation may be appropriate, but preventing a registered parser from implementing an application-specific recovery policy is a separate behavioral change.
• addContentTypeParser was a reasonably clear place to recover body-parser selection for a known malformed header. Why require applications to move that logic into onRequest and mutate the raw header instead?

I sketched out my idea here: #6918 @jsumners I think this actually still aligns with the intention here, while also making this less of a breaking change. Essentially it restores the ability to use a regxp addContentTypeParser to correct for bad headers, and doesn't force you to move this to onRequest while keeping the stricter header validation.

I also put together a WHATWG token based parsing alternative, which retains the stronger validation of this PR, but uses a more forgiving and standards based approach to invalid header recovery. Please check it out #6919

It complements #6918 which restores the location of header validation.

RFC 9110 states:

“Unless noted otherwise, a recipient MAY attempt to recover a usable protocol element from an invalid construct.”

"MAY" is not "MUST". As noted elsewhere, the Fastify project opts to strictly adhere to the RFCs.

I instead have to handle it in an onRequest hook by parsing and mutating the header into a valid value before Fastify performs its validation.

This sounds like you are able to handle these absolutely incorrect header values.

Is onRequest header mutation really the intended layer for recovering correctable external input? This seems like both a spec-conformance fix and a Fastify policy and extension-point change. I am open to being convinced otherwise, but this feels like behavior that should have been introduced behind an option or in a breaking release.

My position is: yes, the onRequest hook is the feature to use if you need to massage the request before it is processed.

The simple fact of the matter is that invalid content-type headers are constant source of "security" reports. My position will always be behind the strict parser, if for no reason other than to get those useless reports to stop.

This sounds like you are able to handle these absolutely incorrect header values.

Yes of course I can accommodate it. But I had to change in a non-breaking update. My point is that you can still tighten the validation without moving where that happens. This change was also a breaking lifecycle policy change.

#6918 effectively keeps your change but avoids the breaking change which doesn't seem nessisary for any of the goals here unless I'm mistaken.

My goals were accomplished. The content-type parser was permitting invalid headers to pass through and/or jump to incorrect logic. This PR fixed those bugs. I am against any change that restores the previous behavior.

The current implementation still lets some invalid headers through to addContentTypeParser. (Do you want test more cases?) Is the intention to strictly gate all of those out now? Clarifying intention here would help me understand what's acceptable here.

I have seen you mention a case where a type+subtype is appended to the parameters list. That's just a bad parameters list and this feature explicitly opts to best effort those at this time. They are not used internally except to match incoming requests to the correct parser. A patch that rejects this malformed content in an efficient manner would be acceptable.

Just chatted with @mcollina on a call about this and have a better understanding of what the issue/constraints are.

• The goal is to strictly drop invalid content type headers into addContentTypeParser, because the content-type narrowed schema validator can be bypassed by invalid content-type headers (kinda seems like an issue with the schema in an app implementation, but I guess its come up enough to necessitate this gate due to the validation guarantees). I may have found some invalid headers that are still getting through. Let me look into weather they matter or not for this concern.
• Adding an option to disable the strict validation branch in into contentTypeParser.run in lib/handle-request.js is a possibility, or maybe adding a hook to swap in custom ContentParser implementation. Might be worth adding as an escape hatch due to breaking nature in a non-major release.
• Mushing around content-type headers in onRequest into something valid is fine, maybe the preferred place to do it, but addContentTypeParser should document about this new limitation (they don't run if they don't pass fastify's validation).
• The whatwg algo doesn't materially improve the security needs here, and the standards behavior defensibility isn't relevant for this issue.

A patch that rejects this malformed content in an efficient manner would be acceptable.

Will poke around if I can come up with something, thank you for the guidance.

Small docs PR: #6925

Explains the new behavior and hazard.

This PR: #6926

Makes strict validation prior to consulting addContentTypeParser optional, defaulting to the new strict behavior.

This PR: #6927

Adds a different/alternative/complementary escape hatch allowing callers to customize/extend/replace the ContentType parser.

I sent some time trying to work through understand what is needed here wrt strict RFC 9110 validation, but also the lax trailer parsing, and normalization recovery that remains.

- \u00a0application/json
- application/json\u00a0
- application/json\u00a0; charset=utf-8

Looks like there might be some whitespace normalization leaks as these seems to leak through. Currently contentType.isValid seems to mean "Fastify recovered a usable type/subtype." Not: “The complete original header value conforms to the RFC 9110 Content-Type grammar.” Not sure what the intention here happy to expand this validation if people think its needed.

I honestly can't remember the full details around it. But my vague recollection is that parts of the codebase allowed for empty values while needing to distinguish such a value from an invalid header.

Improvements are welcome.