Skip to content

Commit fe01f3e

Browse files
onebytegoneonebytegone
committed
feat: Sanitize JSONP callback parameter values (#35)
1 parent f681233 commit fe01f3e

2 files changed

Lines changed: 126 additions & 9 deletions

File tree

‎src/Response.ts‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -463,10 +463,15 @@ export default class Response {
463463
* @param o the object to send in the response
464464
*/
465465
public jsonp(o: unknown): Response {
466-
const queryParamName = this.app.getSetting('jsonp callback name') || 'callback',
467-
callbackFunctionName = this._request.query[queryParamName as string];
466+
const queryParamName = this.app.getSetting('jsonp callback name') || 'callback';
468467

469-
if (_.isString(callbackFunctionName)) {
468+
let callbackFunctionName = this._request.query[queryParamName as string];
469+
470+
if (_.isArray(callbackFunctionName)) {
471+
callbackFunctionName = callbackFunctionName[0];
472+
}
473+
474+
if (_.isString(callbackFunctionName) && this._isValidJSONPCallback(callbackFunctionName)) {
470475
const stringified = JSON.stringify(o)
471476
.replace(/\u2028/g, '\\u2028')
472477
.replace(/\u2029/g, '\\u2029');
@@ -599,4 +604,10 @@ export default class Response {
599604
// format: https://expressjs.com/en/api.html#res.format
600605
// sendFile: https://expressjs.com/en/api.html#res.sendFile
601606

607+
protected _isValidJSONPCallback(name?: string): boolean {
608+
// The "disable" is due to eslint erring because of the `\[`
609+
// eslint-disable-next-line no-useless-escape
610+
return !name || _.isEmpty(name) ? false : /^[\[\]\w$.]+$/.test(name);
611+
}
612+
602613
}

‎tests/Response.test.ts‎

Lines changed: 112 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,15 @@ import { RequestEvent, CookieOpts } from '../src/request-response-types';
1010
// function type used for reusable test extension below
1111
type Extender = (resp: Response, output: any) => void;
1212

13+
// dummy class to make protected functions unit testable
14+
class TestResponse extends Response {
15+
16+
public _isValidJSONPCallback(name?: string): boolean {
17+
return super._isValidJSONPCallback(name);
18+
}
19+
20+
}
21+
1322
describe('Response', () => {
1423
const EMPTY_CB = (): void => {}; // eslint-disable-line no-empty-function
1524

@@ -684,21 +693,27 @@ describe('Response', () => {
684693
evt: RequestEvent,
685694
msg: string | false,
686695
extender?: Extender,
687-
overrides: { queryParamName?: string | false; responseObject?: any } = {},
696+
overrides: { queryParamName?: string | false; queryParamValues?: string[]; responseObject?: any } = {},
688697
): void => {
689-
const o = overrides.responseObject || { foo: 'bar' },
690-
expectedBody = `/**/ typeof fooFunction === 'function' && fooFunction(${JSON.stringify(o)});`;
698+
const o = overrides.responseObject || { foo: 'bar' };
699+
700+
let queryParamValues = overrides.queryParamValues;
701+
702+
if (!queryParamValues || queryParamValues.length === 0) {
703+
queryParamValues = [ 'fooFunction' ];
704+
}
705+
706+
const expectedBody = `/**/ typeof ${queryParamValues[0]} === 'function' && ${queryParamValues[0]}(${JSON.stringify(o)});`;
691707

692708
// A false `queryParamName` means no query parameter was present
693709
if (overrides.queryParamName !== false) {
694710
let queryParamName = overrides.queryParamName || 'callback';
695711

696712
if (evt.multiValueQueryStringParameters) {
697-
// TODO: test with multiple _different_ values
698-
evt.multiValueQueryStringParameters[queryParamName] = [ 'fooFunction' ];
713+
evt.multiValueQueryStringParameters[queryParamName] = queryParamValues;
699714
}
700715
if (evt.queryStringParameters) {
701-
evt.queryStringParameters[queryParamName] = 'fooFunction';
716+
evt.queryStringParameters[queryParamName] = queryParamValues[0];
702717
}
703718
}
704719

@@ -763,6 +778,64 @@ describe('Response', () => {
763778
test(albMultiValHeadersRequest(), 'OK', expectJSON, { queryParamName: false });
764779
});
765780

781+
it('works like JSON when non-callback param is in query - APIGW', () => {
782+
test(apiGatewayRequest(), false, expectJSON, { queryParamName: 'notcallback' });
783+
});
784+
it('works like JSON when non-callback param is in query - ALB', () => {
785+
test(albRequest(), 'OK', expectJSON, { queryParamName: 'notcallback' });
786+
});
787+
it('works like JSON when non-callback param is in query - ALB MV', () => {
788+
test(albMultiValHeadersRequest(), 'OK', expectJSON, { queryParamName: 'notcallback' });
789+
});
790+
791+
it('uses the first callback param value listed - APIGW', () => {
792+
test(apiGatewayRequest(), false, undefined, {
793+
queryParamValues: [ 'callbackOne', 'callbackTwo' ],
794+
});
795+
});
796+
it('uses the first callback param value listed - ALB', () => {
797+
test(albRequest(), 'OK', undefined, {
798+
queryParamValues: [ 'callbackOne', 'callbackTwo' ],
799+
});
800+
});
801+
it('uses the first callback param value listed - ALB MV', () => {
802+
test(albMultiValHeadersRequest(), 'OK', undefined, {
803+
queryParamValues: [ 'callbackOne', 'callbackTwo' ],
804+
});
805+
});
806+
807+
it('allows for the callback param value to contain an array index - APIGW', () => {
808+
test(apiGatewayRequest(), false, undefined, {
809+
queryParamValues: [ 'callbacks[123]' ],
810+
});
811+
});
812+
it('allows for the callback param value to contain an array index - ALB', () => {
813+
test(albRequest(), 'OK', undefined, {
814+
queryParamValues: [ 'callbacks[123]' ],
815+
});
816+
});
817+
it('allows for the callback param value to contain an array index - ALB MV', () => {
818+
test(albMultiValHeadersRequest(), 'OK', undefined, {
819+
queryParamValues: [ 'callbacks[123]' ],
820+
});
821+
});
822+
823+
it('returns JSON when callback param value contains invalid characters - APIGW', () => {
824+
test(apiGatewayRequest(), false, expectJSON, {
825+
queryParamValues: [ 'bad;fn()' ],
826+
});
827+
});
828+
it('returns JSON when callback param value contains invalid characters - ALB', () => {
829+
test(albRequest(), 'OK', expectJSON, {
830+
queryParamValues: [ 'bad;fn()' ],
831+
});
832+
});
833+
it('returns JSON when callback param value contains invalid characters - ALB MV', () => {
834+
test(albMultiValHeadersRequest(), 'OK', expectJSON, {
835+
queryParamValues: [ 'bad;fn()' ],
836+
});
837+
});
838+
766839
const utfInput = { str: 'newline \u2028 paragraph \u2029 end' };
767840

768841
const ext3: Extender = (_r, o): void => {
@@ -970,4 +1043,37 @@ describe('Response', () => {
9701043

9711044
});
9721045

1046+
describe('_isValidJSONPCallback', () => {
1047+
let response: TestResponse;
1048+
1049+
beforeEach(() => {
1050+
response = new TestResponse(app, sampleReq, _.noop);
1051+
});
1052+
1053+
it('returns true for valid JSONP callbacks', () => {
1054+
expect(response._isValidJSONPCallback('callback')).to.strictlyEqual(true);
1055+
expect(response._isValidJSONPCallback('callback_12345')).to.strictlyEqual(true);
1056+
expect(response._isValidJSONPCallback('_abc')).to.strictlyEqual(true);
1057+
expect(response._isValidJSONPCallback('$')).to.strictlyEqual(true);
1058+
expect(response._isValidJSONPCallback('funcs[123]')).to.strictlyEqual(true);
1059+
expect(response._isValidJSONPCallback('window.callback')).to.strictlyEqual(true);
1060+
expect(response._isValidJSONPCallback('document.do_something[42]')).to.strictlyEqual(true);
1061+
expect(response._isValidJSONPCallback('run.$')).to.strictlyEqual(true);
1062+
// Technically "valid" with the current implementation, but not in JS. Please
1063+
// don't actually use these in actual code.
1064+
expect(response._isValidJSONPCallback('[brackets]')).to.strictlyEqual(true);
1065+
expect(response._isValidJSONPCallback('snake_ca$e')).to.strictlyEqual(true);
1066+
expect(response._isValidJSONPCallback('$_[..]_$')).to.strictlyEqual(true);
1067+
expect(response._isValidJSONPCallback('currencies[$]')).to.strictlyEqual(true);
1068+
expect(response._isValidJSONPCallback('Well...the_data_is_back')).to.strictlyEqual(true);
1069+
});
1070+
1071+
it('returns false for invalid JSONP callbacks', () => {
1072+
expect(response._isValidJSONPCallback()).to.strictlyEqual(false);
1073+
expect(response._isValidJSONPCallback(undefined)).to.strictlyEqual(false);
1074+
expect(response._isValidJSONPCallback('')).to.strictlyEqual(false);
1075+
expect(response._isValidJSONPCallback('bad;func()')).to.strictlyEqual(false);
1076+
});
1077+
});
1078+
9731079
});

0 commit comments

Comments
 (0)