monicahq / monica

Personal CRM. Remember everything about your friends, family and business relationships.
https://beta.monicahq.com
GNU Affero General Public License v3.0
21.45k stars 2.14k forks source link

APP_TRUST_PROXIES middleware still broken resulting in bad URLs #6024

Closed neunenak closed 2 years ago

neunenak commented 2 years ago

I'm still seeing exactly the same problem as https://github.com/monicahq/monica/issues/5954 even after upgrading to a version of monica:fpm that includes that fix.

I spent some time looking at the code and googling, and I think https://stackoverflow.com/questions/40722331/laravel-asset-method-doesnt-return-https is the fundamental problem. I'm definitely sending correct Host and X-Forwarded-Proto: https headers - I'm not sure if the code in https://github.com/monicahq/monica/pull/5955/files actually fixes this - there's this project: https://github.com/fideloper/TrustedProxy that implements a Laraval middleware called TrustedProxy that looks more complicated than the one in the above pull request. I'm not super-knowledgable about php, but I can't tell how the middleware included in monica is supposed to tell that it needs to look at the X-Forwarded-Proto header to have the asset function generate correct URLs.

Also the documentation in https://github.com/monicahq/monica/blob/main/docs/installation/ssl.md is out of date, since it refers to the APP_TRUSTED_PROXIES variable which doesn't work.

solarchemist commented 2 years ago

Had the exact same issue. Changing APP_ENV to production fixed it. It seems the old Monica advice about only using local in combination with a TLS reverse proxy is no longer valid.

As long as APP_ENV=production, it did not matter whether APP_TRUSTED_PROXIES=* or APP_TRUSTED_PROXIES=<ip-addresses>, at least in my tests.

I have written an Ansible role where I put an example reverse proxy vhost config and related stuff. Monica 3.7.0, Apache 2.4.41, Ubuntu 20.04.

neunenak commented 2 years ago

Had the exact same issue. Changing APP_ENV to production fixed it. It seems the old Monica advice about only using local in combination with a TLS reverse proxy is no longer valid.

As long as APP_ENV=production, it did not matter whether APP_TRUSTED_PROXIES=* or APP_TRUSTED_PROXIES=<ip-addresses>, at least in my tests.

I have written an Ansible role where I put an example reverse proxy vhost config and related stuff. Monica 3.7.0, Apache 2.4.41, Ubuntu 20.04.

Hm, I do have APP_ENV=production and that's been doing nothing for me.

solarchemist commented 2 years ago

Yeah, that's weird. I'm not using Docker, though, maybe something is going on inside those containers. I don't know if it helps, but here's my .env (which might be hard to glean from the Ansible role without actually running it), for comparison, in the hopes it helps:

# Welcome, friend ❤. Thanks for trying out Monica. We hope you'll have fun.
APP_ENV=production
APP_DEBUG=false
APP_KEY=<secretkey>
HASH_SALT=<salt+32>
HASH_LENGTH=18
APP_URL=https://monica.example.se
DB_CONNECTION=mysql
DB_HOST=127.0.0.1
DB_PORT=3306
# I use socket, which overrides DB_HOST and DB_PORT values
DB_UNIX_SOCKET=/var/run/mysqld/mysqld.sock
DB_DATABASE=monica
DB_USERNAME=monica
DB_PASSWORD=<secretpass>
DB_PREFIX=
DB_TEST_HOST=127.0.0.1
DB_TEST_DATABASE=monica_test
DB_TEST_USERNAME=homestead
DB_TEST_PASSWORD=secret
DB_USE_UTF8MB4=true
MAIL_DRIVER=smtp
MAIL_HOST=smtp.gmail.com
MAIL_PORT=587
MAIL_USERNAME=mailer@example.se
MAIL_PASSWORD=<password>
MAIL_ENCRYPTION=null
MAIL_FROM_ADDRESS=mailer@example.se
MAIL_FROM_NAME="Monica on Giza"
APP_EMAIL_NEW_USERS_NOTIFICATION=webmaster@example.se
APP_DEFAULT_LOCALE=en
APP_DISABLE_SIGNUP=true
APP_SIGNUP_DOUBLE_OPTIN=false
APP_TRUSTED_PROXIES=10.252.116.1,192.168.1.2,192.168.1.1
APP_TRUSTED_CLOUDFLARE=false
LOG_CHANNEL=daily
SENTRY_SUPPORT=false
SENTRY_LARAVEL_DSN=
CHECK_VERSION=true
CACHE_DRIVER=database
SESSION_DRIVER=file
SESSION_LIFETIME=120
# "sync" (default) will bypass the queues entirely (no async, no need for redis)
QUEUE_DRIVER=sync
DEFAULT_MAX_UPLOAD_SIZE=10240
DEFAULT_MAX_STORAGE_SIZE=512
DEFAULT_FILESYSTEM=public
AWS_KEY=
AWS_SECRET=
AWS_REGION=us-east-1
AWS_BUCKET=
AWS_SERVER=
MFA_ENABLED=true
DAV_ENABLED=false
MOBILE_CLIENT_ID=
MOBILE_CLIENT_SECRET=
ALLOW_STATISTICS_THROUGH_PUBLIC_API_ACCESS=false
POLICY_COMPLIANT=false
REQUIRES_SUBSCRIPTION=false
NUMBER_OF_ALLOWED_CONTACTS_FREE_ACCOUNT=10
STRIPE_KEY=
STRIPE_SECRET=
PAID_PLAN_MONTHLY_FRIENDLY_NAME=
PAID_PLAN_MONTHLY_ID=
PAID_PLAN_MONTHLY_PRICE=
PAID_PLAN_ANNUAL_FRIENDLY_NAME=
PAID_PLAN_ANNUAL_ID=
PAID_PLAN_ANNUAL_PRICE=
ENABLE_GEOLOCATION=true
LOCATION_IQ_API_KEY=<apikey>
ENABLE_WEATHER=true
DARKSKY_API_KEY=<apikey>
github-actions[bot] commented 1 year ago

:tada: This issue has been resolved in version 4.0.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

github-actions[bot] commented 7 months ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.