TF-4223 Fix welcome email is not well displayed #4226
Conversation
WalkthroughThis change refactors HTML sanitization and content processing across the email feature. Key modifications include: replacing public sanitizer constants with a private instance in StandardizeHtmlSanitizingTransformers, narrowing the DOM hyperlink sanitizer selector to process only anchors with href attributes, adding CSS word-break rules for table elements and cells, removing the StringConvert.getContentOriginal utility method, updating the sanitize_html dependency reference, and comprehensively expanding tests for sanitization. Transformer configuration in the email controller now explicitly creates both text and DOM transformers. Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-12-09T09:36:45.349ZApplied to files:
📚 Learning: 2025-12-09T12:47:45.861ZApplied to files:
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
🔇 Additional comments (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
core/test/utils/standardize_html_sanitizing_transformers_test.dart (1)
10-80: Comprehensive event attribute coverage.The
eventAttributeslist is thorough. Consider adding a few modern event types likewheel,animationstart,animationend,transitionend, and pointer events (pointerdown,pointerup, etc.) for more complete coverage, though the sanitizer should handle allon*prefixed attributes generically.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
contact/pubspec.lockis excluded by!**/*.lockcore/pubspec.lockis excluded by!**/*.lockmodel/pubspec.lockis excluded by!**/*.lockpubspec.lockis excluded by!**/*.lockscribe/pubspec.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
core/lib/presentation/utils/html_transformer/dom/sanitize_hyper_link_tag_in_html_transformers.dartcore/lib/presentation/utils/html_transformer/text/standardize_html_sanitizing_transformers.dartcore/lib/utils/html/html_utils.dartcore/lib/utils/string_convert.dartcore/pubspec.yamlcore/test/utils/standardize_html_sanitizing_transformers_test.dartlib/features/email/data/local/html_analyzer.dartlib/features/email/presentation/controller/single_email_controller.dartmodel/lib/email/email_content.darttest/features/email/presentation/controller/single_email_controller_test.dart
💤 Files with no reviewable changes (1)
- core/lib/utils/string_convert.dart
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-09T09:36:45.349Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4194
File: lib/features/manage_account/presentation/manage_account_dashboard_controller.dart:174-176
Timestamp: 2025-12-09T09:36:45.349Z
Learning: In Dart/Flutter projects using GetX, do not wrap getBinding<T>() calls in try/catch since they return null when not found. Only wrap Get.find<T>() calls in try/catch because they throw if a dependency is unavailable. When a binding is optional, consider checking Get.isRegistered<T>() or handling the null/exception path gracefully instead of blindly catching, and document the expectation for failure modes where a dependency may not be registered.
Applied to files:
core/lib/utils/html/html_utils.dartmodel/lib/email/email_content.dartcore/lib/presentation/utils/html_transformer/dom/sanitize_hyper_link_tag_in_html_transformers.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/email/data/local/html_analyzer.dartcore/lib/presentation/utils/html_transformer/text/standardize_html_sanitizing_transformers.dartcore/test/utils/standardize_html_sanitizing_transformers_test.darttest/features/email/presentation/controller/single_email_controller_test.dart
📚 Learning: 2025-12-09T12:47:45.861Z
Learnt from: zatteo
Repo: linagora/tmail-flutter PR: 4196
File: scribe/lib/scribe/ai/data/datasource_impl/ai_datasource_impl.dart:34-35
Timestamp: 2025-12-09T12:47:45.861Z
Learning: In the tmail-flutter repository, avoid suggesting replacing DioError with DioException unless there is a codebase-wide migration. This pattern applies to all Dart files; when reviewing, only propose a DioError-safe change (e.g., compatible error handling or conversion) unless a global migration is in place. Ensure consistency of DioError usage across modules and flag any deviations for a repo-wide decision.
Applied to files:
core/lib/utils/html/html_utils.dartmodel/lib/email/email_content.dartcore/lib/presentation/utils/html_transformer/dom/sanitize_hyper_link_tag_in_html_transformers.dartlib/features/email/presentation/controller/single_email_controller.dartlib/features/email/data/local/html_analyzer.dartcore/lib/presentation/utils/html_transformer/text/standardize_html_sanitizing_transformers.dartcore/test/utils/standardize_html_sanitizing_transformers_test.darttest/features/email/presentation/controller/single_email_controller_test.dart
📚 Learning: 2025-12-12T04:54:11.121Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4191
File: lib/features/email/presentation/extensions/handle_email_action_extension.dart:37-80
Timestamp: 2025-12-12T04:54:11.121Z
Learning: In lib/features/email/presentation/extensions/handle_email_action_extension.dart, the mailboxDashBoardController.selectedEmail should only be synchronized when isMobileThreadDisabled is true. This is intentional behavior and should not be changed to update selectedEmail in non-mobile or thread-enabled contexts.
Applied to files:
model/lib/email/email_content.dartlib/features/email/presentation/controller/single_email_controller.darttest/features/email/presentation/controller/single_email_controller_test.dart
📚 Learning: 2025-12-15T06:24:38.823Z
Learnt from: dab246
Repo: linagora/tmail-flutter PR: 4137
File: lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart:32-32
Timestamp: 2025-12-15T06:24:38.823Z
Learning: In lib/features/mailbox_dashboard/presentation/controller/mailbox_dashboard_controller.dart and lib/features/manage_account/presentation/manage_account_dashboard_controller.dart, the code calls SentryManager.instance.setUser() with PII fields (account ID, display name, username, email). This is privacy-sensitive. Review and ensure PII handling complies with policy: avoid sending unnecessary PII to Sentry, redact or hash sensitive fields if possible, or document explicit privacy policy and user consent for such data. If PII must be sent, ensure minimal data, secure handling, and add notes to the privacy policy. Consider adding tests or a lint rule to flag setUser calls that include PII.
Applied to files:
lib/features/email/presentation/controller/single_email_controller.dart
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: analyze-test (server_settings)
- GitHub Check: analyze-test (core)
- GitHub Check: analyze-test (contact)
- GitHub Check: analyze-test (email_recovery)
- GitHub Check: analyze-test (model)
- GitHub Check: analyze-test (forward)
- GitHub Check: analyze-test (default)
- GitHub Check: analyze-test (rule_filter)
- GitHub Check: analyze-test (fcm)
- GitHub Check: Build web version and deploy
🔇 Additional comments (19)
core/lib/presentation/utils/html_transformer/dom/sanitize_hyper_link_tag_in_html_transformers.dart (1)
19-19: LGTM! Selector narrowed to anchors with href.The updated selector
a[href]appropriately limits processing to anchors that are actual hyperlinks, skipping placeholders or non-link anchor elements. This aligns with the sanitization objectives.lib/features/email/data/local/html_analyzer.dart (1)
52-52: LGTM! Sanitization added to plain text email transformation.Adding
StandardizeHtmlSanitizingTransformersto the plain text transformation pipeline ensures consistent sanitization across email content types. The transformer ordering is appropriate.model/lib/email/email_content.dart (1)
23-23: LGTM! Simplified to return raw content.Removing the dependency on
StringConvert.getContentOriginalsimplifies the code and aligns with the broader refactoring where HTML body extraction is now handled upstream. The change is consistent with related updates across the codebase.core/lib/utils/html/html_utils.dart (1)
300-302: LGTM! CSS rule prevents unwanted word breaks in tables.The added CSS rule ensures table content doesn't break unexpectedly, improving email display quality. The
!importantflag appropriately enforces this behavior.test/features/email/presentation/controller/single_email_controller_test.dart (1)
319-322: LGTM! Tests updated to reflect new link sanitization behavior.The test expectations correctly capture the updated anchor tag structure with
rel="noreferrer"security attribute and new attribute ordering. The functional behavior (URL transformation) remains intact.Also applies to: 375-381
lib/features/email/presentation/controller/single_email_controller.dart (1)
1178-1188: LGTM! Explicit transformer configuration improves clarity.The refactoring to use
TransformConfiguration.createwith explicitcustomTextTransformersandcustomDomTransformersmakes the transformer pipeline more transparent. The addition ofSanitizeHyperLinkTagInHtmlTransformerto DOM transformers aligns with the broader sanitization improvements.core/lib/presentation/utils/html_transformer/text/standardize_html_sanitizing_transformers.dart (1)
6-14: LGTM! Refactored to use shared sanitizer instance.The change from per-call allowList construction to a private static sanitizer instance improves performance and encapsulation. The empty string short-circuit is a sensible optimization.
core/pubspec.yaml (1)
36-36: The upstream reffix/missing-internal-cssis available in the dart-neats repository, confirming that the upstream PR #2 has been merged. The dependency can be resolved successfully.core/test/utils/standardize_html_sanitizing_transformers_test.dart (11)
1-9: LGTM!Imports are clean and the test setup with
constinstances is appropriate for reuse across all test cases.
82-83: LGTM!The reusable
sanitize()helper reduces duplication and makes tests more readable.
85-130: LGTM!Good use of parametric testing to cover all event attributes across multiple tag types. The nested script test and iframe removal tests provide solid coverage for critical XSS vectors.
132-155: LGTM!Good distinction between safe (
data:image/*,cid:) and unsafe (javascript:) URI schemes. The test at line 143-148 correctly verifies that legitimate base64 images are preserved.
187-213: LGTM!Good coverage of CSS sanitization scenarios. The inline style test at lines 208-212 correctly verifies that dangerous properties (
position,background:url(javascript:...)) are stripped while safe properties (color) are preserved.
215-227: LGTM!Validates that the sanitizer correctly handles attribute value validation for
idandclass.
229-251: LGTM!Good integration test that verifies multiple attack vectors are sanitized simultaneously while preserving safe content.
253-276: LGTM!Comprehensive coverage of dangerous form attributes including
action,method,formaction, and various submission-related attributes.
278-295: LGTM!Good coverage of interactive form elements that could be exploited for credential harvesting.
297-333: LGTM!Good validation of nested form handling and the important behavior that safe child elements are preserved when dangerous wrappers are removed.
372-444: LGTM!Excellent real-world attack scenario coverage including credential phishing, CSRF token stealing, and mixed XSS/phishing vectors. These serve as valuable regression tests.
core/test/utils/standardize_html_sanitizing_transformers_test.dart
Outdated
Show resolved
Hide resolved
|
This PR has been deployed to https://linagora.github.io/tmail-flutter/4226. |
…er events to eventAttributes
Issue
#4223
Root cause
The sanitizer parsed the HTML document but applied
NodeSanitizeronly todocument.body, while internal CSS<style>tags typically reside indocument.head. As a result, all<style>elements were implicitly dropped.Solution
linagora/dart-neats#2
Dependency
Resolved
Screen.Recording.2025-12-24.at.16.51.56.mov
Summary by CodeRabbit
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.