veliovgroup / Meteor-Files

🚀 Upload files via DDP or HTTP to ☄️ Meteor server FS, AWS, GridFS, DropBox or Google Drive. Fast, secure and robust.
https://packosphere.com/ostrio/files
BSD 3-Clause "New" or "Revised" License
1.11k stars 167 forks source link

Permission checks will fail on cordova because of wrong cookie domain #159

Closed menelike closed 6 years ago

menelike commented 8 years ago

Service-side permission checks depending on cookies can not be used within cordova and will fail. Document.cookie can only access localhost as cordova connects to <content src="http://localhost:12008/"/> where the request for files with go along with the server set my meteor (--mobile-server) e.g. https://foo.bar/cdn/.... Might be wort mentioning that we use Crosswalk.

This may also be related to https://github.com/VeliovGroup/Meteor-Files/issues/97 and http://stackoverflow.com/a/36334857/1981426

Can someone reproduce this? Is this even possible to fix without a cordova plugin?

Update

On deeper inspection I can see that there is client/server handshake which deals with this issue. Still leaves https://github.com/VeliovGroup/Meteor-Files/issues/97#issuecomment-224867402 open.

Headers being sent (Chrome Browser):

image

Headers being sent (Cordova/Crosswalk) (Cookie is missing): image

This can be reproduced if ROOT_URL !== the webpage URL e.g. 'localhost' !== '127.0.0.1'`

dr-dimitru commented 8 years ago

I can reproduce it if I open Meteor app by local IP address or via 127.0.0.1, even without Cordova. Other topic is if document.cookie really unaccessible.

  1. @menelike could you console.log(document.cookie) on the cordova app?
  2. Is it iOS or Android?
menelike commented 8 years ago

@menelike could you console.log(document.cookie) on the cordova app?

Outputs from Cordova/Crosswalk:

run with --mobile-server=https://foo.bar

window.location: {
...
host: "localhost:12008"
hostname: "localhost"
...
}

document.cookie = 'meteor_login_token=CIBXba1jySJ11Efbioi4YwAy2Qy8X8AW6pnAmoWCgA-'

__meteor_runtime_config__.ROOT_URL = 'https://foo.bar'
__meteor_runtime_config__.ROOT_URL_PATH_PREFIX = ''
window.navigator.userAgent = "Mozilla/5.0 (Linux; Android 6.0.1; SHIELD Tablet K1 Build/MRA58K) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/46.0.2490.86 Crosswalk/17.46.448.10 Safari/537.36"

Is it iOS or Android?

Android.

Hope this helps. Thanks a lot @dr-dimitru

dr-dimitru commented 8 years ago

@menelike production Cordova uses localhost too? How it is even possible?

menelike commented 8 years ago

I'm using HMR which is still loaded but disabled in production, hope this doesnt conflict here.

I've never seen anything under cordova besides http://localhost:X/, but I'm not sure as I can't find a valid spec in this. I can think of cordova setting up a layer to cache the webpage. Without that, the app would start with an working internet connection only. Therefore I guess that this is the default behaviour which is indeed a killer for cookies since this conflicts with cross domain security.

dr-dimitru commented 8 years ago

Can't find any info about Cordova cookies usage. I'll update this thread after some testing.

dr-dimitru commented 8 years ago

Hi @menelike ,

Could you reproduce this issue on Meteor@1.4 and MF@1.6.7 ?

menelike commented 8 years ago

@dr-dimitru sure, will test this within the next 24-48h. Thanks a lot for your help!

dr-dimitru commented 8 years ago

@menelike any news?

menelike commented 8 years ago

@dr-dimitru sorry for the waaayyy too long delay.

ostrio:cookies@2.0.5
ostrio:files@1.6.9

The results are the same:

From Webbrowser: bildschirmfoto 2016-08-09 um 17 12 19

From Cordova: bildschirmfoto 2016-08-09 um 17 12 28

At this point I'm not even sure if cookies are possible in cordova, it feels like we are trying to bypass the security system. Do you have any suggestions?

https://forums.meteor.com/t/meteor-1-3-beta-11-setting-cookies-from-cordova-on-android/18637/2 might help here.

dr-dimitru commented 8 years ago

To solve it on your end - use DDP upload, it doesn't relies on cookies. I'm working on the major update, and try to fix it there.

menelike commented 8 years ago

@dr-dimitru thanks for you help.

On a side note, this issue impacts downloading files. Never had problems with uploads because we use DDP for uploads as you advised.

dr-dimitru commented 8 years ago

Hmmm... as fallback we can add support for sessionId= get query. What do you think?

dr-dimitru commented 8 years ago

It won't violate security as you getting new sessionId at every login, and it very hard to guess

menelike commented 8 years ago

As a temporary solution this would work fine. Currently I disabled all permissions checks, this is by far a greater problem. On the long term this needs to be fixed properly, as you mentioned before uploads without DDP should be affected as well. But I'm afraid that at this point I still have no clue how to solve this without touching cordova.

update

I think a solution could be achieved through signed requests. Imho this is the better solution as exposing the sessionId (security: MIM attacks etc.). On the bright side this should make this library also independent from cookies at all.

After all I just do not like the idea of exposing credentials in an unencrypted area.

update

A minimal invasive way would be something like explained here, it looks quiet promising. It would act as an cookie relay.

dr-dimitru commented 8 years ago

Imho this is the better solution as exposing the sessionId (security: MIM attacks etc.)

  1. Use SSL everywhere, get query is encrypted too
  2. From your link ...You would then redirect to /download-file/123?ticket=lahdoiasdhoiwdowijaksjdoaisdjoasidja. The server will check that the ticket is valid and... same implementation, as Meteor already uses sessions and builded auth solution into accounts-.. packages uses "signed" requests via websockets. I'm just exposing it to the http.
menelike commented 8 years ago

You are absolutely right, of course everything gets encrypted when using SSL. But you still leave footprints here and there as explained here or here. As a general rule the used ticket should be encrypted and only valid for X seconds. As a result the plain sessionId should not be used.

I think the cookie relay would be a better solution as it fit's the current stack and would solve cross domain problems in general. I could start working on a solution in September if you like.

dr-dimitru commented 8 years ago

okay, we can put sessionId into body of post request. As I always saying, - we must keep our hands off other parts of application, we do package for file-upload here, not for authentication.

We must use authentication and user validation builded into meteor.

  1. Yes cookies will work, if you can fix it on the cordova client - I'm and other contributors/users will be happy
  2. Or we can pass sessionId in body, of post request. And avoid cookies usage.
dr-dimitru commented 8 years ago

Hi @menelike ,

Latest update has fallback auth, if cookies is unavailable. Could you please reproduce this issue on v1.7.0 or confirm as solved?

dr-dimitru commented 8 years ago

But with one exception, it has fallback for uploads, not protected downloads.

dr-dimitru commented 8 years ago

And instead of sessionId now we use connectionId, which is changed very frequently. So, if you will bless me using connectionId in query as auth fallback for downloads - I'll add it to package

menelike commented 8 years ago

Our problem in particular was downloading protected files. I'll be in my office next week, will test the fallback mechanism then, also will try some approaches I had in mind. Glad you're working on this @dr-dimitru , thanks a lot.

Update

Our problem was not downloading files in first place, it was viewing protected files within a DOM object like <img src="path to protected file" />. Downloading files in a cordova is another unsolved tasked which might not be solved through this project.

menelike commented 8 years ago

I had some time to investigate into cordova file transfers. I think I (we) view this problem from the wrong side. File uploads and downloads work totally different in cordova applications, to make them work further plugins like https://github.com/apache/cordova-plugin-file-transfer are needed. In addition we should just accept that cookies must be avoided in a cordova application, and in the Meteor context the best solution would be to just reuse DDP (performance should be ignored for now). With DDP we could just skip a additional plugin, reuse the existing authentication of Meteor (and local storage) and just use the current file upload mechanism in a reversed way. https://github.com/chfritz/typhone/blob/master/app/client/main.js#L179-L217 shows a working approach how this can be achieved.

Downside: This approach only deals with persistent downloads, not with protected downloads which shall only be used inside DOM objects like <img src="path to protected file" /> As a result we need to differentiate between filesystem downloads and DOM object sources.

This might be totally bogus, also something which this library should not solve at all (I'm pretty sure that persistent downloads should not be solved here), what do you think @dr-dimitru ?

menelike commented 8 years ago

According to http://stackoverflow.com/questions/32009625/how-to-use-cordova-with-crosswalk-and-having-cookies crosswalk has implemented solution for cookies, I'll check if the corresponding Meteor version has already enabled that support and if it is accessible through JS within the next week.

https://crosswalk-project.org/apis/embeddingapidocs_v6/org/xwalk/core/XWalkCookieManager.html

dr-dimitru commented 8 years ago

Our problem was not downloading files in first place, it was viewing protected files within a DOM object like . Downloading files in a cordova is another unsolved tasked which might not be solved through this project.

Isn't download and display files same thing?

dr-dimitru commented 8 years ago

Greetings @menelike ,

Any good news on your end?

menelike commented 8 years ago

@dr-dimitru sorry for the delay, I'm involved in a tough project with no spare time left

Isn't download and display files same thing?

Display deals with transferring data and of course with permissions in this case Downloading on top of transferring tasks, this also needs to deal with filesystem restrictions which are different on cordova and browsers

It's important to differentiate those two. After all, this project should aim on transferring/permission cases only. Therefore storing files persistently on a Cordova device should not be targeted.

https://crosswalk-project.org/apis/embeddingapidocs_v6/org/xwalk/core/XWalkCookieManager.html is not accessible from JS. I think that a simple pass trough plugin should be able to solve the cookie restrictions on Cordova/Crosswalk, though I've no experience with Java or Cordova plugins.

Something I'll try as soon as I've some time.

dr-dimitru commented 8 years ago

@menelike okay, ping me sometime

dr-dimitru commented 8 years ago

Hi @menelike ,

I'm planning big update on next week. Any news from your end? I've tried this package with few Cordova with and without Crosswalk - haven't met any issues

menelike commented 8 years ago

@dr-dimitru

Sorry for this delay ping-pong. This issue is bugs me for a while now, but has been prioritized down in our project. It's been scheduled to the week of 10.14.16 - 14.10.16.

I can image that this open issue bugs you, if you wish we can close this as it sounds that you've implemented a workaround. We can start over with a PR or a reopen it then. What do you say?

dr-dimitru commented 8 years ago

Got your timing...

as it sounds that you've implemented a workaround

No, no workaround, idk why - it just works on my end on both iOS and Android without modifications.

dr-dimitru commented 8 years ago

@menelike I you believe there is no such issues now, you can close this thread

s-ol commented 8 years ago

@dr-dimitru to keep you in the loop; I've been investigating this issue the past few days. The main problem consists of these three points:

  1. Files loaded via src attributes are losded out-of-band and therefore require seperate authentication from meteor in general. Local storage is not available for raw (GET) requests. Therefore ostrio:files falls back on cookie authentication (which is perfectly valid).
  2. Meteor employs a caching mechanism in Cordova apps that bundles and updates the build, hosting it from localhost on a port around 12000 (hahsed from app identity). This makes offline apps easy but means that the origin host (window.location.hostname) is always localhost on Cordova. The ROOT_URL however remains the FQDN of the production server, since all assets that are not cached in the build need to be dynamically fetched from the production server. Therefore all Cookie and Set-Cookie headers are considered to be 3rd Party Cookies per definition. This is also a valid approach but flawed design IMO because it basically forces CORS issues that don't need to exist.
  3. The latest Cookie RFC (dont have the number handy but could provide it on request) specifies that "3rd party Cookies" may or may not be rejected by Clients and that the choice may or may not be up to the user. Before android Lollipop 3rd Party cookies had been enabled by default, but since then the policy has been changed. Cordova used to still allow 3rd party cookies up until version 5.0-ish but has dropped support in a bigger release. The underlying Android CookieManager interface [allows disabling of the cookie-stripping mechanism](https://developer.android.com/reference/android/webkit/CookieManager.html#setAcceptThirdPartyCookies%28android.webkit.WebView, boolean%29) but is currently not available for configuration through Crosswalk. This is also valid, since the RFC allows it and blocking increases user privacy.

All of these together create this issue and adjusting one of the three can fix the problem.

I have created a report on the cordova JIRA but it might become a WONTFIX since the RFC doesn't require the feature and it is arguably bad practice.

Alternatively the cordova-plugin-meteor-webapp may be modified to tunnel all requests through the local server thus unifying traffic on one host; or the cordova application can be configured to use a different caching method and talk to the production server directly.

Lastly, ostrio:files could incorporate a limited-time-token mechanism, which would complicate the package a lot. I guess it is your choice whether you want to do something to avoid this issue on your end, but I suppose it is not really in your scope for this package.

dr-dimitru commented 8 years ago

@s-ol thank you for this report. Great investigation. From my end. I thought to implement authentication via get parameter appended to the requested file. Let me know what do you think. Seems like it will be simplest and quickest solution.

dr-dimitru commented 8 years ago

Hi folks,

Let me know what do you think about authentication via get parameter appended to the requested file, as mentioned above. Planning to work on next release.

s-ol commented 8 years ago

Hey @dr-dimitru, we experimented for some time now with a serviceWorker to basically replace the cordova-plugin-meteor-webapp package's role in the Cordova app. Unfortunately it's impossible to remove that package from the build completely since it is strongly integrated into the rest of the meteor build process, so most likely forks for meteor/meteor and meteor/cordova-plugin-meteor-webapp would be needed for a working solution where the Cordova app runs on the ROOT_URL itself (not to mention cordova loading tweaks and a fixed dependency on crosswalk).

...Therefore we are putting that research on hold for the time being and would appreciate a GET-Parameter authentication feature to be integrated with the authentication handling and .link() in MF. This seems to be the most 'stock-meteor-compatible' solution to this issue other users of your package might be having. We suggest that the new authentication be an opt-in option.

menelike commented 8 years ago

@s-ol great work. In addition this opt-in should be set only if client isCordova.

dr-dimitru commented 8 years ago

@s-ol thank you, going to implement it. @menelike not sure about only if client isCordova, because guys who working on python client is going to use authentication via GET parameter. I'm planning to set global flag to allow/deny "get-auth", which set to false by default

s-ol commented 8 years ago

@dr-dimitru isCordova could only be applied clientside anyway since the server is statically built and this information is never transferred (and couldn't be trusted if it were).

menelike commented 8 years ago

Indeed, and we need to determine if we really need get parameters (e.g. client isCordova), cookies should always be default option. I thought that we could add it as an option to the link() call, but this would enforce an explicit compatibility check on every call...ugly and bad developer experience.

I think that allow deny rules could work great, but they might need to splitted in server and client as well.

dr-dimitru commented 8 years ago

Well guys, please confirm next:

  1. Add allowGetAuth option to Constructor configuration. Which is false by default and prevents "get-authentication" on both Client (even if it's truthy isCordova) and Server.
  2. Add third argument appendGetAuth to .link() method which will append to links the "get-authentication" parameter
  3. Force "get-authentication" parameter if Client isCordova and allowGetAuth is true

Have I missed something?

menelike commented 8 years ago

@dr-dimitru

Add third argument appendGetAuth to .link() method which will append to links the "get-authentication" parameter

this should be avoided because

...but this would enforce an explicit compatibility check on every call...ugly and bad developer experience.

Why not just something like (just to get the idea)

var Images = new FilesCollection({
  collectionName: 'Images',
  useGetParameter: () => {
  // now let the developer decide what shall be done, like
  if (Meteor.isServer) {
    return true;
  } else if (Meteor.isCordova) {
    return true
  } else if (foo === bar) {
  return true
  }
  return false
  },
});

per default this function is undefined (do not use get params) or () => false;

dr-dimitru commented 8 years ago

@menelike not sure. @s-ol what do you think?

s-ol commented 8 years ago

why not; fits the constructor style with things like config.storagePath, config.protected etc.

I agree that it should be at least possible to set a global default value 'semi-dynamically' (vs. always having to pass it into .link()) since this is a compatibility thing and not really context dependant in most cases.

dr-dimitru commented 8 years ago

Okay. Agree.

One more thing, .link() method can be called on class directly (not from its instance), and will work. Is any of you using it this way (FilesCollection#link() vs MyFiles.link())?

s-ol commented 8 years ago

@dr-dimitru we are actually using FilesCollection#link in a collection helper to overwrite file.link in some cases.

dr-dimitru commented 8 years ago

@s-ol okay, why?

lanmower commented 7 years ago

erm, cordova supports localstorage doesnt it?

https://cordova.apache.org/docs/en/latest/cordova/storage/storage.html#localstorage

I see the userId, token, expiration date etc in localstorage, could that be used instead of the cookie?

s-ol commented 7 years ago

@Ianmower yes it does and that's the way it is done for the main view authentication. But localStorage cannot be used on HTTP requests because it is Clientside and not networked. If you embed documents the localStorage'd data can never be used on the server for authentication, unless you inject it into the request via an URI paramater. Only session and regular cookies can do that.

You could of course use ajax to load every asset on your page that is critical, but this does also introduce a lot of unnecessary complexity in places where it shouldn't be (in clientside application logic).

s-ol commented 7 years ago

@dr-dimitru sorry, I missed that question. We have an overloaded function because we extensively use the 'version's to thumbnail images. When we use the seperate sizes we often need to prevent the fallback to the 'default' version because we don't want clients to load high res images on mobile connections unless they specifically request the large one.

lanmower commented 7 years ago

I see yes that it what I've missed, the token is available when making the request but the actual authentication mechanism is in question.

I don't know if this is a dumb question, but could the responsibility of handling the transfer be offloaded to another package, like simple:rest or restivus? they appear to handle authentication in ddp and http scenarios in a predictable way.