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

Not considering the situation when the user domain name starts with “admin”. #3415

Closed
fxzxmic opened this issue Apr 3, 2023 · 17 comments · Fixed by #3436
Closed

Not considering the situation when the user domain name starts with “admin”. #3415

fxzxmic opened this issue Apr 3, 2023 · 17 comments · Fixed by #3436
Labels
bug Something isn't working enhancement New feature or request

Comments

@fxzxmic
Copy link

fxzxmic commented Apr 3, 2023

Subject of the issue

The file "admin.js" contains the following code:

function getBaseUrl() {
    // If the base URL is `https://vaultwarden.example.com/base/path/`,
    // `window.location.href` should have one of the following forms:
    //
    // - `https://vaultwarden.example.com/base/path/`
    // - `https://vaultwarden.example.com/base/path/#/some/route[?queryParam=...]`
    //
    // We want to get to just `https://vaultwarden.example.com/base/path`.
    const baseUrl = window.location.href;
    const adminPos = baseUrl.indexOf("/admin");
    return baseUrl.substring(0, adminPos != -1 ? adminPos : baseUrl.length);
}

If the domain name is abc.com, the function returns https://abc.com.
But if the domain name is admin.abc.com or just adminxxx.com, the function returns https:/. This is not the expected value.

Additional statements

In addition, I hope you can consider the following discussion when fixing this issue. It is possible not to provide relevant functions, but please do not undermine the currently feasible workarounds.
#1494 (comment)

If my domain name is abc.com, in my use case, my background address is https://admin.abc.com/vault.

@BlackDex
Copy link
Collaborator

BlackDex commented Apr 3, 2023

So, you internally redirect admin. to /admin?
I'm not sure if we can easily resolve this. It's also a corner case in my opinion.

Since this is a feature request, i tend to move this to the discussions > ideas.

@fxzxmic

This comment was marked as off-topic.

@BlackDex
Copy link
Collaborator

BlackDex commented Apr 3, 2023

Could you provide your reverse proxy config? And what you configured as your DOMAIN string?

And, provide an answer to my question above please too.

@fxzxmic
Copy link
Author

fxzxmic commented Apr 3, 2023

The additional statement has the meaning of some feature request, but the previous part was a bug.
I think you should use window.location.protocol, window.location.host, etc in the function getBaseUrl().

@fxzxmic

This comment was marked as duplicate.

@fxzxmic
Copy link
Author

fxzxmic commented Apr 3, 2023

So, you internally redirect admin. to /admin? I'm not sure if we can easily resolve this. It's also a corner case in my opinion.

Yes, you are right. Everything worked fine in the previous version. The newly introduced admin.js messed up everything.

@BlackDex
Copy link
Collaborator

BlackDex commented Apr 3, 2023

Please let me finish speaking first. I think you should use window.location.protocol, window.location.host, etc in the function getBaseUrl().

You can put everything in a single comment 😉.
Still, i need more info/details to reproduce this correctly.

@stefan0xC
Copy link
Contributor

stefan0xC commented Apr 3, 2023

@BlackDex If you have DOMAIN=https://admin.example.com and access https://admin.example.com/admin

const adminPos = baseUrl.indexOf("/admin");

will find the first /admin instead of the second.

So we should probably use lastIndexOf instead of indexOf to find the BASE_URL

baseUrl = "https://admin.example.com/admin"
"https://admin.example.com/admin"
baseUrl.indexOf('/admin')
7
baseUrl.lastIndexOf('/admin')
25 

@fxzxmic
Copy link
Author

fxzxmic commented Apr 3, 2023

Could you provide your reverse proxy config? And what you configured as your DOMAIN string?

  location /vault {
    auth_basic "Admin Login";
    auth_basic_user_file /data/nginx-server/htpasswd.conf;

    sub_filter "/vw_static" "//abc.com/vw_static";
    sub_filter "/admin/" "/vault/";
    sub_filter "\"/admin\"" "\"/vault\"";
    sub_filter "class=\"nav-link\" href=\"/\"" "style=\"display: none;\" href=\"/\"";
    sub_filter "class=\"btn btn-sm btn-secondary\"" "style=\"display: none;\"";
    sub_filter_once off;

    proxy_pass http://localhost:80/admin/;
    proxy_set_header Host abc.com;
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header X-Forwarded-Proto $scheme;
    more_set_headers "Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline' abc.com; style-src 'self' 'unsafe-inline' abc.com; img-src 'self' data: abc.com https://haveibeenpwned.com/ https://www.gravatar.com ; child-src 'self' https://*.duosecurity.com https://*.duofederal.com; frame-src 'self' https://*.duosecurity.com https://*.duofederal.com; connect-src 'self' https://api.pwnedpasswords.com/range/ https://2fa.directory/api/ https://app.simplelogin.io/api/ https://app.anonaddy.com/api/ https://relay.firefox.com/api/; object-src 'self' blob:; frame-ancestors 'self' chrome-extension://nngceckbapebfimnlniiiahkandclblb chrome-extension://jbkfoedolllekgbhcbcoahefnbanhhlh moz-extension://* ;";
  }

I replaced my front-end domain name with abc.com. My backend address is https://admin.abc.com/vault.

@BlackDex
Copy link
Collaborator

BlackDex commented Apr 3, 2023

Ah, well, the that is clear info, thx @stefan0xC .
We do indeed need to address that.

And we did need the extra info, since the OP removes /admin also.
With this data we could try and reproduce the same behavior.

@fxzxmic
Copy link
Author

fxzxmic commented Apr 3, 2023

@BlackDex If you have DOMAIN=https://admin.example.com and access https://admin.example.com/admin

const adminPos = baseUrl.indexOf("/admin");

will find the first /admin instead of the second.
So we should probably use lastIndexOf instead of indexOf to find the BASE_URL

baseUrl = "https://admin.example.com/admin"
"https://admin.example.com/admin"
baseUrl.indexOf('/admin')
7
baseUrl.lastIndexOf('/admin')
25 

In my use case, there is no /admin in the path at all. That's why I said please consider some special situations when fixing.

@fxzxmic
Copy link
Author

fxzxmic commented Apr 3, 2023

Ah, well, the that is clear info, thx @stefan0xC . We do indeed need to address that.

And we did need the extra info, since the OP removes /admin also. With this data we could try and reproduce the same behavior.

I think you understand my situation now, thank you very much.

At present, it seems that you don't need to fix anything to the main program, except for admin.js and anywhere the BASE_URL variable is used.

@BlackDex BlackDex added bug Something isn't working enhancement New feature or request labels Apr 3, 2023
@fxzxmic
Copy link
Author

fxzxmic commented Apr 3, 2023

_post(`${BASE_URL}/admin/organizations/${org_uuid}/delete`,

I don't understand why not just use it this way:
_post(`organizations/${org_uuid}/delete`,

@BlackDex
Copy link
Collaborator

BlackDex commented Apr 3, 2023

_post(`${BASE_URL}/admin/organizations/${org_uuid}/delete`,

I don't understand why not just use it this way: _post(`organizations/${org_uuid}/delete`,

That doesn't work as far as i know, since fetch needs a FQDN.
And, even if it does not need a FQDN, it would be an issue, since we need it relative to /admin, some links might not go to there respective page.

Anyways, i think we have the right info to try and see if we can fix this.

@BlackDex
Copy link
Collaborator

BlackDex commented Apr 4, 2023

I did some quick testing. And the problem more lays in that you do not use sub_filter on the new *.js files.
If you do that, your problem would be solved for your specific use-case.

So, if you use this for example, that should solve your issues.

  location /vault {
    proxy_pass http://127.0.0.1:8080/admin;
    proxy_set_header Host $host;
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header X-Forwarded-Proto $scheme;

    # Replace cookie path so authentication still works
    proxy_cookie_path /admin /vault;

    sub_filter "/vw_static" "//$host/vw_static";
    sub_filter "/admin/" "/vault/";
    sub_filter "\"/admin\"" "\"/vault\"";
    sub_filter_once off;

    more_set_headers "Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline'; style-src 'self' 'unsafe-inline' abc.com; img-src 'self' data: abc.com https://haveibeenpwned.com/ https://www.gravatar.com ; child-src 'self' https://*.duosecurity.com https://*.duofederal.com; frame-src 'self' https://*.duosecurity.com https://*.duofederal.com; connect-src 'self' https://api.pwnedpasswords.com/range/ https://2fa.directory/api/ https://app.simplelogin.io/api/ https://app.anonaddy.com/api/ https://relay.firefox.com/api/; object-src 'self' blob:; frame-ancestors 'self' chrome-extension://nngceckbapebfimnlniiiahkandclblb chrome-extension://jbkfoedolllekgbhcbcoahefnbanhhlh moz-extension://* ;";
  }

  location /vw_static {
    proxy_pass http://127.0.0.1:8080/vw_static;
    proxy_set_header Host $host;
    proxy_set_header X-Real-IP $remote_addr;
    proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
    proxy_set_header X-Forwarded-Proto $scheme;

    sub_filter_types "application/javascript"
    sub_filter "/vw_static" "//$host/vw_static";
    sub_filter "/admin/" "/vault/";
    sub_filter "\"/admin\"" "\"/vault\"";
    sub_filter_once off;

    more_set_headers "Content-Security-Policy: default-src 'self'; script-src 'self' 'unsafe-inline' admin.bwrsdev.vyus.nl; style-src 'self' 'unsafe-inline' abc.com; img-src 'self' data: abc.com https://haveibeenpwned.com/ https://www.gravatar.com ; child-src 'self' https://*.duosecurity.com https://*.duofederal.com; frame-src 'self' https://*.duosecurity.com https://*.duofederal.com; connect-src 'self' https://api.pwnedpasswords.com/range/ https://2fa.directory/api/ https://app.simplelogin.io/api/ https://app.anonaddy.com/api/ https://relay.firefox.com/api/; object-src 'self' blob:; frame-ancestors 'self' chrome-extension://nngceckbapebfimnlniiiahkandclblb chrome-extension://jbkfoedolllekgbhcbcoahefnbanhhlh moz-extension://* ;";
  }

I'm not planning on adding something flexible to change the /admin path it self. That is to much hassle in my opinion, and probably causes more issues then it would solve.

I Already have a fix so it will not replace the starting /admin locally on my laptop, but not yet pushed.

@fxzxmic
Copy link
Author

fxzxmic commented Apr 5, 2023

I did some quick testing. And the problem more lays in that you do not use sub_filter on the new *.js files. If you do that, your problem would be solved for your specific use-case.

So, if you use this for example, that should solve your issues.

Thanks.

@fxzxmic
Copy link
Author

fxzxmic commented Apr 5, 2023

I found an issue when I specified disable_admin_token in the configuration file config.json, the disable_admin_token item will be removed from the configuration file every time I save the settings.

Update: Actually, I noticed that many settings are reset to default values after every time I save the settings.

BlackDex added a commit to BlackDex/vaultwarden that referenced this issue Apr 10, 2023
- Fixed issue with domains starting with `admin`
- Fixed issue with DUO not being enabled globally anymore (regression)
- Renamed `Ciphers` to `Entries` in overview
- Improved `ADMIN_TOKEN` description
- Updated jquery-slim and datatables

Resolves dani-garcia#3382
Resolves dani-garcia#3415
Resolves discussion on dani-garcia#3288
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants