Skip to content

feat: Add signature and quotes in RichHtmlEditorWebview#2811

Open
solrubado wants to merge 94 commits intomainfrom
rich-editor
Open

feat: Add signature and quotes in RichHtmlEditorWebview#2811
solrubado wants to merge 94 commits intomainfrom
rich-editor

Conversation

@solrubado
Copy link
Copy Markdown
Contributor

@solrubado solrubado commented Feb 16, 2026

@solrubado solrubado changed the title feature: Add signature and quotes in RichHtmlEditorWebview feat: Add signature and quotes in RichHtmlEditorWebview Feb 16, 2026
@LunarX LunarX self-requested a review February 20, 2026 13:30
Copy link
Copy Markdown
Contributor

@LunarX LunarX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The textfield for the redaction has a different color than the background now. Also the placeholder and my signature are not displayed at all.

Edit: Ok it looks like it's more of an issue of some of the logic not loading in time instead.

In this before:
Image

On main:
Image

Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/BodyContentPayload.kt Outdated
Comment thread app/src/main/res/raw/editor_style.css Outdated
Copy link
Copy Markdown
Contributor

@LunarX LunarX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check if the computation of saveSnapshot and isSnapshotTheSame are correct when you open a new message, modify nothing and then leave. Because right now it saves a new draft everytime even when there are no modifications

Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/utils/WebViewUtils.kt
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/utils/SignatureUtils.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/utils/MessageBodyUtils.kt
@solrubado solrubado force-pushed the rich-editor branch 2 times, most recently from 34ee10e to b9d10ac Compare March 3, 2026 14:02
@github-actions github-actions Bot added the dependent This MR depends on another PR label Mar 3, 2026
@LunarX LunarX self-requested a review March 3, 2026 15:14
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Copy link
Copy Markdown
Contributor

@LunarX LunarX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't finished reviewing yet but here a some of the comments. We can review them together

Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/utils/HtmlFormatter.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Copy link
Copy Markdown
Contributor

@LunarX LunarX left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more comments the rest comes later

Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
@github-actions github-actions Bot removed the dependent This MR depends on another PR label Mar 6, 2026
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 6, 2026

This PR/issue depends on:

Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
Comment thread app/src/main/res/raw/replace_signature_script.js
Comment thread app/src/main/res/raw/show_quotes_script.js Outdated
Comment thread app/src/main/res/raw/style.css Outdated
Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageViewModel.kt Outdated
*/

(function() {
var signatureElement = document.querySelector('.%s');
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to const

Comment thread app/src/main/java/com/infomaniak/mail/ui/newMessage/NewMessageFragment.kt Outdated
@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 18, 2026

PR Reviewer Guide 🔍

(Review updated until commit ca4cec7)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
🧪 No relevant tests
🔒 Security concerns

Potential XSS via Signature Injection:
The replaceSignatureScript injects HTML content directly into the DOM via outerHTML. While the JavaScript string is escaped, the HTML payload itself is not sanitized before insertion. If an attacker can control the signature content (e.g., via a compromised account or malicious signature sync), they could inject arbitrary HTML/JavaScript into the editor WebView. Recommendation: Sanitize the signature HTML using the same sanitization logic used for body content before injection, or use DOM APIs that don't execute scripts (e.g., textContent for text-only signatures).

⚡ Recommended focus areas for review

Memory Leak Risk

The jsBridge and editorJsBridge are declared as lateinit var in the companion object (static). These hold references to callbacks (onImagesDeletedFromQuotes, onWebViewFinishedLoading) which likely capture Activity/Fragment contexts. This can cause memory leaks when the WebView is destroyed but the static reference remains. Consider using weak references or clearing these references when the WebView is destroyed.

lateinit var jsBridge: JavascriptBridge // TODO: Avoid excessive memory consumption with injection
lateinit var editorJsBridge: EditorJavascriptBridge

fun initJavascriptBridge(
    onWebViewFinishedLoading: (() -> Unit)? = null,
) {
    jsBridge = JavascriptBridge(onWebViewFinishedLoading = onWebViewFinishedLoading)
}

fun initEditorJsBridge(
    onImagesDeletedFromQuotes: ((List<String>) -> Unit)? = null,
) {
    editorJsBridge = EditorJavascriptBridge(onImagesDeletedFromQuotes = onImagesDeletedFromQuotes)
}
XSS Injection Risk

The updateBodySignature method injects signature HTML directly into the WebView via evaluateJavascript using outerHTML. While looselyEscapeAsStringLiteralForJs escapes for JavaScript context, the HTML content itself is inserted unsanitized into the DOM. If signature.content contains malicious JavaScript (e.g., <img src=x onerror=alert(1)>), it could execute in the WebView context. Ensure signature HTML is sanitized before injection or use a safer DOM manipulation approach.

private fun updateBodySignature(signature: Signature) {
    val selectedSignature = if (signature.isDummy) "" else signature.content
    val signatureWithClass = signatureUtils.encapsulateSignatureContentWithInfomaniakClass(selectedSignature)
    val escapedSignature = looselyEscapeAsStringLiteralForJs(signatureWithClass)

    val replaceSignatureScript = replaceSignatureScript.format(
        MessageBodyUtils.INFOMANIAK_SIGNATURE_HTML_CLASS_NAME,
        escapedSignature,
        MessageBodyUtils.INFOMANIAK_REPLY_QUOTE_HTML_CLASS_NAME
    )

    binding.editorWebView.evaluateJavascript(replaceSignatureScript, null)
}
Resource Leak

The MutationObserver is set up to monitor quote elements for removed images, but there is no corresponding cleanup logic to disconnect the observer when the WebView is destroyed or the quotes are hidden. This could lead to memory leaks or stale callbacks if the WebView is reused or destroyed. Ensure mutationObserver.disconnect() is called when appropriate (e.g., when hiding quotes or destroying the view).

    var mutationObserver;
    var QUOTE_SELECTORS = '.ik_mail_quote, .forwardContentMessage';

    function setupObserver() {
        // Find all quotes containers to observe
        var quoteElements = document.querySelectorAll(QUOTE_SELECTORS);
        if (quoteElements.length === 0) return ;

        mutationObserver = new MutationObserver(function(mutationRecords) {
           var removedCids = [];

           mutationRecords.forEach(function (mutation) {
             for (var removedNode, nodeIndex = 0; removedNode = mutation.removedNodes[nodeIndex]; ++nodeIndex) {
                if (removedNode.nodeType == Node.ELEMENT_NODE) {
                   if ("img" == removedNode.tagName.toLowerCase()) {
                       removedCids.push(removedNode.src);
                   } else {
                       var childImages = removedNode.getElementsByTagName("img");
                       for (var childIndex = 0; childIndex < childImages.length; childIndex++) {
                        removedCids.push(childImages[childIndex].src);
                       }
                   }
                }
             }
           });

           // Notify if images were removed
           if (removedCids.length > 0) {
               onImagesDeletedFromQuotes(JSON.stringify(removedCids));
           }
        });

        // Observe quotes containers
        quoteElements.forEach(function(quoteElement) {
           mutationObserver.observe(quoteElement, {
              childList: true,
              subtree: true,
           })
        })
    }

    if (document.readyState === 'loading') {
        document.addEventListener('DOMContentLoaded', setupObserver);
    } else {
        setupObserver();
    }
})();

@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

@github-actions
Copy link
Copy Markdown

Persistent review updated to latest commit ca4cec7

@github-actions
Copy link
Copy Markdown

Failed to generate code suggestions for PR

LunarX and others added 25 commits April 27, 2026 16:35
@sonarqubecloud
Copy link
Copy Markdown

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 49 out of 49 changed files in this pull request and generated 4 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

}

fun deleteInlineAttachments(cids: List<String>) {
val cidsToDelete = cids.map { it.removePrefix("cid:") }
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

deleteInlineAttachments() builds cidsToDelete as a List and then calls contains() inside a filter, making the removal O(n*m) for large attachment/cid lists.

Consider converting cidsToDelete to a Set before filtering so lookups are O(1).

Suggested change
val cidsToDelete = cids.map { it.removePrefix("cid:") }
val cidsToDelete = cids.mapTo(mutableSetOf()) { it.removePrefix("cid:") }

Copilot uses AI. Check for mistakes.
}

private fun Document.getElementPositionOrMax(className: String): Int {
return this.selectFirst(className)?.sourceRange()?.start()?.pos() ?: Int.MAX_VALUE
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

getElementPositionOrMax() relies on Element.sourceRange() to decide whether reply vs forward quotes come first. However JsoupParserUtil.jsoupParseWithLog() currently uses Jsoup.parse(value) without enabling source position tracking, so sourceRange() will be null and this will always return Int.MAX_VALUE, making the quote-splitting decision incorrect.

Consider either (a) switching parsing here to a jsoup Parser configured to track source positions, or (b) reverting to a string-index based approach (as was done previously) to pick the earliest quote block reliably.

Suggested change
return this.selectFirst(className)?.sourceRange()?.start()?.pos() ?: Int.MAX_VALUE
val element = this.selectFirst(className) ?: return Int.MAX_VALUE
val documentHtml = outerHtml()
val elementHtml = element.outerHtml()
val position = documentHtml.indexOf(elementHtml)
return if (position >= 0) position else Int.MAX_VALUE

Copilot uses AI. Check for mistakes.
// (the text could get hidden later with the show quotes button).
doc.removeEmptyElements(MessageBodyUtils.INFOMANIAK_REPLY_QUOTE_HTML_CLASS_NAME)
doc.removeEmptyElements(MessageBodyUtils.INFOMANIAK_FORWARD_QUOTE_HTML_CLASS_NAME)
return doc.html()
Copy link

Copilot AI Apr 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removeUnwantedHtml() returns doc.html(), which includes jsoup’s document-level wrappers (e.g. <head>…</head><body>…</body>). Since the editor typically works with an HTML fragment (body inner HTML), this can change what gets saved/sent and potentially introduce unexpected tags.

Consider returning the body fragment (doc.body().html()) or reusing the existing “unwrap document wrapping” logic (like EditorContentManager’s getHtmlWithoutDocumentWrapping()).

Suggested change
return doc.html()
return doc.body().html()

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants