Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(node): ensure that unknown request bodies are handled without errors #321

Merged
merged 3 commits into from
Dec 2, 2021
Merged
Show file tree
Hide file tree
Changes from 2 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
58 changes: 57 additions & 1 deletion packages/node/__tests__/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,11 @@ import config from '../src/config';
import pkg from '../package.json';
import { expressMiddleware } from '../src';
import { ServerResponse } from 'http';
import FormData from 'form-data';
Copy link
Member

Choose a reason for hiding this comment

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

For what it's worth,form-data isn't spec-compliant and it's given me lots of headaches (readmeio/httpsnippet#1) in the past.

  • This fixes the implementation of form-data in multipart/form-data content types in order to accommodate library utilization within the browser where the native FormData component is used.
    • Key differences between form-data and FormData that was causing the library to fail in the browser were two issues:
      • The API for FormData.append has three arguments, and the third should only be present if the second is a Blob or USVString. It is never an object, as form-data requires it to be.
      • FormData.pipe() isn't a function.

node-fetch folks recommend formdata-polyfill these days.

But that said, what you're using it here for should be fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's great to know, and I'll make a note of it, but yeah prob not an issue for these tests.

import multer from 'multer';

const upload = multer();


const apiKey = 'mockReadMeApiKey';
const incomingGroup = {
Expand Down Expand Up @@ -166,7 +171,7 @@ describe('#metrics', () => {
.reply(200);

const app = express();
app.use((req, res, next) => {
app.use((req: any, res: any, next) => {
req.a = 'a';
res.b = 'b';
res.c = 'c';
Expand Down Expand Up @@ -527,4 +532,55 @@ describe('#metrics', () => {
mock.done();
});
});

describe('`req.body`', () => {
let apiMock;

function createMock(checkLocation: 'text' | 'params', requestBody: unknown) {
return nock(config.host, {
reqheaders: {
'Content-Type': 'application/json',
'User-Agent': `${pkg.name}/${pkg.version}`,
},
})
.post('/v1/request', ([body]) => {
expect(body.request.log.entries[0].request.postData[checkLocation]).toBe(requestBody);
return true;
})
.reply(200);
}

beforeEach(() => {
apiMock = getReadMeApiMock(1);
});

afterEach(() => {
apiMock.done();
});

it('should accept multipart/form-data', async () => {

const form = new FormData();
form.append('password', '123456');
form.append('apiKey', 'abc');
form.append('another', 'Hello world');

// If the request body for a multipart/form-data request comes in as an object (as it does with the express middleware) we expect it to be recorded json encoded
const mock = createMock('text', JSON.stringify({password: '123456', apiKey: 'abc', another: 'Hello world'}));
const app = express();
app.use(upload.none());
app.use(expressMiddleware(apiKey, () => incomingGroup));
app.post('/test', (req, res) => {
res.status(200).end();
});

await request(app)
.post('/test')
.set(form.getHeaders())
.send(form.getBuffer().toString())
.expect(200);

mock.done();
});
});
});
21 changes: 21 additions & 0 deletions packages/node/__tests__/lib/process-request.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ import request from 'supertest';
import * as http from 'http';
import processRequest from '../../src/lib/process-request';
import { LogOptions } from 'src/lib/construct-payload';
import FormData from 'form-data';

function createApp(reqOptions?: LogOptions, shouldPreParse: boolean = false) {
const requestListener = function (req: http.IncomingMessage, res: http.ServerResponse) {
Expand Down Expand Up @@ -96,6 +97,26 @@ it('should work with *+json', () => {
});
});

it('should work with multipart/form-data', () => {
const app = createApp({ denylist: ['password']});

const form = new FormData();
form.append('password', '123456');
form.append('apiKey', 'abc');
form.append('another', 'Hello world');

const formHeaders = form.getHeaders();

return request(app)
.post('/')
.set(formHeaders)
.send(form.getBuffer().toString())
.expect(({ body }) => {
// If the request body for multipart form comes in as a string, we record it as is.
expect(body.postData.text).toBe(form.getBuffer().toString());
});
});

describe('options', () => {
describe('denylist/allowlist', () => {
it('should strip denylisted json properties', () => {
Expand Down
Loading