Stack overflow on multipart upload with domains

[jfaissolle]

Hello,

I am encountering a stack overflow exception on file uploads with connect-multiparty 1.2.1 when using domains. Version 1.1.0 did not have it.

I have traced the problem to the last versions of the qs library. This version performs a clone operation. The file object produced by node-multiparty contains a stream which in turns contains the domain which in turn contains the request and reply objects. Cloning the file object gives an infinite recursion.

I have submitted a pull request on node-multiparty (pillarjs/multiparty#85) to remove the stream from the file object (which seems not necessary).

Looking at the code of connect-multiparty, I do not understand why the parse function is called on the files object. Is there a case where this array contains query strings instead of file descriptions ?
If there is no such case, getting rid of the parsing operation would also solve the problem.

Regards

[dougwilson]

I do not understand why the parse function is called on the files object.

It's the API from qs to parse the flat urlencoded list that we get into a rich object. You can see exactly how it works if you remove it and then run the tests with it gone. qs does not provide us a API to do this well. The best solution would be if qs provided an API where it would only look at the first-level keys and transform those instead of recusing through it. A PR to fix this would be welcome (though not just by deleting the ws property).

[jfaissolle]

Ok, I understand. It is for file nesting. I do not know how to fix this...

The qs lib seems to be OK. There is a guard for too much nesting, but that is independant of the object cloning thing, which seems right because qs lib is only interested in parsing the first level keys in the object (the parameter names) and not the subobjects.

What I see is that we pass the qs lib an object that it is not designed to process (and should not). As a consumer of the multiparty lib, I am interested in receiving deserialized objects passed to me by the client or file descriptions with paths on the file system. The ws stream which has served to write the file on the filesystem is of no us.

Providing the ws property as public API seems to be a design error but you are right to be over cautious about deleting it, since perhaps some people are hacking it in a strange way...

[dougwilson]

But even then, just deleting the ws property only cures the symptom of this qs issue, not the cause. I favor fixing the actual root issue here, not just deleting some property that exposes a flaw in some design. Basically, the solution needs to be a real way to expand the flat list into the expanded qs list and it should work if there is a ws, even if we might remove that later.

[jfaissolle]

I fear the root cause is using qs for something it is not intended. So the solution involves writing the key parse/expand for file parts directly in connect-multiparty, am I right ?

[dougwilson]

I fear the root cause is using qs for something it is not intended

No, the old qs was designed just for this. The problem is that the new qs was completely rewritten and this interface was not correctly replicated.

[jfaissolle]

Mmm, it seems that the problem is that clone thing... The previous code was doing more or less the same except that the input wasn't cloned. I wonder why this was introduced...

[dougwilson]

I wonder why this was introduced...

IDK :) That's one of the reasons I suggested filing a ticket with qs in the first place :)

[andrewrk]

Another possible solution: don't use the qs module. E.g. don't use connect-multiparty and use multiparty directly. I've never agreed with qs's philosophy. It puts too much power in the hands of the user (possible attacker).

[dougwilson]

E.g. don't use connect-multiparty and use multiparty directly.

👍 :)

[jfaissolle]

OK, I'll just use node-multiparty directly as advised on the connect-multiparty readme...

[dougwilson]

OK, this should be fixed with version 1.2.2.

[jfaissolle]

FYI, issue is still there. qs 2.2.0 does not fix the problem. There is still a cloning operation on parse input.

[dougwilson]

Thanks, @jfaissolle . I put in an issue here: ljharb/qs#31

[dougwilson]

@jfaissolle so I was assuming the error was because of a simple circular reference, but qs 0.6.6 (the old qs that you said didn't have the issue) I'm told also crashes with circular references. Would you be able to modify https://github.com/andrewrk/connect-multiparty/blob/master/test/multipart.js to add domains in there that cause the error in the current version but not an older version of this module? You can just modify the existing tests; I have never used domains, so I have no idea how to use them in a way to reproduce what you are reporting.

[dougwilson]

@jfaissolle I was able to confirm that [email protected] did work with circular structures like you're reporting; I think the report otherwise was a mistake. I think the new qs is going to fix this. Your usage depends on if qs is fixed or not, really :)

[nlf]

I published [email protected] just now, which should handle circular structures appropriately. Give it a shot and let me know :)

[dougwilson]

Thanks so much, @nlf ! :)

@jfaissolle I also published version 1.2.3 of this module to npm that requires a minimum version of qs at 2.2.1. Let us know if the issue is still not fixed.

[andrewrk]

Thanks for taking care of this @dougwilson and @nlf 👍

[jfaissolle]

Original issue (stack overflow) fixed. New issue with the i18n modules which create a locals object without a hasOwnProperty method in the ServerResponse. The qs parse method throws err: TypeError: Object object has no method 'hasOwnProperty.

[dougwilson]

ok. i submitted an issue here: ljharb/qs#33

i'm not going to re-open this issue because it seems we are going to keep finding new things, but i don't know what you are doing to be able to verify myself that your project is all fixed :)

[jfaissolle]

Ok thanks.

[dougwilson]

@jfaissolle ok, version 1.2.4 should hopefully fix all this stuff for you.

[jfaissolle]

Fix confirmed, thanks !

[dougwilson]

yay!