Fix trailing slash not getting removed from domain

[BlockListed]

Bitwarden send won't work if the domain includes a trailing slash. This should be documented, as it may lead to confusion among users.

[BlackDex]

Could you explain what is the issue? Because it works just fine for me, with, without a / at the end. Same goes for using port numbers or sub-path, or a combination, and all with or without a trailing /.

[BlockListed]

Could you explain what is the issue? Because it works just fine for me, with, without a / at the end. Same goes for using port numbers or sub-path, or a combination, and all with or without a trailing /.

If you have a trailing slash, the download link for the send has two slashes after the URL https://your.domain.with.trailing//api/sends/..., which caused issues with CSP for me yesterday. So I thought it may be useful to document.

[BlackDex]

But, as just mentioned, i tested it, and it doesn't happen for me at all.
How do you change the DOMAIN, just via env? or an other way?

[BlackDex]

What version of Vaultwarden are you using?

[BlockListed]

In the admin panel, if you set a domain with a trailing slash (general settings -> domain url), the /sends/<send_id>/access/file/<file_id> handler will return a url with two slashes after the domain, https://your.domain.with.trailing//api/sends/.... This issues seems to stem from the fact the the Host parameter doesn't remove the trailing slash.
https://github.com/dani-garcia/vaultwarden/blob/main/src/auth.rs#L278

#[rocket::async_trait]
impl<'r> FromRequest<'r> for Host {
    type Error = &'static str;

    async fn from_request(request: &'r Request<'_>) -> Outcome<Self, Self::Error> {
        let headers = request.headers();

        // Get host
        let host = if CONFIG.domain_set() {
            // Current logic
            CONFIG.domain()
            
            // Possible fix
            CONFIG.domain().trim_end_matches("/")
        } else {
            ...
    }
}

[BlockListed]

What version of Vaultwarden are you using?

My web interface is Version 2022.12.0, but reading the code suggests, that the issue may still exist.
I have to do something else now, so can't check my server version.

[BlackDex]

In the later versions of the web-vault they have changed the endpoint if I'm correct.
So this issue shouldn't be there anymore.

What happens with attachments then?

[BlockListed]

In the later versions of the web-vault they have changed the endpoint if I'm correct. So this issue shouldn't be there anymore.

What happens with attachments then?

I checked and I am running the newest version, but that's beside the point. In my other comment (#3228 (comment)) I'm pretty sure I showed how this issue is present in the current master branch.

[BlackDex]

But, i can't reproduce it at all!, i can use

• https://vaultwarden.domain.tld/
• https://vaultwarden.domain.tld
• https://vaultwarden.domain.tld:8080/
• https://vaultwarden.domain.tld:8080
• https://vaultwarden.domain.tld/sub-path
• https://vaultwarden.domain.tld/sub-path/
• https://vaultwarden.domain.tld:8080/sub-path/
• https://vaultwarden.domain.tld:8080/sub-path

And they all work just fine!

[BlackDex]

See if you can reproduce it with the testing tagged image and if that solves your issue. Because if so, then it is already fixed in a not yet released latest image.

[BlockListed]

See if you can reproduce it with the testing tagged image and if that solves your issue. Because if so, then it is already fixed in a not yet released latest image.

I managed to show it returning a URL with two slashes in this video. I know the download didn't fail, but I have experienced the double-slashing causing some issues with CSP and I hope we can agree, that it's not intended behavior. The commits I added would fix this issue.

[BlackDex]

See if you can reproduce it with the testing tagged image and if that solves your issue. Because if so, then it is already fixed in a not yet released latest image.

I managed to show it returning a URL with two slashes in this video. I know the download didn't fail, but I have experienced the double-slashing causing some issues with CSP and I hope we can agree, that it's not intended behavior. The commits I added would fix this issue.

Ok, i see the double slashes there indeed. But then comes my next question. How does that causes a CSP issue?
Do you provide your own CSP? Because the CSP provided by Vaultwarden is working just fine out of the box.
If you provide your own CSP that could causes issues with some features of Bitwarden or maybe in the future.
The same goes for origin headers etc...

[BlackDex]

And, besides it having a slash there, i think we should rather solve this in a better way then adjusting the comments. Always trimming a slash is maybe not the best solution.

And there is still an item open to better handle the HOST for items like send, attachments and 2fa where possible. Which i my self didn't had any time for to look at.

[BlockListed]

And, besides it having a slash there, i think we should rather solve this in a better way then adjusting the comments. Always trimming a slash is maybe not the best solution.

And there is still an item open to better handle the HOST for items like send, attachments and 2fa where possible. Which i my self didn't had any time for to look at.

Where is the item you mentioned?
Also I would say, that a host header / value, should never have a trailing slash, maybe it should log a warning if there is a trailing slash, but still fix it. I know it's a little hacky having it in the host function, but I don't know if there's a way to have it directly in the configuration system.

[BlackDex]

The issue is this one: #2690
It currently doesn't support multiple domains, also for some security reasons of course, just always blindingly using the HOST could be a security issue in my eyes.

Further, maybe we should add a different function to get a parsed domain, which uses domain_origin and domain_path.
Also, parsing or modifying could be done during the config load, which then prevents a trim or whatever being done on every request which causes a new memory allocation and more processing time.

There are maybe some more optimizations to be done there, but as mentioned, i haven't had time my self yet to really dive into it.

[BlockListed]

We could use a domain "allow list" instead of a domain URL, which would alleviate some security concerns and be identical for a user, using a correct configuration, to the current system.

I think parsing could be done at request time, since the trim itself doesn't need another allocation (the current implementation always allocates/clones the domain). I only created a new string, because the Host struct requires a string and I didn't want to deal with changing so much.

But that should maybe be done in a different PR.

[BlockListed]

After thinking about it, why shouldn't we remove the trailing slash. For the domain URL ,it's perfectly normal to have a trailing slash, but for a host it's not really intended. The backup method grabs the domain part from the Host header, which according to MDN shouldn't have a trailing slash. Removing a trailing slash should therefor be acceptable, since we're translating from a format which allows trailing slashes, to one which does not. Removing the trailling slash in the Host struct makes the most sense, since the domain is used to derive other values, which will result in a much greater surface for bugs.

[BlackDex]

Again, if removing, then i would suggest to do something like that during the config parse run, and not with every request.
Why do extra work in one location if it probably is best if it is removed from all places where this variable is being used?

Then it would be much better to have this done within the config code, and all locations using CONFIG.domain() are fixed instantly.

[BlockListed]

Again, if removing, then i would suggest to do something like that during the config parse run, and not with every request. Why do extra work in one location if it probably is best if it is removed from all places where this variable is being used?

Then it would be much better to have this done within the config code, and all locations using CONFIG.domain() are fixed instantly.

I'm just wondering, fix the trailing slash with a warning or deny domains with trailing slashes in validate_config()?

[BlackDex]

Again, if removing, then i would suggest to do something like that during the config parse run, and not with every request. Why do extra work in one location if it probably is best if it is removed from all places where this variable is being used?
Then it would be much better to have this done within the config code, and all locations using CONFIG.domain() are fixed instantly.

I'm just wondering, fix the trailing slash with a warning or deny domains with trailing slashes in validate_config()?

Fix it automatically for them. Just sanitize it I think

[BlockListed]

Other similar things are being fixed in the build() method of ConfigBuilder, so I am gonna decide to implement these fixes there.

config.signups_domains_whitelist = config.signups_domains_whitelist.trim().to_lowercase();
config.org_creation_users = config.org_creation_users.trim().to_lowercase();

[BlockListed]

It should be fine now. The domain simply gets sanitized and the trailing slash gets removed. I decided to not pop the slash off in-place, because the readability of the trim feels better and a single allocation at startup won't kill us.