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

Bug with form arrays and bodyParser #1025

Closed
silkcom opened this issue Mar 16, 2014 · 7 comments
Closed

Bug with form arrays and bodyParser #1025

silkcom opened this issue Mar 16, 2014 · 7 comments

Comments

@silkcom
Copy link

silkcom commented Mar 16, 2014

I just found a wierd bug when using bodyParser and form arrays.

I have a form element that looks like:

<input type="checkbox" name="items[1]">
<input type="checkbox" name="items[2]">

etc.

When these submit, I get an array from bodyParser that looks like:

{ items: ['on', 'on'] }

rather than:

{ items: { 1: 'on', 2: 'on' } }

If instead I change the form to look like:

<input type="checkbox" name="items[id_1]">
<input type="checkbox" name="items[id_2]">

it works great (except now I need to replace id_)

Seems like it should either work like php, and treat it like an object, or else it should be an array which would do:

{ items: [null, 'on', 'on'] }

One way or the other it's definitely wrong right now

@dougwilson
Copy link
Contributor

Hi! The connect bodyParser module is there for backwards-compatibility. It has already been removed in connect 3.x, which is coming out soon and as such, no fixes are accepted. The behavior you are seeing is actually intentional, effectively the indices are compressed such that a malicious user cannot send items[0] and then items[99999999999999999999] and consume an enormous amount of server memory.

If you are making a urlencoded POST, test out the replacement middleware https://github.com/expressjs/body-parser and if that doesn't work to your liking, please file a bug there.

@silkcom
Copy link
Author

silkcom commented Mar 16, 2014

seems like treating it like an object is the best answer then no?

@dougwilson
Copy link
Contributor

Perhaps. I actually checked it out and the parsing (in body-parser I sent you to as well as the 2.x branch of connect) is actually being done by https://github.com/visionmedia/node-querystring so I think it actually needs to be fixed in the qs module there first, and we could pull in the updated version of qs here for you.

@dougwilson
Copy link
Contributor

Here is the issue you'll want to argue in, it looks like: tj/node-querystring#77

@dougwilson
Copy link
Contributor

Nope, looks like someone already submitted a PR to qs that makes them objects instead of arrays: tj/node-querystring#62

@silkcom
Copy link
Author

silkcom commented Mar 20, 2014

I submitted a 2nd PR tj/node-querystring#98 that is a more simple change, and should make it basically work the same as php does

@dougwilson
Copy link
Contributor

Thanks for the update! I'll keep a watch on that PR. If it gets accepted and a new release of qs comes out and you don't see anything happening here, please give me a nudge :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants