Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fixed private mode cookie for local development #17938

Merged
merged 3 commits into from
Oct 16, 2023
Merged

Fixed private mode cookie for local development #17938

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
acad566
Fixed: Set cookie in privateBlogging - Update middleware.js
joe-blocher Sep 2, 2023
ae0f64e
Merge branch 'main' into joe-blocher-patch-1
joe-blocher Sep 2, 2023
cde006a
Merge branch 'main' into joe-blocher-patch-1
joe-blocher Oct 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion ghost/core/core/frontend/apps/private-blogging/lib/middleware.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,8 @@ const privateBlogging = {
name: 'ghost-private',
maxAge: constants.ONE_MONTH_MS,
signed: false,
sameSite: 'none'
sameSite: urlUtils.isSSL(config.get('url')) ? 'none' : 'lax',
secure: urlUtils.isSSL(config.get('url'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ghost is intended to work locally, over http, as well in production, over https.

Before, the sameSite attribute for the cookie was always set to none.

Here's what the MDN docs have to say about the sameSite attribute:

None specifies that cookies are sent on both originating and cross-site requests, but only in secure contexts (i.e., if SameSite=None then the Secure attribute must also be set).

But for the local-development case in the bug report, the secure flag wasn't being set and can't be set.

Here, the proposed solution it continue to sameSite: none just as before in the https/production case, but resolves the issue by setting sameSite: lax for the local case.

As @joe-blocher points out, Ghost is already using the exact same solution in core/server/services/auth/session/express-session.js for a similar case.

This change LGTM. I recommend merging.

})(req, res, next);
},

Expand Down
Loading