Hidden album is shown in album listing, if password matches with parent album

[mniemela]

Detailed description of the problem [REQUIRED]

When you open the album listing after opening a password protected album, a hidden subalbum of it is listed in it if it's protected with same password. This happens with Lychee 4.3.0.

Steps to reproduce the issue

Steps to reproduce the behavior:

1. Create a password protected album (I used a hidden album, if it matters)
2. Create a hidden and a visible subalbum under it, that are protected with same password
3. Log in with password (eg. with incognito mode of browser) to the parent directory that is password protected
4. Open album listing from the drop-down menu
5. You see both hidden and visible subalbum in the listing

Screenshots

Output of the diagnostics [REQUIRED]

Is this applicable for this issue?

Browser and system

Happens both with Chrome and Firefox in Windows 10.

[kamil4]

Yeah... It's a security issue, but a fairly narrow one as it only affects hidden albums that have a password and that are already unlocked...

I'm not sure if it's worth fixing on master or should we just make sure that it's not an issue on #1055?

[kamil4]

To clarify, this actually affects all hidden, password-protected albums. The album does not need to be inside another password-protected album; it can be in a regular public album or even at the top level.

The issue is that once a password-protected album is unlocked, it's included in the response to Albums::tree even if it's hidden. Albums::tree appears to use a different logic to Albums::get or Album::get to decide what to include/exclude, as it excludes password-protected albums that are visible but locked, but includes invisible ones if they are unlocked.

[nagmat84]

I guess "hidden" means "requires direct link"? I am not 100% sure, but I am fairly convinced that this doesn't happen with PR #1055. I will test it this evening when I am home.

[kamil4]

Yep, direct link.

I'm guessing it's because the get_visible_albums call is in a wrong place here:

Lychee/app/Actions/Albums/Extensions/PublicIds.php

Lines 51 to 53 in 5cf7a06

	// unlocked albums
	$query = DB::table('albums')->select('_lft', '_rgt')
	->whereNotIn('id', AccessControl::get_visible_albums());

It should probably be here instead:

Lychee/app/Actions/Albums/Extensions/PublicIds.php

Line 46 in 5cf7a06

->orWhere(fn ($q) => $q->where('public', '=', '1')->where('password', '<>', ''));

But since we are not planning any more releases out of this codebase, I'm not inclined to investigate any further...

[nagmat84]

Good news. The PR #1055 is not affected by this bug. I tried the following setup:

Top Album       (public, password-protected "A")
  |
  +-- Sub Album 1 (public)
  |
  +-- Sub Album 2 (public, password-protected "A")
  |
  +-- Sub Album 3 (public, password-protected "A", requires direct link)
  |
  +-- Sub Album 4 (public, password-protected "B")
  |
  +-- Sub Album 5 (public, password-protected "B", requires direct link)

Results:

1. On the root album, the top album is visible, but without a thumbnail (it is still locked)
2. If you try to open the top album, you are asked for a password. If you enter "A", access is granted.
3. If you enter the top album
   • you can see sub album 1 with a thumbnail
   • you can see sub album 2 with a thumbnail (it has become unlocked simultaneously, because it has the same password)
   • you don't see sub album 3
   • you see sub album 4, but without a thumbnail (it is still locked due to a different password)
   • you don't see sub album 5
4. If you enter the direct link for sub album 3, access is directly granted without asking for a password (it has already been given)
5. If you enter the direct link for sub album 5, you are asked for a password. If you enter "B", access is granted.
6. If you go back to the top album, you still only see the same sub-albums 1, 2 and 4. But this time sub-album 4 also shows a thumbnail (it has become unlocked, because you entered password "B" in step 5.)

Yeah! My refactoring pays off! 😎

[kamil4]

Erm, if I understood your steps correctly, this all works the same way on master as well. The part that fails on master is when you list the album tree (by clicking on the down arrow next to the album name in the header -- see the screenshot in the top post). That's where the unlocked but hidden albums are incorrectly listed on master (and locked but visible are not -- which is also wrong).

I still think there's a good chance you may have fixed it (that code is so twisted on master that I know I would've wanted to rewrite it 😉) but I want to make sure that we test the right thing...

[nagmat84]

Sorry, you are right. I also tested that, but did not mention it explicitly: The album tree (by clicking on the down arrow next to the album name in the header) is also correctly shown from every album.

Yes, you are right. As my code base actually uses the same SQL "filters" everywhere, it has been fixed, too.

[nagmat84]

By the way: Although I added 4,600 LOC (well mostly comments) the test coverage of the whole project increases from ~61% to ~63.5%. 😄