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

Rework Caddyfile #255

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Rework Caddyfile #255

wants to merge 5 commits into from

Conversation

inetol
Copy link
Contributor

@inetol inetol commented Aug 6, 2024

This PR completely reworks the Caddy config file to solve several inconveniences that are not worth having them in separate PRs, the changes are these...

  • Try to protect the user data (query searches, IPs and headers) logged on Caddy if there is an error with the upstream response (or Caddy itself).
  • Overwrite the Cache-Control header if its matcher token matches rather than doing the comparison twice in a row.
  • Prefer to set the ENV defaults directly in the Caddyfile.
  • Modified the Strict-Transport-Security header to not include all subdomains in the HSTS policy nor mark the latter to enter the HSTS preload list.
  • Modified the Permissions-Policy header to remove useless features.
  • Remove the X-XSS-Protection and Feature-Policy headers following the standards recommendations and evaluating the possible impact with older clients.
  • Other minor structure and format changes.

All changes have been tested and validated without requiring major changes for users.

inetol added 4 commits August 6, 2024 12:50
Since this header is attached to each outgoing request the overall size is slightly increased by 300 bytes, we don't care if the site is allowed to play media in the background, so with the vast majority of features that have been added (and those that were already present)
Caddyserver actually did passthrough the precompressed resources served by uWSGI (Oops)
log {
output discard
}
{$SEARXNG_HOSTNAME:http://localhost}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caddy is able to read from the .env file?

Copy link
Contributor Author

@inetol inetol Jan 3, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably?, if used from Docker Compose should load all the variables from there, it also supports default value fallback https://caddyserver.com/docs/caddyfile/concepts#environment-variables

@inetol
Copy link
Contributor Author

inetol commented Jan 3, 2025

I note as I almost forgot this PR existed, the default CSP is broken and forced you to use the image_proxy all the time, also some cache changes to try to reduce the number of requests to the server (specially /image_proxy)

@unixfox
Copy link
Member

unixfox commented Jan 3, 2025

the default CSP is broken and forced you to use the image_proxy all the time, also some cache changes to try to reduce the number of requests to the server (specially /image_proxy)

That's indeed an issue. We should allow images to load without the image_proxy in case the user has disabled it.

I kinda wish SearXNG would handle that by itself instead of having to set the CSP on the reverse proxy. Invidious does it natively: https://github.com/iv-org/invidious/blob/98926047586154269bb269d01e3e52e60e044035/src/invidious/routes/before_all.cr#L40-L52

I note as I almost forgot this PR existed

Feel free to tell me if you want to continue this PR or not. If not, please close it.

@return42
Copy link
Member

return42 commented Jan 3, 2025

I kinda wish SearXNG would handle that by itself instead of having to set the CSP ..

@inetol
Copy link
Contributor Author

inetol commented Jan 3, 2025

That's indeed an issue. We should allow images to load without the image_proxy in case the user has disabled it.

cf3b060 commit allows the CSP policy to load all images from all sources to be loaded, it's kinda meh, but I don't know any better way to do it, putting all URLs is not an option

Feel free to tell me if you want to continue this PR or not. If not, please close it.

No problem, the PR is ready

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