Skip to content

Commit

Permalink
feat: loosen content-type checking (#4450)
Browse files Browse the repository at this point in the history
  • Loading branch information
@climba03003
climba03003 committed Mar 1, 2024
1 parent 20cff61 commit f776447
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 181 deletions.
9 changes: 8 additions & 1 deletion docs/Reference/ContentTypeParser.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,13 @@ is given in the content-type header. If it is not given, the
[catch-all](#catch-all) parser is not executed as with `POST`, `PUT` and
`PATCH`, but the payload is simply not parsed.

> ## ⚠ Security Notice
> When using with RegExp to detect `Content-Type`, you should beware of
> how to properly detect the `Content-Type`. For example, if you need
> `application/*`, you should use `/^application\/([\w-]+);?/` to match the
> [essence MIME type](https://mimesniff.spec.whatwg.org/#mime-type-miscellaneous)
> only.
### Usage
```js
fastify.addContentTypeParser('application/jsoff', function (request, payload, done) {
Expand All @@ -52,7 +59,7 @@ fastify.addContentTypeParser('application/jsoff', async function (request, paylo
})

// Handle all content types that matches RegExp
fastify.addContentTypeParser(/^image\/.*/, function (request, payload, done) {
fastify.addContentTypeParser(/^image\/([\w-]+);?/, function (request, payload, done) {
imageParser(payload, function (err, body) {
done(err, body)
})
Expand Down
81 changes: 13 additions & 68 deletions lib/contentTypeParser.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

const { AsyncResource } = require('node:async_hooks')
const { FifoMap: Fifo } = require('toad-cache')
const { safeParse: safeParseContentType, defaultContentType } = require('fast-content-type-parse')
const secureJson = require('secure-json-parse')
const {
kDefaultJsonParse,
Expand All @@ -27,14 +26,15 @@ const {
FST_ERR_CTP_EMPTY_JSON_BODY,
FST_ERR_CTP_INSTANCE_ALREADY_STARTED
} = require('./errors')
const { FSTSEC001 } = require('./warnings')

function ContentTypeParser (bodyLimit, onProtoPoisoning, onConstructorPoisoning) {
this[kDefaultJsonParse] = getDefaultJsonParser(onProtoPoisoning, onConstructorPoisoning)
// using a map instead of a plain object to avoid prototype hijack attacks
this.customParsers = new Map()
this.customParsers.set('application/json', new Parser(true, false, bodyLimit, this[kDefaultJsonParse]))
this.customParsers.set('text/plain', new Parser(true, false, bodyLimit, defaultPlainTextParser))
this.parserList = [new ParserListItem('application/json'), new ParserListItem('text/plain')]
this.parserList = ['application/json', 'text/plain']
this.parserRegExpList = []
this.cache = new Fifo(100)
}
Expand Down Expand Up @@ -70,9 +70,9 @@ ContentTypeParser.prototype.add = function (contentType, opts, parserFn) {
this.customParsers.set('', parser)
} else {
if (contentTypeIsString) {
this.parserList.unshift(new ParserListItem(contentType))
this.parserList.unshift(contentType)
} else {
contentType.isEssence = contentType.source.indexOf(';') === -1
validateRegExp(contentType)
this.parserRegExpList.unshift(contentType)
}
this.customParsers.set(contentType.toString(), parser)
Expand Down Expand Up @@ -102,20 +102,11 @@ ContentTypeParser.prototype.getParser = function (contentType) {
const parser = this.cache.get(contentType)
if (parser !== undefined) return parser

const parsed = safeParseContentType(contentType)

// dummyContentType always the same object
// we can use === for the comparison and return early
if (parsed === defaultContentType) {
return this.customParsers.get('')
}

// eslint-disable-next-line no-var
for (var i = 0; i !== this.parserList.length; ++i) {
const parserListItem = this.parserList[i]
if (compareContentType(parsed, parserListItem)) {
const parser = this.customParsers.get(parserListItem.name)
// we set request content-type in cache to reduce parsing of MIME type
if (contentType.indexOf(parserListItem) === 0) {
const parser = this.customParsers.get(parserListItem)
this.cache.set(contentType, parser)
return parser
}
Expand All @@ -124,9 +115,8 @@ ContentTypeParser.prototype.getParser = function (contentType) {
// eslint-disable-next-line no-var
for (var j = 0; j !== this.parserRegExpList.length; ++j) {
const parserRegExp = this.parserRegExpList[j]
if (compareRegExpContentType(contentType, parsed.type, parserRegExp)) {
if (parserRegExp.test(contentType)) {
const parser = this.customParsers.get(parserRegExp.toString())
// we set request content-type in cache to reduce parsing of MIME type
this.cache.set(contentType, parser)
return parser
}
Expand Down Expand Up @@ -376,60 +366,15 @@ function removeAllContentTypeParsers () {
this[kContentTypeParser].removeAll()
}

function compareContentType (contentType, parserListItem) {
if (parserListItem.isEssence) {
// we do essence check
return contentType.type.indexOf(parserListItem) !== -1
} else {
// when the content-type includes parameters
// we do a full-text search
// reject essence content-type before checking parameters
if (contentType.type.indexOf(parserListItem.type) === -1) return false
for (const key of parserListItem.parameterKeys) {
// reject when missing parameters
if (!(key in contentType.parameters)) return false
// reject when parameters do not match
if (contentType.parameters[key] !== parserListItem.parameters[key]) return false
}
return true
function validateRegExp (regexp) {
// RegExp should either start with ^ or include ;?
// It can ensure the user is properly detect the essence
// MIME types.
if (regexp.source[0] !== '^' && regexp.source.includes(';?') === false) {
FSTSEC001(regexp.source)
}
}

function compareRegExpContentType (contentType, essenceMIMEType, regexp) {
if (regexp.isEssence) {
// we do essence check
return regexp.test(essenceMIMEType)
} else {
// when the content-type includes parameters
// we do a full-text match
return regexp.test(contentType)
}
}

function ParserListItem (contentType) {
this.name = contentType
// we pre-calculate all the needed information
// before content-type comparison
const parsed = safeParseContentType(contentType)
this.isEssence = contentType.indexOf(';') === -1
// we should not allow empty string for parser list item
// because it would become a match-all handler
if (this.isEssence === false && parsed.type === '') {
// handle semicolon or empty string
const tmp = contentType.split(';', 1)[0]
this.type = tmp === '' ? contentType : tmp
} else {
this.type = parsed.type
}
this.parameters = parsed.parameters
this.parameterKeys = Object.keys(parsed.parameters)
}

// used in ContentTypeParser.remove
ParserListItem.prototype.toString = function () {
return this.name
}

module.exports = ContentTypeParser
module.exports.helpers = {
buildContentTypeParser,
Expand Down
11 changes: 10 additions & 1 deletion lib/warnings.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ const { createDeprecation, createWarning } = require('process-warning')
* - FSTDEP018
* - FSTDEP019
* - FSTWRN001
* - FSTSEC001
*/

const FSTDEP005 = createDeprecation({
Expand Down Expand Up @@ -97,6 +98,13 @@ const FSTWRN001 = createWarning({
unlimited: true
})

const FSTSEC001 = createWarning({
name: 'FastifySecurity',
code: 'FSTSEC001',
message: 'You are using /%s/ Content-Type which may be vulnerable to CORS attack. Please make sure your RegExp start with "^" or include ";?" to proper detection of the essence MIME type.',
unlimited: true
})

module.exports = {
FSTDEP005,
FSTDEP006,
Expand All @@ -112,5 +120,6 @@ module.exports = {
FSTDEP018,
FSTDEP019,
FSTDEP020,
FSTWRN001
FSTWRN001,
FSTSEC001
}
121 changes: 10 additions & 111 deletions test/content-parser.test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,32 +421,25 @@ test('Safeguard against content-type spoofing - string', async t => {
})
})

test('Safeguard against content-type spoofing - regexp', async t => {
t.plan(1)
test('Warning against improper content-type - regexp', t => {
t.plan(2)

const fastify = Fastify()

process.on('warning', onWarning)
function onWarning (warning) {
t.equal(warning.name, 'FastifySecurity')
t.equal(warning.code, 'FSTSEC001')
}
t.teardown(() => process.removeListener('warning', onWarning))

fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser(/text\/plain/, function (request, body, done) {
t.pass('should be called')
done(null, body)
})
fastify.addContentTypeParser(/application\/json/, function (request, body, done) {
t.fail('shouldn\'t be called')
done(null, body)
})

fastify.post('/', async () => {
return 'ok'
})

await fastify.inject({
method: 'POST',
path: '/',
headers: {
'content-type': 'text/plain; content-type="application/json"'
},
body: ''
})
})

test('content-type match parameters - string 1', async t => {
Expand Down Expand Up @@ -477,34 +470,6 @@ test('content-type match parameters - string 1', async t => {
})
})

test('content-type match parameters - string 2', async t => {
t.plan(1)

const fastify = Fastify()
fastify.removeAllContentTypeParsers()
fastify.addContentTypeParser('application/json; charset=utf8; foo=bar', function (request, body, done) {
t.pass('should be called')
done(null, body)
})
fastify.addContentTypeParser('text/plain; charset=utf8; foo=bar', function (request, body, done) {
t.fail('shouldn\'t 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: ''
})
})

test('content-type match parameters - regexp', async t => {
t.plan(1)

Expand Down Expand Up @@ -650,72 +615,6 @@ test('content-type regexp list should be cloned when plugin override', async t =
}
})

test('allow partial content-type - essence check', 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: ''
})
})

test('allow partial content-type - not essence check', 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: ''
})
})

test('edge case content-type - ;', async t => {
t.plan(1)

Expand Down

0 comments on commit f776447

Please sign in to comment.