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.

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

Already on GitHub? Sign in to your account

Send Clear-Site-Data header and let browsers ignore it if unsupported #37405

Merged
merged 2 commits into from
Mar 28, 2023

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Mar 25, 2023

Summary

According to https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Clear-Site-Data#browser_compatibility only Safari don't support it.

Checklist

@solracsf solracsf added the 3. to review Waiting for reviews label Mar 25, 2023
@solracsf solracsf added this to the Nextcloud 27 milestone Mar 25, 2023
@szaimen szaimen requested a review from a team March 26, 2023 08:17
@skjnldsv
Copy link
Member

Do we gain anything to just not send it if safari?
Wouldn't safari just ignore the header?

If so, we should not even bother and send it for all browsers, no?

@solracsf
Copy link
Member Author

Sure but as i didn't knew the reason why some were balcklisted i assumed it was a compatiblity issue; but yes, this is just a header, browsers will just ignore it if unsupported.

@solracsf solracsf changed the title Don't send Clear-Site-Data header to Safari Send Clear-Site-Data header and let browsers ignore it if unsupported Mar 26, 2023
@solracsf
Copy link
Member Author

solracsf commented Mar 26, 2023

@skjnldsv I've changed the code accordingly.

@solracsf solracsf force-pushed the clear-site-data branch 2 times, most recently from d708174 to cfd7a57 Compare March 26, 2023 13:28
Signed-off-by: Git'Fellow <[email protected]>

Don't send Clear-Site-Data to Safari

Signed-off-by: Git'Fellow <[email protected]>

Fix lint

Signed-off-by: Git'Fellow <[email protected]>
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish 2. developing Work in progress and removed 3. to review Waiting for reviews 4. to release Ready to be released and/or waiting for tests to finish labels Mar 28, 2023
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

tests/Core/Controller/LoginControllerTest.php:160 needs a fix :)

Signed-off-by: Git'Fellow <[email protected]>
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Mar 28, 2023
@skjnldsv skjnldsv merged commit 8ee52d3 into master Mar 28, 2023
@skjnldsv skjnldsv deleted the clear-site-data branch March 28, 2023 12:02
@joshtrichards
Copy link
Member

The blacklist was due to #17784 which is Chrome/Chromium specific and apparently still a problem in the field (e.g. see #41196).

@HLFH
Copy link
Contributor

HLFH commented Oct 30, 2023

I cancelled the regression code of the PR here: #42544.

HLFH added a commit to HLFH/server that referenced this pull request Jan 2, 2024
HLFH added a commit to HLFH/server that referenced this pull request Jan 2, 2024
szaimen pushed a commit to HLFH/server that referenced this pull request Jan 5, 2024
backportbot bot pushed a commit that referenced this pull request Feb 12, 2024
Signed-off-by: Gaspard d'Hautefeuille <[email protected]>
skjnldsv pushed a commit that referenced this pull request Feb 22, 2024
Signed-off-by: Gaspard d'Hautefeuille <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants