Merge pull request #59090 from nextcloud/backport/58226/stable33

[stable33] fix(sharing): Prevent empty password when checkbox is enabled

Author: tobiasKaminsky

apps/files_sharing/src/mixins/SharesMixin.js

@@ -180,7 +180,10 @@ export default {
 			async set(enabled) {
 				if (enabled) {
 					this.passwordProtectedState = true
-					this.$set(this.share, 'newPassword', await GeneratePassword(true))
+					const generatedPassword = await GeneratePassword(true)
+					if (!this.share.newPassword) {
+						this.$set(this.share, 'newPassword', generatedPassword)
+					}
 				} else {
 					this.passwordProtectedState = false
 					this.$set(this.share, 'newPassword', '')

apps/files_sharing/src/views/SharingDetailsTab.spec.ts

@@ -0,0 +1,301 @@
+/**
+ * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
+ * SPDX-License-Identifier: AGPL-3.0-or-later
+ */
+
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+const mockGeneratePassword = vi.fn().mockResolvedValue('generated-password-123')
+
+vi.mock('../services/ConfigService.ts', () => ({
+	default: vi.fn().mockImplementation(() => ({
+		enableLinkPasswordByDefault: false,
+		enforcePasswordForPublicLink: false,
+		isPublicUploadEnabled: true,
+		isDefaultExpireDateEnabled: false,
+		isDefaultInternalExpireDateEnabled: false,
+		isDefaultRemoteExpireDateEnabled: false,
+		defaultExpirationDate: null,
+		defaultInternalExpirationDate: null,
+		defaultRemoteExpirationDateString: null,
+		isResharingAllowed: true,
+		excludeReshareFromEdit: false,
+		showFederatedSharesAsInternal: false,
+		defaultPermissions: 31,
+	})),
+}))
+
+vi.mock('../utils/GeneratePassword.ts', () => ({
+	default: (...args: unknown[]) => mockGeneratePassword(...args),
+}))
+
+/**
+ * Simulates the isPasswordProtected getter from SharesMixin.js
+ */
+function getIsPasswordProtected(state: {
+	enforcePasswordForPublicLink: boolean
+	passwordProtectedState: boolean | undefined
+	newPassword: string | undefined
+	password: string | undefined
+}): boolean {
+	if (state.enforcePasswordForPublicLink) {
+		return true
+	}
+	if (state.passwordProtectedState !== undefined) {
+		return state.passwordProtectedState
+	}
+	return typeof state.newPassword === 'string'
+		|| typeof state.password === 'string'
+}
+
+/**
+ * Simulates the isPasswordProtected setter from SharesMixin.js
+ * Returns the resulting share state after the async operation completes.
+ */
+async function setIsPasswordProtected(
+	enabled: boolean,
+	share: { newPassword?: string },
+): Promise<{ passwordProtectedState: boolean, share: { newPassword?: string } }> {
+	if (enabled) {
+		const generatedPassword = await mockGeneratePassword(true)
+		if (!share.newPassword) {
+			share.newPassword = generatedPassword
+		}
+		return { passwordProtectedState: true, share }
+	} else {
+		share.newPassword = ''
+		return { passwordProtectedState: false, share }
+	}
+}
+
+describe('SharingDetailsTab - Password State Management Logic', () => {
+	beforeEach(() => {
+		mockGeneratePassword.mockClear()
+		mockGeneratePassword.mockResolvedValue('generated-password-123')
+	})
+
+	describe('isPasswordProtected getter', () => {
+		it('returns true when enforcePasswordForPublicLink is true regardless of other state', () => {
+			expect(getIsPasswordProtected({
+				enforcePasswordForPublicLink: true,
+				passwordProtectedState: false,
+				newPassword: undefined,
+				password: undefined,
+			})).toBe(true)
+		})
+
+		it('returns true when passwordProtectedState is explicitly true', () => {
+			expect(getIsPasswordProtected({
+				enforcePasswordForPublicLink: false,
+				passwordProtectedState: true,
+				newPassword: undefined,
+				password: undefined,
+			})).toBe(true)
+		})
+
+		it('returns false when passwordProtectedState is explicitly false', () => {
+			expect(getIsPasswordProtected({
+				enforcePasswordForPublicLink: false,
+				passwordProtectedState: false,
+				newPassword: 'some-password',
+				password: undefined,
+			})).toBe(false)
+		})
+
+		it('falls back to inferring from newPassword when passwordProtectedState is undefined', () => {
+			expect(getIsPasswordProtected({
+				enforcePasswordForPublicLink: false,
+				passwordProtectedState: undefined,
+				newPassword: 'some-password',
+				password: undefined,
+			})).toBe(true)
+		})
+
+		it('falls back to inferring from password when passwordProtectedState is undefined', () => {
+			expect(getIsPasswordProtected({
+				enforcePasswordForPublicLink: false,
+				passwordProtectedState: undefined,
+				newPassword: undefined,
+				password: 'existing-password',
+			})).toBe(true)
+		})
+
+		it('returns false when passwordProtectedState is undefined and no passwords exist', () => {
+			expect(getIsPasswordProtected({
+				enforcePasswordForPublicLink: false,
+				passwordProtectedState: undefined,
+				newPassword: undefined,
+				password: undefined,
+			})).toBe(false)
+		})
+
+		it('checkbox remains checked when passwordProtectedState is true even if password is cleared', () => {
+			expect(getIsPasswordProtected({
+				enforcePasswordForPublicLink: false,
+				passwordProtectedState: true,
+				newPassword: '',
+				password: undefined,
+			})).toBe(true)
+		})
+	})
+
+	describe('isPasswordProtected setter (race condition fix)', () => {
+		it('generated password does NOT overwrite user-typed password', async () => {
+			const share = { newPassword: 'user-typed-password' }
+			const result = await setIsPasswordProtected(true, share)
+
+			expect(mockGeneratePassword).toHaveBeenCalledWith(true)
+			expect(result.passwordProtectedState).toBe(true)
+			expect(result.share.newPassword).toBe('user-typed-password')
+		})
+
+		it('generated password IS applied when user has not typed anything', async () => {
+			const share: { newPassword?: string } = {}
+			const result = await setIsPasswordProtected(true, share)
+
+			expect(mockGeneratePassword).toHaveBeenCalledWith(true)
+			expect(result.passwordProtectedState).toBe(true)
+			expect(result.share.newPassword).toBe('generated-password-123')
+		})
+
+		it('generated password IS applied when newPassword is empty string (user cleared input)', async () => {
+			const share = { newPassword: '' }
+			const result = await setIsPasswordProtected(true, share)
+
+			expect(result.share.newPassword).toBe('generated-password-123')
+		})
+
+		it('disabling password clears newPassword and sets state to false', async () => {
+			const share = { newPassword: 'some-password' }
+			const result = await setIsPasswordProtected(false, share)
+
+			expect(result.passwordProtectedState).toBe(false)
+			expect(result.share.newPassword).toBe('')
+		})
+	})
+
+	describe('initializeAttributes sets passwordProtectedState', () => {
+		it('should set passwordProtectedState when enableLinkPasswordByDefault is true for new public share', () => {
+			const config = { enableLinkPasswordByDefault: true, enforcePasswordForPublicLink: false }
+			const isNewShare = true
+			const isPublicShare = true
+			let passwordProtectedState: boolean | undefined
+
+			if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
+				passwordProtectedState = true
+			}
+
+			expect(passwordProtectedState).toBe(true)
+		})
+
+		it('should set passwordProtectedState when isPasswordEnforced is true for new public share', () => {
+			const config = { enableLinkPasswordByDefault: false, enforcePasswordForPublicLink: true }
+			const isNewShare = true
+			const isPublicShare = true
+			let passwordProtectedState: boolean | undefined
+
+			if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
+				passwordProtectedState = true
+			}
+
+			expect(passwordProtectedState).toBe(true)
+		})
+
+		it('should not set passwordProtectedState for non-public shares', () => {
+			const config = { enableLinkPasswordByDefault: true, enforcePasswordForPublicLink: false }
+			const isNewShare = true
+			const isPublicShare = false
+			let passwordProtectedState: boolean | undefined
+
+			if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
+				passwordProtectedState = true
+			}
+
+			expect(passwordProtectedState).toBe(undefined)
+		})
+
+		it('should not set passwordProtectedState for existing shares', () => {
+			const config = { enableLinkPasswordByDefault: true, enforcePasswordForPublicLink: false }
+			const isNewShare = false
+			const isPublicShare = true
+			let passwordProtectedState: boolean | undefined
+
+			if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
+				passwordProtectedState = true
+			}
+
+			expect(passwordProtectedState).toBe(undefined)
+		})
+	})
+
+	describe('saveShare validation blocks empty password', () => {
+		const isValidShareAttribute = (attr: unknown) => {
+			return typeof attr === 'string' && attr.length > 0
+		}
+
+		it('should set passwordError when isPasswordProtected but newPassword is empty for new share', () => {
+			const isPasswordProtected = true
+			const isNewShare = true
+			const newPassword = ''
+			let passwordError = false
+
+			if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
+				passwordError = true
+			}
+
+			expect(passwordError).toBe(true)
+		})
+
+		it('should set passwordError when isPasswordProtected but newPassword is undefined for new share', () => {
+			const isPasswordProtected = true
+			const isNewShare = true
+			const newPassword = undefined
+			let passwordError = false
+
+			if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
+				passwordError = true
+			}
+
+			expect(passwordError).toBe(true)
+		})
+
+		it('should not set passwordError when password is valid for new share', () => {
+			const isPasswordProtected = true
+			const isNewShare = true
+			const newPassword = 'valid-password-123'
+			let passwordError = false
+
+			if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
+				passwordError = true
+			}
+
+			expect(passwordError).toBe(false)
+		})
+
+		it('should not set passwordError when isPasswordProtected is false', () => {
+			const isPasswordProtected = false
+			const isNewShare = true
+			const newPassword = ''
+			let passwordError = false
+
+			if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
+				passwordError = true
+			}
+
+			expect(passwordError).toBe(false)
+		})
+
+		it('should not validate password for existing shares', () => {
+			const isPasswordProtected = true
+			const isNewShare = false
+			const newPassword = ''
+			let passwordError = false
+
+			if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
+				passwordError = true
+			}
+
+			expect(passwordError).toBe(false)
+		})
+	})
+})

apps/files_sharing/src/views/SharingDetailsTab.vue

@@ -974,7 +974,11 @@ export default {
 		async initializeAttributes() {
 			if (this.isNewShare) {
 				if ((this.config.enableLinkPasswordByDefault || this.isPasswordEnforced) && this.isPublicShare) {
-					this.$set(this.share, 'newPassword', await GeneratePassword(true))
+					this.passwordProtectedState = true
+					const generatedPassword = await GeneratePassword(true)
+					if (!this.share.newPassword) {
+						this.$set(this.share, 'newPassword', generatedPassword)
+					}
 					this.advancedSectionAccordionExpanded = true
 				}
 				/* Set default expiration dates if configured */
@@ -1087,8 +1091,9 @@ export default {
 				this.share.note = ''
 			}
 			if (this.isPasswordProtected) {
-				if (this.isPasswordEnforced && this.isNewShare && !this.isValidShareAttribute(this.share.newPassword)) {
+				if (this.isNewShare && !this.isValidShareAttribute(this.share.newPassword)) {
 					this.passwordError = true
+					return
 				}
 			} else {
 				this.share.password = ''

dist/2517-2517.js

@@ -0,0 +1 @@
+9809-9809.js.license