Browse Source

Merge pull request #44438 from nextcloud/feat/login-form-timeout

feat(login): Clear login form (password) after IDLE timeout
Ferdinand Thiessen 1 month ago
parent
commit
7d51b6fff5

+ 8 - 0
config/config.sample.php

@@ -2307,6 +2307,14 @@ $CONFIG = [
 
 'login_form_autocomplete' => true,
 
+/**
+ * Timeout for the login form, after this time the login form is reset.
+ * This prevents password leaks on public devices if the user forgots to clear the form.
+ * 
+ * Default is 5 minutes (300 seconds), a value of 0 means no timeout.
+ */
+'login_form_timeout' => 300,
+
 /**
  * If your user is using an outdated or unsupported browser, a warning will be shown
  * to offer some guidance to upgrade or switch and ensure a proper Nextcloud experience.

+ 17 - 24
core/Controller/LoginController.php

@@ -53,9 +53,9 @@ use OCP\AppFramework\Http\Attribute\UseSession;
 use OCP\AppFramework\Http\DataResponse;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\AppFramework\Services\IInitialState;
 use OCP\Defaults;
 use OCP\IConfig;
-use OCP\IInitialStateService;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\ISession;
@@ -81,7 +81,7 @@ class LoginController extends Controller {
 		private IURLGenerator $urlGenerator,
 		private Defaults $defaults,
 		private IThrottler $throttler,
-		private IInitialStateService $initialStateService,
+		private IInitialState $initialState,
 		private WebAuthnManager $webAuthnManager,
 		private IManager $manager,
 		private IL10N $l10n,
@@ -148,19 +148,18 @@ class LoginController extends Controller {
 		}
 		if (is_array($loginMessages)) {
 			[$errors, $messages] = $loginMessages;
-			$this->initialStateService->provideInitialState('core', 'loginMessages', $messages);
-			$this->initialStateService->provideInitialState('core', 'loginErrors', $errors);
+			$this->initialState->provideInitialState('loginMessages', $messages);
+			$this->initialState->provideInitialState('loginErrors', $errors);
 		}
 		$this->session->remove('loginMessages');
 
 		if ($user !== null && $user !== '') {
-			$this->initialStateService->provideInitialState('core', 'loginUsername', $user);
+			$this->initialState->provideInitialState('loginUsername', $user);
 		} else {
-			$this->initialStateService->provideInitialState('core', 'loginUsername', '');
+			$this->initialState->provideInitialState('loginUsername', '');
 		}
 
-		$this->initialStateService->provideInitialState(
-			'core',
+		$this->initialState->provideInitialState(
 			'loginAutocomplete',
 			$this->config->getSystemValue('login_form_autocomplete', true) === true
 		);
@@ -168,12 +167,11 @@ class LoginController extends Controller {
 		if (!empty($redirect_url)) {
 			[$url, ] = explode('?', $redirect_url);
 			if ($url !== $this->urlGenerator->linkToRoute('core.login.logout')) {
-				$this->initialStateService->provideInitialState('core', 'loginRedirectUrl', $redirect_url);
+				$this->initialState->provideInitialState('loginRedirectUrl', $redirect_url);
 			}
 		}
 
-		$this->initialStateService->provideInitialState(
-			'core',
+		$this->initialState->provideInitialState(
 			'loginThrottleDelay',
 			$this->throttler->getDelay($this->request->getRemoteAddress())
 		);
@@ -182,9 +180,9 @@ class LoginController extends Controller {
 
 		$this->setEmailStates();
 
-		$this->initialStateService->provideInitialState('core', 'webauthn-available', $this->webAuthnManager->isWebAuthnAvailable());
+		$this->initialState->provideInitialState('webauthn-available', $this->webAuthnManager->isWebAuthnAvailable());
 
-		$this->initialStateService->provideInitialState('core', 'hideLoginForm', $this->config->getSystemValueBool('hide_login_form', false));
+		$this->initialState->provideInitialState('hideLoginForm', $this->config->getSystemValueBool('hide_login_form', false));
 
 		// OpenGraph Support: http://ogp.me/
 		Util::addHeader('meta', ['property' => 'og:title', 'content' => Util::sanitizeHTML($this->defaults->getName())]);
@@ -199,8 +197,9 @@ class LoginController extends Controller {
 			'pageTitle' => $this->l10n->t('Login'),
 		];
 
-		$this->initialStateService->provideInitialState('core', 'countAlternativeLogins', count($parameters['alt_login']));
-		$this->initialStateService->provideInitialState('core', 'alternativeLogins', $parameters['alt_login']);
+		$this->initialState->provideInitialState('countAlternativeLogins', count($parameters['alt_login']));
+		$this->initialState->provideInitialState('alternativeLogins', $parameters['alt_login']);
+		$this->initialState->provideInitialState('loginTimeout', $this->config->getSystemValueInt('login_form_timeout', 5 * 60));
 
 		return new TemplateResponse(
 			$this->appName,
@@ -224,14 +223,12 @@ class LoginController extends Controller {
 
 		$passwordLink = $this->config->getSystemValueString('lost_password_link', '');
 
-		$this->initialStateService->provideInitialState(
-			'core',
+		$this->initialState->provideInitialState(
 			'loginResetPasswordLink',
 			$passwordLink
 		);
 
-		$this->initialStateService->provideInitialState(
-			'core',
+		$this->initialState->provideInitialState(
 			'loginCanResetPassword',
 			$this->canResetPassword($passwordLink, $user)
 		);
@@ -255,11 +252,7 @@ class LoginController extends Controller {
 				array_push($emailStates, $emailConfig->__get('ldapLoginFilterEmail'));
 			}
 		}
-		$this->initialStateService->
-			provideInitialState(
-				'core',
-				'emailStates',
-				$emailStates);
+		$this->initialState->provideInitialState('emailStates', $emailStates);
 	}
 
 	/**

+ 2 - 0
core/src/components/login/LoginButton.vue

@@ -33,6 +33,8 @@
 </template>
 
 <script>
+import { translate as t } from '@nextcloud/l10n'
+
 import NcButton from '@nextcloud/vue/dist/Components/NcButton.js'
 import ArrowRight from 'vue-material-design-icons/ArrowRight.vue'
 

+ 72 - 0
core/src/components/login/LoginForm.cy.ts

@@ -0,0 +1,72 @@
+import LoginForm from './LoginForm.vue'
+
+describe('core: LoginForm', { testIsolation: true }, () => {
+	beforeEach(() => {
+		// Mock the required global state
+		cy.window().then(($window) => {
+			$window.OC = {
+				theme: {
+					name: 'J\'s cloud',
+				},
+				requestToken: 'request-token',
+			}
+		})
+	})
+
+	/**
+	 * Ensure that characters like ' are not double HTML escaped.
+	 * This was a bug in https://github.com/nextcloud/server/issues/34990
+	 */
+	it('does not double escape special characters in product name', () => {
+		cy.mount(LoginForm, {
+			propsData: {
+				username: 'test-user',
+			},
+		})
+
+		cy.get('h2').contains('J\'s cloud')
+	})
+
+	it('fills username from props into form', () => {
+		cy.mount(LoginForm, {
+			propsData: {
+				username: 'test-user',
+			},
+		})
+
+		cy.get('input[name="user"]')
+			.should('exist')
+			.and('have.attr', 'id', 'user')
+
+		cy.get('input[name="user"]')
+			.should('have.value', 'test-user')
+	})
+
+	it('clears password after timeout', () => {
+		// mock timeout of 5 seconds
+		cy.window().then(($window) => {
+			const state = $window.document.createElement('input')
+			state.type = 'hidden'
+			state.id = 'initial-state-core-loginTimeout'
+			state.value = btoa(JSON.stringify(5))
+			$window.document.body.appendChild(state)
+		})
+
+		// mount forms
+		cy.mount(LoginForm)
+
+		cy.get('input[name="password"]')
+			.should('exist')
+			.type('MyPassword')
+
+		cy.get('input[name="password"]')
+			.should('have.value', 'MyPassword')
+
+		// Wait for timeout
+		// eslint-disable-next-line cypress/no-unnecessary-waiting
+		cy.wait(5100)
+
+		cy.get('input[name="password"]')
+			.should('have.value', '')
+	})
+})

+ 54 - 7
core/src/components/login/LoginForm.vue

@@ -57,7 +57,9 @@
 				<!-- the following div ensures that the spinner is always inside the #message div -->
 				<div style="clear: both;" />
 			</div>
-			<h2 class="login-form__headline" data-login-form-headline v-html="headline" />
+			<h2 class="login-form__headline" data-login-form-headline>
+				{{ headlineText }}
+			</h2>
 			<NcTextField id="user"
 				ref="user"
 				:label="loginText"
@@ -102,7 +104,7 @@
 				:value="timezoneOffset">
 			<input type="hidden"
 				name="requesttoken"
-				:value="OC.requestToken">
+				:value="requestToken">
 			<input v-if="directLogin"
 				type="hidden"
 				name="direct"
@@ -112,15 +114,17 @@
 </template>
 
 <script>
+import { loadState } from '@nextcloud/initial-state'
+import { translate as t } from '@nextcloud/l10n'
 import { generateUrl, imagePath } from '@nextcloud/router'
+import { debounce } from 'debounce'
 
 import NcPasswordField from '@nextcloud/vue/dist/Components/NcPasswordField.js'
 import NcTextField from '@nextcloud/vue/dist/Components/NcTextField.js'
 import NcNoteCard from '@nextcloud/vue/dist/Components/NcNoteCard.js'
 
-import LoginButton from './LoginButton.vue'
-
 import AuthMixin from '../../mixins/auth.js'
+import LoginButton from './LoginButton.vue'
 
 export default {
 	name: 'LoginForm',
@@ -131,6 +135,7 @@ export default {
 		NcTextField,
 		NcNoteCard,
 	},
+
 	mixins: [AuthMixin],
 
 	props: {
@@ -170,18 +175,43 @@ export default {
 		},
 	},
 
-	data() {
+	setup() {
+		// non reactive props
 		return {
-			loading: false,
+			t,
+
+			// Disable escape and sanitize to prevent special characters to be html escaped
+			// For example "J's cloud" would be escaped to "J&#39; cloud". But we do not need escaping as Vue does this in `v-text` automatically
+			headlineText: t('core', 'Log in to {productName}', { productName: OC.theme.name }, undefined, { sanitize: false, escape: false }),
+
+			loginTimeout: loadState('core', 'loginTimeout', 300),
+			requestToken: window.OC.requestToken,
 			timezone: (new Intl.DateTimeFormat())?.resolvedOptions()?.timeZone,
 			timezoneOffset: (-new Date().getTimezoneOffset() / 60),
-			headline: t('core', 'Log in to {productName}', { productName: OC.theme.name }),
+		}
+	},
+
+	data() {
+		return {
+			loading: false,
 			user: '',
 			password: '',
 		}
 	},
 
 	computed: {
+		/**
+		 * Reset the login form after a long idle time (debounced)
+		 */
+		resetFormTimeout() {
+			// Infinite timeout, do nothing
+			if (this.loginTimeout <= 0) {
+				return () => {}
+			}
+			// Debounce for given timeout (in seconds so convert to milli seconds)
+			return debounce(this.handleResetForm, this.loginTimeout * 1000)
+		},
+
 		isError() {
 			return this.invalidPassword || this.userDisabled
 				|| this.throttleDelay > 5000
@@ -230,6 +260,15 @@ export default {
 		},
 	},
 
+	watch: {
+		/**
+		 * Reset form reset after the password was changed
+		 */
+		password() {
+			this.resetFormTimeout()
+		},
+	},
+
 	mounted() {
 		if (this.username === '') {
 			this.$refs.user.$refs.inputField.$refs.input.focus()
@@ -240,6 +279,14 @@ export default {
 	},
 
 	methods: {
+		/**
+		 * Handle reset of the login form after a long IDLE time
+		 * This is recommended security behavior to prevent password leak on public devices
+		 */
+		handleResetForm() {
+			this.password = ''
+		},
+
 		updateUsername() {
 			this.$emit('update:username', this.user)
 		},

+ 1 - 1
cypress/support/component.ts

@@ -36,7 +36,7 @@ Cypress.Commands.add('mount', (component, optionsOrProps) => {
 		// eslint-disable-next-line
 		instance = this
 		if (oldMounted) {
-			oldMounted()
+			oldMounted.call(instance)
 		}
 	}
 

File diff suppressed because it is too large
+ 0 - 0
dist/core-login.js


+ 22 - 0
dist/core-login.js.LICENSE.txt

@@ -328,6 +328,28 @@
  *
  */
 
+/**
+ * @copyright Copyright (c) 2024 Fon E. Noel NFEBE <opensource@nfebe.com>
+ *
+ * @author Fon E. Noel NFEBE <opensource@nfebe.com>
+ *
+ * @license AGPL-3.0-or-later
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as
+ * published by the Free Software Foundation, either version 3 of the
+ * License, or (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
 /**
  * Copyright (c) 2014 Vincent Petry <pvince81@owncloud.com>
  * Copyright (c) 2005-2010 Zend Technologies USA Inc. (http://www.zend.com)

File diff suppressed because it is too large
+ 0 - 0
dist/core-login.js.map


+ 9 - 19
tests/Core/Controller/LoginControllerTest.php

@@ -33,9 +33,9 @@ use OC\User\Session;
 use OCP\App\IAppManager;
 use OCP\AppFramework\Http\RedirectResponse;
 use OCP\AppFramework\Http\TemplateResponse;
+use OCP\AppFramework\Services\IInitialState;
 use OCP\Defaults;
 use OCP\IConfig;
-use OCP\IInitialStateService;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\ISession;
@@ -78,8 +78,8 @@ class LoginControllerTest extends TestCase {
 	/** @var IThrottler|MockObject */
 	private $throttler;
 
-	/** @var IInitialStateService|MockObject */
-	private $initialStateService;
+	/** @var IInitialState|MockObject */
+	private $initialState;
 
 	/** @var \OC\Authentication\WebAuthn\Manager|MockObject */
 	private $webAuthnManager;
@@ -104,7 +104,7 @@ class LoginControllerTest extends TestCase {
 		$this->twoFactorManager = $this->createMock(Manager::class);
 		$this->defaults = $this->createMock(Defaults::class);
 		$this->throttler = $this->createMock(IThrottler::class);
-		$this->initialStateService = $this->createMock(IInitialStateService::class);
+		$this->initialState = $this->createMock(IInitialState::class);
 		$this->webAuthnManager = $this->createMock(\OC\Authentication\WebAuthn\Manager::class);
 		$this->notificationManager = $this->createMock(IManager::class);
 		$this->l = $this->createMock(IL10N::class);
@@ -135,7 +135,7 @@ class LoginControllerTest extends TestCase {
 			$this->urlGenerator,
 			$this->defaults,
 			$this->throttler,
-			$this->initialStateService,
+			$this->initialState,
 			$this->webAuthnManager,
 			$this->notificationManager,
 			$this->l,
@@ -265,10 +265,9 @@ class LoginControllerTest extends TestCase {
 					],
 				]
 			);
-		$this->initialStateService->expects($this->exactly(12))
+		$this->initialState->expects($this->exactly(13))
 			->method('provideInitialState')
 			->withConsecutive([
-				'core',
 				'loginMessages',
 				[
 					'MessageArray1',
@@ -277,7 +276,6 @@ class LoginControllerTest extends TestCase {
 				],
 			],
 				[
-					'core',
 					'loginErrors',
 					[
 						'ErrorArray1',
@@ -285,7 +283,6 @@ class LoginControllerTest extends TestCase {
 					],
 				],
 				[
-					'core',
 					'loginUsername',
 					'',
 				]);
@@ -307,14 +304,12 @@ class LoginControllerTest extends TestCase {
 			->expects($this->once())
 			->method('isLoggedIn')
 			->willReturn(false);
-		$this->initialStateService->expects($this->exactly(13))
+		$this->initialState->expects($this->exactly(14))
 			->method('provideInitialState')
 			->withConsecutive([], [], [], [
-				'core',
 				'loginAutocomplete',
 				false
 			], [
-				'core',
 				'loginRedirectUrl',
 				'login/flow'
 			]);
@@ -378,14 +373,12 @@ class LoginControllerTest extends TestCase {
 			->method('get')
 			->with('LdapUser')
 			->willReturn($user);
-		$this->initialStateService->expects($this->exactly(12))
+		$this->initialState->expects($this->exactly(13))
 			->method('provideInitialState')
 			->withConsecutive([], [], [
-				'core',
 				'loginUsername',
 				'LdapUser'
 			], [], [], [], [
-				'core',
 				'loginCanResetPassword',
 				$expectedResult
 			]);
@@ -428,18 +421,15 @@ class LoginControllerTest extends TestCase {
 			->method('get')
 			->with('0')
 			->willReturn($user);
-		$this->initialStateService->expects($this->exactly(12))
+		$this->initialState->expects($this->exactly(13))
 			->method('provideInitialState')
 			->withConsecutive([], [], [], [
-				'core',
 				'loginAutocomplete',
 				true
 			], [], [
-				'core',
 				'loginResetPasswordLink',
 				false
 			], [
-				'core',
 				'loginCanResetPassword',
 				false
 			]);

Some files were not shown because too many files changed in this diff