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

Calls to manifest, without a referrer, affect session history and back redirect navigation #4649

Closed
ntimo opened this issue Nov 5, 2023 · 13 comments
Labels
Milestone

Comments

@ntimo
Copy link

ntimo commented Nov 5, 2023

Describe the Bug

When logging in with OIDC the login is successful, but the browser is then redirected to /manifest.json which is confusing, since the user should be redirected to the normal Booktack interface. But after removing /manifest.json from the url to open booktack normally the user is logged in.

Steps to Reproduce

  1. Make sure you are signed out
  2. Login using OIDC
  3. Get redirected to /manifest.json

Expected Behaviour

The normal landing page should be open on the / url.

Screenshots or Additional Context

My OIDC callback is configured to `/oidc/callback

Browser Details

Chrome Linux

Exact BookStack Version

v23.10.1

@ntimo ntimo added the 🐛 Bug label Nov 5, 2023
@ssddanbrown
Copy link
Member

Hi @ntimo,
Thanks for reporting, but I have been unable to reproduce this on either Firefox or Chrome (on Fedora 39) in my testing.

  • Does this occur in other browsers? Or other browsing environments where you have no extensions or previous history?
  • Are you using BookStack in any special mode within the browser (used as it's own app window or similar)?

@ntimo
Copy link
Author

ntimo commented Nov 5, 2023

@ssddanbrown Yes I just tested this with Firefox and there the issue does not happen. But when using Chrome or Chromium it happens, which is kind of odd. The Chromium I tested this with has 0 extensions installed.

Chromium version: Version 118.0.5993.70 (Official Build) Fedora Project (64-bit)

@ssddanbrown
Copy link
Member

Realised my Chromium was a bit behind. Updated but still works fine for me.

I could kind see ways how this could occur if the manifest was being loaded on every page load, but not sure why that might be, and have not been able to quickly force-replicate that scenario.

  • Does going to /manifest.json on your instance show JSON in response (including your BookStack instance name)?
  • If you view the source of any normal BookStack view in the browser, then scroll to about line 31 and find the line starting with <link rel="manifest", does the href URL exactly match <BOOKSTACK_APP_URL>/manifest.json where <BOOKSTACK_APP_URL> is the base URL for your BookStack instance? No double slashes or different https/http protocol used?

@ntimo
Copy link
Author

ntimo commented Nov 5, 2023

  1. Yes it does, this is the top part of the json response (I left out the icons section from the code snippet below):
{
    "name": "BookStack",
    "short_name": "BookStack",
    "start_url": ".\/",
    "scope": "\/",
    "display": "standalone",
    "background_color": "#111111",
    "description": "BookStack",
    "theme_color": "#206ea7",
    "launch_handler": {
        "client_mode": "focus-existing"
    },
    "orientation": "portrait",
....
}
  1. Yes it does <link rel="manifest" href="https://bookstack.domain.com/manifest.json" crossorigin="use-credentials">

Maybe its worth noting that I am using the linux server docker image to host booktack

@fdelapena
Copy link

I can reproduce this, just by clicking the Toggle dark mode button even without being logged. It fails on Firefox on Android, but works on Firefox for Desktop.

@ssddanbrown
Copy link
Member

@fdelapena Is this reproducible on our demo instance? If not, how are you running/hosting BookStack?

@arl4223
Copy link

arl4223 commented Nov 6, 2023

Hello,

as mentioned on the Discord server I encounter the same issue.
For further testing I set up a different notebook. My primary laptop is an Apple MacBook Pro. The testing device is an Lenovo Thinkpad. One is running the latest macOS and the other one is running Ubuntu 22.04 LTS.

When I switch to dark mode on the macOS system I get the /manifest.json error. If I login to my instance on the Ubuntu system and switch to dark mode a thumbnail is shown and the URL is amended with /uploads/images/cover_bookshelf/2023-11/thumbs-440-250/XXX.png where XXX is a redacted filename so I do not disclose private details. This is running Firefox without any plugins. It was just setup fresh for testing this issue.

Both ways are reproducible.

Let me know if I can provide more information which can help.

@fdelapena
Copy link

fdelapena commented Nov 7, 2023

@fdelapena Is this reproducible on our demo instance? If not, how are you running/hosting BookStack?

Can't reproduce it on the demo instance. The server setup is:

  • Server with Debian 12, php-8.2 possibly from deb.sury.org repo, Apache httpd 2.4.57, mod_proxy_fcgi, php-fpm 8.2.12 (stock fpm configuration), mariadb 10.11.4. mod_md for ACME certificate stuff, mod_h2 for HTTP/2.
  • Using AllowOverride All with .htaccess (not using the more efficient .conf yet).
  • Using modern default debian php-fpm with sethandler for fastcgi UDS localhost for .php handling via mod_proxy_fcgi.
  • Apache httpd virtual host settings:
<VirtualHost *:443>
	ServerName foo.example.org
	SSLEngine on
	DocumentRoot /var/www/foo.example/public
	<Directory /var/www/foo.example/public>
		AllowOverride All
		Header set Strict-Transport-Security "max-age=31536000; includeSubDomains; preload"
	        Header set Expect-Staple "max-age=31536000; includeSubDomains; preload"
	        Header set Permissions-Policy "usb=()"
	        Header set Referrer-Policy "no-referrer"
	        Header set X-Content-Type-Options "nosniff"
	        Header set X-Frame-Options "DENY"
	        Header set X-XSS-Protection "0"
	</Directory>
</VirtualHost>
  • Stock PHP modules configuration (.ini files). Loaded modules: mysqlnd opcache pdo xml calendar ctype curl dom exif ffi fileinfo ftp gd gettext iconv intl mbstring mysqli pdo_mysql phar posix readline shmop simplexml sockets sysvmsg sysvsem sysvshm tokenizer xmlreader xmlwriter xsl zip
  • .env file contained APP_URL=https://foo.example.org before running the initial php artisan migrate. APP_KEY also looked fine. Deployment was pretty similar to documentation setup instructions.
  • From admin settings, made wiki public, no user registration allowed. Other settings mainly stock.

@arl4223
Copy link

arl4223 commented Nov 7, 2023

I don't know if this helps but I found another way to reproduce this issue: when ever I change the view from grid to list or the other way around I will see the /manifest.json site.

@ssddanbrown ssddanbrown added this to the v23.10.2 milestone Nov 7, 2023
@ssddanbrown
Copy link
Member

ssddanbrown commented Nov 7, 2023

Thank you @fdelapena, @ntimo and @arl4223 for all the extra information.

I have now been able to reproduce this.
This specifically occurs in environments where no refferrer header is passed (possible browser control or via a server-side no-referrer policy as demonstrated above) and where the browser frequently requests the manifest file.
When no referrer is provided, then BookStack will fall back to the last url requested in the session, which ends up being the manifest file.

This was made a little more annoying to reproduce by browsers seemingly having strict and differing behaviour as to when the manifest would be requested. Self-signed certs, or private windows, would change the behaviour.

I'll update the manifest link to not pass credentials, so a session is not used for this call and therefore this isn't added to the user session history.
It does mean we can't customize the colors of the manifest to the user's dark mode preference, but I think that's a fair trade to avoid this issue for now.

A better way to address this would be to avoid this URL being tracked but that requires some deeper overriding of the framework, which I wouldn't want to do in a patch and I'm not sure it'd be worthwhile to maintain.

Will get a patch release out soon.

@arl4223 Your example with redirect to the image is of the same underlying cause, but specifically when certain image storage options are in use and/or when images are missing/broken. I won't address that specific case right now, since that hasn't be really reported as an issue and would have existed for a long time already, but it's tied up within the above.
If it's specifically problematic within an environment, this could be helped by allowing a referrer to be used within the environment (You can limit to same-origin if needed).

@ssddanbrown ssddanbrown changed the title OIDC login redirects to /manifest.json Calls to manifest, without a referrer, affect session histroy and back redirect navigation Nov 7, 2023
@ssddanbrown ssddanbrown changed the title Calls to manifest, without a referrer, affect session histroy and back redirect navigation Calls to manifest, without a referrer, affect session history and back redirect navigation Nov 7, 2023
ssddanbrown added a commit that referenced this issue Nov 7, 2023
For #4649
More of a patch around the issue for now.
Have opened #4656 to properly address.
@ssddanbrown
Copy link
Member

I have now patched this via ea0469e to avoid manifest calls using the session as per the above.

I've opened up #4656 with the intent to implement a longer-term solution for this (and similar cases like the image example).

This patch will be part of v23.10.2 which should be released within the next hour or so.

@arl4223
Copy link

arl4223 commented Nov 7, 2023

I have just deployed the mentioned update and can confirm, that the manifest.json ist not the issue anymore.

Instead I get the "image" issue. I will have a look into the referrer option mentioned above.

@arl4223
Copy link

arl4223 commented Nov 7, 2023

Adding add_header 'Referrer-Policy' 'origin'; to my nginx config helped.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants