Record ids with / are not accepted

RESOLVED FIXED

Status

Cloud Services
Server: Sync
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Mardak, Assigned: tarek)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

8 years ago
The form rewrite generates GUIDs with btoa and base64 uses alphanumeric, +, and /. Technically there's nothing really preventing / from being in a guid, but it might be confusing in the context of a url, but that can always be escaped correctly.

http://hg.mozilla.org/mozilla-central/rev/cabbe056fd9c#l5.453

2010-05-27 11:18:02Collection          TRACEPOST body: {"success":["z_SoDELu5o","!wh(i5hBcv","oONRKANrAR","Ig3*CSQ0J(","3xLzv3azRH","r_LTxCdtHa","Hm.wVChXO*","Bnn(UHwAhx","_nRmhe)k1t"],"failed":{"tXue2\/2jokyBLT\/f":["invalid id"],"Vf\/7xRHJ1E+B1hnS":["invalid id"],"z\/QGd990sEOMFn9P":["invalid id"],"0\/Onl9fnCUKpVfrF":["invalid id"]}}

Seems like satchel is generating 16 character guids... so 63/64 probability that a character won't be a / out of 16 characters.. 22.27% chance that the guid will contain a /?
Err, I don't believe the JS rewrite changed anything here.

The C++ code (as of bug 506402) was using PL_Base64Encode() to generate 16 character GUIDs. That bug also landed tests, and those same tests continue to be run successfully, unmodified, with the JS rewrite.
(Reporter)

Comment 2

8 years ago
Oh indeed. I grabbed a random build 20100520 and generated some guids and indeed there was a /.
Depends on: 506402
No longer depends on: 439716

Updated

8 years ago
Blocks: 592376
Created attachment 471459 [details] [diff] [review]
Allows slashes in ids

The attached patch enables slashes in ids. 

Note that we will need to activate AllowEncodedSlashes in Apache for this to work.
Attachment #471459 - Flags: review?(telliott)
Created attachment 471489 [details] [diff] [review]
Allows slashes in ids

Use php-style indentation
Attachment #471459 - Attachment is obsolete: true
Attachment #471489 - Flags: review?(telliott)
Attachment #471459 - Flags: review?(telliott)

Updated

8 years ago
Assignee: telliott → tarek
Comment on attachment 471489 [details] [diff] [review]
Allows slashes in ids

rather than doing such an extensive regex, wouldn't it be more efficient to do 

$ipath = explode('/', $path . '///');
$username = shift($ipath);
$function = shift($ipath);
if ($ipath)
  $id = implode('/', $ipath)

(syntax hasn't been checked for exactness)

?
Created attachment 475495 [details] [diff] [review]
Allows slashes in ids

OK - Python usually uses compiled regexp, but it sounds more efficient this way in PHP. 

So I suppose 'shift' is 'array_shift' here, here's a new patch using a regular split.
Attachment #471489 - Attachment is obsolete: true
Attachment #475495 - Flags: review?(telliott)
Attachment #471489 - Flags: review?(telliott)
Let's get this wrapped up and on the 1.3.x branch?  This prevents some form history records from syncing.
Attachment #475495 - Flags: review?(telliott) → review+
Fixed in all branches:
- stable: http://hg.mozilla.org/services/sync-server/rev/9f1f11605f8d
- default: http://hg.mozilla.org/services/sync-server/rev/92e488eaaff1

Toby, looks like 1.0/tests/test_weave_utils.php was not commited
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED

Updated

8 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.