search

internetstandards / Internet.nl

Internet standards compliance test suite
https://internet.nl
171 stars 35 forks source link

Fix #1274 - various nginx batch improvements #1286

Closed mxsasha closed 5 months ago

mxsasha commented 6 months ago

@bwbroersma I'm having some trouble getting batch auth apply to /, conditional on the batch setting. I bet I'll figure it out eventually, but you might be a bit more familiar with it? The templating is also really limited. The changes in the last commit definitely do not work right.

bwbroersma commented 5 months ago

@mxsasha sorry for the late reply:

  1. It's pretty tricky to use if correctly in nginx, see these examples 'why if is considered evil' in nginx location context, instead of if map can be used.
  2. env can be used directly if specified, which might be useful sometimes.
  3. Another option would be to use include, since the default nginx.conf is not overwritten, it already includes a include /etc/nginx/conf.d/*.conf; in the http-context, so it just works by adding a conf file, like authentication.sh is already doing.

I think BATCH and BASIC_AUTH should work similar, so I would go for option 3, but it's also an option to rewrite the BASIC_AUTH logic. It would be nice to add a hint about htpasswd and the link to authentication.sh, since it took me a few seconds the first time.

mxsasha commented 5 months ago

@bwbroersma any clue what I am missing in https://github.com/internetstandards/Internet.nl/pull/1286/commits/0c6e797b25416eedc8799a66c304c3e658a45478 ? I have confirmed that mime.types with my YAML line is in the container, and nginx.conf has include /etc/nginx/mime.types;