Skip to content

Commit 318ebe4

Browse files
alexkoonclaude
andcommitted
Fix subfolder sharing with directory scope enforcement
Fixes issue #1068 — subfolders inaccessible when browsing a shared folder link. Root causes fixed: - SearchManager: recursiveDir flag adds path LIKE clause so subdirectory media and directory listings are visible under a share root - SessionManager: passes recursiveDir=true to both projection query builders - GalleryRouter + AuthenticationMWs: LimitedGuest role now reaches the directory listing route; authoriseSharingDirectory enforces that the requested path is the share root or a descendant — unrelated paths get 401 - SharingMWs + SharingDTO: /share/<key>/key endpoint returns passwordProtected flag so the frontend can decide whether to show a password prompt - navigation.service / error.interceptor / authentication.service: fixed blank-screen deadlock and 401 redirect loop on password-protected shares - gallery.component: directory shares navigate to gallery/<dir> view so subfolder cards are clickable, not to the flat search results view - navigator: breadcrumb links enabled for paths at or below the share root; routes observable now uses combineLatest so breadcrumbs recompute when share metadata arrives (fixes stale-null race on cached content loads) - share.service: removed debug console.log Security: - authoriseSharingDirectory fails closed — unrecognised AND query shapes return 401 rather than silently granting access - normalizeDirPath canonicalises '.' (Utils.concatUrls root form), '/' and '' to the same empty string so root-directory shares are not falsely rejected - Non-directory shares (date/person) permitted at the route level; content scope is enforced by the DB-layer projectionQuery Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent b9d62f7 commit 318ebe4

17 files changed

Lines changed: 204 additions & 57 deletions

src/backend/middlewares/SharingMWs.ts

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,9 +47,11 @@ export class SharingMWs {
4747
return next();
4848
}
4949
const sharingKey = req.params[QueryParams.gallery.sharingKey_params];
50-
51-
req.resultPipe =
52-
{sharingKey: (await ObjectManagers.getInstance().SharingManager.findOne(sharingKey)).sharingKey} as SharingDTOKey;
50+
const sharing = await ObjectManagers.getInstance().SharingManager.findOne(sharingKey);
51+
req.resultPipe = {
52+
sharingKey: sharing.sharingKey,
53+
passwordProtected: !!(sharing.password || Config.Sharing.passwordRequired),
54+
} as SharingDTOKey;
5355
return next();
5456
} catch (err) {
5557
return next(

src/backend/middlewares/user/AuthenticationMWs.ts

Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {QueryParams} from '../../../common/QueryParams';
99
import * as path from 'path';
1010
import {Logger} from '../../Logger';
1111
import {ContextUser} from '../../model/SessionContext';
12+
import {ANDSearchQuery, SearchQueryDTO, SearchQueryTypes, TextSearch, TextSearchQueryMatchTypes} from '../../../common/entities/SearchQueryDTO';
1213

1314
const LOG_TAG = 'AuthenticationMWs';
1415

@@ -298,6 +299,71 @@ export class AuthenticationMWs {
298299
return next();
299300
}
300301

302+
/**
303+
* Canonical form for directory path comparison:
304+
* - backslashes → forward slashes
305+
* - trailing slashes stripped
306+
* - '.' → '' (Utils.concatUrls returns '.' for the gallery root; normalise to empty string)
307+
*/
308+
private static normalizeDirPath(p: string): string {
309+
const fwd = p.replace(/\\/g, '/');
310+
let end = fwd.length;
311+
while (end > 0 && fwd[end - 1] === '/') end--;
312+
const s = fwd.slice(0, end);
313+
return s === '.' ? '' : s;
314+
}
315+
316+
/**
317+
* Middleware: allows LimitedGuest (sharing) users through only when the requested
318+
* directory is the share root or a descendant of it. All other roles pass unconditionally.
319+
*/
320+
public static authoriseSharingDirectory(req: Request, res: Response, next: NextFunction): void {
321+
if (req.session?.context?.user?.role !== UserRoles.LimitedGuest) {
322+
return next();
323+
}
324+
const allowQuery = req.session.context.user.allowQuery;
325+
if (!allowQuery) {
326+
return next();
327+
}
328+
const requestedDir = AuthenticationMWs.normalizeDirPath(req.params['directory'] || '');
329+
if (AuthenticationMWs.isDirInShareScope(requestedDir, allowQuery)) {
330+
return next();
331+
}
332+
res.status(401);
333+
return next(new ErrorDTO(ErrorCodes.NOT_AUTHORISED));
334+
}
335+
336+
/**
337+
* Returns true when `dir` is within the directory scope of a share's allowQuery.
338+
*
339+
* - Exact-match directory share: dir must equal or be a descendant of the share root.
340+
* - AND query: the top-level list must contain an exact-match directory clause; if none
341+
* is found the function fails closed (false) rather than granting access silently.
342+
* - Non-directory shares (date, person, etc.): any directory is permitted here because
343+
* the DB-layer projectionQuery already restricts which media rows are returned.
344+
*/
345+
private static isDirInShareScope(dir: string, query: SearchQueryDTO): boolean {
346+
if (
347+
query.type === SearchQueryTypes.directory &&
348+
(query as TextSearch).matchType === TextSearchQueryMatchTypes.exact_match
349+
) {
350+
const shareRoot = AuthenticationMWs.normalizeDirPath((query as TextSearch).value);
351+
// Empty shareRoot means the gallery root — every directory is in scope.
352+
if (shareRoot === '') return true;
353+
return dir === shareRoot || dir.startsWith(shareRoot + '/');
354+
}
355+
if (query.type === SearchQueryTypes.AND) {
356+
const dirClause = (query as ANDSearchQuery).list.find(
357+
q => q.type === SearchQueryTypes.directory &&
358+
(q as TextSearch).matchType === TextSearchQueryMatchTypes.exact_match
359+
);
360+
// Fail closed: an AND query with no recognisable directory clause is not granted access.
361+
return dirClause ? AuthenticationMWs.isDirInShareScope(dir, dirClause) : false;
362+
}
363+
// Non-directory shares: projectionQuery enforces content scope at the DB layer.
364+
return true;
365+
}
366+
301367
private static async getSharingUser(req: Request): Promise<ContextUser> {
302368
if (
303369
Config.Sharing.enabled === true &&

src/backend/model/database/SearchManager.ts

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -481,15 +481,16 @@ export class SearchManager {
481481

482482
public async prepareAndBuildWhereQuery(
483483
queryIN: SearchQueryDTO, directoryOnly = false,
484-
aliases: { [key: string]: string } = {}): Promise<Brackets> {
484+
aliases: { [key: string]: string } = {},
485+
recursiveDir = false): Promise<Brackets> {
485486
let query = await this.prepareQuery(queryIN);
486487
if (directoryOnly) {
487488
query = this.filterDirectoryQuery(query);
488489
if (query === null) {
489490
return null;
490491
}
491492
}
492-
return this.buildWhereQuery(query, directoryOnly, aliases);
493+
return this.buildWhereQuery(query, directoryOnly, aliases, recursiveDir);
493494
}
494495

495496
public async prepareQuery(queryIN: SearchQueryDTO): Promise<SearchQueryDTO> {
@@ -509,22 +510,23 @@ export class SearchManager {
509510
public buildWhereQuery(
510511
query: SearchQueryDTO,
511512
directoryOnly = false,
512-
aliases: { [key: string]: string } = {}
513+
aliases: { [key: string]: string } = {},
514+
recursiveDir = false
513515
): Brackets {
514516
const queryId = (query as SearchQueryDTOWithID).queryId;
515517
switch (query.type) {
516518
case SearchQueryTypes.AND:
517519
return new Brackets((q): unknown => {
518520
(query as ANDSearchQuery).list.forEach((sq) => {
519-
q.andWhere(this.buildWhereQuery(sq, directoryOnly));
521+
q.andWhere(this.buildWhereQuery(sq, directoryOnly, aliases, recursiveDir));
520522
}
521523
);
522524
return q;
523525
});
524526
case SearchQueryTypes.OR:
525527
return new Brackets((q): unknown => {
526528
(query as ANDSearchQuery).list.forEach((sq) => {
527-
q.orWhere(this.buildWhereQuery(sq, directoryOnly));
529+
q.orWhere(this.buildWhereQuery(sq, directoryOnly, aliases, recursiveDir));
528530
}
529531
);
530532
return q;
@@ -968,6 +970,19 @@ export class SearchManager {
968970
return dq;
969971
})
970972
);
973+
// For sharing projection queries, also match subdirectories recursively
974+
if (
975+
recursiveDir &&
976+
query.type === SearchQueryTypes.directory &&
977+
(query as TextSearch).matchType === TextSearchQueryMatchTypes.exact_match
978+
) {
979+
const normalizedDirPath = dirPathStr.endsWith('/') ? dirPathStr : dirPathStr + '/';
980+
textParam['subDirPath' + queryId] = normalizedDirPath + '%';
981+
q[whereFN](
982+
`${alias}.path ${LIKE} :subDirPath${queryId} COLLATE ` + SQL_COLLATE,
983+
textParam
984+
);
985+
}
971986
}
972987

973988
if (

src/backend/model/database/SessionManager.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -41,10 +41,10 @@ export class SessionManager {
4141

4242
if (finalQuery) {
4343
// Build the Brackets-based query
44-
context.projectionQuery = await ObjectManagers.getInstance().SearchManager.prepareAndBuildWhereQuery(finalQuery);
44+
context.projectionQuery = await ObjectManagers.getInstance().SearchManager.prepareAndBuildWhereQuery(finalQuery, false, {}, true);
4545
context.hasDirectoryProjection = ObjectManagers.getInstance().SearchManager.hasDirectoryQuery(finalQuery);
4646
if (context.hasDirectoryProjection) {
47-
context.projectionQueryForSubDir = await ObjectManagers.getInstance().SearchManager.prepareAndBuildWhereQuery(finalQuery, true, {directory: 'directories'});
47+
context.projectionQueryForSubDir = await ObjectManagers.getInstance().SearchManager.prepareAndBuildWhereQuery(finalQuery, true, {directory: 'directories'}, true);
4848
}
4949
context.user.projectionKey = this.createProjectionKey(finalQuery);
5050
if (SearchQueryUtils.isQueryEmpty(finalQuery)) {

src/backend/routes/GalleryRouter.ts

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,8 +35,9 @@ export class GalleryRouter {
3535
[Config.Server.apiPath + '/gallery/content/:directory(*)', Config.Server.apiPath + '/gallery/', Config.Server.apiPath + '/gallery//'],
3636
// common part
3737
AuthenticationMWs.authenticate,
38-
AuthenticationMWs.authorise(UserRoles.Guest), //sharing user can only go through search. They can't just wander through the whole gallery
38+
AuthenticationMWs.authorise(UserRoles.LimitedGuest),
3939
AuthenticationMWs.normalizePathParam('directory'),
40+
AuthenticationMWs.authoriseSharingDirectory, // LimitedGuest: scope-checks the requested directory against the share root
4041
VersionMWs.injectGalleryVersion,
4142

4243
// specific part
@@ -51,7 +52,7 @@ export class GalleryRouter {
5152
protected static addDirectoryZip(app: Express): void {
5253
app.get(
5354
[Config.Server.apiPath + '/gallery/zip/:searchQueryDTO(*)'],
54-
// common part
55+
// LimitedGuest allowed; content is scoped to the share by the session projectionQuery
5556
AuthenticationMWs.authenticate,
5657
AuthenticationMWs.authorise(UserRoles.LimitedGuest),
5758

@@ -162,7 +163,7 @@ export class GalleryRouter {
162163
protected static addRandom(app: Express): void {
163164
app.get(
164165
[Config.Server.apiPath + '/gallery/random/:searchQueryDTO'],
165-
// common part
166+
// LimitedGuest allowed; content is scoped to the share by the session projectionQuery
166167
AuthenticationMWs.authenticate,
167168
AuthenticationMWs.authorise(UserRoles.LimitedGuest),
168169
VersionMWs.injectGalleryVersion,
@@ -256,7 +257,7 @@ export class GalleryRouter {
256257
protected static addSearch(app: Express): void {
257258
app.get(
258259
Config.Server.apiPath + '/search/:searchQueryDTO(*)',
259-
// common part
260+
// LimitedGuest allowed; content is scoped to the share by the session projectionQuery
260261
AuthenticationMWs.authenticate,
261262
AuthenticationMWs.authorise(UserRoles.LimitedGuest),
262263
VersionMWs.injectGalleryVersion,
@@ -274,7 +275,7 @@ export class GalleryRouter {
274275
protected static addAutoComplete(app: Express): void {
275276
app.get(
276277
Config.Server.apiPath + '/autocomplete/:value(*)',
277-
// common part
278+
// LimitedGuest allowed; content is scoped to the share by the session projectionQuery
278279
AuthenticationMWs.authenticate,
279280
AuthenticationMWs.authorise(UserRoles.LimitedGuest),
280281
VersionMWs.injectGalleryVersion,

src/backend/routes/SharingRouter.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ export class SharingRouter {
4141
// its a public path
4242
SharingMWs.getSharingKey,
4343
ServerTimingMWs.addServerTiming,
44-
RenderingMWs.renderSharing
44+
RenderingMWs.renderResult
4545
);
4646
}
4747

src/common/entities/SharingDTO.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import {SearchQueryDTO} from './SearchQueryDTO';
33

44
export interface SharingDTOKey {
55
sharingKey: string;
6+
passwordProtected?: boolean;
67
}
78

89
export interface BaseSharingDTO extends SharingDTOKey {

src/frontend/app/model/navigation.service.ts

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,6 @@ import {IsActiveMatchOptions, Router} from '@angular/router';
44
import {ShareService} from '../ui/gallery/share.service';
55
import {Config} from '../../../common/config/public/Config';
66
import {NavigationLinkTypes} from '../../../common/config/public/ClientConfig';
7-
import {firstValueFrom} from 'rxjs';
8-
97
@Injectable()
108
export class NavigationService {
119
constructor(private router: Router, private shareService: ShareService) {
@@ -31,7 +29,8 @@ export class NavigationService {
3129
public async toLogin(): Promise<boolean> {
3230
await this.shareService.wait();
3331
if (this.shareService.isSharing()) {
34-
if ((await firstValueFrom(this.shareService.currentSharing)).passwordProtected === true) {
32+
// sharingPasswordProtected is set synchronously from the /key endpoint before ReadyPR resolves
33+
if (this.shareService.sharingPasswordProtected === true) {
3534
return this.router.navigate(['shareLogin'], {
3635
queryParams: {sk: this.shareService.getSharingKey()},
3736
});

src/frontend/app/model/network/authentication.service.ts

Lines changed: 11 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ declare module ServerInject {
1919
@Injectable({providedIn: 'root'})
2020
export class AuthenticationService {
2121
public readonly user: BehaviorSubject<UserDTO>;
22+
private sessionUserPromise: Promise<void> | null = null;
2223

2324
constructor(
2425
private userService: UserService,
@@ -39,7 +40,8 @@ export class AuthenticationService {
3940
) {
4041
this.user.next(ServerInject.user);
4142
}
42-
this.getSessionUser().catch(console.error);
43+
this.sessionUserPromise = this.getSessionUser();
44+
this.sessionUserPromise.catch(console.error);
4345
} else {
4446
if (Config.Users.authenticationRequired === false) {
4547
this.user.next({
@@ -99,14 +101,19 @@ export class AuthenticationService {
99101
public async logout(): Promise<void> {
100102
await this.userService.logout();
101103
this.user.next(null);
102-
// even on logout try to get sharing user if it's a sharing
104+
// Restore a non-password-protected sharing session after logout.
105+
// Skip for password-protected shares — getSessionUser always returns 401 until re-auth.
103106
await this.shareService.wait();
104-
if(this.shareService.isSharing()){
107+
if (this.shareService.isSharing() && this.shareService.sharingPasswordProtected !== true) {
105108
await this.getSessionUser();
106109
}
107110
}
108111

109-
private async getSessionUser(): Promise<void> {
112+
public waitForSessionUser(): Promise<void> {
113+
return this.sessionUserPromise ?? Promise.resolve();
114+
}
115+
116+
public async getSessionUser(): Promise<void> {
110117
try {
111118
this.user.next(await this.userService.getSessionUser());
112119
} catch (error) {

src/frontend/app/model/network/helper/auth.guard.ts

Lines changed: 22 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -2,23 +2,37 @@ import {Injectable} from '@angular/core';
22
import {ActivatedRouteSnapshot, CanActivate, RouterStateSnapshot} from '@angular/router';
33
import {AuthenticationService} from '../authentication.service';
44
import {NavigationService} from '../../navigation.service';
5+
import {ShareService} from '../../../ui/gallery/share.service';
56

67
@Injectable({providedIn: 'root'})
7-
export class AuthGuard implements CanActivate{
8+
export class AuthGuard implements CanActivate {
89
constructor(
9-
private authenticationService: AuthenticationService,
10-
private navigationService: NavigationService
10+
private authenticationService: AuthenticationService,
11+
private navigationService: NavigationService,
12+
private shareService: ShareService,
1113
) {
1214
}
1315

14-
canActivate(
15-
route: ActivatedRouteSnapshot,
16-
state: RouterStateSnapshot
17-
): boolean {
18-
if (this.authenticationService.isAuthenticated() === true) {
16+
async canActivate(
17+
route: ActivatedRouteSnapshot,
18+
state: RouterStateSnapshot
19+
): Promise<boolean> {
20+
// Wait for session cookie restoration (async HTTP call started in constructor)
21+
await this.authenticationService.waitForSessionUser();
22+
23+
if (this.authenticationService.isAuthenticated()) {
1924
return true;
2025
}
2126

27+
// For no-password shares, try backend auto-authentication via the sharing key
28+
await this.shareService.wait();
29+
if (this.shareService.isSharing() && this.shareService.sharingPasswordProtected === false) {
30+
await this.authenticationService.getSessionUser();
31+
if (this.authenticationService.isAuthenticated()) {
32+
return true;
33+
}
34+
}
35+
2236
this.navigationService.toLogin().catch(console.error);
2337
return false;
2438
}

0 commit comments

Comments
 (0)