Do not set cookies to https only #351

2020-10-20T13:52:08Z

matthijskooijman commented

2020-10-20 13:52:08 +00:00

(Migrated from github.com)

This was only done for HTTPS-requests, but it turns out this breaks
mixed HTTP/HTTPS configurations, because browser do not store a separate
cookie for HTTP and HTTPS. So when you access the HTTPS version of a
site, the cookie is set as secure (https only) and subsequent HTTP
request will simply not get the cookie at all and cannot change the
cookie into non-secure either.

So this protection can only be enabled again when we know a site is
HTTPS-only, i.e. when #247 is implemented.

This was only done for HTTPS-requests, but it turns out this breaks mixed HTTP/HTTPS configurations, because browser do not store a separate cookie for HTTP and HTTPS. So when you access the HTTPS version of a site, the cookie is set as secure (https only) and subsequent HTTP request will simply not get the cookie at all and cannot change the cookie into non-secure either. So this protection can only be enabled again when we know a site is HTTPS-only, i.e. when #247 is implemented.

laurensmartina (Migrated from github.com) reviewed

2020-12-23 10:40:48 +00:00

system/core/HyphaSession.php

					
				@ -31,4 +31,3 @@

							ini_set('session.cookie_path', $request->getRootUrlPath());

							ini_set('session.cookie_secure', $request->isSecure());

							ini_set('session.cookie_httponly', true);

							// This enables browser protections against CSRF

Why not put the set secure within an if statement?

matthijskooijman (Migrated from github.com) reviewed

2020-12-23 11:50:16 +00:00

system/core/HyphaSession.php

					
				@ -31,4 +31,3 @@

							ini_set('session.cookie_path', $request->getRootUrlPath());

							ini_set('session.cookie_secure', $request->isSecure());

							ini_set('session.cookie_httponly', true);

							// This enables browser protections against CSRF

How do you mean exactly? Only set the property when isSecure() is true? That is what the old code effectively did, but that breaks mixed HTTP/HTTPS setups. Or am I misunderstanding you?

Writing this, I do realize that maybe we could include http/https in the session name so really support two different sessions for http and https and have secure cookies?

How do you mean exactly? Only set the property when `isSecure()` is true? That is what the old code effectively did, but that breaks mixed HTTP/HTTPS setups. Or am I misunderstanding you? Writing this, I do realize that maybe we could include http/https in the session name so really support two different sessions for http and https *and* have secure cookies?

matthijskooijman commented

2020-12-30 12:25:19 +00:00

(Migrated from github.com)

I came up with a better approach, see #368.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

No reviewers

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

harmen/hypha!351

Rows
Columns