As a security precaution, we have turned on the setting "Require API key authentication for API requests" for everyone. If this has broken something, please contact bugzilla-admin@mozilla.org
Last Comment Bug 424712 - Server location needs a slash at the end
: Server location needs a slash at the end
Status: VERIFIED FIXED
:
Product: Cloud Services
Classification: Client Software
Component: Firefox Sync: Backend (show other bugs)
: unspecified
: All All
: -- minor (vote)
: mozilla20
Assigned To: Michal Jaskurzynski
:
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-23 18:40 PDT by Vincent Robert
Modified: 2013-01-08 10:21 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (489 bytes, patch)
2012-09-29 10:04 PDT, Michal Jaskurzynski
no flags Details | Diff | Splinter Review
serverURL setter patch (623 bytes, patch)
2012-10-01 13:05 PDT, Michal Jaskurzynski
rnewman: review-
Details | Diff | Splinter Review
revised patch (579 bytes, patch)
2012-10-02 11:25 PDT, Michal Jaskurzynski
gps: feedback+
Details | Diff | Splinter Review
patch with test (1.69 KB, patch)
2012-11-07 12:51 PST, Michal Jaskurzynski
rnewman: feedback+
Details | Diff | Splinter Review
patch with test (2.33 KB, patch)
2012-11-10 10:09 PST, Michal Jaskurzynski
rnewman: review+
Details | Diff | Splinter Review

Description User image Vincent Robert 2008-03-23 18:40:21 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; fr; rv:1.9b4) Gecko/2008030317 Firefox/3.0b4
Build Identifier: 0.1.22

I tried to use a personal server instead of services.mozilla.org to sync my profile using weave.

I discovered that what Weave 0.1.22 do is concatenate its pathes with the URL configured in the preferences. If this URL does not end with a slash, Weave just use the last part of the URL as a prefix for created folders.

Ex: 
Using https://example.com/weave/ creates https://example.com/weave/user/...
Using https://example.com/weave  creates https://example.com/weaveuser/...

Reproducible: Always

Steps to Reproduce:
1.Configure the server location as something like https://example.com/weave
2.Synchronize

Actual Results:  
The created folder will be https://example.com/weaveuser/

Expected Results:  
The created folder should be https://example.com/weave/user/
Comment 1 User image Dan Mills [:thunder] 2008-03-31 15:22:43 PDT
Fixed in 0.1.27.
Comment 2 User image Michal Jaskurzynski 2012-09-29 09:58:02 PDT
This issue appears again. I tested it with Firefox 15.0.1 and last trunk build.
Comment 3 User image Michal Jaskurzynski 2012-09-29 10:04:12 PDT
Created attachment 666222 [details] [diff] [review]
patch
Comment 4 User image Dan Mills [:thunder] 2012-09-29 10:10:14 PDT
I'm really out of the loop on this, but reopening so someone else will look at it.
Comment 5 User image Richard Newman [:rnewman] 2012-09-29 11:27:29 PDT
Comment on attachment 666222 [details] [diff] [review]
patch

I'd rather this logic live in services/sync/service.js, in the setter for serverURL. That will catch every case of assignment, and IMO is the right place to enforce this part of the rule. gps, do you agree?
Comment 6 User image Gregory Szorc [:gps] 2012-10-01 11:34:18 PDT
Comment on attachment 666222 [details] [diff] [review]
patch

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

I agree that this logic should be in service.js, closer to the core API.
Comment 7 User image Michal Jaskurzynski 2012-10-01 13:05:20 PDT
Created attachment 666665 [details] [diff] [review]
serverURL setter patch

Ok, I moved slash adding to serverURL setter.
Comment 8 User image Richard Newman [:rnewman] 2012-10-01 18:14:05 PDT
Comment on attachment 666665 [details] [diff] [review]
serverURL setter patch

Check should be done prior to equality check. 

(sorry for brevity, on phone)
Comment 9 User image Michal Jaskurzynski 2012-10-02 11:25:40 PDT
Created attachment 667074 [details] [diff] [review]
revised patch
Comment 10 User image Gregory Szorc [:gps] 2012-11-05 10:23:58 PST
Comment on attachment 667074 [details] [diff] [review]
revised patch

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

Could you please add a test for this?

Sorry the review took so long. It got lost in my review queue.

::: services/sync/modules/service.js
@@ +65,5 @@
>    get serverURL() Svc.Prefs.get("serverURL"),
>    set serverURL(value) {
> +    // Add slash at the end
> +    // See Bug 424712
> +    if (value.charAt(value.length - 1) != "/")

Use value.endsWith("/").
Comment 11 User image Michal Jaskurzynski 2012-11-07 12:51:11 PST
Created attachment 679329 [details] [diff] [review]
patch with test
Comment 12 User image Richard Newman [:rnewman] 2012-11-07 13:20:41 PST
Comment on attachment 679329 [details] [diff] [review]
patch with test

Stealing this.
Comment 13 User image Richard Newman [:rnewman] 2012-11-09 13:14:30 PST
Comment on attachment 679329 [details] [diff] [review]
patch with test

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

Thanks for the patch!

"test_service_serverUrl.js" should be "test_service_set_serverURL.js".

Please set the User etc. fields in your patch according to:

<https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F>

::: services/sync/modules/service.js
@@ +74,5 @@
>  
>    get serverURL() Svc.Prefs.get("serverURL"),
>    set serverURL(value) {
> +    // Add slash at the end
> +    // See Bug 424712

No need for these comments.

@@ +76,5 @@
>    set serverURL(value) {
> +    // Add slash at the end
> +    // See Bug 424712
> +    if (!value.endsWith("/"))
> +      value += "/";

Always use {} in conditional expressions.

::: services/sync/tests/unit/test_service_serverUrl.js
@@ +2,5 @@
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +Cu.import("resource://services-sync/service.js");
> +
> +// Bug 424712: Server location needs a slash at the end

Trailing period.

@@ +4,5 @@
> +Cu.import("resource://services-sync/service.js");
> +
> +// Bug 424712: Server location needs a slash at the end
> +function run_test() {
> +  Service.serverURL = "http://example.com/weave";

s/weave/sync/g

"Weave" died a death in ~2009; we shouldn't be introducing new instances of that string.

@@ +6,5 @@
> +// Bug 424712: Server location needs a slash at the end
> +function run_test() {
> +  Service.serverURL = "http://example.com/weave";
> +  do_check_eq(Service.serverURL,
> +              "http://example.com/weave/");

These do_check_eq calls shouldn't span multiple lines -- total length is 61 columns.
Comment 14 User image Michal Jaskurzynski 2012-11-10 10:09:57 PST
Created attachment 680376 [details] [diff] [review]
patch with test
Comment 15 User image Richard Newman [:rnewman] 2012-11-12 09:47:02 PST
Comment on attachment 680376 [details] [diff] [review]
patch with test

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

::: services/sync/modules/service.js
@@ +76,5 @@
>    set serverURL(value) {
> +    if (!value.endsWith("/")) {
> +      value += "/";
> +    }
> +    

Nit: trailing whitespace.

::: services/sync/tests/unit/xpcshell.ini
@@ +66,5 @@
>  [test_service_login.js]
>  [test_service_migratePrefs.js]
>  [test_service_passwordUTF8.js]
>  [test_service_persistLogin.js]
> +[test_service_set_serverUrl.js]

Should be serverURL.
Comment 16 User image Richard Newman [:rnewman] 2012-11-12 09:50:01 PST
Pushed with nits addressed:

https://hg.mozilla.org/services/services-central/rev/a090e3ede2e7

STR: custom server URL should work with or without a single trailing slash. Not an urgent verification.
Comment 17 User image Richard Newman [:rnewman] 2013-01-02 19:43:40 PST
https://hg.mozilla.org/mozilla-central/rev/a090e3ede2e7
Comment 18 User image Tracy Walker [:tracy] 2013-01-08 10:21:54 PST
If I leave off the slash, it is automatically added a brief time after .com is complete.  That appears to me what the code is meant to do.

Note You need to log in before you can comment on or make changes to this bug.