958499 - FormData.jsm should escape invalid object property names

Status: NEW

(Firefox :: Session Restore, defect)

Description

[Tim Taubert [:ttaubert] (inactive)]

SessionStore stores form data for input fields with "id" attributes like so:

formdata = {
  id: {input_field_id: "value"}
}

Should a field now have the id "prototype" or "__proto__" we will probably see an error message or things breaking. We should come up with some format that is a little more resistant to those field names.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

The typical workaround is to prefix all identifiers with ":".

Comment 2

[Michael Hoffmann]

Attachment: bug-958499-fix.patch - first draft

Here's a patch that escapes all identifiers by prefixing a colon. What do you think?

The test browser_formdata_format.js should also be updated and perhaps some documentation, but I will await your feedback first.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8562852 [details] [diff] [review]
bug-958499-fix.patch - first draft

Review of attachment 8562852 [details] [diff] [review]:
-----------------------------------------------------------------

Michael, thanks for your patch. I'm afraid we can't do it like that... We would throw away all currently stored form data for sessions out there. We introduce a new format here and lose the ability to handle the old one. Keeping support for the old format might introduce ambiguity... or maybe not given that id attributes probably can't contain colons. Didn't try though if that's true or not.

Comment 4

[Michael Hoffmann]

I did keep support for reading the current format. When reading stored form data I do check if the identifier starts with a colon or not, so that it can load both types. It does not throw away the old sessions. But I agree, it's better to not introduce a new format if possible.

So, what would be a better approach? Perhaps keeping the format on disk as it is, but when reading it we prefix identifiers with colon so that it's never possible to have an object with an invalid property name. I don't know how easy or hard that is, but I can take a look later.

If we do escape identifiers, should we do it for all of them or only for a select few, like the ones you mention, "prototype", "__proto__"?

Comment 5

[David Teller [:Yoric] - still alive but not very active]

Out of curiosity, is it possible to end up with `prototype` in data that should be stored by FormData?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8562852 [details] [diff] [review]
bug-958499-fix.patch - first draft

Review of attachment 8562852 [details] [diff] [review]:
-----------------------------------------------------------------

I believe that the strategy is ok. Most users (hopefully all of them) should have their data upgraded transparently. There is just a chance that some websites may already be using form data with labels starting with ":". If so, we would lose the content of these forms during the upgrade. Could anyone check whether this is actually possible?

I believe that the only good strategy to handle such cases. would be to introduce versioning in our Session Restore files, which may come in useful in the future anyway.

Also, I would like a test that we can parse with the old format and produce the new format.

Comment 7

[Michael Hoffmann]

I manually checked with inputs with different IDs, typing in values and see if they were restored on restart of Firefox.

An ID of `prototype` works fine, both before my patch and after. An ID of `__proto__` just disappears, it will not be saved to the Session Restore file, and if I manually add it before starting Firefox, it will not be restored (See below for a reason). This is fixed with the patch. I could confirm the problem with IDs already starting with ":", a Session Restore file with the old format will not be restored with this patch. Firefox didn't seem to have any issues with colons in names, so this is a case were data would be lost.

So if we are to escape the names I believe we need versioning. How would you prefer that to work? Should we have a value inside the Session Restore file identifying the Firefox version, or a separate versioning number?

But I wonder whether escaping really should be necessary. Please correct me if I'm wrong here, but for instance, `prototype` shouldn't be any problem as it has a special meaning only(?) when dealing with constructor functions. I also think `__proto__` should work as we can hide the [[Prototype]]-accessor by creating a new `__proto__` property on an object with `Object.defineProperty`. Parsing a json string containing a `__proto__` key also gives us such a property which is hiding the [[Prototype]]-accessor. So as long as we use `Object.defineProperty` for that particular case, we should be ok. Are there any other special properties we should watch out for?

Or so I thought. For some reason I really cannot figure out, SessionFile.read() will mystically lose any properties named `__proto__` in its parsed response. Inside `read` it is there, but when the generator function returns, the value is gone. I have tried reproducing it in the Firefox console by using Task.async and generator functions, but in those cases it never fails, `__proto__` is always there.