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

Add optional CORS header middleware #2000

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

bgervan
Copy link

@bgervan bgervan commented Aug 16, 2024

Optional CORS header middleware with configs like:

[cors]
enabled = true
allowed_origins = ["example.com"]

or with environment variable like (in docker compose for example):

  app:
    <<: *app-defaults
    container_name: listmonk_app
    command: [ sh, -c, "yes | ./listmonk --config config.toml" ]
    depends_on:
      - db
    volumes:
      - ./config.toml:/listmonk/config.toml
    environment:
      - LISTMONK_cors__enabled=true
      - LISTMONK_cors__allowed_origins__0=*
      - LISTMONK_cors__allowed_origins__1=http://localhost:3000
      - LISTMONK_cors__allowed_origins__2=google.com
      - LISTMONK_cors__allowed_origins__2=another.com

Note: the google.com is overriden with another.com in this example.

With enabled false, everything works as before, so it doesn't break backward the compatibility. Without array of origins, it shows a warning, but works like cors disabled.

Inspiration for this PR: #1521

@knadh
Copy link
Owner

knadh commented Aug 18, 2024

Thanks for the PR @bgervan, but I don't think it's ideal for CORS configuration to be in listmonk. CORS, TLS and any other HTTP configuration is best handled in the reverse proxy that you would be using to expose listmonk to the internet, like Nginx or Caddy.

Please see my comment on the old issue: #1523 (comment)

@bgervan
Copy link
Author

bgervan commented Aug 18, 2024

Thanks for the PR @bgervan, but I don't think it's ideal for CORS configuration to be in listmonk. CORS, TLS and any other HTTP configuration is best handled in the reverse proxy that you would be using to expose listmonk to the internet, like Nginx or Caddy.

Please see my comment on the old issue: #1523 (comment)

I saw that, but it's very common to handle this on application side (in django for example).
Also on caprover the same issue mentions that it's better to handle on app side not in the nginx config. That comment made me create it optional.

In pikapod, it blocks the usage as they are not supporting cors configuration.

@knadh
Copy link
Owner

knadh commented Aug 18, 2024

Django is a framework for building web applications, so it's natural for it to have configuration for a wide variety of HTTP related things. In fact, it is very uncommon for "consumer" web applications like listmonk to have CORS configuration.

listmonk does have built in SSL/TLS either and fully relies on a reverse proxy to provide it. Also, except for a couple of /api/public/* handlers, every other endpoint is private and meant to be used only with authentication.

that it's better to handle on app side not in the nginx config

That would be incorrect. A reverse proxy like Nginx is the best place to handle config like TLS, CORS etc. transparently without having every web application build support for those.

@bgervan
Copy link
Author

bgervan commented Aug 18, 2024

It still gives extra complexity to run nginx beside listmonk. Local docker compose too, but in case of cloudflare tunnel the ssl is handled by cloudflare, but for cors only I would need to tun a reverse proxy as well and point my tunnel to that.

Pikapod is a cheap way to run it without needing to manage, but with custom integrated form, we cannot use it.

@candidexmedia
Copy link

Pikapod is a cheap way to run it without needing to manage, but with custom integrated form, we cannot use it.

Is this also an issue with Railway.app?

@bgervan
Copy link
Author

bgervan commented Aug 19, 2024

Pikapod is a cheap way to run it without needing to manage, but with custom integrated form, we cannot use it.

Is this also an issue with Railway.app?

I am not familiar much with Railway, but if you use the docker image for the deployment without an nginx, yes.

https://docs.railway.app/guides/create

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

Successfully merging this pull request may close these issues.

3 participants