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

Allow send() on Cordova/non-same-origin #11

Closed
wants to merge 2 commits into from
Closed

Allow send() on Cordova/non-same-origin #11

wants to merge 2 commits into from

Conversation

s-ol
Copy link
Contributor

@s-ol s-ol commented Feb 27, 2019

This PR enables cookies.send() on Cordova (iOS/Android), where it was before unusable because Cookies never reached the non-origin domain $ROOT_URL, regardless of whether .send() was called or not since document.cookie referred only to the localhost:12008 "proxy" domain and the .send() request was also sent there and was unanswered.

Some notable changes:

  • this.cookies now stores the cookieString; not the sanitized Value. This is necessary because after setting attributes like Path=/ in document.cookie, these attributes are not accessible by the client anymore, i.e. they do not show up in document.cookie, and therefore need to be tracked independently.
  • send() passes the Cookies by URI-string because they otherwise couldn't reach the server
  • if Meteor.isCordova send() reaches out to Meteor.absoluteUrl() instead of the default, which is localhost:12008 in these cases, where the server cannot reply and instead index.html is served.
  • Access-Control-Allow-Origin: http://localhost:12008 and Access-Control-Allow-Credentials: true are set on the ___cookie___/set route.

The overall API remains consistent and as far as I could tell meteor test-packages agrees that nothing got hurt.

@dr-dimitru
Copy link
Member

Hey @s-ol ,

I'd love to merge it, but I'm getting errors:

Screen Shot 2019-04-27 at 2 01 21 AM

Any ideas on what may went wrong?

@s-ol
Copy link
Contributor Author

s-ol commented Apr 28, 2019

@dr-dimitru hm, it looks like some values are being urlEncoded where they shouldn't be - or hadn't been (or they are not being urlDecoded, where they used to be).
I'm not quite sure how those test-cases work and when it occurs, I guess the changes in serialize triggered this?

@s-ol
Copy link
Contributor Author

s-ol commented Apr 28, 2019

@dr-dimitru i just pushed a blind fix, since apparently the way i was running the tests the last time didn't work. Could you please let me know whether this passes the tests?

@dr-dimitru
Copy link
Member

Added info about running tests to docs.

I still experiencing issues:

Screen Shot 2019-04-28 at 12 40 05 PM
Screen Shot 2019-04-28 at 12 39 19 PM

@dr-dimitru
Copy link
Member

dr-dimitru commented May 21, 2019

Hello @s-ol ,

I've got to reject this PR due to failed test and major (seems like unnecessary) changes in core codebase. I'm going manually implement Access-Control-Allow-Origin headers and other necessaries for Cordova.

@dr-dimitru dr-dimitru closed this May 21, 2019
@s-ol
Copy link
Contributor Author

s-ol commented May 21, 2019

@dr-dimitru you can try implementing it yourself of course, but i think you will find out why it is necessary to change some of the internals for implementing proper cookie setting for cordova. The important thing is that cookies need to be transmitted using an XHR with xhr.withCredentials = true; and Access-Control-Allow-Credentials: true. Also the cookies need to be passed (at least on cordova) by URI string because they cannot reach the server otherwise.

@dr-dimitru
Copy link
Member

@s-ol if we are going to send "cookies" as query string, it isn't really "cookies".
For this case you may simply setup middleware to accept query string in request and respond with Set-Cookie header. wdyt?

@dr-dimitru
Copy link
Member

@s-ol our goal — keep this library RFC 6265 compatible

@s-ol
Copy link
Contributor Author

s-ol commented May 22, 2019

For this case you may simply setup middleware to accept query string in request and respond with Set-Cookie header. wdyt?

isn't that exactly what the middleware in this PR does?

following only RFC6265 will not lead very far because the cordova-plugin-meteor-webapp completely circumvents any regular cookie behavior with its same-origin violation.
This PR is still "compatible" - nothing changed, except for the middleware you just described, that is only used where necessary (on Cordova).

@dr-dimitru
Copy link
Member

isn't that exactly what the middleware in this PR does?

Why this should be part of this library, when it can be simply implemented on demand?
This is very "edgy" case, isn't it?

/CC @jankapunkt

@s-ol
Copy link
Contributor Author

s-ol commented May 22, 2019

@dr-dimitru: no, it is the case whenever cookies are used on Cordova - i would not call this an edge case, it is one of the main intended use cases of meteor and this library: right now your package does not support all the platforms that meteor supports - and this is not mentioned in ths README. If any package includes the fix that makes cookies work on cordova - shouldn't it be the one that provides "isomorphic bulletproof cookies"?

dr-dimitru added a commit that referenced this pull request Jul 8, 2019
__Major Changes:__

 - 👨‍💻 `handler` option now called even if `auto` option is set to
`false`
 - 👷‍♂️ `Path=/` now is default `path` of all cookies
 - 👨‍🔬 Partly implemented suggested changes from #11 to provide
support over Cordova platform, via `Access-Control-Allow-Credentials`
and `Access-Control-Allow-Origin` headers and supplying XHR request
with `withCredentials - true` option, thanks to @s-ol

Other Changes:

 - 👨‍💻 Overall security and stability enhancements
 - 👷‍♂️ Add `onCookies` *Server* callback/hook triggered only when
client invokes `.send()` method
 - 📦 Internal Meteor dependencies update
@dr-dimitru dr-dimitru mentioned this pull request Jul 8, 2019
dr-dimitru added a commit that referenced this pull request Jul 8, 2019
Cordova (v2.4.0)

__Major Changes:__

 - 👨‍💻 `handler` option now called even if `auto` option is set to `false`
 - 👷‍♂️ `Path=/` now is default `path` of all cookies
 - 👨‍🔬 Partly implemented suggested changes from #11 to provide support over Cordova platform, via `Access-Control-Allow-Credentials` and `Access-Control-Allow-Origin` headers and supplying XHR request with `withCredentials - true` option, thanks to @s-ol

Other Changes:

 - 👨‍💻 Overall security and stability enhancements
 - 👷‍♂️ Add `onCookies` *Server* callback/hook triggered only when client invokes `.send()` method
 - 📦 Internal Meteor dependencies update
@dr-dimitru
Copy link
Member

Hello @s-ol please take a look on v2.4.0, curious if we accomplished everything you've requested

@s-ol
Copy link
Contributor Author

s-ol commented Jul 17, 2019

As outlined in veliovgroup/Meteor-Files#656 (comment), there is still open issues / fixes that were part of this PR. @menelike is currently working on rebasing the missing changes onto your v2.4.0 in risetechnologies/Meteor-Files (wip/testing in progress).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants