upstream/nextcloud
1
0
Fork 0
mirror of https://github.com/nextcloud/server.git synced 2026-04-15 22:11:17 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 27

fix(sharing): Prevent generated password from overwriting user input

Browse source 
Signed-off-by: nfebe <fenn25.fn@gmail.com>
This commit is contained in:
nfebe 2026-03-19 13:05:03 +01:00 committed by skjnldsv
parent 56fdf0ed37
commit f482c3fa96
No known key found for this signature in database
GPG key ID: 20CCE4F37ED06566
3 changed files with 164 additions and 188 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
apps/files_sharing/src/mixins/SharesMixin.js
View file

@ -179,7 +179,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', '')

342
apps/files_sharing/src/views/SharingDetailsTab.spec.ts
View file

@ -5,6 +5,8 @@
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,
@ -24,180 +26,202 @@ vi.mock('../services/ConfigService.ts', () => ({
}))
vi.mock('../utils/GeneratePassword.ts', () => ({
default: vi.fn().mockResolvedValue('generated-password-123'),
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', () => {
describe('isPasswordProtected getter 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', () => {
const passwordProtectedState: boolean | undefined = true
const enforcePasswordForPublicLink = false
const newPassword: string | undefined = undefined
const password: string | undefined = undefined
const isPasswordProtected = (() => {
if (enforcePasswordForPublicLink) {
return true
}
if (passwordProtectedState !== undefined) {
return passwordProtectedState
}
return typeof newPassword === 'string'
|| typeof password === 'string'
})()
expect(isPasswordProtected).toBe(true)
expect(getIsPasswordProtected({
enforcePasswordForPublicLink: false,
passwordProtectedState: true,
newPassword: undefined,
password: undefined,
})).toBe(true)
})
it('returns false when passwordProtectedState is explicitly false', () => {
const passwordProtectedState: boolean | undefined = false
const enforcePasswordForPublicLink = false
const newPassword: string | undefined = 'some-password'
const password: string | undefined = undefined
const isPasswordProtected = (() => {
if (enforcePasswordForPublicLink) {
return true
}
if (passwordProtectedState !== undefined) {
return passwordProtectedState
}
return typeof newPassword === 'string'
|| typeof password === 'string'
})()
expect(isPasswordProtected).toBe(false)
expect(getIsPasswordProtected({
enforcePasswordForPublicLink: false,
passwordProtectedState: false,
newPassword: 'some-password',
password: undefined,
})).toBe(false)
})
it('returns true when enforcePasswordForPublicLink is true regardless of other state', () => {
const passwordProtectedState: boolean | undefined = false
const enforcePasswordForPublicLink = true
const newPassword: string | undefined = undefined
const password: string | undefined = undefined
const isPasswordProtected = (() => {
if (enforcePasswordForPublicLink) {
return true
}
if (passwordProtectedState !== undefined) {
return passwordProtectedState
}
return typeof newPassword === 'string'
|| typeof password === 'string'
})()
expect(isPasswordProtected).toBe(true)
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', () => {
const passwordProtectedState: boolean | undefined = undefined
const enforcePasswordForPublicLink = false
const newPassword: string | undefined = 'some-password'
const password: string | undefined = undefined
const isPasswordProtected = (() => {
if (enforcePasswordForPublicLink) {
return true
}
if (passwordProtectedState !== undefined) {
return passwordProtectedState
}
return typeof newPassword === 'string'
|| typeof password === 'string'
})()
expect(isPasswordProtected).toBe(true)
expect(getIsPasswordProtected({
enforcePasswordForPublicLink: false,
passwordProtectedState: undefined,
newPassword: undefined,
password: 'existing-password',
})).toBe(true)
})
it('returns false when passwordProtectedState is undefined and no passwords exist', () => {
const passwordProtectedState: boolean | undefined = undefined
const enforcePasswordForPublicLink = false
const newPassword: string | undefined = undefined
const password: string | undefined = undefined
expect(getIsPasswordProtected({
enforcePasswordForPublicLink: false,
passwordProtectedState: undefined,
newPassword: undefined,
password: undefined,
})).toBe(false)
})
const isPasswordProtected = (() => {
if (enforcePasswordForPublicLink) {
return true
}
if (passwordProtectedState !== undefined) {
return passwordProtectedState
}
return typeof newPassword === 'string'
|| typeof password === 'string'
})()
it('checkbox remains checked when passwordProtectedState is true even if password is cleared', () => {
expect(getIsPasswordProtected({
enforcePasswordForPublicLink: false,
passwordProtectedState: true,
newPassword: '',
password: undefined,
})).toBe(true)
})
})
expect(isPasswordProtected).toBe(false)
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 to true when enableLinkPasswordByDefault is true', async () => {
const config = {
enableLinkPasswordByDefault: true,
enforcePasswordForPublicLink: false,
}
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) {
if ((config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
passwordProtectedState = true
}
if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
passwordProtectedState = true
}
expect(passwordProtectedState).toBe(true)
})
it('should set passwordProtectedState to true when isPasswordEnforced is true', async () => {
const config = {
enableLinkPasswordByDefault: false,
enforcePasswordForPublicLink: 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) {
if ((config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
passwordProtectedState = true
}
if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
passwordProtectedState = true
}
expect(passwordProtectedState).toBe(true)
})
it('should not set passwordProtectedState for non-public shares', async () => {
const config = {
enableLinkPasswordByDefault: true,
enforcePasswordForPublicLink: false,
}
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) {
if ((config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
passwordProtectedState = true
}
if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
passwordProtectedState = true
}
expect(passwordProtectedState).toBe(undefined)
})
it('should not set passwordProtectedState for existing shares', async () => {
const config = {
enableLinkPasswordByDefault: true,
enforcePasswordForPublicLink: false,
}
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) {
if ((config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
passwordProtectedState = true
}
if (isNewShare && (config.enableLinkPasswordByDefault || config.enforcePasswordForPublicLink) && isPublicShare) {
passwordProtectedState = true
}
expect(passwordProtectedState).toBe(undefined)
@ -215,10 +239,8 @@ describe('SharingDetailsTab - Password State Management Logic', () => {
const newPassword = ''
let passwordError = false
if (isPasswordProtected) {
if (isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
expect(passwordError).toBe(true)
@ -230,10 +252,8 @@ describe('SharingDetailsTab - Password State Management Logic', () => {
const newPassword = undefined
let passwordError = false
if (isPasswordProtected) {
if (isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
expect(passwordError).toBe(true)
@ -245,10 +265,8 @@ describe('SharingDetailsTab - Password State Management Logic', () => {
const newPassword = 'valid-password-123'
let passwordError = false
if (isPasswordProtected) {
if (isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
expect(passwordError).toBe(false)
@ -260,10 +278,8 @@ describe('SharingDetailsTab - Password State Management Logic', () => {
const newPassword = ''
let passwordError = false
if (isPasswordProtected) {
if (isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
expect(passwordError).toBe(false)
@ -275,57 +291,11 @@ describe('SharingDetailsTab - Password State Management Logic', () => {
const newPassword = ''
let passwordError = false
if (isPasswordProtected) {
if (isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
if (isPasswordProtected && isNewShare && !isValidShareAttribute(newPassword)) {
passwordError = true
}
expect(passwordError).toBe(false)
})
})
describe('checkbox persistence after clearing password', () => {
it('checkbox remains checked when passwordProtectedState is explicitly true even if password is cleared', () => {
let passwordProtectedState: boolean | undefined = true
const enforcePasswordForPublicLink = false
let newPassword: string | undefined = 'initial-password'
newPassword = ''
const isPasswordProtected = (() => {
if (enforcePasswordForPublicLink) {
return true
}
if (passwordProtectedState !== undefined) {
return passwordProtectedState
}
return typeof newPassword === 'string'
|| false
})()
expect(isPasswordProtected).toBe(true)
})
it('checkbox unchecks incorrectly if passwordProtectedState was never set (bug scenario)', () => {
let passwordProtectedState: boolean | undefined = undefined
const enforcePasswordForPublicLink = false
let newPassword: string | undefined = 'initial-password'
newPassword = undefined
const isPasswordProtected = (() => {
if (enforcePasswordForPublicLink) {
return true
}
if (passwordProtectedState !== undefined) {
return passwordProtectedState
}
return typeof newPassword === 'string'
|| false
})()
expect(isPasswordProtected).toBe(false)
})
})
})

5
apps/files_sharing/src/views/SharingDetailsTab.vue
View file

@ -901,7 +901,10 @@ export default {
if (this.isNewShare) {
if ((this.config.enableLinkPasswordByDefault || this.isPasswordEnforced) && this.isPublicShare) {
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)
}
this.advancedSectionAccordionExpanded = true
}
/* Set default expiration dates if configured */