Skip to content

fix: smb: don't fail hard if we can't load acls for a file #49288

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.

Sign up for GitHub

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

Jump to bottom
Merged
icewind1991 merged 1 commit into from
Dec 4, 2024
Merged

fix: smb: don't fail hard if we can't load acls for a file #49288

icewind1991 merged 1 commit into from
Dec 4, 2024

Conversation

@icewind1991
Copy link
Member

@icewind1991 icewind1991 commented Nov 14, 2024

Instead we log the error and behave as if no ACL was found.

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Nov 14, 2024
@icewind1991 icewind1991 added this to the Nextcloud 31 milestone Nov 14, 2024
@icewind1991 icewind1991 requested review from a team, ArtificialOwl, come-nc and nfebe and removed request for a team November 14, 2024 15:02
come-nc
come-nc reviewed Nov 14, 2024
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks dangerous, it means when unable to get acls it gives default rights to the user? So it may result in more rights because of some system hiccup, no?

@icewind1991
Copy link
Member Author

icewind1991 commented Nov 14, 2024

the acl check only hides folders that users don't have access to from the file listing. Any attempt to enter the folder when the acl check is disabled/not working will result in the user seeing an empty folder.

@come-nc
Copy link
Contributor

come-nc commented Nov 18, 2024

the acl check only hides folders that users don't have access to from the file listing. Any attempt to enter the folder when the acl check is disabled/not working will result in the user seeing an empty folder.

And seeing the folder name is not already an information leak?

@icewind1991
Copy link
Member Author

icewind1991 commented Nov 26, 2024

Windows has the same behavior of showing the non-readable folders

@icewind1991 icewind1991 requested a review from artonge December 3, 2024 18:11
@icewind1991 icewind1991 merged commit 0e10bb5 into master Dec 4, 2024
189 checks passed
@icewind1991 icewind1991 deleted the smb-acl-fail-soft branch December 4, 2024 17:22
@icewind1991
Copy link
Member Author

icewind1991 commented Dec 4, 2024

/backport to stable30

@icewind1991
Copy link
Member Author

icewind1991 commented Dec 4, 2024

/backport to stable29

@backportbot backportbot bot mentioned this pull request Dec 4, 2024
Merged
@backportbot backportbot bot mentioned this pull request Dec 4, 2024
Merged
@skjnldsv skjnldsv mentioned this pull request Jan 7, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@artonge artonge artonge approved these changes

@come-nc come-nc come-nc approved these changes

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl ArtificialOwl is a code owner automatically assigned from nextcloud/server-backend

@nfebe nfebe Awaiting requested review from nfebe nfebe is a code owner automatically assigned from nextcloud/server-backend

Assignees

No one assigned

Labels

3. to review Waiting for reviews

Projects

None yet

Milestone

Nextcloud 31

Development

Successfully merging this pull request may close these issues.

4 participants

@icewind1991 @come-nc @artonge