Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 34 additions & 3 deletions src/lib/functions/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,6 @@ const chalk = require('chalk')
const { warn } = require('../../utils/command-helpers')
const { getLogMessage } = require('../log')

const BASE_64_MIME_REGEXP = /image|audio|video|application\/pdf|application\/zip|applicaton\/octet-stream/i

const DEFAULT_LAMBDA_OPTIONS = {
verboseLevel: 3,
}
Expand All @@ -21,8 +19,41 @@ const detectAwsSdkError = ({ error }) => {

const formatLambdaError = (err) => chalk.red(`${err.errorType}: ${err.errorMessage}`)

// should be equivalent to https://github.com/netlify/proxy/blob/main/pkg/functions/request.go#L105
const exceptionsList = new Set([
Comment thread
netlify-team-account-1 marked this conversation as resolved.
'application/csp-report',
'application/graphql',
'application/json',
'application/javascript',
'application/x-www-form-urlencoded',
'application/x-ndjson',
'application/xml',
])

/**
* @param {string | undefined} contentType
* @returns {boolean}
*/
const shouldBase64Encode = function (contentType) {
return Boolean(contentType) && BASE_64_MIME_REGEXP.test(contentType)
if (!contentType) {
return true
}

contentType = contentType.toLowerCase()

if (contentType.startsWith('text/')) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason why we can't move these to a single condition?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how each of the conditions is its own case, à la "if it's a text, don't encode; if it's another content-type encoded as json or xml, don't encode; here's a set of exceptions we don't encode". Moving them into a single condition would make that grouping harder to see. Can you elaborate on why we should move them into a single condition?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I think having 3 termination points when you can have 1 makes it harder to trace and debug. I also don't see how having a single condition makes the grouping harder to see. It's not a big deal though, and I respect the personal preference if you think it adds more clarity.

return false
}

if (contentType.endsWith('+json') || contentType.endsWith('+xml')) {
return false
}

if (exceptionsList.has(contentType)) {
return false
}

return true
}

const styleFunctionName = (name) => chalk.magenta(name)
Expand Down
6 changes: 3 additions & 3 deletions tests/command.dev.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -487,7 +487,7 @@ testMatrix.forEach(({ args }) => {
t.is(response.body, undefined)
t.is(response.headers.host, `${server.host}:${server.port}`)
t.is(response.httpMethod, 'GET')
t.is(response.isBase64Encoded, false)
t.is(response.isBase64Encoded, true)
t.is(response.path, '/api/echo')
t.deepEqual(response.queryStringParameters, { ding: 'dong' })
})
Expand Down Expand Up @@ -591,7 +591,7 @@ testMatrix.forEach(({ args }) => {
form.append('some', 'thing')

const expectedBoundary = form.getBoundary()
const expectedResponseBody = form.getBuffer().toString()
const expectedResponseBody = form.getBuffer().toString('base64')

const response = await got
.post(`${server.url}/api/echo?ding=dong`, {
Expand All @@ -603,7 +603,7 @@ testMatrix.forEach(({ args }) => {
t.is(response.headers['content-type'], `multipart/form-data; boundary=${expectedBoundary}`)
t.is(response.headers['content-length'], '164')
t.is(response.httpMethod, 'POST')
t.is(response.isBase64Encoded, false)
t.is(response.isBase64Encoded, true)
t.is(response.path, '/api/echo')
t.deepEqual(response.queryStringParameters, { ding: 'dong' })
t.is(response.body, expectedResponseBody)
Expand Down
4 changes: 2 additions & 2 deletions tests/eleventy.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ test('functions rewrite echo without body', async (t) => {
'x-forwarded-for': '::ffff:127.0.0.1',
})
t.is(response.httpMethod, 'GET')
t.is(response.isBase64Encoded, false)
t.is(response.isBase64Encoded, true)
t.is(response.path, '/api/echo')
t.deepEqual(response.queryStringParameters, { ding: 'dong' })
})
Expand Down Expand Up @@ -114,7 +114,7 @@ test('functions echo with multiple query params', async (t) => {
'x-forwarded-for': '::ffff:127.0.0.1',
})
t.is(response.httpMethod, 'GET')
t.is(response.isBase64Encoded, false)
t.is(response.isBase64Encoded, true)
t.is(response.path, '/.netlify/functions/echo')
t.deepEqual(response.queryStringParameters, { category: 'a, b' })
t.deepEqual(response.multiValueQueryStringParameters, { category: ['a', 'b'] })
Expand Down
36 changes: 36 additions & 0 deletions tests/serving-functions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -743,6 +743,42 @@ exports.handler = () => ({
})
})
})

test(testName('Resembles base64 encoding of production', args), async (t) => {
await withSiteBuilder('function-base64-encoding', async (builder) => {
const bundlerConfig = args.includes('esbuild') ? { node_bundler: 'esbuild' } : {}

await builder
.withNetlifyToml({
config: {
build: { publish: 'public' },
functions: { directory: 'functions' },
...bundlerConfig,
},
})
.withFunction({
path: 'echoEncoding.js',
handler: async (event) => ({
statusCode: 200,
body: event.isBase64Encoded ? 'base64' : 'plain',
}),
})
.buildAsync()

await withDevServer({ cwd: builder.directory, args }, async ({ outputBuffer, port }) => {
await tryAndLogOutput(async () => {
t.is(
await got(`http://localhost:${port}/.netlify/functions/echoEncoding`, {
headers: {
'Content-Type': 'multipart/form-data',
},
}).text(),
'base64',
)
}, outputBuffer)
})
})
})
})

test('Serves functions that dynamically load files included in the `functions.included_files` config property', async (t) => {
Expand Down