DJANGO_ALLOWED_HOSTS only allows for 1 host

[raph-topo]

Tag/version of Container Images
0.4.5

Bug

services:
  web:
    environment:
      - DJANGO_ALLOWED_HOSTS=host1.example.com,host2.example.com

results in error `Invalid HTTP_HOST header: 'host1.example.com'. You may need to add 'host1.example.com' to ALLOWED_HOSTS.

Reason

In \opt\mailman-web\settings.py

ALLOWED_HOSTS = [
    "localhost",  # Archiving API from Mailman, keep it.
    # "lists.your-domain.org",
    # Add here all production URLs you may have.
    "mailman-web",
    gethostbyname("mailman-web"),
    os.environ.get('SERVE_FROM_DOMAIN'),
    os.environ.get('DJANGO_ALLOWED_HOSTS'),
]

reads the env var as a string (instead of parsing a comma-separated list), which mangles the array ALLOWED_HOSTS:

['localhost',
'mailman-web',
'192.168.64.4',
'host1.example.com',
'host1.example.com,host2.example.com']

Workaround

Override in settings_local.py:

ALLOWED_HOSTS = ['localhost','mailman-web','host1.example.com','host2.example.com']

[maxking]

Yeah, it has always been a challenge to figure out how to do complex python types when using environment variables as the way to set values. I've seen comma , as the delimiter at various places now, so I feel it is okay to use it that way.

Do you want to work on a PR for the same? Do keep in mind that we want to make sure that we don't break the current settings.

[raph-topo]

Here's how Healthchecks.io, also based on Django, did it:

ALLOWED_HOSTS = os.getenv("ALLOWED_HOSTS", "*").split(",")

https://github.com/healthchecks/healthchecks/blob/34530e0e917243eab8bf01ce9ea7bb2aaf747bae/hc/settings.py#L35
Would that fit?

[maxking]

That works, but we need to add the existing options too.

I am thinking something like:

ALLOWED_HOSTS = [
    "localhost",  # Archiving API from Mailman, keep it.
    # "lists.your-domain.org",
    # Add here all production URLs you may have.
    "mailman-web",
    gethostbyname("mailman-web"),
    os.environ.get('SERVE_FROM_DOMAIN'),
]

ALLOWED_HOSTS.extend(os.getenv("ALLOWED_HOSTS", "").split(","))

I think it should work (gotta check the default case, I don't want to use "*" as the default.

[raph-topo]

I do agree, * default makes having this "protective" setting useless by default.
I am wondering though about hard-coding the container name, as someone reusing the docker-compose.yml while changing service names would definitely overlook this setting here. (But maybe that's an edge case you accept.)

[maxking]

I am wondering though about hard-coding the container name, as someone reusing the docker-compose.yml while changing service names would definitely overlook this setting here. (But maybe that's an edge case you accept.)

Someone going to just change the names will have some fun discovering the issues, but there are ways to configure ALLOWED_HOSTS and other configs in Core to talk to mailman-web, so I don’t think it should be an issue. Keeping default helps simplify things IMO.