unquote params extracted from paths

[pjenvey]

The extractors module produces BsoParam, CollectionParam and HawkIdentifier from
extracted bits of the request path.

Our <..>_from_path methods aren't unquoting path segments. E.g. a bso_id of "{password-id}" comes in as PUT /1.5/uid/storage/passwords/%7Bpassword-id%7D: we're incorrectly considering this a bso_id of "%7Bpassword-id%7D" instead.

This likely only affects bso_ids. collection_ids are restricted to [a-zA-Z0-9._-]{1,32} and uids to [0-9]{1,10} (and we ensure uids match the hawk payload's, so there's no funny business there anyway). Whereas bso_ids allows any printable ascii character.

We've likely written garbage bso_ids via PUT. Should we attempt to fix the garbage bso_ids in the db? It would require extensive scanning of our request logs to do correctly.

DELETEs against migrated data would unexpectedly 404, causing:

https://bugzilla.mozilla.org/show_bug.cgi?id=1640123#c2

[jrconlin]

Tempted to say we could just fix bad bso_ids on the fly when we write them back out. That would save the scan costs and let bad records expire out naturally. It does run the risk of possibly breaking any client that is expecting a bad bso_id, though.

[pjenvey]

The good news, according to @mhammond, is our browsers aren't going to hit this in practice.

They tend to upload records via POST instead of PUT (which completely avoids the bug). I see the crypto + meta collections do use PUT but those engines are special cases (and don't use non-alphanumeric bso_ids). passwords seems to be the only collection affected by this. The issue1640123 that found the bug was also a special case: a user running a custom script against the server.

Hence why this never occurred during QA's testing.

So we should fix this ASAP but probably don't need to worry much about garbage bso_ids.

[pjenvey]

Added server-syncstorage#148 as this could really use an addition to the e2e tests

[fzzzy]

I do think we should try scanning for garbage data, because otherwise it might just sit there forever.

[fzzzy]

Tempted to say we could just fix bad bso_ids on the fly when we write them back out. That would save the scan costs and let bad records expire out naturally. It does run the risk of possibly breaking any client that is expecting a bad bso_id, though.

I'm not sure those records would expire naturally. Most records don't have an expire time, right? I feel like enforcing a stricter uuid format might be better in the long run so that the uuid is always in the same format?

[jrconlin]

I'm not sure those records would expire naturally. Most records don't have an expire time, right? I feel like enforcing a stricter uuid format might be better in the long run so that the uuid is always in the same format?

I'm all in favor of enforcing strict UUID formatting. I though all BSOs had some TTL associated with them. If not, we may want to consider adding one.

[fzzzy]

I keep saying I will work on this but then I don't. I'm going to unassign myself for now.

[jrconlin]

I can try taking a shot at this.