Skip to content

Commit

Permalink
[node-bridge] Ensure content-type is always set with multi-payloads (#…
Browse files Browse the repository at this point in the history
…7681)

### Related Issues

This ensures a content-type is always set inside of each part of the multi-part payload as it's needed for proper parsing. This also ensures non-200 status codes/headers are passed back separate when they differ so that they can be handled independently without assuming they all match. 

x-ref: https://vercel.slack.com/archives/C03AYHB6MA9
x-ref: #7507

### 📋 Checklist

<!--
  Please keep your PR as a Draft until the checklist is complete
-->

#### Tests

- [ ] The code changed/added as part of this PR has been covered with tests
- [ ] All tests pass locally with `yarn test-unit`

#### Code Review

- [ ] This PR has a concise title and thorough description useful to a reviewer
- [ ] Issue from task tracker has a link to this PR
  • Loading branch information
ijjk committed Apr 14, 2022
1 parent 9225246 commit 46bf95e
Show file tree
Hide file tree
Showing 2 changed files with 76 additions and 15 deletions.
54 changes: 44 additions & 10 deletions packages/node-bridge/bridge.js
Expand Up @@ -186,9 +186,6 @@ class Bridge {
'payloads' in normalizedEvent &&
Array.isArray(normalizedEvent.payloads)
) {
// statusCode and headers are required to match when using
// multiple payloads in a single invocation so we can use
// the first
let statusCode = 200;
/**
* @type {import('http').IncomingHttpHeaders}
Expand All @@ -200,6 +197,14 @@ class Bridge {
let combinedBody = '';
const multipartBoundary = 'payload-separator';
const CLRF = '\r\n';
/**
* @type {Record<string, any>[]}
*/
const separateHeaders = [];
/**
* @type {Set<string>}
*/
const allHeaderKeys = new Set();

// we execute the payloads one at a time to ensure
// lambda semantics
Expand All @@ -209,20 +214,49 @@ class Bridge {
// build a combined body using multipart
// https://www.w3.org/Protocols/rfc1341/7_2_Multipart.html
combinedBody += `--${multipartBoundary}${CLRF}`;
if (response.headers['content-type']) {
combinedBody += `content-type: ${response.headers['content-type']}${CLRF}${CLRF}`;
}
combinedBody += response.body;
combinedBody += `content-type: ${
response.headers['content-type'] || 'text/plain'
}${CLRF}${CLRF}`;
combinedBody += response.body || '';
combinedBody += CLRF;

if (i === normalizedEvent.payloads.length - 1) {
combinedBody += `--${multipartBoundary}--${CLRF}`;
}

statusCode = response.statusCode;
headers = response.headers;
// pass non-200 status code in header so it can be handled
// separately from other payloads e.g. HTML payload redirects
// (307) but data payload does not (200)
if (response.statusCode !== 200) {
headers[`x-vercel-payload-${i + 1}-status`] =
response.statusCode + '';
}
separateHeaders.push(response.headers);
Object.keys(response.headers).forEach(key => allHeaderKeys.add(key));
}

allHeaderKeys.forEach(curKey => {
/**
* @type string | string[] | undefined
*/
const curValue = separateHeaders[0] && separateHeaders[0][curKey];
const canDedupe = separateHeaders.every(
headers => headers[curKey] === curValue
);

if (canDedupe) {
headers[curKey] = curValue;
} else {
// if a header is unique per payload ensure it is prefixed
// so it can be parsed and provided separately
separateHeaders.forEach((curHeaders, idx) => {
if (curHeaders[curKey]) {
headers[`x-vercel-payload-${idx + 1}-${curKey}`] =
curHeaders[curKey];
}
});
}
});

headers[
'content-type'
] = `multipart/mixed; boundary="${multipartBoundary}"`;
Expand Down
37 changes: 32 additions & 5 deletions packages/node-bridge/test/bridge.test.js
Expand Up @@ -86,6 +86,12 @@ test('`NowProxyEvent` normalizing', async () => {

test('multi-payload handling', async () => {
const server = new Server((req, res) => {
if (req.url === '/redirect') {
res.setHeader('Location', '/somewhere');
res.statusCode = 307;
res.end('/somewhere');
return;
}
res.setHeader(
'content-type',
req.url.includes('_next/data') ? 'application/json' : 'text/html'
Expand Down Expand Up @@ -117,6 +123,11 @@ test('multi-payload handling', async () => {
headers: { foo: 'baz' },
path: '/_next/data/build-id/nowproxy.json',
},
{
method: 'GET',
headers: { foo: 'baz' },
path: '/redirect',
},
],
}),
},
Expand All @@ -137,20 +148,36 @@ test('multi-payload handling', async () => {
!item.startsWith('content-type:') &&
!item.startsWith('--payload')
) {
bodies.push(
JSON.parse(
Buffer.from(item.split('--payload-separator')[0], 'base64').toString()
)
);
const content = Buffer.from(
item.split('--payload-separator')[0],
'base64'
).toString();
bodies.push(content.startsWith('{') ? JSON.parse(content) : content);
}
});

// ensure content-type is always specified as is required for
// proper parsing of the multipart body
assert(payloadParts.some(part => part.includes('content-type: text/plain')));

assert.equal(bodies[0].method, 'GET');
assert.equal(bodies[0].path, '/nowproxy');
assert.equal(bodies[0].headers.foo, 'baz');
assert.equal(bodies[1].method, 'GET');
assert.equal(bodies[1].path, '/_next/data/build-id/nowproxy.json');
assert.equal(bodies[1].headers.foo, 'baz');
assert.equal(bodies[2], '/somewhere');
assert.equal(result.headers['x-vercel-payload-3-status'], '307');
assert.equal(result.headers['x-vercel-payload-2-status'], undefined);
assert.equal(result.headers['x-vercel-payload-1-status'], undefined);
assert.equal(result.headers['x-vercel-payload-1-content-type'], 'text/html');
assert.equal(
result.headers['x-vercel-payload-2-content-type'],
'application/json'
);
assert.equal(result.headers['x-vercel-payload-3-content-type'], undefined);
assert.equal(result.headers['x-vercel-payload-3-location'], '/somewhere');
assert.equal(result.headers['x-vercel-payload-2-location'], undefined);
assert.equal(context.callbackWaitsForEmptyEventLoop, false);

server.close();
Expand Down

0 comments on commit 46bf95e

Please sign in to comment.