Skip to content

Commit

Permalink
@uppy/companion: only body parse when needed & increased body size fo…
Browse files Browse the repository at this point in the history
…r s3 (transloadit#4372)

* refine body parsers

to make it possible to adjust them individually
also improves security and speed
because we don't have to parse all body types (and all sizes) for all endpoints

* increase body size for s3 complete endpoint

fixes transloadit#1945
  • Loading branch information
mifi authored Mar 28, 2023
1 parent 011fdd5 commit 9420b52
Show file tree
Hide file tree
Showing 5 changed files with 28 additions and 19 deletions.
13 changes: 8 additions & 5 deletions src/companion.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,8 +97,11 @@ module.exports.app = (optionsArg = {}) => {
app.use(cookieParser()) // server tokens are added to cookies

app.use(interceptGrantErrorResponse)

// override provider credentials at request time
app.use('/connect/:authProvider/:override?', getCredentialsOverrideMiddleware(providers, options))
// Making `POST` request to the `/connect/:provider/:override?` route requires a form body parser middleware:
// See https://github.com/simov/grant#dynamic-http
app.use('/connect/:authProvider/:override?', express.urlencoded({ extended: false }), getCredentialsOverrideMiddleware(providers, options))
app.use(Grant(grantConfig))

app.use((req, res, next) => {
Expand All @@ -116,19 +119,19 @@ module.exports.app = (optionsArg = {}) => {
app.use('/s3', s3(options.s3))
app.use('/url', url())

app.post('/:providerName/preauth', middlewares.hasSessionAndProvider, controllers.preauth)
app.post('/:providerName/preauth', express.json(), express.urlencoded({ extended: false }), middlewares.hasSessionAndProvider, middlewares.hasBody, controllers.preauth)
app.get('/:providerName/connect', middlewares.hasSessionAndProvider, controllers.connect)
app.get('/:providerName/redirect', middlewares.hasSessionAndProvider, controllers.redirect)
app.get('/:providerName/callback', middlewares.hasSessionAndProvider, controllers.callback)
app.post('/:providerName/deauthorization/callback', middlewares.hasSessionAndProvider, controllers.deauthorizationCallback)
app.post('/:providerName/deauthorization/callback', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, controllers.deauthorizationCallback)
app.get('/:providerName/logout', middlewares.hasSessionAndProvider, middlewares.gentleVerifyToken, controllers.logout)
app.get('/:providerName/send-token', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.sendToken)
app.get('/:providerName/list/:id?', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.list)
app.post('/:providerName/get/:id', middlewares.hasSessionAndProvider, middlewares.verifyToken, controllers.get)
app.post('/:providerName/get/:id', express.json(), middlewares.hasSessionAndProvider, middlewares.hasBody, middlewares.verifyToken, controllers.get)
app.get('/:providerName/thumbnail/:id', middlewares.hasSessionAndProvider, middlewares.cookieAuthToken, middlewares.verifyToken, controllers.thumbnail)
// @ts-ignore Type instantiation is excessively deep and possibly infinite.
app.get('/search/:searchProviderName/list', middlewares.hasSearchQuery, middlewares.loadSearchProviderToken, controllers.list)
app.post('/search/:searchProviderName/get/:id', middlewares.loadSearchProviderToken, controllers.get)
app.post('/search/:searchProviderName/get/:id', express.json(), middlewares.loadSearchProviderToken, controllers.get)

app.param('providerName', providerManager.getProviderMiddleware(providers, true))
app.param('searchProviderName', providerManager.getProviderMiddleware(searchProviders))
Expand Down
9 changes: 5 additions & 4 deletions src/server/controllers/s3.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const router = require('express').Router
const express = require('express')

module.exports = function s3 (config) {
if (typeof config.acl !== 'string' && config.acl != null) {
Expand Down Expand Up @@ -342,13 +342,14 @@ module.exports = function s3 (config) {
})
}

return router()
return express.Router()
.get('/params', getUploadParameters)
.post('/multipart', createMultipartUpload)
.post('/multipart', express.json(), createMultipartUpload)
.get('/multipart/:uploadId', getUploadedParts)
.get('/multipart/:uploadId/batch', batchSignPartsUpload)
.get('/multipart/:uploadId/:partNumber', signPartUpload)
.post('/multipart/:uploadId/complete', completeMultipartUpload)
// limit 1mb because maybe large upload with a lot of parts, see https://github.com/transloadit/uppy/issues/1945
.post('/multipart/:uploadId/complete', express.json({ limit: '1mb' }), completeMultipartUpload)
.delete('/multipart/:uploadId', abortMultipartUpload)
}

Expand Down
8 changes: 4 additions & 4 deletions src/server/controllers/url.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const router = require('express').Router
const express = require('express')
const validator = require('validator')

const { startDownUpload } = require('../helpers/upload')
Expand Down Expand Up @@ -112,6 +112,6 @@ const get = async (req, res) => {
startDownUpload({ req, res, getSize, download, onUnhandledError })
}

module.exports = () => router()
.post('/meta', meta)
.post('/get', get)
module.exports = () => express.Router()
.post('/meta', express.json(), meta)
.post('/get', express.json(), get)
13 changes: 11 additions & 2 deletions src/server/middlewares.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,8 @@ const getS3Client = require('./s3-client')
const { getURLBuilder } = require('./helpers/utils')

exports.hasSessionAndProvider = (req, res, next) => {
if (!req.session || !req.body) {
logger.debug('No session/body attached to req object. Exiting dispatcher.', null, req.id)
if (!req.session) {
logger.debug('No session attached to req object. Exiting dispatcher.', null, req.id)
return res.sendStatus(400)
}

Expand All @@ -22,6 +22,15 @@ exports.hasSessionAndProvider = (req, res, next) => {
return next()
}

exports.hasBody = (req, res, next) => {
if (!req.body) {
logger.debug('No body attached to req object. Exiting dispatcher.', null, req.id)
return res.sendStatus(400)
}

return next()
}

exports.hasSearchQuery = (req, res, next) => {
if (typeof req.query.q !== 'string') {
logger.debug('search request has no search query', 'search.query.check', req.id)
Expand Down
4 changes: 0 additions & 4 deletions src/standalone/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ const express = require('express')
const qs = require('node:querystring')
const helmet = require('helmet')
const morgan = require('morgan')
const bodyParser = require('body-parser')
const { URL } = require('node:url')
const session = require('express-session')
const addRequestId = require('express-request-id')()
Expand Down Expand Up @@ -96,9 +95,6 @@ module.exports = function server (inputCompanionOptions) {
return undefined
})

router.use(bodyParser.json())
router.use(bodyParser.urlencoded({ extended: false }))

// Use helmet to secure Express headers
router.use(helmet.frameguard())
router.use(helmet.xssFilter())
Expand Down

0 comments on commit 9420b52

Please sign in to comment.