fixup! TF-4224 Reduce ReDoS vulnerability

2026-01-05 16:18:25 +07:00
parent 782d21d796
commit 9f1373c7e5
3 changed files with 210 additions and 55 deletions
@@ -21,10 +21,23 @@ class StringConvert {
  );
  static final _asciiArtRegex = RegExp(r'[+\-|/\\=]');
  static final _namedAddressRegex = RegExp(r'''(?:(?:"([^"]+)"|'([^']+)')\s*)?<([^>]+)>''');
+  static final _emailLocalhostRegex = RegExp(
+    r'^(?:"[^"\r\n]+"|[^<>()[\]\\.,;:\s@"]+(?:\.[^<>()[\]\\.,;:\s@"]+)*)@localhost$',
+  );

  @visibleForTesting
  static RegExp get base64ValidationRegex => _base64ValidationRegex;

+  @visibleForTesting
+  static RegExp get emailLocalhostRegex => _emailLocalhostRegex;
+
+  /// Checks if the given email address is a localhost email
+  /// Returns true if the email ends with @localhost
+  static bool isEmailLocalhost(String email) {
+    final normalized = email.trim();
+    return _emailLocalhostRegex.hasMatch(normalized);
+  }
+
  static String? writeEmptyToNull(String text) {
    if (text.isEmpty) return null;
    return text;
@@ -1,7 +1,13 @@
+import 'package:core/data/network/dio_client.dart';
 import 'package:core/presentation/utils/html_transformer/dom/image_transformers.dart';
+import 'package:core/presentation/utils/html_transformer/dom/normalize_line_height_in_style_transformer.dart';
 import 'package:core/utils/html/html_utils.dart';
 import 'package:core/utils/string_convert.dart';
 import 'package:flutter_test/flutter_test.dart';
+import 'package:html/parser.dart' as html_parser;
+import 'package:mockito/mockito.dart';
+
+class MockDioClient extends Mock implements DioClient {}

 /// Tests to verify that ReDoS (Regular Expression Denial of Service) vulnerabilities
 /// have been properly patched. These tests use inputs that would cause catastrophic
@@ -65,30 +71,130 @@ void main() {
      });
    });

-    group('NormalizeLineHeightInStyleTransformer tests regex patterns in isolation', () {
-      test('should handle very long style strings efficiently', () {
+    group('NormalizeLineHeightInStyleTransformer', () {
+      late NormalizeLineHeightInStyleTransformer transformer;
+      late MockDioClient mockDioClient;
+
+      setUp(() {
+        transformer = const NormalizeLineHeightInStyleTransformer();
+        mockDioClient = MockDioClient();
+      });
+
+      test('should handle very long style strings efficiently', () async {
+        // Create HTML with very long style attribute
        final longStyle = 'color: red; ' * 10000 + 'line-height: 1px;';
+        final htmlContent = '<div style="$longStyle">Content</div>';
+        final document = html_parser.parse(htmlContent);

        final stopwatch = Stopwatch()..start();
-        final normalized = longStyle.replaceAll(
-          RegExp(r'line-height\s*:\s*(?:1px|100%)\s*;?', caseSensitive: false),
-          '',
+        await transformer.process(
+          document: document,
+          dioClient: mockDioClient,
        );
        stopwatch.stop();

        expect(stopwatch.elapsedMilliseconds, lessThan(100));
-        expect(normalized, isNot(contains('line-height')));
+
+        // Verify the transformer removed line-height
+        final divElement = document.querySelector('div');
+        final resultStyle = divElement?.attributes['style'] ?? '';
+        expect(resultStyle, isNot(contains('line-height')));
      });

-      test('should handle many consecutive spaces efficiently', () {
-        final manySpaces = 'text${' ' * 10000}more text';
+      test('should handle many consecutive spaces efficiently', () async {
+        // Create HTML with many spaces in style that includes line-height
+        final manySpaces = 'color:${' ' * 10000}red; line-height: 1px;';
+        final htmlContent = '<p style="$manySpaces">Text</p>';
+        final document = html_parser.parse(htmlContent);

        final stopwatch = Stopwatch()..start();
-        final normalized = manySpaces.replaceAll(RegExp(r' {2,}'), ' ');
+        await transformer.process(
+          document: document,
+          dioClient: mockDioClient,
+        );
        stopwatch.stop();

        expect(stopwatch.elapsedMilliseconds, lessThan(100));
-        expect(normalized, equals('text more text'));
+
+        // After removing line-height, spaces should be normalized
+        final pElement = document.querySelector('p');
+        final resultStyle = pElement?.attributes['style'] ?? '';
+        expect(resultStyle, isNot(contains('line-height')));
+        // Multiple consecutive spaces should be normalized to single space
+        expect(resultStyle, isNot(matches(r' {2,}')));
+      });
+
+      test('should remove line-height: 1px and 100% patterns', () async {
+        final testCases = [
+          '<div style="line-height: 1px; color: blue;">Test 1px</div>',
+          '<div style="line-height: 100%; color: red;">Test 100%</div>',
+          '<div style="line-height:1px">No spaces</div>',
+          '<div style="LINE-HEIGHT: 1PX;">Case insensitive</div>',
+        ];
+
+        for (final htmlContent in testCases) {
+          final document = html_parser.parse(htmlContent);
+          await transformer.process(
+            document: document,
+            dioClient: mockDioClient,
+          );
+
+          final divElement = document.querySelector('div');
+          final resultStyle = divElement?.attributes['style'] ?? '';
+          expect(resultStyle, isNot(contains('line-height')),
+              reason: 'Should remove line-height from: $htmlContent');
+        }
+      });
+
+      test('should preserve other line-height values', () async {
+        final testCases = [
+          '<div style="line-height: 1.5; color: blue;">Test 1.5</div>',
+          '<div style="line-height: 20px; color: red;">Test 20px</div>',
+          '<div style="line-height: normal;">Test normal</div>',
+        ];
+
+        for (final htmlContent in testCases) {
+          final document = html_parser.parse(htmlContent);
+          await transformer.process(
+            document: document,
+            dioClient: mockDioClient,
+          );
+
+          final divElement = document.querySelector('div');
+          final resultStyle = divElement?.attributes['style'] ?? '';
+          expect(resultStyle, contains('line-height'),
+              reason: 'Should preserve line-height from: $htmlContent');
+        }
+      });
+
+      test('should remove style attribute when empty after transformation', () async {
+        const htmlContent = '<div style="line-height: 1px;">Empty after</div>';
+        final document = html_parser.parse(htmlContent);
+
+        await transformer.process(
+          document: document,
+          dioClient: mockDioClient,
+        );
+
+        final divElement = document.querySelector('div');
+        expect(divElement?.attributes.containsKey('style'), isFalse,
+            reason: 'Should remove empty style attribute');
+      });
+
+      test('should normalize multiple spaces after removing line-height', () async {
+        const htmlContent = '<div style="color: red;   line-height: 1px;   font-size: 12px;">Multi-space</div>';
+        final document = html_parser.parse(htmlContent);
+
+        await transformer.process(
+          document: document,
+          dioClient: mockDioClient,
+        );
+
+        final divElement = document.querySelector('div');
+        final resultStyle = divElement?.attributes['style'] ?? '';
+        expect(resultStyle, isNot(contains('  '))); // No double spaces
+        expect(resultStyle, contains('color: red'));
+        expect(resultStyle, contains('font-size: 12px'));
      });
    });

@@ -366,35 +472,53 @@ void main() {
        expect(result, contains('another@test.com'));
      });

-      test('Should handle emails with special characters and edge cases', () {
-        final specialCharEmails = [
-          // Valid special characters in local part
-          'user+tag@example.com',
-          'user.name@example.com',
-          'user_name@example.com',
-          'user-name@example.com',
-          'user123@example.com',
-          '123user@example.com',
-          // International domain
-          'user@example.co.uk',
-          'user@sub.domain.example.com',
-          // Quoted local part (valid but rare)
+      test('Should handle emails with special characters - valid cases', () {
+        // Valid emails that should be extracted successfully
+        final validEmailInputs = {
+          'user+tag@example.com': 'user+tag@example.com',
+          'user.name@example.com': 'user.name@example.com',
+          'user_name@example.com': 'user_name@example.com',
+          'user-name@example.com': 'user-name@example.com',
+          'user123@example.com': 'user123@example.com',
+          '123user@example.com': '123user@example.com',
+          'user@example.co.uk': 'user@example.co.uk',
+          'user@sub.domain.example.com': 'user@sub.domain.example.com',
+          'a.b.c.d@example.com': 'a.b.c.d@example.com',
+          'user@例え.jp': 'user@例え.jp',
+          '${'a' * 64}@example.com': '${'a' * 64}@example.com',
+          'Contact us at support@example.com for help': 'support@example.com',
+          'Multiple emails: alice@test.com, bob@test.org': contains('alice@test.com'),
+        };
+
+        for (final entry in validEmailInputs.entries) {
+          final input = entry.key;
+          final expectedMatch = entry.value;
+
+          final stopwatch = Stopwatch()..start();
+          final result = StringConvert.extractEmailAddress(input);
+          stopwatch.stop();
+
+          expect(stopwatch.elapsedMilliseconds, lessThan(100),
+              reason: 'Should process quickly: $input');
+
+          if (expectedMatch is String) {
+            expect(result, contains(expectedMatch),
+                reason: 'Should extract "$expectedMatch" from: $input');
+          } else {
+            expect(result, expectedMatch,
+                reason: 'Should match expected pattern from: $input');
+          }
+        }
+      });
+
+      test('Should handle quoted email addresses', () {
+        // Quoted email addresses have special handling in email specs
+        const quotedEmails = [
          '"user@name"@example.com',
          '"user name"@example.com',
-          // Edge cases with dots
-          'a.b.c.d@example.com',
-          'user.@example.com', // Invalid but should handle gracefully
-          '.user@example.com', // Invalid but should handle gracefully
-          // Unicode/international emails (if supported)
-          'user@例え.jp',
-          // Very long local part
-          '${'a' * 64}@example.com',
-          // Mixed with other text
-          'Contact us at support@example.com for help',
-          'Multiple emails: alice@test.com, bob@test.org',
        ];

-        for (final input in specialCharEmails) {
+        for (final input in quotedEmails) {
          final stopwatch = Stopwatch()..start();
          final result = StringConvert.extractEmailAddress(input);
          stopwatch.stop();
@@ -403,11 +527,39 @@ void main() {
          expect(stopwatch.elapsedMilliseconds, lessThan(100),
              reason: 'Should process quickly: $input');

-          // If it looks like a valid email, should extract something
-          if (input.contains('@') && !input.startsWith('.') && !input.endsWith('.')) {
-            expect(result, isNotEmpty,
-                reason: 'Should extract email from: $input');
-          }
+          // Should extract something (even if split) - no crash
+          expect(result, isA<List<String>>(),
+              reason: 'Should return a list for: $input');
+          expect(result, isNotEmpty,
+              reason: 'Should extract something from: $input');
+        }
+      });
+
+      test('Should gracefully handle invalid or edge case emails', () {
+        // Invalid emails or edge cases - should handle gracefully without crashing
+        const edgeCaseInputs = [
+          'user.@example.com',  // Dot before @
+          '.user@example.com',  // Leading dot
+          'user..name@example.com',  // Consecutive dots
+          '@example.com',  // Missing local part
+          'user@',  // Missing domain
+          'user',  // No @ symbol
+          '',  // Empty string
+          '   ',  // Only whitespace
+        ];
+
+        for (final input in edgeCaseInputs) {
+          final stopwatch = Stopwatch()..start();
+          final result = StringConvert.extractEmailAddress(input);
+          stopwatch.stop();
+
+          // Performance check - should handle gracefully without hanging
+          expect(stopwatch.elapsedMilliseconds, lessThan(100),
+              reason: 'Should process quickly even for invalid: $input');
+
+          // Result should be a list (even if empty) - no crash
+          expect(result, isA<List<String>>(),
+              reason: 'Should return a list for: $input');
        }
      });

@@ -446,17 +598,12 @@ void main() {
      });
    });

-    group('AppUtils._emailLocalhostRegex', () {
-      final localhostRegex = RegExp(
-        r'^(?:"[^"\r\n]+"|[^<>()[\]\\.,;:\s@"]+(?:\.[^<>()[\]\\.,;:\s@"]+)*)@localhost$',
-      );
+    group('StringConvert.isEmailLocalhost', () {
      test('should handle very long email addresses efficiently', () {
        final longEmail = '${'a' * 10000}@localhost';

        final stopwatch = Stopwatch()..start();
-        // Note: We can't directly test AppUtils.isEmailLocalhost in core tests
-        // But we can test the regex pattern
-        final result = localhostRegex.hasMatch(longEmail);
+        final result = StringConvert.isEmailLocalhost(longEmail);
        stopwatch.stop();

        expect(stopwatch.elapsedMilliseconds, lessThan(100));
@@ -471,7 +618,7 @@ void main() {
        ];

        for (final email in validEmails) {
-          expect(localhostRegex.hasMatch(email.trim()), isTrue,
+          expect(StringConvert.isEmailLocalhost(email), isTrue,
              reason: 'Should match: $email');
        }
      });
@@ -485,7 +632,7 @@ void main() {
        ];

        for (final email in invalidEmails) {
-          expect(localhostRegex.hasMatch(email.trim()), isFalse,
+          expect(StringConvert.isEmailLocalhost(email), isFalse,
              reason: 'Should not match: $email');
        }
      });
@@ -14,10 +14,6 @@ import 'package:tmail_ui_user/main/universal_import/html_stub.dart' as html;
 class AppUtils {
  const AppUtils._();

-  static final _emailLocalhostRegex = RegExp(
-    r'^(?:"[^"\r\n]+"|[^<>()[\]\\.,;:\s@"]+(?:\.[^<>()[\]\\.,;:\s@"]+)*)@localhost$',
-  );
-
  static Future<void> loadEnvFile() async {
    await dotenv.load(fileName: AppConfig.envFileName);
    final mapEnvData = Map<String, String>.from(dotenv.env);
@@ -56,8 +52,7 @@ class AppUtils {
  static TextDirection getCurrentDirection(BuildContext context) => Directionality.maybeOf(context) ?? TextDirection.ltr;

  static bool isEmailLocalhost(String email) {
-    final normalized = email.trim();
-    return _emailLocalhostRegex.hasMatch(normalized);
+    return StringConvert.isEmailLocalhost(email);
  }

  static void copyEmailAddressToClipboard(BuildContext context, String emailAddress) {