Prevent file uploads in `pm.sendRequest` #351

kamalaknn · 2017-08-14T14:23:17Z

No description provided.

kamalaknn · 2017-08-14T14:25:46Z

lib/runner/extensions/event.command.js

+        if (request.body.mode === REQUEST_BODY_MODE_FILE) {
+            // clone the request and remove file in body
+            sanitizedRequest = _.clone(request);
+            delete sanitizedRequest.body.file;


Unsure if delete sanitizedRequest.body.file is better or sanitizedRequest.body.file = {} is better?

Please set the value to null

Got it 👍

shamasis · 2017-08-14T14:29:40Z

lib/runner/extensions/event.command.js

+        if (request.body.mode === REQUEST_BODY_MODE_FILE) {
+            // clone the request and remove file in body
+            sanitizedRequest = _.clone(request);
+            delete sanitizedRequest.body.file;


Please set the value to null

kunagpal · 2017-08-14T14:56:48Z

lib/runner/extensions/event.command.js

+     * @returns {Request~definition}
+     */
+    sanitizeFiles = function (request, warn) {
+        var sanitizedRequest,


The var block can be shifted to below the next if, makes the code easier to read.

Will do 👍

kunagpal · 2017-08-14T15:04:37Z

lib/runner/extensions/event.command.js

+            if (sanitizedFormdata.length !== _.get(request, 'body.formdata.length')) {
+                // clone the request and set form data without file params
+                sanitizedRequest = _.clone(request);
+                sanitizedRequest.formdata = sanitizedFormdata;


sanitisedRequest[REQUEST_BODY_MODE_FORMDATA] = sanitizedFormdata;

Will change 👍

Is there any particular benefit of this or code quality in general?

Should there ever be a change in the name given to formdata bodies, we'll have to change it in just one place.

Ah okay, makes sense.

kunagpal · 2017-08-14T15:09:55Z

lib/runner/extensions/event.command.js

+
+        // handle request body in `formdata` mode
+        if (request.body.mode === REQUEST_BODY_MODE_FORMDATA) {
+            // extract any file type params


I feel this segment could be made cleaner by:

Cloning the request.

Traversing the members of the cloned formdata request body.

Setting any encountered src values to null.

This ensures that at most one copy+traversal of the request/request-body is made. Currently, the request body has to be traversed once during _.reject, and then a second time via clone.

@kunagpal the reason I do _.reject first is, not all requests have file types in form data.

We should do a clone only when we detect a file param. Otherwise, we would be doing the clone even for requests without files which is costlier than _.reject of formdata which would be much smaller.

This is another form of early bailout and deferring costly operations.

Fair enough.

kunagpal · 2017-08-16T09:20:40Z

@kamalaknn Any specific reason for returning the sanitized request body via a callback?

shamasis · 2017-08-16T09:19:21Z

lib/runner/extensions/event.command.js

+
+    /**
+     * Removes files in Request body if any.
+     *


@private this please

shamasis · 2017-08-16T09:23:33Z

lib/runner/extensions/event.command.js

+     * @param {Function} callback function invoked with error and sanitizedRequest.
+     *                            sanitizedRequest is `null` if no files are detected.
+     */
+    sanitizeFiles = function (request, callback) {


I think callback should always get the request from parameter so that it doesn't have to refer to the outer closure. ... as such the signature of callback could be error:Error, request:Request, affectedEntries:Array.<src:String> - in this way, we would know that nothing was affected if _.isEmpty(affectedEntries) = true.

@shamasis I have a question.

If callback always gets the request, raw or mutated, how is that different from the synchronous version with the return?

Also, the signature of affectedEntries is not always an Array, it could the entire body for file mode. If I had to send the raw request and affected entries, and let the callback do the reconstruction of new request then sanitizeFiles would not do much and it leaks concerns, right?

var outerFunction = function (request, callback) { innerFunction(request, function (request) { if (!request) { return callback(new Error()); } callback(null, request); }); }; var innerFunction = function (callback) { }; // we can then write outerFunction(myReqFromSandbox, function (err, ...) {});

shamasis · 2017-08-16T09:25:50Z

lib/runner/extensions/event.command.js

+                            return;
+                        }
+
+                        var tempRequest = request,


we would not need tempRequest variable if sanitiseFIles always return a request or null if request itself is missing / cannot be constructed.

…ntries

shamasis · 2017-08-16T15:07:18Z

lib/runner/extensions/event.command.js

-                    // clone cursor and add source
-                    var cursorWithSource = _.clone(cursor),
+                    // remove files in request body if any
+                    sanitizeFiles(request, function (err, sanitizedRequest, affectedEntries) {


call this parameter request itself so that it overrides the outer closure variable in request and prevent bigs.

I renamed it so linter does not complain about duplicate variable names.

Will change.

* release/6.2.6: (49 commits) Released v6.2.6 fix(package): update postman-sandbox to version 2.3.1 fix(package): update postman-collection to version 2.1.1 Move new cursor properties to _properties affetcedEntries -> sanitizedFiles Updated sanitiseFiles to mutate request and reduced code complexity chore(package): update sinon to version 3.2.1 sanitizedRequest -> request so that it overrides closure Use `cloneDeep` for cloning requests. Change signature of callback to accept sanitizedRequest and affectedEntries Make sanitizeFiles async Addressed PR #351 comments Set body.file to null instead of `delete` Added tests for requests with file uploads Sanitize files in request body if present chore(package): update editorconfig to version 0.14.1 chore(package): update tmp to version 0.0.33 chore(package): update sinon to version 3.2.0 Update package.json towards v6.2.6-beta.2 release  Update postman-sandbox to 2.3.1-beta.3 ...

kamalaknn requested review from shamasis and kunagpal August 14, 2017 14:23

kamalaknn added 2 commits August 14, 2017 19:54

Sanitize files in request body if present

1d1eac3

Added tests for requests with file uploads

2fbfa88

kamalaknn force-pushed the feature/sanitize-files branch from 3a91c14 to 2fbfa88 Compare August 14, 2017 14:24

kamalaknn commented Aug 14, 2017

View reviewed changes

shamasis requested changes Aug 14, 2017

View reviewed changes

Set body.file to null instead of delete

0a58838

kunagpal requested changes Aug 14, 2017

View reviewed changes

Addressed PR #351 comments

7b8807b

kunagpal approved these changes Aug 14, 2017

View reviewed changes

Make sanitizeFiles async

7d6a5f6

kamalaknn force-pushed the feature/sanitize-files branch from 6c823b3 to 7d6a5f6 Compare August 16, 2017 09:04

shamasis requested changes Aug 16, 2017

View reviewed changes

kamalaknn added 2 commits August 16, 2017 18:12

Change signature of callback to accept sanitizedRequest and affectedE…

363e763

…ntries

Use cloneDeep for cloning requests.

6b69660

shamasis reviewed Aug 16, 2017

View reviewed changes

kamalaknn and others added 3 commits August 16, 2017 20:41

sanitizedRequest -> request so that it overrides closure

24569a8

Updated sanitiseFiles to mutate request and reduced code complexity

19bffee

affetcedEntries -> sanitizedFiles

332ee0f

kamalaknn merged commit 0a2ce04 into feature/send-request-pre-post-helper-flows Aug 17, 2017

kunagpal deleted the feature/sanitize-files branch August 18, 2017 16:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prevent file uploads in `pm.sendRequest` #351

Prevent file uploads in `pm.sendRequest` #351

kamalaknn commented Aug 14, 2017

kamalaknn Aug 14, 2017

shamasis Aug 14, 2017

kamalaknn Aug 14, 2017

shamasis Aug 14, 2017

kunagpal Aug 14, 2017

kamalaknn Aug 14, 2017

kunagpal Aug 14, 2017

kamalaknn Aug 14, 2017

kunagpal Aug 14, 2017

kamalaknn Aug 14, 2017

kunagpal Aug 14, 2017

kamalaknn Aug 14, 2017 •

edited

Loading

kunagpal Aug 14, 2017

kunagpal commented Aug 16, 2017

shamasis Aug 16, 2017

shamasis Aug 16, 2017

kamalaknn Aug 16, 2017 •

edited

Loading

shamasis Aug 16, 2017

shamasis Aug 16, 2017

shamasis Aug 16, 2017

kamalaknn Aug 16, 2017

Prevent file uploads in pm.sendRequest #351

Prevent file uploads in pm.sendRequest #351

Conversation

kamalaknn commented Aug 14, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalaknn Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kunagpal commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kamalaknn Aug 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Prevent file uploads in `pm.sendRequest` #351

Prevent file uploads in `pm.sendRequest` #351

kamalaknn Aug 14, 2017 •

edited

Loading

kamalaknn Aug 16, 2017 •

edited

Loading