Fix path and secure flag of cookie_test

[PVince81]

Fixes #26651

@ogoffart @davivel please test

[PVince81]

I baked this cookie taking inspiration from the other cookies in that file, so I think now the path and secure flag should be correct.

cc @phisch @DeepDiver1975 @Peter-Prochaska

[davivel]

@jesmrec , could you have a look with the mobile apps? Thanks.

[PVince81]

I suspect that the problem only appears on environments where ownCloud is hosted in a subfolder, like "/owncloud". If it's hosted on the root of the vhost, the path will be correct since it defaults to "/".
That might explain why I never saw this issue on my own server.

[PVince81]

@guruz

[jesmrec]

@davivel iOS devices send correctly the cookie_test but Android devices does not, using the same server with the current patch and performing the same actions.

CC @PVince81

[guruz]

Tried testing this with the desktop client.
In general I agree with the fix, HOWEVER for me it seems to have no effect.

If you send the cookie only when checking supportsCookies and you call supportsCookies only once from logClientIn then there was no chance to get the cookie even.

I also don't see the desktop client session in index.php/settings/personal?sectionid=security so no token was created.

[guruz]

you need to set the cookie when sending the first 401
but even that won't work for clients like the desktop client that send the password blindly
we wanted to change that in the desktop but then old server versions borked so we never changed it

https://github.com/owncloud/client/blob/c6f4f446192e5f11df80186e391b636471c15984/src/libsync/creds/httpcredentials.cpp#L53

[davivel]

@davivel iOS devices send correctly the cookie_test but Android devices does not, using the same server with the current patch and performing the same actions.

I'll have a look to the Android app.

[PVince81]

@guruz indeed, I have the feeling this could never have worked

[PVince81]

I do wonder how the iOS client works then ? @jesmrec

From the code path it looks like the iOS client would need to login twice, the first login would receive the cookie and the second login will tell the code that cookies work.

Unless maybe the first request receives a 401 with challenge ? But I don't think supportsCookie is reached for a 401.

[jesmrec]

Sorry, i have rechecked this issue. Both mobile clients are not sending the cookie in all requests. The cookie is received in the first OK response (propfind 207), but not resent in all the following requests. Needs to be checked deeply @davivel.

[jesmrec]

Maybe related with #26651 (comment)

• If the server is in "", the cookie_test does not contain the path:

Set-Cookie: ocfgt15emms5=2hhgs4fc6s0tle7oc48pgq3puf; path=/; secure; HttpOnly
Set-Cookie: cookie_test=test; expires=Fri, 28-Apr-2017 23:04:31 GMT; Max-Age=36000; secure; HttpOnly

the first cookie contains the path, but the second does not.

• If the server is not in "", the cookie_test contains the path:

Set-Cookie: ocfjcfv3pcvs=i6sboee9j1hnp9uqcser74gfd6; path=/stable91/owncloud; secure; HttpOnly
Set-Cookie: cookie_test=test; expires=Fri, 28-Apr-2017 14:08:52 GMT; Max-Age=3600; path=/stable91/owncloud; secure; httponly

Both cases are tested with patched servers.

@davivel @PVince81

[davivel]

@jesmrec , in the second case (server not in root path), are the mobile clients sending cookie_test in all the requests after the first one receiving Set-Cookie: cookie_test=test?

[davivel]

@jesmrec , if I recall correctly, in the first case, the requests to Webdav path are sending cookie_test, while the requests to OCS are not sending it, right?

That would be a correct behaviour. If Set-Cookie has no path set, the path of the current request is used by default. In the tested cases, that was the Webdav path, so the app will not send the cookie to OCS path.

The confusing part is that the PR seems to set a path. @PVince81 , is it possible that OC::$WEBROOT is not set the first time that supportsCookie is run if OC is in the root path of the web server?

[guruz]

@davivel @jesmrec The server is broken anyway because it does not send the cookie at the right place: #27549 (comment)
(Independant of whatever the path is)

[PVince81]

Cannot work, likely needs a different approach

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.