octet-stream / form-data

Spec-compliant FormData implementation for Node.js
https://www.npmjs.com/package/formdata-node
MIT License
142 stars 17 forks source link

Field names should be coerced to string #22

Closed jaydenseric closed 4 years ago

jaydenseric commented 4 years ago

The native FormData implementations coerce field names that are numbers or other types to a string.

const form = new FormData();
form.append('1', '');
form.append(2, '');

const entries = Array.from(form.entries());
console.log(typeof entries[0][0]); // `string`.
console.log(typeof entries[1][0]); // `number`, but should be `string`.

This is a gotcha I have encountered in multiple projects when doing strict equal assertions on field names in tests.

jaydenseric commented 4 years ago

Just realised, the other way is true too; form.get('1') and form.get(1) work the same in native implementations. The name is coerced to string.

jaydenseric commented 4 years ago

We should not coerce to string using String(name), because in native implementations field names are not instances of String; they are string primitives.

const form = new FormData();
form.append(1, '');
const [fieldname] = Array.from(form.keys());
console.log(fieldname instanceof String); // Should be `false`.
console.log(typeof fieldname === 'string'); // Should be `true`.

Coercing with `${name}` should be fine.

You might want to check other places in the code already coercing using String() for correctness.

jaydenseric commented 4 years ago

get, getAll, set, append, and delete all need fixing. PR coming soon…

jaydenseric commented 4 years ago

I was mistaken about https://github.com/octet-stream/form-data/issues/22#issuecomment-661529328 , I was confusing String('foo') with new String('foo'). I think String(name) should be the same as `${name}`.

Another thing I checked is form.set(10e-1, 'a') natively results in the field name being the string '1', not '10e-1'.

octet-stream commented 4 years ago

Saw it in one of your project's changelog few days ago, but as I wasn't able to use my computer last week I didn't fix that. :) But as I see you have opened a PR, so I'll just take it. Thank you!

octet-stream commented 4 years ago

I was mistaken about #22 (comment) , I was confusing String('foo') with new String('foo'). I think String(name) should be the same as ${name}.

Yes, native type constructors works that way. String("foo") produces a string (which makes it useful for type casting), where new String("foo") will return an object :)