Bugzilla

(In reply to Mark Hammond [:markh] [:mhammond] from comment #18)
> I guess my greater point is that JSONFile has used paths like that for well over 10 years

Not to quibble too much about specifics, but the oldest blame I see on toolkit/modules/JSONFile.jsm is from 2014. Are you talking about a different bit of code? Did sync have its own copy of the module?

> and this seems like an unintentional change in semantics that may impact other existing code. I don't feel incredibly strongly about this, but it seems like a new footgun is being introduced. I'd love to hear from Gijs and :asuth here.

I can see that it feels like a footgun if OS.File coped with this, but XPCOM IO interfaces never did, and NSPR and other places are full of `#ifdef`'d path separator defines for precisely that reason.

XPCOM files mitigated this via the `append` API and passing objects rather than paths around (and indeed, IIRC there are source code admonitions in `nsLocalFIle` telling you that passing around just paths is going to make you sad, so perhaps this bug is evidence those admonitions were right - but that was already true for JSONFile/OS.File for a long time, so that ship has sailed...)

We could try to mitigate this "footgun" in IOUtils by preprocessing every instance of an incoming path, but I'm inclined not to try to layer magic of our own on top of OS path munging. From the downloads work, we have extensive experience with the fact that platform-specific requirements coupled with filesystem-specific requirements cause no end of pain (you can mount NTFS disks on Linux, and now you have, as they say, 2 problems). Just yesterday I saw a report basically asking us to allow people to add `/` into filenames on macOS (which supports this) when using the "save as" dialog for downloads. Trying to automate arbitrary-input-into-what-they-really-meant processing here feels super duper risky.

Instead, I think what we should do is add a Windows-only automation-only assertion that fires whenever you call any IOUtils API with a path that contains `/` on Windows. That will fire even when not actually used on shares, and will allow catching these cases more proactively, should there be any others. It should probably be a separate bug though, as AIUI the patch Barret attached here fixes the issue as reported, and is safe enough we could uplift to 95.

(In reply to Mark Hammond [:markh] [:mhammond] from comment #18)
> I guess my greater point is that JSONFile has used paths like that for well over 10 years

Not to quibble too much about specifics, but the oldest blame I see on toolkit/modules/JSONFile.jsm is from 2014. Are you talking about a different bit of code? Did sync have its own copy of the module?

> and this seems like an unintentional change in semantics that may impact other existing code. I don't feel incredibly strongly about this, but it seems like a new footgun is being introduced. I'd love to hear from Gijs and :asuth here.

I can see that it feels like a footgun if OS.File coped with this, but XPCOM IO interfaces never did, and NSPR and other places are full of `#ifdef`'d path separator defines for precisely that reason.

XPCOM files mitigated this via the `append` API and passing objects rather than paths around (and indeed, IIRC there are source code admonitions in `nsLocalFIle` telling you that passing around just paths is going to make you sad, so perhaps this bug is evidence those admonitions were right - but that was already true for JSONFile/OS.File for a long time, so that ship has sailed...)

We could try to mitigate this "footgun" in IOUtils by preprocessing every instance of an incoming path, but I'm inclined not to try to layer magic of our own on top of OS path munging. From the downloads work, we have extensive experience with the fact that platform-specific requirements coupled with filesystem-specific requirements cause no end of pain (you can mount NTFS disks on Linux, and now you have, as they say, 2 problems). Just yesterday I saw a report basically asking us to allow people to add `/` into filenames on macOS (ie as part of a filename, not a path separator, which is possible on macOS!) when using the "save as" dialog for downloads. Trying to automate arbitrary-input-into-what-they-really-meant processing here feels super duper risky.

Instead, I think what we should do is add a Windows-only automation-only assertion that fires whenever you call any IOUtils API with a path that contains `/` on Windows. That will fire even when not actually used on shares, and will allow catching these cases more proactively, should there be any others. It should probably be a separate bug though, as AIUI the patch Barret attached here fixes the issue as reported, and is safe enough we could uplift to 95.