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 166 forks source link

this.userId sometimes null in protected function on server #737

Closed Lickshotz closed 4 years ago

Lickshotz commented 4 years ago

I'm having an issue:

I have been reading through the issues concerning the "this.userId null in protected" function for quite abit, but I'm not getting this solved.

On my dev machine, I'm not having this issue, however in production, for some users the issue persists. On all my machines, regardless if chrome (or chrome mobile), edge or firefox, I do not have this issue. Other users have this issue in safari, firefox and chrome and chrome mobile.

I have checked with one of the users that have this issue, that the 'x_mok' cookie is set.

I have tried using transport http as well as ddp.

I have added additional logging in the protected function as follows:

const params = this.request.cookies;
const sessionId = String(params.x_mtok);
const userId =
        Meteor.server.sessions.get(sessionId) &&
        Meteor.server.sessions.get(sessionId).userId
            ? Meteor.server.sessions.get(sessionId).userId
            : null;
console.warn(params);
console.warn(sessionId);
console.warn('this.userId,', this.userId);
console.warn('userId,', userId);

which returns the following outcome (given that the problem is present)

[Object: null prototype] {}
undefined
this.userId, null
userId, null

Version of Meteor-Files: 1.13.0* Version of Meteor: METEOR@1.8.1

My package file:

meteor-base@1.4.0             # Packages every Meteor app needs to have
mobile-experience@1.0.5       # Packages for a great mobile UX
mongo@1.6.2                   # The database Meteor supports right now
reactive-var@1.0.11            # Reactive variable for tracker
tracker@1.2.0                 # Meteor's client-side reactive programming library

standard-minifier-js@2.4.1    # JS minifier run for production mode
es5-shim@4.8.0                # ECMAScript 5 compatibility for older browsers
ecmascript@0.12.4              # Enable ECMAScript2015+ syntax in app code
shell-server@0.4.0            # Server-side component of the `meteor shell` command

accounts-base
accounts-ui@1.3.1
accounts-password@1.5.1
useraccounts:bootstrap
alanning:roles
ostrio:files
mdg:validated-method
akryum:vue-component
fortawesome:fontawesome
seba:minifiers-autoprefixer
akryum:vue-router2
akryum:vue-sass
akryum:vue-blaze-template
msavin:mongol
vuejs:blaze-integration
aldeed:template-extension
matb33:collection-hooks
momentjs:moment
underscore@1.0.10
check@1.3.1
reywood:publish-composite
static-html
akryum:vue-less
fourseven:scss
jquery
email
aldeed:collection2@3.0.0
mrt:gsap

ros:publish-counts
littledata:synced-cron
http
meteortesting:mocha
practicalmeteor:faker
lmieulet:meteor-coverage
jackyqiu:meteor-jvectormap
percolate:migrations

The client side logs do not show anything out of the ordinary.

The server side logs of course show the following:

[FilesCollection._checkAccess] WARN: Access denied!

Thanks in advance and have a nice day! :)

dr-dimitru commented 4 years ago

Hello @Lickshotz,

While I'm looking in the issue, please help me answering:

  1. Single or multi-server setup?
  2. Single instance or multiple?
Lickshotz commented 4 years ago

Thanks for the quick reply.

  1. Single server setup
  2. I'm not sure what you mean, single instance server? If so, yes.
dr-dimitru commented 4 years ago

@Lickshotz on the second: sometimes solutions like MUP, Phusion Passenger, and other launch multiple application instances for balancing and durability

Lickshotz commented 4 years ago

@dr-dimitru I'm using pm2 to manage my application.

I'm not running it in cluster mode.

dr-dimitru commented 4 years ago

@Lickshotz can it be related to the #738 issue? Are you testing from Firefox?

Lickshotz commented 4 years ago

@dr-dimitru I don't know, as this has happened on chrome mobile, safari and firefox and I also don't know what he means by:

window.location.href = url;
jankapunkt commented 4 years ago

May this be related to browser's privacy settings?

Lickshotz commented 4 years ago

@jankapunkt du you have any hints on how I could find that out?

jankapunkt commented 4 years ago

Since cookies are involved I would compare browser settings accept all cookies vs rejecting all cookies.

Lickshotz commented 4 years ago

This seems odd, as the cookies are set in the browser of the users having this issue. I will give it a try though

Lickshotz commented 4 years ago

@jankapunkt @dr-dimitru I did indeed check with the user that has the cookie, but does not have the userId in the protected function having the same firefox privacy settings as I do (working for me) which are the standard privacy settings

jankapunkt commented 4 years ago

Any other plugins installed that could break things?

Lickshotz commented 4 years ago

@jankapunkt not that I know of. I have listed my packages above and here are my npm packages

 "dependencies": {
    "@babel/runtime": "^7.4.5",
    "@fortawesome/fontawesome-svg-core": "^1.2.15",
    "@fortawesome/free-brands-svg-icons": "^5.11.2",
    "@fortawesome/free-solid-svg-icons": "^5.7.2",
    "@fortawesome/vue-fontawesome": "^0.1.5",
    "babel-runtime": "^6.25.0",
    "bcrypt": "^3.0.7",
    "bootstrap-select": "^1.13.12",
    "browser-detect": "^0.2.28",
    "chai": "^4.2.0",
    "chai-spies": "^1.0.0",
    "core-js": "^2.6.5",
    "event-emitter": "^0.3.5",
    "flatpickr": "^4.5.7",
    "fs-extra": "^3.0.1",
    "gm": "^1.23.0",
    "i": "^0.3.6",
    "i18n-iso-countries": "^4.3.1",
    "jquery-slimscroll": "^1.3.8",
    "meteor-node-stubs": "~0.2.4",
    "nodemailer": "^6.2.1",
    "npm": "^6.13.6",
    "placeholder-loading": "^0.2.4",
    "popper.js": "^1.15.0",
    "simpl-schema": "^1.5.5",
    "sweetalert2": "^9.5.3",
    "toastr": "^2.1.4",
    "v-media-query": "^1.0.4",
    "vue": "^2.6.11",
    "vue-fragment": "^1.5.1",
    "vue-highlight-words": "^1.0.0",
    "vue-i18n": "^8.12.0",
    "vue-meteor-tracker": "^2.0.0-beta.5",
    "vue-router": "^3.0.0",
    "vue-server-renderer": "^2.6.10",
    "vue2-perfect-scrollbar": "^1.2.1",
    "vuex": "^3.1.0",
    "webfontloader": "^1.6.28"
  },
  "devDependencies": {
    "@meteorjs/eslint-config-meteor": "^1.0.5",
    "babel-eslint": "^10.0.1",
    "babel-plugin-istanbul": "^5.2.0",
    "concurrently": "^4.1.1",
    "css-loader": "^2.1.0",
    "eslint": "^4.19.1",
    "eslint-config-airbnb": "^17.1.0",
    "eslint-import-resolver-meteor": "^0.4.0",
    "eslint-plugin-import": "^2.17.2",
    "eslint-plugin-jsx-a11y": "^6.2.1",
    "eslint-plugin-meteor": "^4.1.4",
    "eslint-plugin-react": "^7.13.0",
    "eslint-plugin-vue": "^5.2.2",
    "eslint_d": "^7.3.0",
    "maildev": "^1.1.0",
    "prettier": "^1.17.0"
  },
Lickshotz commented 4 years ago

hey @dr-dimitru, what does the flag "question" mean? Doesn't this seem to be a bug?

dr-dimitru commented 4 years ago

@Lickshotz so far I'm considering something is off on your end. No one else has reported same or similar issues. Minimal reproduction repo may help investigating this issue.

Cookies and authentication pipeline is something we (@jankapunkt, @menelike, and @s-ol) was working on for long time. So far it's well tested (browser/mobile/cordova) and well supported part of the library.

You've mentioned it happens only on production, possible scenarios:

  1. Maybe it happens on attempt to directly access file without clicking on link?
  2. Maybe something is off with cookies policies? Is your setup multi-(sub)domain?
menelike commented 4 years ago

Willing to help if that happens on Cordova, on the browser this should actually be no issue. A repro as @dr-dimitru suggested is the requirement for further help.

Lickshotz commented 4 years ago

@dr-dimitru @menelike I will try to create a minimal reproduction repo. I have just found out that this is not just in production by accessing my dev machine with a phone that has the problem in production.

The result is the same. No userId in the protected function.

Lickshotz commented 4 years ago

Here is a minimal reproduction repo:

https://gitlab.com/Lickshotz/files-collection-no-userid-in-protected.git

email: test@mail.com pw: test1234

Last night, I tested it on production (using the production db as well)

Chrome on OS and Windows 10 had no problems.

Safari had problems regardless of device.

I cannot say about Firefox as I never have issues with firefox, but other users do.

In production, I'm running a nginx with a reverse proxy set up, the app is managed by pm2.

menelike commented 4 years ago

@Lickshotz Great, one question though, when you speak about safari, are you speaking about Safari mobile browser only and/or ios Cordova? Just asking to be sure.

Lickshotz commented 4 years ago

@menelike I'm talking only about Safari mobile browser and Safari OS browser.

menelike commented 4 years ago

I still can not reproduce this in my end:

 ROOT_URL=http://localhost:3000 meteor run
[[[[[ ~/WebstormProjects/files-collection-no-userid-in-protected ]]]]]

=> Started proxy.                             
=> [HMR] Dev server listening on port 3003.
=> Started MongoDB.                           
Browserslist: caniuse-lite is outdated. Please run next command `npm update`
Browserslist: caniuse-lite is outdated. Please run next command `npm update caniuse-lite browserslist`
=> Started your app.                          

=> App running at: http://localhost:3000
I20200427-18:01:08.137(2)? 3T2nfTe68ypQgBmbQ
I20200427-18:01:09.311(2)? 3T2nfTe68ypQgBmbQ
I20200427-18:01:14.333(2)? 3T2nfTe68ypQgBmbQ

I can upload a video file in Safari Version 13.0.4 (15608.4.9.1.3) on Mac 10.15.2 (19C57) without any issues. Any further hints @Lickshotz ?


Update: I need to fix your code first, upload ended somewhere on the disk, URL seems to be malformed hence I can not test the playback e.g. http://localhost:3000/cdn/storage/videos/Rczsm7xj647oMgdc9/original/Rczsm7xj647oMgdc9. it has a dot on the end.


Update:

I just used the URL directly (http://localhost:3000/cdn/storage/videos/Rczsm7xj647oMgdc9/original/Rczsm7xj647oMgdc9), while I can play the video file in chrome directly in Safari it fails for whatever reason (no time to debug this, out of scope). The most important part is that even in Safari I still always see the userId on each request.


Update I tried a different video format (MOV file), Safari works fine while uploading and viewing.

Result: I can not reproduce this issue.

jankapunkt commented 4 years ago

Works in Xubuntu 18.04. LTS on Firefox (75.0 64bit) and chromium (80.0.3987.163, 64bit)

=> App running at: http://localhost:3030/
I20200428-08:54:51.266(2)? YFZAcvSz85qLnmAJP
I20200428-08:57:33.878(2)? YFZAcvSz85qLnmAJP
jankapunkt commented 4 years ago

I made it to nullin Firefox using the following settings:

Untitled

Lickshotz commented 4 years ago

@menelike thank you for testing it.

I am completely baffled.

Not because you cannot reproduce it, as in my dev environment I am not able to do so either. Only once I go into production, some users experience it.


As to the "." at the end of the link, this equally baffles me, I have noticed it beeing there, but did not know that it shouldn't be there as I'm simply using the Videos.link(file) function to get that link.

I removed the link (Edit: ".") manually and pushed to production. The error persisted.


I have an android device at home that has that issue running Chrome 81.0.4044.111 Yesterday I installed Opera and Firefox on it, no Issue. Using the android google search, no Issue. Uninstalling Chrome and reinstalling the latest version (Chrome 81.0.4044.117), the issue persists.


I have ran the application without pm2 in production, the issue persists.


I will now setup a docker container and build the app locally to see if I can reproduce the issue.

Lickshotz commented 4 years ago

@jankapunkt thank you for testing it.

Do you have the cookie set in the browser when using that custom setting?

As I have confirmed that:

  1. The cookie is always set, wether or not the user has the problem or not (independent from which browser is used)
  2. Atleast one user using the standard cookie policy of firefox who has the issue.
Lickshotz commented 4 years ago

Hey @jankapunkt @dr-dimitru @menelike,

I was pretty busy setting up the docker image today but in the end, could not reproduce the problem (the device that doesn't work in production had a userId in the protected function).

I assumed that nginx is the problem as this was the only thing that I haven't tested. I setup the log inorder for it to log the x_mtok cookie and indeed it was empty for the users who had no userId in the protected function.

Later today a user was having problem uploading a video with the following error:

image

which i did do some brief reading on and seems to have something to do with cookies (but im honestly not 100% sure about that).

I just reverted back to ddp hoping to solve that issue (without even thinking about the protected function), which it did and the nginx started to show cookies for every request. Checking with my "problematic" device, cookie was there, userId was there.

I assume that it seems to be an issue in the way I have setup https in my nginx.

Does any one of you have insight about that?


Update:

Seems that I have been mistaken, even though it worked on my "problematic" device, It does not show a cookie on the server or a userId, for as it seems safari users.

I will log it tomorrow during the day and check with some firefox users.

dr-dimitru commented 4 years ago

@jankapunkt, @menelike thank you for participating in this one @Lickshotz I have next ideas:

  1. If you believe it's related to nginx, please, make sure proxy_pass_request_headers on option is set in your configuration; And posting your nginx config file may help to silve this, perhaps;
  2. As mentioned by @jankapunkt there's a way to brake it with disabling cookies. For Cordova we implemented special option allowQueryStringCookies which is passed to Meteor-Cookies package emulating cookies via query string request; @jankapunkt, @s-ol, @menelike Shall we consider making it default for all environments?
menelike commented 4 years ago

@Lickshotz

I feel your pain bro ;). It had to debug those things so many times, its frustrating, but I always learnt a lot in the end. As already suggested the Nginx configuration might be the issue here, here is the Meteor-Files part from our config (though we use Nginx in docker only locally with self-created SSL certs):

    location /cdn/ {
        client_body_buffer_size 128k;

        proxy_pass http://app;
        proxy_redirect      off;
        proxy_set_header    Host              $host;
        proxy_set_header    X-Real-IP         $remote_addr;
        proxy_set_header    X-Forwarded-For   $proxy_add_x_forwarded_for;
        proxy_set_header    X-Forwarded-Proto $scheme;
        proxy_http_version 1.1;
    }

and this is our CSP (you need to remove a lot of stuff and replace ${PUBLIC_HOSTNAME})

"default-src 'self'; font-src data: 'self' https://fonts.gstatic.com; script-src 'self' 'unsafe-eval' 'unsafe-inline' data: https://www.google-analytics.com https://cdn.ravenjs.com https://www.googletagmanager.com https://tagmanager.google.com; connect-src filesystem: 'self' wss://${PUBLIC_HOSTNAME} https://sentry.io; img-src data: blob: filesystem: 'self' https://stats.g.doubleclick.net https://www.google-analytics.com https://ssl.gstatic.com https://www.gstatic.com; style-src 'self' 'unsafe-inline' https://tagmanager.google.com https://fonts.googleapis.com/; media-src data: blob: 'self' filesystem: https://${PUBLIC_HOSTNAME}; child-src blob: 'self';" always

Please keep in mind, that once you do scaling in Nginx you need to have sticky sessions, no matter what.

@dr-dimitru

thank you for participating in this one

🖖

Shall we consider making it default for all environments?

I highly advocate against that, this is a special dirty workaround designed for Meteor/Cordova, also I am not sure if this helps here at all.

s-ol commented 4 years ago

This shouldn't be the problem if it affects desktop Safari, but you should check the CORS headers coming out of and going into nginx if possible.

Shall we consider making it default for all environments?

I don't think so, it is an insecure way of doing it (sensitive data in query string is bad practice) and pollutes the browser URL history. Also the client has to programmed specifically with this in mind.

Lickshotz commented 4 years ago

@dr-dimitru @menelike Im certainly a total beginner when it comes to nginx, did most of it with a friend and some code snippets from issues in this repo, but im only slowly starting to understand what is happening here, so any help will be greatly appreciated.


Btw. After enabling logging in nginx and observing today I noticed that a lot of time a user will have the x_mtok cookie while beeing on /sockjs and it beeing gone once on /cdn.


I also found out about ostrio:logger logger today (awesome!) which I'm now using to logg the cookies to the server and I can confirm, that every user has a cookie once they get on a site where a video or an image is.


Here the nginx.conf

http {
    ##
    # Basic Settings
    ##

    map $http_upgrade $connection_upgrade {
            default upgrade;
            ''      close;
    }

    sendfile                        on;
    tcp_nopush                      on;
    tcp_nodelay                     on;
    keepalive_timeout               65;
    types_hash_max_size             2048;
    client_body_buffer_size         1M;

    server_names_hash_bucket_size   64;

    include /etc/nginx/mime.types;
    default_type application/octet-stream;

    ##
    # Logging Settings
    ##
    log_format custom   '$remote_addr[$time_local]'
                        '"$scheme $host $request" $status $body_bytes_sent "$http_referer" '
                        '"$http_user_agent" "$http_x_forwarded_for" '
                        '($request_time)'
                        '{$cookie_x_mtok}'
                        '$request_body_file';

    access_log /var/log/nginx/access.log custom;
    error_log /var/log/nginx/error.log;

    ##
    # Gzip Settings
    ##

    gzip on;

    # gzip_proxied expired no-cache no-store private auth;
    # gzip_comp_level 6;
    # gzip_buffers 16 8k;
    # gzip_http_version 1.1;
    # gzip_types text/plain text/css application/json application/javascript text/xml application/xml application/xml+rss text/javascript;

    server {
        listen 443 ssl; # managed by Certbot
        ssl_certificate /etc/letsencrypt/live/example.com/fullchain.pem; # managed by Certbot
        ssl_certificate_key /etc/letsencrypt/live/example.com/privkey.pem; # managed by Certbot
        include /etc/letsencrypt/options-ssl-nginx.conf; # managed by Certbot
        ssl_dhparam /etc/letsencrypt/ssl-dhparams.pem; # managed by Certbot

        server_name example.com www.example.com;

        client_max_body_size 1024M;
        add_header Strict-Transport-Security "max-age=15768000; includeSubDomains";
        add_header X-Content-Type-Options nosniff;
        add_header X-Frame-Options "SAMEORIGIN";
        add_header X-XSS-Protection "1; mode=block";
        add_header X-Robots-Tag none;
        add_header X-Download-Options noopen;
        add_header X-Permitted-Cross-Domain-Policies none;
        add_header Referrer-Policy "strict-origin";

        if ($http_user_agent ~ "MSIE" ) {
                return 303 https://browser-update.org/update.html;
        }

        access_log /var/log/nginx/example.com.access.log custom;
        error_log /var/log/nginx/example.com.error.log;

        location / {
            proxy_pass_request_headers      on;
            proxy_set_header Proxy "";
            proxy_set_header Upgrade $http_upgrade;
            proxy_set_header Connection "upgrade";
            proxy_http_version 1.1;
            proxy_set_header Host $host;
            real_ip_header X-Forwarded-For;
            real_ip_recursive on;
            proxy_pass http://127.0.0.1:3000;
            break;
        }

        location /sockjs/ {
            proxy_pass_request_headers      on;
            client_body_in_file_only        on;
            sendfile                        off;
            proxy_http_version              1.1;
            proxy_no_cache                  1;
            proxy_cache_bypass              1;
            proxy_set_header                Host $host;
            proxy_set_header                X-Real-IP $remote_addr;
            proxy_set_header                Upgrade $http_upgrade;
            proxy_set_header                Connection $connection_upgrade;
            proxy_set_header                X-Forwarded-Proto https;
            proxy_redirect                  off;
            proxy_pass                      http://127.0.0.1:3000;
        }

        location /cdn/ {
            proxy_pass_request_headers      on;
            client_body_in_file_only        on;
            sendfile                        off;
            proxy_http_version              1.1;
            proxy_no_cache                  1;
            proxy_cache_bypass              1;
            proxy_set_header                Host $host;
            proxy_set_header                X-Real-IP         $remote_addr;
            proxy_set_header                Upgrade $http_upgrade;
            proxy_set_header                Connection $connection_upgrade;
            proxy_set_header                X-Forwarded-Proto https;
            proxy_set_header                X-Forwarded-For   $proxy_add_x_forwarded_for;
            proxy_redirect                  off;
            proxy_pass                      http://127.0.0.1:3000;
        }
}

server {
    if ($host = www.example.com) {
        return 301 https://$host$request_uri;
    } # managed by Certbot

    if ($host = example.com) {
        return 301 https://$host$request_uri;
    } # managed by Certbot

    listen 80 default_server;

    server_name example.com www.example.com;
    return 404; # managed by Certbot
}
menelike commented 4 years ago

Can you slim the config for the beginning? e.g.

        add_header Strict-Transport-Security "max-age=15768000; includeSubDomains";
        add_header X-Content-Type-Options nosniff;
        add_header X-Frame-Options "SAMEORIGIN";
        add_header X-XSS-Protection "1; mode=block";
        add_header X-Robots-Tag none;
        add_header X-Download-Options noopen;
        add_header X-Permitted-Cross-Domain-Policies none;
        add_header Referrer-Policy "strict-origin";

are not crucial here. Your first goal should be to simplify the Nginx config as much as you can. Also please use my CDN config part from above as it is proven to be working.

Lickshotz commented 4 years ago

@menelike I slimmed it down as you suggested. and used your CDN config, while replacing http://app with http://127.0.0.1, right?

This did not do the trick.

menelike commented 4 years ago

@Lickshotz Just for orientation, I slimmed down our working configuration.

upstream app {
    server localhost:3000;
}

# HTTP
server {
    listen 80 default_server;
    listen [::]:80 default_server;
    server_name YOUR_SERVER_NAME;
    return 301 https://$server_name$request_uri;
}

# HTTPS server
server {
    listen 443 ssl http2;
    listen [::]:443 ssl http2;
    server_name YOUR_SERVER_NAME;

    # from https://mozilla.github.io/server-side-tls/ssl-config-generator/
    # certs sent to the client in SERVER HELLO are concatenated in ssl_certificate

    ssl_certificate /etc/nginx/certs/host.crt;
    ssl_certificate_key /etc/nginx/certs/host.key;
    ssl_session_timeout 1d;
    ssl_session_cache shared:SSL:50m;
    ssl_session_tickets off;

    # intermediate configuration. tweak to your needs.
    ssl_protocols TLSv1 TLSv1.1 TLSv1.2;
    ssl_ciphers 'ECDHE-ECDSA-CHACHA20-POLY1305:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-ECDSA-AES128-GCM-SHA256:ECDHE-RSA-AES128-GCM-SHA256:ECDHE-ECDSA-AES256-GCM-SHA384:ECDHE-RSA-AES256-GCM-SHA384:DHE-RSA-AES128-GCM-SHA256:DHE-RSA-AES256-GCM-SHA384:ECDHE-ECDSA-AES128-SHA256:ECDHE-RSA-AES128-SHA256:ECDHE-ECDSA-AES128-SHA:ECDHE-RSA-AES256-SHA384:ECDHE-RSA-AES128-SHA:ECDHE-ECDSA-AES256-SHA384:ECDHE-ECDSA-AES256-SHA:ECDHE-RSA-AES256-SHA:DHE-RSA-AES128-SHA256:DHE-RSA-AES128-SHA:DHE-RSA-AES256-SHA256:DHE-RSA-AES256-SHA:ECDHE-ECDSA-DES-CBC3-SHA:ECDHE-RSA-DES-CBC3-SHA:EDH-RSA-DES-CBC3-SHA:AES128-GCM-SHA256:AES256-GCM-SHA384:AES128-SHA256:AES256-SHA256:AES128-SHA:AES256-SHA:DES-CBC3-SHA:!DSS';
    ssl_prefer_server_ciphers on;

    location /cdn/ {
        client_body_buffer_size 128k;

        proxy_pass http://app;
        proxy_redirect      off;
        proxy_set_header    Host              $host;
        proxy_set_header    X-Real-IP         $remote_addr;
        proxy_set_header    X-Forwarded-For   $proxy_add_x_forwarded_for;
        proxy_set_header    X-Forwarded-Proto $scheme;
        proxy_http_version 1.1;
    }

    location / {
        proxy_pass http://app;
        proxy_redirect      off;
        proxy_set_header    Host              $host;
        proxy_set_header    X-Real-IP         $remote_addr;
        proxy_set_header    X-Forwarded-For   $proxy_add_x_forwarded_for;
        proxy_set_header    X-Forwarded-Proto $scheme;

        # WebSocket specific
        proxy_http_version 1.1;
        proxy_set_header    Upgrade           $http_upgrade;
        proxy_set_header    Connection        "upgrade";
    }
}

You must replace YOUR_SERVER_NAME and add proper cert paths. Maybe that will help?

Note: I don't have CSP in here, you might want to add that as well if it doesn't work

Lickshotz commented 4 years ago

@menelike Thank you alot for your effort.

I did do that, whilste on the second try stripping down the csp and adding them, without success.

After logging from the client to the database for the whole day, I can see that cookies.get('x_mtok') for some users returns a value while the protected function returns null, as well as cookies.get('x_mtok') returns undefined for other users while the protected function of course returns null as well (even though the cookie is confirmed set in the browser).

It seems that I should focus my attention back onto my application code again, considering that this nginx setup works fine for you. What do you think?

menelike commented 4 years ago

@Lickshotz I'd debug through https://github.com/VeliovGroup/Meteor-Files/blob/master/server.js#L1100-L1128 and try to understand what actually leads to the missing x-mtok. I'd try to understand what happens on the protocol level first before taking a look into your application.

Lickshotz commented 4 years ago

@menelike @dr-dimitru @s-ol @jankapunkt It looks like I have found the issue, while debugging as suggested, but I don't know if this is then actually an issue with nginx/specific browsers or the package.

Whoever acccess the website via example.com gets the cookie on the sever.

Once someone access the website via www.example.com has the cookie set in the browser, but will not have it on the server.

menelike commented 4 years ago

Just to be sure, can you confirm that users visiting www.example.com make requests to www.example.com and not to example.com. Apart from that, cookie security is a moving object targeted by many security policies/flags, I don't have a precise idea about the root cause. At least without a proper repro, I can't elaborate.

If you want to work around this issue, just redirect www.example.com to example.com

Lickshotz commented 4 years ago

Every request is made to www.example.com, apart from the one to /cdn, that one is to example.com.

Well, the repro above was having this issue in production as well.

For now I will redirect users from www.example.com to example.com.

menelike commented 4 years ago

Every request is made to www.example.com, apart from the one to /cdn, that one is to example.com.

Well, I think that this is exactly your problem. https://www.thinktecture.com/en/identity/samesite/samesite-in-a-nutshell/

On a general note, you should just use one domain if applicable to make your life easier. https://docs.meteor.com/environment-variables.html#ROOT-URL needs to match as well.

Lickshotz commented 4 years ago

As you suggested, redirecting incoming requests to example.com solved the issue.

Thank you!

jankapunkt commented 4 years ago

Maybe this is worth an entry in the wiki?

dr-dimitru commented 4 years ago

@jankapunkt sure, added in the latest release

armellarcier commented 1 year ago

Hi @dr-dimitru

First, I'm impressed how hard you tried to help the OP.

I have further hypothesis on this because it does happen to us, slightly differently.

The user's x_mtok cookie is sent and read server side.

But in certain cases, Meteor.server.sessions has a new session id for the user. Making the download request, sent with a previous sessionId value in its cookie, invalid.

In that case, I will end up with a null userId.

I'm trying to wrap my head around how this all works but a little help would be really appreciated.

So far, I've seen that Meteor.server.sessions will change on server restart (not always). I've also seen that if I close the app's tab, Meteor.server.sessions will not contain my sessionid anymore. I'll get a new one, setting a new cookie value, on next app page load.

armellarcier commented 1 year ago

@dr-dimitru really simple way to reproduce my issue :

dr-dimitru commented 1 year ago

Hello @armellarcier ,

Thank you for the feedback and reading this thread. This is expected behaviour for the session-based downloads. Opening new tab/window is a new session, and it's expected behaviour to end previous session.

What you need — token/tokenized download links, which can get implemented via downloadCallback() and/or protected hook(s).

Related discussion: https://github.com/veliovgroup/Meteor-Files/issues/500

armellarcier commented 1 year ago

Thx @dr-dimitru , indeed I will need to implement a custom authorization logic for downloads but relying on session ids only seems hacky.

As a quick workaround, I'll programmatically set the x_mtok cookie when the user clicks a download link.

Something like

const onClick = () => {
    const cookie = new Cookies({
        allowQueryStringCookies: false,
    });
    if (Meteor.connection._lastSessionId) {
        cookie.set('x_mtok', Meteor.connection._lastSessionId, {
            path: '/',
        });
    }
};

This is just temporary though.

Is there more info about how to implement a custom logic with getUser and protected out there that you know of ?

armellarcier commented 1 year ago

This is expected behaviour for the session-based downloads. Opening new tab/window is a new session, and it's expected behaviour to end previous session.

For clarification purposes:

Opening a new tab creates a new session. It does not end the previous session.

The "problem" lies in the fact that the cookie used for downloads is set by the last open session, and that session will be destroyed when its tab is closed, making download impossible even if another tab is still open.

About implementing custom logic :

I really think that putting the session_id or a login token in the url is a security issue. I think hiding the authorization logic from the user (in a cookie) is a good think. As users will share these urls eventually.

But I guess this should be discussed in another thread ?!

dr-dimitru commented 1 year ago

@armellarcier

I will need to implement a custom authorization logic for downloads but relying on session ids only seems hacky.

I really think that putting the session_id or a login token in the url is a security issue. I think hiding the authorization logic from the user (in a cookie) is a good think. As users will share these urls eventually.

You're free to set cookie not to a session_id but to some other value (e.g. token) stored on the user's profile, file's record, or somewhere else. Referenced thread and hooks are just for the example

But I guess this should be discussed in another thread ?!

We are good here, feel free to send PR to the documentation if necessary