Skip to content

Do not send Clear-Site-Data header to Chrome-like browsers - #41204

Closed
HLFH wants to merge 4 commits into
nextcloud:masterfrom
HLFH:master
Closed

Do not send Clear-Site-Data header to Chrome-like browsers#41204
HLFH wants to merge 4 commits into
nextcloud:masterfrom
HLFH:master

Conversation

@HLFH

@HLFHHLFH commented Oct 30, 2023

Copy link
Copy Markdown
Contributor

Summary

Cancels #37405 regression.

Checklist

This was referenced Oct 30, 2023
Closed
Merged
@HLFH

HLFH commented Oct 30, 2023

Copy link
Copy Markdown
ContributorAuthor

I confirm this PR is working.

@HLFHHLFH changed the title Do not send Send Clear-Site-Data header to Chrome-like browsersDo not send Clear-Site-Data header to Chrome-like browsersOct 31, 2023
@szaimenszaimen added this to the Nextcloud 28 milestone Nov 3, 2023
@szaimen

szaimen commented Nov 3, 2023

Copy link
Copy Markdown
Contributor

Hi @HLFH can you please fix dco? See https://github.com/nextcloud/server/pull/41204/checks?check_run_id=18198838092. otherwise wr cannot merge this. Thanks!

tcitworld
tcitworld reviewed Nov 3, 2023
View reviewed changes
Comment threadcore/Controller/LoginController.php Outdated
Comment on lines +107 to +108
if ($this->request->getServerProtocol() === 'https') {
// This feature is available only in secure contexts
if (!$this->request->isUserAgent([Request::USER_AGENT_CHROME, Request::USER_AGENT_ANDROID_MOBILE_CHROME])) {

@tcitworldtcitworldNov 3, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We can keep both checks

@HLFHHLFHNov 5, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I only reverted to the former code that was working for Chrome-like browsers.
I fixed the regression and did not add any minor feature such as the HTTPS check.
This might be best to add the HTTPS check in an additional PR.

@HLFH

HLFH commented Nov 5, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@szaimen DCO is fixed. Feel free to merge.

@szaimen

szaimen commented Nov 5, 2023

Copy link
Copy Markdown
Contributor

@szaimen DCO is fixed. Feel free to merge.

Thanks! First some reviews need to be done

@blizzzblizzz mentioned this pull request Nov 6, 2023
Merged
nickvergessen
nickvergessen reviewed Nov 7, 2023
View reviewed changes
Comment threadcore/Controller/LoginController.php Outdated
This was referenced Nov 10, 2023
Merged
Merged
@HLFH

HLFH commented Nov 16, 2023

Copy link
Copy Markdown
ContributorAuthor

Any other reviews? @szaimen Or it could be merged for the beta 4.

@szaimen

szaimen commented Nov 16, 2023

Copy link
Copy Markdown
Contributor

php-lint failure seems to be related...

nickvergessen
nickvergessen reviewed Nov 16, 2023
View reviewed changes
Comment threadcore/Controller/LoginController.php Outdated
@szaimen

szaimen commented Nov 16, 2023

Copy link
Copy Markdown
Contributor

nodb failure seems to be related: https://drone.nextcloud.com/nextcloud/server/44081/9/4

@blizzzblizzz mentioned this pull request Nov 20, 2023
Merged
5 tasks
@blizzzblizzz modified the milestones: Nextcloud 28, Nextcloud 29Nov 23, 2023
@HLFH@solracsf
Do not send Clear-Site-Data to Chrome-like browsers
9643cb1
Clear-Site-Data is broken on Chrome-like browsers.
https://bugs.chromium.org/p/chromium/issues/detail?id=1349087
Signed-off-by: Gaspard d'Hautefeuille <github@dhautefeuille.eu>
HLFHand others added 3 commits December 9, 2023 00:39
@HLFH@solracsf
Do not send Clear-Site-Data to Chrome-like browsers - test
9bc6d9a
Signed-off-by: Gaspard d'Hautefeuille <github@dhautefeuille.eu>
@HLFH@solracsf
Make sure Firefox doesn't use Clear-Site-Data header on unsupported s…
66ba1cd
…ites
Signed-off-by: Gaspard d'Hautefeuille <github@dhautefeuille.eu>
@nickvergessen@solracsf
Update core/Controller/LoginController.php
ec2d8e6
Signed-off-by: Joas Schilling <213943+nickvergessen@users.noreply.github.com>
@HLFH

HLFH commented Dec 15, 2023

Copy link
Copy Markdown
ContributorAuthor

@szaimen Still any issues with @solracsf changes?

@HLFH

HLFH commented Dec 25, 2023

Copy link
Copy Markdown
ContributorAuthor

@karlitschek

@HLFH

HLFH commented Jan 2, 2024

Copy link
Copy Markdown
ContributorAuthor

Closing as no one cares about this 30s logout delay bug on Chrome-like browsers, which makes Nextcloud a broken solution since 9 months.

@HLFHHLFH closed this Jan 2, 2024
@skjnldsvskjnldsv removed this from the Nextcloud 29 milestone Feb 23, 2024
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nickvergessennickvergessennickvergessen left review comments
@solracsfsolracsfAwaiting requested review from solracsf
@tcitworldtcitworldAwaiting requested review from tcitworld

Assignees

No one assigned

Labels

3. to reviewWaiting for reviewsbugfeature: authenticationperformance 🚀regression

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Slow logout on Chrome-like browsers

7 participants

@HLFH@szaimen@nickvergessen@tcitworld@joshtrichards@blizzz@skjnldsv