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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[stable10] Don't check for Same-Site cookie on Chrome Android #1456

Closed
wants to merge 1 commit into from
Closed

[stable10] Don't check for Same-Site cookie on Chrome Android #1456

wants to merge 1 commit into from

Conversation

LukasReschke
Copy link
Member

Backport of #1454

@LukasReschke
Don't check for Same-Site cookie on Chrome Android
232a4d9 
Chrome on Android has a bug that it doesn't sent cookies with the
same-site attribute for the download manager. To work around that
all same-site cookies get deleted and recreated directly. Awesome!
FIXME: Remove once Chrome 54 is deployed to end-users
@see #1454
@LukasReschke LukasReschke added the 3. to review Waiting for reviews label Sep 20, 2016
@LukasReschke LukasReschke added this to the Nextcloud 10.0.2 milestone Sep 20, 2016
@mention-bot
Copy link

@LukasReschke, thanks for your PR! By analyzing the annotation information on this pull request, we identified @bartv2, @icewind1991 and @nickvergessen to be potential reviewers

@LukasReschke LukasReschke mentioned this pull request Sep 20, 2016
Merged
@MorrisJobke
Copy link
Member

This breaks login and the public share page.

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Sep 20, 2016
@LukasReschke
Copy link
Member Author

This breaks login and the public share page.

This is the same code than on master, so I suspect this also to be not properly working in Android then. My guess is that the cookies are simply never sent then.

@MorrisJobke Can you add a self::sendSameSiteCookies(); before the return in line 529? I suppose that way it should work then… Should, as I don't have an Android device to test this myself 🙈

@MorrisJobke
Copy link
Member

This is the same code than on master, so I suspect this also to be not properly working in Android then. My guess is that the cookies are simply never sent then.

But I applied the patch also on my productive instance ... weird

@MorrisJobke
Copy link
Member

This breaks login and the public share page.

I retested this and it works fine now. I guess this was a non-stable10 compatible app back then.

👍 from me

@MorrisJobke MorrisJobke added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Sep 20, 2016
@MorrisJobke
Copy link
Member

@nickvergessen Please test this :)

  • create public link share of a folder with an image in it
  • open this link in Chrome on Android
  • download of image should succeed

LukasReschke
LukasReschke commented Sep 26, 2016
View reviewed changes
lib/base.php
// FIXME: Remove once Chrome 54 is deployed to end-users
// @see https://github.com/nextcloud/server/pull/1454
if($request->isUserAgent([\OC\AppFramework\Http\Request::USER_AGENT_ANDROID_MOBILE_CHROME])) {
return;
Copy link
Member Author

Choose a reason for hiding this comment

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

I guess I also should move the logic from "self::sendSameSiteCookies();" above this so it catches some more cases. Will correct later.

@LukasReschke LukasReschke mentioned this pull request Sep 30, 2016
Closed
@nickvergessen
Copy link
Member

status @LukasReschke ?

@MorrisJobke MorrisJobke added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 5, 2016
@LukasReschke
Copy link
Member Author

Chrome 54 is out for mobile => Unnecessary.

@LukasReschke LukasReschke deleted the stable10-backport-1454 branch November 25, 2016 10:58
@MorrisJobke MorrisJobke removed this from the Nextcloud 10.0.2 milestone Nov 25, 2016
@nickvergessen nickvergessen mentioned this pull request Dec 9, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
2. developing Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@LukasReschke @mention-bot @MorrisJobke @nickvergessen