Stack overflow on multipart upload with domains about connect-multiparty HOT 24 CLOSED

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).

from connect-multiparty.

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...

from connect-multiparty.

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.

from connect-multiparty.

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 ?

from connect-multiparty.

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.

from connect-multiparty.

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...

from connect-multiparty.

I wonder why this was introduced...

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

from connect-multiparty.

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).

from connect-multiparty.

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

👍 :)

from connect-multiparty.

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

from connect-multiparty.

OK, this should be fixed with version 1.2.2.

from connect-multiparty.

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

from connect-multiparty.

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

from connect-multiparty.

@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.

from connect-multiparty.

@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 :)

from connect-multiparty.

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

from connect-multiparty.

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.

from connect-multiparty.

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

from connect-multiparty.

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.

from connect-multiparty.

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 :)

from connect-multiparty.

Ok thanks.

from connect-multiparty.

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

from connect-multiparty.

Fix confirmed, thanks !

from connect-multiparty.

yay!

from connect-multiparty.