cschlaefke/Shuffle
1
0
Fork 0
Code Issues 33 Pull requests 2 Projects Releases Packages Wiki Activity Actions

fix(auth): address review findings for login

Browse source 
Fixed-wording tests for the generic credentials error, redacted toString for
login/register state, shared AuthFailure.isRetryable, autofill context
finished even when the router removes the screen, stricter from validation
(encoded and control-character variants), more widget tests.

Refs #1

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
This commit is contained in:
cschlaefke 2026-11-03 09:07:56 +01:00
parent dd91c078e0
commit 57305dad4b
14 changed files with 218 additions and 46 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
app/lib/features/auth/presentation/auth_failure_texts.dart
View file

@ -36,3 +36,8 @@ String weakPasswordMessage(
};
return texts.isEmpty ? l10n.authErrorWeakPasswordCharacters : texts.join(' ');
}
/// Failures the user can fix by simply trying again (offline, unexpected).
extension AuthFailureRetry on AuthFailure {
bool get isRetryable => this is NetworkFailure || this is Unexpected;
}

12
app/lib/features/auth/presentation/login/login_controller.dart
View file

@ -60,15 +60,17 @@ class LoginController extends _$LoginController {
final result = await ref
.read(authRepositoryProvider)
.signIn(email, state.password);
if (!ref.mounted) {
return null;
}
final failure = result.errorOrNull;
if (failure == null) {
state = state.copyWith(status: LoginStatus.editing);
// Success: the target is returned even if the screen is already gone
// (the router redirect can remove it before this future completes), so
// the caller can still finish the autofill context. The status stays
// `submitting`, the form is not reused after a successful sign-in.
return safeFromPath(from) ?? AppRoutes.home;
}
if (!ref.mounted) {
return null;
}
state = state.copyWith(
status: LoginStatus.editing,
bannerFailure: failure,

2
app/lib/features/auth/presentation/login/login_controller.g.dart
View file

@ -44,7 +44,7 @@ final class LoginControllerProvider
}
}
String _$loginControllerHash() => r'f8c37f2b7b942c8cfffbf8b61f3296872b5bcb70';
String _$loginControllerHash() => r'693066c260db8abe8f70d04b88b216b2e0411579';
/// Logic of the sign-in form (KTO-01 Kriterien 5, 6 und 7).

16
app/lib/features/auth/presentation/login/login_screen.dart
View file

@ -67,10 +67,15 @@ class _LoginScreenState extends ConsumerState<LoginScreen> {
final target = await ref
.read(loginControllerProvider.notifier)
.submit(from: widget.from);
if (!mounted || target == null) {
if (target == null) {
return;
}
// Also when the router redirect (status signedIn) already removed this
// screen: the password manager must still learn that sign-in worked.
TextInput.finishAutofillContext();
if (!mounted) {
return;
}
context.go(target);
}
@ -136,13 +141,16 @@ class _LoginScreenState extends ConsumerState<LoginScreen> {
message: l10n.authCheckEmailResent,
),
],
if (state.showsNotConfirmed && resend.failure != null) ...[
if ((state.showsNotConfirmed, resend.failure) case (
true,
final resendFailure?,
)) ...[
const SizedBox(height: AppSpacing.md),
AppBanner(
key: const Key('login_resend_error_banner'),
type: AppBannerType.error,
typeLabel: l10n.authBannerTypeError,
message: authFailureMessage(l10n, resend.failure!),
message: authFailureMessage(l10n, resendFailure),
),
],
const SizedBox(height: AppSpacing.xl),
@ -226,7 +234,7 @@ class _FailureBanner extends StatelessWidget {
@override
Widget build(BuildContext context) {
final l10n = context.l10n;
final retryable = failure is NetworkFailure || failure is Unexpected;
final retryable = failure.isRetryable;
return AppBanner(
type: AppBannerType.error,
typeLabel: l10n.authBannerTypeError,

6
app/lib/features/auth/presentation/login/login_state.dart
View file

@ -37,6 +37,12 @@ abstract class LoginState with _$LoginState {
const LoginState._();
/// No credentials in logs or error reports.
@override
String toString() =>
'LoginState(email: <redacted>, password: <redacted>, status: $status, '
'bannerFailure: $bannerFailure)';
bool get isSubmitting => status == LoginStatus.submitting;
EmailError? get emailError => EmailFormat.validate(email);

9
app/lib/features/auth/presentation/login/login_state.freezed.dart
View file

@ -42,11 +42,6 @@ int get hashCode {
return Object.hash(runtimeType,_this.email,_this.password,const DeepCollectionEquality().hash(_this.visibleFields),_this.status,_this.bannerFailure,_this.unconfirmedEmail,_this.focusField,_this.focusRequest);
}
@override
String toString() {
final _this = this as LoginState;
return 'LoginState(email: ${_this.email}, password: ${_this.password}, visibleFields: ${_this.visibleFields}, status: ${_this.status}, bannerFailure: ${_this.bannerFailure}, unconfirmedEmail: ${_this.unconfirmedEmail}, focusField: ${_this.focusField}, focusRequest: ${_this.focusRequest})';
}
}
@ -278,10 +273,6 @@ int get hashCode {
return Object.hash(runtimeType,email,password,const DeepCollectionEquality().hash(_visibleFields),status,bannerFailure,unconfirmedEmail,focusField,focusRequest);
}
@override
String toString() {
return 'LoginState(email: $email, password: $password, visibleFields: $visibleFields, status: $status, bannerFailure: $bannerFailure, unconfirmedEmail: $unconfirmedEmail, focusField: $focusField, focusRequest: $focusRequest)';
}
}

11
app/lib/features/auth/presentation/providers/auth_redirect.dart
View file

@ -69,10 +69,21 @@ String? safeFromPath(String? from) {
if (from.startsWith('//') || from.startsWith(r'/\')) {
return null;
}
// Browsers drop tabs and line breaks inside URLs (`/\t/evil` -> `//evil`),
// so control characters are never allowed.
if (RegExp(r'[\x00-\x1f\x7f]').hasMatch(from)) {
return null;
}
final uri = Uri.tryParse(from);
if (uri == null || uri.hasScheme || uri.hasAuthority) {
return null;
}
// Percent-encoded variants (`/%2F%2Fevil`, `/%5Cevil`) must not decode to a
// protocol-relative path either.
final decoded = Uri.decodeFull(uri.path);
if (decoded.startsWith('//') || decoded.startsWith(r'/\')) {
return null;
}
// `/login/` is the same page as `/login` (go_router ignores the trailing
// slash), so it is not allowed either.
final path = uri.path.length > 1

2
app/lib/features/auth/presentation/register/register_screen.dart
View file

@ -319,7 +319,7 @@ class _FailureBanner extends StatelessWidget {
@override
Widget build(BuildContext context) {
final l10n = context.l10n;
final retryable = failure is NetworkFailure || failure is Unexpected;
final retryable = failure.isRetryable;
return AppBanner(
type: AppBannerType.error,
typeLabel: l10n.authBannerTypeError,

7
app/lib/features/auth/presentation/register/register_state.dart
View file

@ -56,6 +56,13 @@ abstract class RegisterState with _$RegisterState {
const RegisterState._();
/// No credentials or personal data in logs or error reports.
@override
String toString() =>
'RegisterState(email: <redacted>, password: <redacted>, '
'displayName: <redacted>, status: $status, '
'bannerFailure: $bannerFailure)';
bool get isSubmitting => status == RegisterStatus.submitting;
EmailError? get emailError => EmailFormat.validate(email);

9
app/lib/features/auth/presentation/register/register_state.freezed.dart
View file

@ -44,11 +44,6 @@ int get hashCode {
return Object.hash(runtimeType,_this.email,_this.password,_this.displayName,_this.ageConfirmed,_this.termsAccepted,const DeepCollectionEquality().hash(_this.visibleFields),_this.status,_this.commonPasswords,_this.bannerFailure,const DeepCollectionEquality().hash(_this.serverPasswordReasons),_this.serverEmailInvalid,_this.focusField,_this.focusRequest);
}
@override
String toString() {
final _this = this as RegisterState;
return 'RegisterState(email: ${_this.email}, password: ${_this.password}, displayName: ${_this.displayName}, ageConfirmed: ${_this.ageConfirmed}, termsAccepted: ${_this.termsAccepted}, visibleFields: ${_this.visibleFields}, status: ${_this.status}, commonPasswords: ${_this.commonPasswords}, bannerFailure: ${_this.bannerFailure}, serverPasswordReasons: ${_this.serverPasswordReasons}, serverEmailInvalid: ${_this.serverEmailInvalid}, focusField: ${_this.focusField}, focusRequest: ${_this.focusRequest})';
}
}
@ -301,10 +296,6 @@ int get hashCode {
return Object.hash(runtimeType,email,password,displayName,ageConfirmed,termsAccepted,const DeepCollectionEquality().hash(_visibleFields),status,commonPasswords,bannerFailure,const DeepCollectionEquality().hash(_serverPasswordReasons),serverEmailInvalid,focusField,focusRequest);
}
@override
String toString() {
return 'RegisterState(email: $email, password: $password, displayName: $displayName, ageConfirmed: $ageConfirmed, termsAccepted: $termsAccepted, visibleFields: $visibleFields, status: $status, commonPasswords: $commonPasswords, bannerFailure: $bannerFailure, serverPasswordReasons: $serverPasswordReasons, serverEmailInvalid: $serverEmailInvalid, focusField: $focusField, focusRequest: $focusRequest)';
}
}

8
app/test/features/auth/presentation/auth_redirect_test.dart
View file

@ -150,5 +150,13 @@ void main() {
expect(safeFromPath(''), isNull);
expect(safeFromPath('javascript:alert(1)'), isNull);
});
test('rejects encoded and control-character tricks', () {
expect(safeFromPath('/%2F%2Fevil.example'), isNull);
expect(safeFromPath('%2F%2Fevil'), isNull);
expect(safeFromPath('/%5Cevil.example'), isNull);
expect(safeFromPath('/\t/evil.example'), isNull);
expect(safeFromPath('/a\nb'), isNull);
});
});
}

19
app/test/features/auth/presentation/login/login_controller_test.dart
View file

@ -47,7 +47,8 @@ void main() {
verify(() => repository.signIn('Anna@Example.com', 'pw')).called(1);
expect(target, '/');
expect(state().isSubmitting, isFalse);
// The form is left after success; it is not unlocked again.
expect(state().isSubmitting, isTrue);
expect(state().bannerFailure, isNull);
});
@ -68,6 +69,10 @@ void main() {
'/register',
'/reset-password',
'/auth/confirm?token_hash=x',
'/%2F%2Fevil.example',
'%2F%2Fevil',
'/\t/evil.example',
'/%5Cevil.example',
'',
]) {
test('ignores the unsafe from "$unsafe"', () async {
@ -123,6 +128,18 @@ void main() {
});
});
group('no credentials in logs', () {
test('toString masks email and password', () {
fill('anna@example.com', 'geheim-und-lang');
final text = state().toString();
expect(text, isNot(contains('anna@example.com')));
expect(text, isNot(contains('geheim-und-lang')));
expect(text, contains('<redacted>'));
});
});
group('KTO-01 Kriterium 7: failures', () {
test('invalid credentials keep the input and show the failure', () async {
stubSignIn(failed(const AuthFailure.invalidCredentials()));

140
app/test/features/auth/presentation/login/login_screen_test.dart
View file

@ -243,9 +243,13 @@ void main() {
testWidgets(
'wrong email and wrong password show exactly the same screen text',
(tester) async {
// The repository maps both cases to invalidCredentials (Spec 4.3); the
// screen must not add anything that tells them apart.
// Screen level only: the repository mock returns invalidCredentials
// for both cases, so this proves the screen adds nothing that tells
// them apart. The real proof that an unknown address and a wrong
// password yield the same `invalid_credentials` answer is the API
// E2E test (T3).
stubSignInFailure(const AuthFailure.invalidCredentials());
final handle = tester.ensureSemantics();
await open(tester);
await type(tester, 'login_email', 'unknown@example.com');
await type(tester, 'login_password', 'irgendein-passwort');
@ -258,22 +262,16 @@ void main() {
final wrongPassword = visibleTexts(tester);
expect(wrongEmail, wrongPassword);
expect(wrongEmail, contains(_de.authErrorInvalidCredentials));
verify(() => repository.signIn(any(), any())).called(2);
// Fixed wording, independent of the l10n getter.
const fixed = 'E-Mail-Adresse oder Passwort ist nicht korrekt.';
expect(wrongEmail, contains(fixed));
expect(_de.authErrorInvalidCredentials, fixed);
expect(find.text(fixed), findsOneWidget);
expect(find.bySemanticsLabel('Fehler: $fixed'), findsOneWidget);
handle.dispose();
},
);
testWidgets('english text of the generic failure', (tester) async {
stubSignInFailure(const AuthFailure.invalidCredentials());
await open(tester, locale: const Locale('en'));
await fill(tester);
await submit(tester);
expect(find.text(_en.authErrorInvalidCredentials), findsOneWidget);
expect(find.text(_de.authErrorInvalidCredentials), findsNothing);
});
testWidgets('throttling (429) shows the throttle hint', (tester) async {
stubSignInFailure(const AuthFailure.rateLimited());
await open(tester);
@ -378,6 +376,99 @@ void main() {
});
});
group('KTO-01 Kriterium 5: autofill context', () {
// Only calls that save the credentials (`shouldSave: true`).
int finishCalls(WidgetTester tester) => tester.testTextInput.log
.where(
(call) =>
call.method == 'TextInput.finishAutofillContext' &&
call.arguments == true,
)
.length;
testWidgets('is finished after a successful sign-in', (tester) async {
await open(tester);
await fill(tester);
tester.testTextInput.log.clear();
await submit(tester);
// Our explicit call, plus the AutofillGroup committing on disposal
// when the screen is replaced.
expect(finishCalls(tester), greaterThanOrEqualTo(1));
});
testWidgets('is finished even if the router already removed the screen', (
tester,
) async {
final completer = Completer<Result<void, AuthFailure>>();
when(() => repository.signIn(any(), any()))
.thenAnswer((_) => completer.future);
final (:container, :router) = await open(tester);
await fill(tester);
await tapKey(tester, 'login_submit');
// The status change makes the router redirect away before the
// sign-in future completes.
router.go('/private');
await tester.pumpAndSettle();
expect(find.byKey(const Key('login_screen')), findsNothing);
tester.testTextInput.log.clear();
completer.complete(ok());
await tester.pumpAndSettle();
expect(finishCalls(tester), 1);
expect(currentPath(router), '/private');
});
testWidgets('is not finished after a failed sign-in', (tester) async {
stubSignInFailure(const AuthFailure.invalidCredentials());
await open(tester);
await fill(tester);
tester.testTextInput.log.clear();
await submit(tester);
expect(finishCalls(tester), 0);
});
});
group('Zustände Kriterium 3: banners and focus', () {
testWidgets('unexpected error shows the banner with retry', (tester) async {
stubSignInFailure(const AuthFailure.unexpected());
await open(tester);
await fill(tester);
await submit(tester);
expect(find.text(_de.authErrorUnexpected), findsOneWidget);
expect(find.text(_de.authActionRetry), findsOneWidget);
stubSignInFailure(const AuthFailure.unexpected());
await tester.tap(find.text(_de.authActionRetry));
await tester.pumpAndSettle();
verify(() => repository.signIn(any(), any())).called(2);
});
testWidgets('a failed submit focuses the first invalid field', (
tester,
) async {
await open(tester);
await submit(tester);
expect(
tester.widget<TextField>(fieldOf('login_email')).focusNode!.hasFocus,
isTrue,
);
await type(tester, 'login_email', 'anna@example.com');
await submit(tester);
expect(
tester.widget<TextField>(fieldOf('login_password')).focusNode!.hasFocus,
isTrue,
);
});
});
group('QUE-05 Kriterium 2: de and en', () {
testWidgets('english texts', (tester) async {
await open(tester, locale: const Locale('en'));
@ -405,6 +496,23 @@ void main() {
});
}
for (final mode in [ThemeMode.light, ThemeMode.dark]) {
testWidgets('error banner meets the contrast guideline ($mode)', (
tester,
) async {
final handle = tester.ensureSemantics();
stubSignInFailure(const AuthFailure.invalidCredentials());
await open(tester, themeMode: mode);
await fill(tester);
await submit(tester);
expect(find.byKey(const Key('login_banner')), findsOneWidget);
await expectLater(tester, meetsGuideline(textContrastGuideline));
await expectLater(tester, meetsGuideline(androidTapTargetGuideline));
handle.dispose();
});
}
testWidgets('the title is a heading and the banner a live region', (
tester,
) async {
@ -416,7 +524,7 @@ void main() {
expect(
find.bySemanticsLabel(
'${_de.authBannerTypeError}: ${_de.authErrorInvalidCredentials}',
'Fehler: E-Mail-Adresse oder Passwort ist nicht korrekt.',
),
findsOneWidget,
);

18
app/test/features/auth/presentation/register/register_controller_test.dart
View file

@ -71,6 +71,24 @@ void main() {
..termsAcceptedChanged(value: true);
}
group('no personal data in logs', () {
test('toString masks email, password and display name', () {
final container = makeContainer();
fillValid(
container.read(registerControllerProvider.notifier),
email: 'anna@example.com',
name: 'Anna Geheim',
);
final text = container.read(registerControllerProvider).toString();
expect(text, isNot(contains('anna@example.com')));
expect(text, isNot(contains('geheim-und-lang')));
expect(text, isNot(contains('Anna Geheim')));
expect(text, contains('<redacted>'));
});
});
group('KTO-01 Kriterium 1: submit', () {
test('KTO-01 Kriterium 11 (Client): sends all data, terms version and locale to signUp', () async {
final container = makeContainer(locale: const Locale('en'));