-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Replace HOST_URL by Host header #5195
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
base: master
Are you sure you want to change the base?
Conversation
Just asking out of curiosity: This would change something from a know source to an "attacker controlled" source, right? Does this need any additional sanitizing? |
As far as I know, no. I may be wrong, but I can't think of any way to exploit this. Ref: https://crystal-lang.org/reference/1.15/tutorials/basics/40_strings.html#interpolation |
I was more thinking of something like having a newline in there :) not just invalid binary stuff, just saw that "external controlled" data gets written directly back, just wanted to make sure |
Having to check if the cookie is inside a list of allowed domains on invidious doesn't seem really useful because a reverse proxy like NGINX and HAProxy will only send the client request to Invidious if the Host header that the client sent to the server, matches with the `hdr(host)` (haproxy) or `server_name` (nginx) set by the server configuration.
From my previous research on this subject, many implementations have a "trust 'Host' header" option for that purpose, as well as a "Trust the first N headers" to allow more than one level of proxying, and make sure that none of the attacker-controlled headers gets parsed. Though some sanitization never hurts.
To override other header, the usual way is by sending But you can also do some XSS injections! @Fijxu Some places like the thumbnails would require a significative rewrite, as they never see the HTTP context of the request (and worse, it ends up in the cache): invidious/src/invidious/videos.cr Lines 362 to 374 in 0c07e9d
|
Currently, each invidious process is fixed to it's
domain
config value which makes it really difficult for anyone to host Invidious using multiple domains, Tor and I2P.If someone wants to host Invidious on Tor, a dedicated Invidious instance for it needs to be made setting
domain
to the Tor address, and the same goes for I2P.On some places,
HOST_URL
is still used and it shouldn't be changed, like for example:invidious/src/invidious/helpers/utils.cr
Line 297 in adcdb8c
If this get merged when it's finished, it shouldn't break the setup of anyone using NGINX since the invidious documentation already mentions the use of the Host header on the NGINX configuration: https://github.com/iv-org/documentation/blob/65ecfccb10f15b4b9ce95c03b0d6d6ebd88bca7a/docs/nginx.md?plain=1#L24
Consider this a WIP since it's a big change that could break a lot of things, however, most of the changes made on the first commit are already applied to my Invidious fork at https://git.nadeko.net/Fijxu/invidious for a very very long time already, multiple domains works fine and Tor works fine.