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

Don't check Same-Site attribute for mobile chrome #1454

Merged
merged 1 commit into from
Sep 20, 2016
Merged

Don't check Same-Site attribute for mobile chrome #1454

merged 1 commit into from
Sep 20, 2016

Conversation

LukasReschke
Copy link
Member

Android is really insane when it is about cookies. So what happens is:

  1. Nextcloud sets cookies with a Same-Site attribute
  2. Chrome Android accepts it and sends it properly
  3. The first download using Chrome works
  4. It is redownloaded with the Download Manager which does just completely drops cookies with the same-site attribute

This makes downloads fails on mobile Chrome.

Fixes #342

@LukasReschke LukasReschke added the 3. to review Waiting for reviews label Sep 19, 2016
@LukasReschke LukasReschke added this to the Nextcloud 11.0 milestone Sep 19, 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 19, 2016
Closed
@LukasReschke
Copy link
Member Author

LukasReschke commented Sep 19, 2016 •
edited
Loading

See https://bugs.chromium.org/p/chromium/issues/detail?id=628859#c12 … Chrome 54 will by default disable system download manager. To be released in 3 weeks or so… 🙈

https://www.chromium.org/developers/calendar

@LukasReschke
Copy link
Member Author

Also left some more information at https://bugs.chromium.org/p/chromium/issues/detail?id=628859#c13 for the Android developers.

@LukasReschke LukasReschke force-pushed the morris branch 2 times, most recently from 04dbd78 to d5337f5 Compare September 19, 2016 23:15
LukasReschke added a commit that referenced this pull request Sep 19, 2016
@LukasReschke
Don't check for Same-Site cookie on Chrome Android
d5337f5 
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 changed the title Don't set Same-Site attribute for mobile chrome Don't check Same-Site attribute for mobile chrome Sep 19, 2016
MorrisJobke
MorrisJobke reviewed Sep 19, 2016
View reviewed changes
lib/base.php
// all same-site cookies get deleted and recreated directly. Awesome!
// FIXME: Remove once Chrome 54 is deployed to end-users
// @see https://github.com/nextcloud/server/pull/1454
if(\OC::$server->getRequest()->isUserAgent([\OC\AppFramework\Http\Request::USER_AGENT_ANDROID_MOBILE_CHROME])) {
Copy link
Member

Choose a reason for hiding this comment

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

use $request

Copy link
Member Author

Choose a reason for hiding this comment

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

use git to fix it yourself :)

@LukasReschke @MorrisJobke
Don't check for Same-Site cookie on Chrome Android
9a2511f 
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
MorrisJobke
MorrisJobke approved these changes Sep 19, 2016
View reviewed changes
Copy link
Member

@MorrisJobke MorrisJobke left a comment

Choose a reason for hiding this comment

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

Tested and works 👍

@MorrisJobke
Copy link
Member

I tested this and it works now 👍

@MorrisJobke
Copy link
Member

cc @nickvergessen @ChristophWurst @schiessle please review

@LukasReschke
Copy link
Member Author

@schiessle reviewed it carefully together with me in a review session and had a really critical view on it and they say it's ok. 👍

@LukasReschke LukasReschke merged commit 09e1218 into master Sep 20, 2016
@LukasReschke LukasReschke deleted the morris branch September 20, 2016 09:08
@MorrisJobke
Copy link
Member

@karlitschek Could we backport this to stable10?

@karlitschek
Copy link
Member

nice. please backport 👍

LukasReschke added a commit that referenced this pull request Sep 20, 2016
@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 mentioned this pull request Sep 20, 2016
Closed
@LukasReschke
Copy link
Member Author

Backport at #1456

@ivanvermeyen ivanvermeyen mentioned this pull request Dec 3, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MorrisJobke MorrisJobke MorrisJobke approved these changes

Assignees
No one assigned
Labels
3. to review Waiting for reviews
Projects
None yet
Milestone
Nextcloud 11.0
Development

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