From e18785a9ca2854d907ce6961a7a787683af4b5e3 Mon Sep 17 00:00:00 2001 From: Darren Yu Date: Mon, 3 Aug 2026 01:10:12 +0800 Subject: [PATCH 1/2] fix: revalidate share and direct links after permission changes Revalidate existing share links against the owner's current status and group permissions. Reject redirected direct links when the owner's current group disables direct links. Refresh restored share navigator state before serving cached paths. Prevent client-side redirect caching and add regression coverage for restricted owners. Co-authored-by: Codex --- inventory/share.go | 19 ++++++-- inventory/share_test.go | 44 +++++++++++++++++++ pkg/filemanager/fs/dbfs/manage.go | 11 +++++ .../fs/dbfs/manage_direct_link_test.go | 38 ++++++++++++++++ pkg/filemanager/fs/dbfs/share_navigator.go | 32 ++++++++++++++ service/explorer/file.go | 4 +- 6 files changed, 142 insertions(+), 6 deletions(-) create mode 100644 inventory/share_test.go create mode 100644 pkg/filemanager/fs/dbfs/manage_direct_link_test.go diff --git a/inventory/share.go b/inventory/share.go index 3f18b40c..932405bd 100644 --- a/inventory/share.go +++ b/inventory/share.go @@ -24,9 +24,10 @@ type ( ) var ( - ErrShareLinkExpired = fmt.Errorf("share link expired") - ErrOwnerInactive = fmt.Errorf("owner is inactive") - ErrSourceFileInvalid = fmt.Errorf("source file is deleted") + ErrShareLinkExpired = fmt.Errorf("share link expired") + ErrOwnerInactive = fmt.Errorf("owner is inactive") + ErrOwnerShareDisabled = fmt.Errorf("owner is not allowed to share files") + ErrSourceFileInvalid = fmt.Errorf("source file is deleted") ) type ( @@ -229,6 +230,15 @@ func IsValidShare(share *ent.Share) error { return ErrOwnerInactive } + // Creating and accessing share links are governed by the owner's current + // group. This makes existing links unavailable as soon as the permission is + // revoked, instead of only preventing the creation of new links. + ownerGroup, err := owner.Edges.GroupOrErr() + if err != nil || ownerGroup.Permissions == nil || + !ownerGroup.Permissions.Enabled(int(types.GroupPermissionShare)) { + return ErrOwnerShareDisabled + } + // Check source file status file, err := share.Edges.FileOrErr() if err != nil || file.FileChildren == 0 || file.OwnerID != owner.ID { @@ -418,7 +428,8 @@ func withShareEagerLoading(ctx context.Context, q *ent.ShareQuery) *ent.ShareQue } if v, ok := ctx.Value(LoadShareUser{}).(bool); ok && v { q.WithUser(func(q *ent.UserQuery) { - withUserEagerLoading(ctx, q) + userCtx := context.WithValue(ctx, LoadUserGroup{}, true) + withUserEagerLoading(userCtx, q) }) } diff --git a/inventory/share_test.go b/inventory/share_test.go new file mode 100644 index 00000000..3ce31d71 --- /dev/null +++ b/inventory/share_test.go @@ -0,0 +1,44 @@ +package inventory + +import ( + "testing" + + "github.com/cloudreve/Cloudreve/v4/ent" + entuser "github.com/cloudreve/Cloudreve/v4/ent/user" + "github.com/cloudreve/Cloudreve/v4/inventory/types" + "github.com/cloudreve/Cloudreve/v4/pkg/boolset" +) + +func TestIsValidShareChecksCurrentOwnerAccess(t *testing.T) { + tests := []struct { + name string + status entuser.Status + canShare bool + wantErr bool + }{ + {name: "active owner with share permission", status: entuser.StatusActive, canShare: true}, + {name: "active owner without share permission", status: entuser.StatusActive, wantErr: true}, + {name: "manually banned owner", status: entuser.StatusManualBanned, canShare: true, wantErr: true}, + {name: "system banned owner", status: entuser.StatusSysBanned, canShare: true, wantErr: true}, + {name: "inactive owner", status: entuser.StatusInactive, canShare: true, wantErr: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + permissions := &boolset.BooleanSet{} + boolset.Set(types.GroupPermissionShare, tt.canShare, permissions) + group := &ent.Group{Permissions: permissions} + owner := &ent.User{ID: 1, Status: tt.status} + owner.SetGroup(group) + file := &ent.File{OwnerID: owner.ID, FileChildren: 1} + share := &ent.Share{} + share.SetUser(owner) + share.SetFile(file) + + err := IsValidShare(share) + if (err != nil) != tt.wantErr { + t.Fatalf("IsValidShare() error = %v, wantErr %v", err, tt.wantErr) + } + }) + } +} diff --git a/pkg/filemanager/fs/dbfs/manage.go b/pkg/filemanager/fs/dbfs/manage.go index f0471df5..e05c590d 100644 --- a/pkg/filemanager/fs/dbfs/manage.go +++ b/pkg/filemanager/fs/dbfs/manage.go @@ -643,6 +643,17 @@ func (f *DBFS) GetFileFromDirectLink(ctx context.Context, dl *ent.DirectLink) (f return nil, fs.ErrDirectLinkInvalid.WithError(fmt.Errorf("file owner is not active")) } + // Revalidate the owner's current direct-link permission so existing links + // are revoked when direct links are disabled for the current group, whether + // by changing the group setting or moving the owner to another group. + group, err := owner.Edges.GroupOrErr() + if err != nil { + return nil, fs.ErrDirectLinkInvalid.WithError(fmt.Errorf("file owner group is unavailable: %w", err)) + } + if group.Settings == nil || group.Settings.SourceBatchSize <= 0 { + return nil, fs.ErrDirectLinkInvalid.WithError(fmt.Errorf("file owner is not allowed to create direct links")) + } + file := newFile(nil, fileModel) // Traverse to the root file diff --git a/pkg/filemanager/fs/dbfs/manage_direct_link_test.go b/pkg/filemanager/fs/dbfs/manage_direct_link_test.go new file mode 100644 index 00000000..485355ff --- /dev/null +++ b/pkg/filemanager/fs/dbfs/manage_direct_link_test.go @@ -0,0 +1,38 @@ +package dbfs + +import ( + "context" + "testing" + + "github.com/cloudreve/Cloudreve/v4/ent" + entuser "github.com/cloudreve/Cloudreve/v4/ent/user" + "github.com/cloudreve/Cloudreve/v4/inventory/types" +) + +func TestGetFileFromDirectLinkRejectsRestrictedOwner(t *testing.T) { + tests := []struct { + name string + status entuser.Status + batchSize int + }{ + {name: "active owner without direct link permission", status: entuser.StatusActive}, + {name: "manually banned owner", status: entuser.StatusManualBanned, batchSize: 1}, + {name: "system banned owner", status: entuser.StatusSysBanned, batchSize: 1}, + {name: "inactive owner", status: entuser.StatusInactive, batchSize: 1}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + owner := &ent.User{Status: tt.status} + owner.SetGroup(&ent.Group{Settings: &types.GroupSetting{SourceBatchSize: tt.batchSize}}) + file := &ent.File{} + file.SetOwner(owner) + link := &ent.DirectLink{} + link.SetFile(file) + + if _, err := (&DBFS{}).GetFileFromDirectLink(context.Background(), link); err == nil { + t.Fatal("GetFileFromDirectLink() error = nil, want restricted owner to be rejected") + } + }) + } +} diff --git a/pkg/filemanager/fs/dbfs/share_navigator.go b/pkg/filemanager/fs/dbfs/share_navigator.go index c03caf83..11823e88 100644 --- a/pkg/filemanager/fs/dbfs/share_navigator.go +++ b/pkg/filemanager/fs/dbfs/share_navigator.go @@ -57,6 +57,7 @@ type ( share *ent.Share owner *ent.User disableRecycle bool + restoredState bool persist func() } @@ -90,6 +91,7 @@ func (n *shareNavigator) RestoreState(s State) error { n.singleFileShare = state.SingleFileShare n.share = state.Share n.owner = state.Owner + n.restoredState = true return nil } @@ -179,6 +181,36 @@ func (n *shareNavigator) Root(ctx context.Context, path *fs.URI) (*File, error) } func (n *shareNavigator) To(ctx context.Context, path *fs.URI) (*File, error) { + // Navigator state contains the share owner's status and group. Revalidate it + // after a cache restore so a ban or group permission change takes effect + // immediately instead of being bypassed for ContextHintTTL. + if n.restoredState { + shareCtx := context.WithValue(ctx, inventory.LoadShareUser{}, true) + shareCtx = context.WithValue(shareCtx, inventory.LoadShareFile{}, true) + share, err := n.shareClient.GetByHashID(shareCtx, path.ID(hashid.EncodeUserID(n.hasher, n.user.ID))) + if err != nil { + return nil, ErrShareNotFound.WithError(err) + } + + if err := inventory.IsValidShare(share); err != nil { + return nil, ErrShareNotFound.WithError(err) + } + + if share.Password != "" && share.Password != path.Password() { + return nil, ErrShareIncorrectPassword + } + + n.share = share + n.owner = share.Edges.User + if n.shareRoot != nil { + n.shareRoot.OwnerModel = n.owner + } + if n.ownerRoot != nil { + n.ownerRoot.OwnerModel = n.owner + } + n.restoredState = false + } + if n.shareRoot == nil { root, err := n.Root(ctx, path) if err != nil { diff --git a/service/explorer/file.go b/service/explorer/file.go index e82a60c3..fe44e942 100644 --- a/service/explorer/file.go +++ b/service/explorer/file.go @@ -678,7 +678,7 @@ func RedirectDirectLink(c *gin.Context, name string, download bool) error { // Request entity URL expire := time.Now().Add(settings.EntityUrlValidDuration(c)) - res, earliestExpire, err := m.GetUrlForRedirectedDirectLink(c, dl, + res, _, err := m.GetUrlForRedirectedDirectLink(c, dl, fs.WithUrlExpire(&expire), fs.WithIsDownload(download), ) @@ -686,8 +686,8 @@ func RedirectDirectLink(c *gin.Context, name string, download bool) error { return err } + c.Header("Cache-Control", "no-store") c.Redirect(http.StatusFound, res) - c.Header("Cache-Control", fmt.Sprintf("public, max-age=%d", int(earliestExpire.Sub(time.Now()).Seconds()))) return nil } From e4a0e888b5097a87e47422dc3ebd9468e84bdd75 Mon Sep 17 00:00:00 2001 From: Darren Yu Date: Mon, 3 Aug 2026 01:29:48 +0800 Subject: [PATCH 2/2] Update share_navigator.go Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- pkg/filemanager/fs/dbfs/share_navigator.go | 21 ++++ .../fs/dbfs/share_navigator_test.go | 105 ++++++++++++++++++ 2 files changed, 126 insertions(+) create mode 100644 pkg/filemanager/fs/dbfs/share_navigator_test.go diff --git a/pkg/filemanager/fs/dbfs/share_navigator.go b/pkg/filemanager/fs/dbfs/share_navigator.go index 11823e88..f0e8c933 100644 --- a/pkg/filemanager/fs/dbfs/share_navigator.go +++ b/pkg/filemanager/fs/dbfs/share_navigator.go @@ -202,8 +202,29 @@ func (n *shareNavigator) To(ctx context.Context, path *fs.URI) (*File, error) { n.share = share n.owner = share.Edges.User + + // Root() is skipped when using restored state, so re-check requester permissions + // to ensure group permission changes take effect immediately. + if n.user.ID != n.owner.ID && !n.user.Edges.Group.Permissions.Enabled(int(types.GroupPermissionShareDownload)) { + if inventory.IsAnonymousUser(n.user) { + return nil, serializer.NewError( + serializer.CodeAnonymouseAccessDenied, + fmt.Sprintf("You don't have permission to access share links"), + nil, + ) + } + + return nil, serializer.NewError( + serializer.CodeNoPermissionErr, + fmt.Sprintf("You don't have permission to access share links"), + nil, + ) + } + if n.shareRoot != nil { n.shareRoot.OwnerModel = n.owner + n.shareRoot.disableView = (share.Props == nil || !share.Props.ShareView) && n.user.ID != n.owner.ID + n.shareRoot.CapabilitiesBs = n.Capabilities(false).Capability } if n.ownerRoot != nil { n.ownerRoot.OwnerModel = n.owner diff --git a/pkg/filemanager/fs/dbfs/share_navigator_test.go b/pkg/filemanager/fs/dbfs/share_navigator_test.go new file mode 100644 index 00000000..b696a9c7 --- /dev/null +++ b/pkg/filemanager/fs/dbfs/share_navigator_test.go @@ -0,0 +1,105 @@ +package dbfs + +import ( + "context" + "testing" + + "github.com/cloudreve/Cloudreve/v4/ent" + entuser "github.com/cloudreve/Cloudreve/v4/ent/user" + "github.com/cloudreve/Cloudreve/v4/inventory" + "github.com/cloudreve/Cloudreve/v4/inventory/types" + "github.com/cloudreve/Cloudreve/v4/pkg/boolset" + "github.com/cloudreve/Cloudreve/v4/pkg/filemanager/fs" + "github.com/cloudreve/Cloudreve/v4/pkg/hashid" + "github.com/cloudreve/Cloudreve/v4/pkg/setting" +) + +type restoredStateShareClient struct { + inventory.ShareClient + share *ent.Share +} + +func (c *restoredStateShareClient) GetByHashID(context.Context, string) (*ent.Share, error) { + return c.share, nil +} + +func TestShareNavigatorRestoredStateRevalidatesRequester(t *testing.T) { + ownerPermissions := &boolset.BooleanSet{} + boolset.Set(types.GroupPermissionShare, true, ownerPermissions) + owner := &ent.User{ID: 1, Status: entuser.StatusActive} + owner.SetGroup(&ent.Group{Permissions: ownerPermissions}) + + file := &ent.File{OwnerID: owner.ID, FileChildren: 1} + share := &ent.Share{ID: 1, Props: &types.ShareProps{ShareView: true}} + share.SetUser(owner) + share.SetFile(file) + + hasher, err := hashid.New("restored-state-test") + if err != nil { + t.Fatalf("hashid.New() error = %v", err) + } + path, err := fs.NewUriFromString(fs.NewShareUri(hashid.EncodeShareID(hasher, share.ID), "")) + if err != nil { + t.Fatalf("fs.NewUriFromString() error = %v", err) + } + + tests := []struct { + name string + canDownload bool + wantErr bool + }{ + {name: "permission revoked", wantErr: true}, + {name: "permission retained", canDownload: true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + requesterPermissions := &boolset.BooleanSet{} + boolset.Set(types.GroupPermissionShareDownload, tt.canDownload, requesterPermissions) + requester := &ent.User{ID: 2, Status: entuser.StatusActive} + requester.SetGroup(&ent.Group{Permissions: requesterPermissions}) + + cachedOwner := &ent.User{ID: owner.ID, Status: entuser.StatusActive} + root := newFile(nil, file) + t.Cleanup(root.Recycle) + root.OwnerModel = cachedOwner + root.disableView = true + root.CapabilitiesBs = &boolset.BooleanSet{0xff} + + navigator := NewShareNavigator( + requester, + nil, + &restoredStateShareClient{share: share}, + nil, + &setting.DBFS{}, + hasher, + ).(*shareNavigator) + if err := navigator.RestoreState(shareNavigatorState{ + ShareRoot: root, + OwnerRoot: root, + Share: share, + Owner: cachedOwner, + }); err != nil { + t.Fatalf("RestoreState() error = %v", err) + } + + _, err := navigator.To(context.Background(), path) + if (err != nil) != tt.wantErr { + t.Fatalf("To() error = %v, wantErr %v", err, tt.wantErr) + } + if tt.wantErr { + return + } + + if root.OwnerModel != owner { + t.Fatal("To() did not refresh the cached owner") + } + if root.disableView { + t.Fatal("To() did not refresh the cached share view setting") + } + if root.CapabilitiesBs != shareNavigatorCapability { + t.Fatal("To() did not refresh the cached navigator capabilities") + } + }) + } +}