feat(mail): enforce HTML lint for compose paths #735
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,237 @@ | ||
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| package htmllint | ||
|
|
||
| import ( | ||
| "bytes" | ||
| "fmt" | ||
| "strings" | ||
|
|
||
| xhtml "golang.org/x/net/html" | ||
| ) | ||
|
|
||
| type Finding struct { | ||
| RuleID string `json:"rule_id"` | ||
| Severity string `json:"severity"` | ||
| TagOrAttr string `json:"tag_or_attr"` | ||
| Excerpt string `json:"excerpt,omitempty"` | ||
| Hint string `json:"hint"` | ||
| } | ||
|
|
||
| type Result struct { | ||
| Warnings []Finding `json:"warnings"` | ||
| Errors []Finding `json:"errors"` | ||
| CleanedHTML string `json:"cleaned_html,omitempty"` | ||
| } | ||
|
|
||
| func (r Result) HasFindings() bool { | ||
| return len(r.Warnings) > 0 || len(r.Errors) > 0 | ||
| } | ||
|
|
||
| func Lint(raw string, autoFix bool) (Result, error) { | ||
| result := Result{} | ||
| nodes, err := xhtml.ParseFragment(strings.NewReader(raw), &xhtml.Node{Type: xhtml.ElementNode, Data: "body"}) | ||
| if err != nil { | ||
| result.Errors = append(result.Errors, Finding{ | ||
|
Comment on lines
+32
to
+36
There was a problem hiding this comment. New HTML sanitization/linting logic is introduced here (tag conversions, blocked tags, URL/style filtering), but there are no accompanying unit tests in this PR. Since shortcuts/mail already has extensive test coverage, adding tests for key cases (void tags, blocked tags/attrs, javascript: URLs, style property filtering, parse failure) would help prevent regressions. |
||
| RuleID: "HTML_PARSE_FAILED", Severity: "error", TagOrAttr: "html", | ||
| Excerpt: truncate(raw), Hint: "HTML cannot be parsed reliably", | ||
| }) | ||
| if autoFix { | ||
| result.CleanedHTML = raw | ||
| } | ||
| return result, nil | ||
| } | ||
| var out bytes.Buffer | ||
| for _, n := range nodes { | ||
| renderNode(&out, n, &result, autoFix) | ||
| } | ||
| if autoFix { | ||
| result.CleanedHTML = out.String() | ||
| } | ||
| return result, nil | ||
| } | ||
|
Comment on lines
+32
to
+53
There was a problem hiding this comment. Add regression tests for the new lint engine before merge This introduces substantial behavior changes (HTML parse handling, tag/attr stripping, CSS sanitization, auto-fix output) but no accompanying tests are present in the provided changes. Please add coverage for core rules and edge cases before release. As per coding guidelines "Every behavior change must have an accompanying test". Also applies to: 55-238 🤖 Prompt for AI Agents |
||
|
|
||
| func renderNode(out *bytes.Buffer, n *xhtml.Node, result *Result, autoFix bool) { | ||
| if n == nil { | ||
| return | ||
| } | ||
| switch n.Type { | ||
| case xhtml.TextNode: | ||
| _ = xhtml.Render(out, n) | ||
| case xhtml.CommentNode, xhtml.DoctypeNode: | ||
| return | ||
| case xhtml.ElementNode: | ||
| tag := strings.ToLower(n.Data) | ||
| if blockedTags[tag] { | ||
| result.Errors = append(result.Errors, Finding{ | ||
| RuleID: "TAG_BLOCKED", Severity: "error", TagOrAttr: tag, | ||
| Excerpt: "<" + tag + ">", Hint: "removed before writing mail HTML", | ||
| }) | ||
| return | ||
| } | ||
| convertedCenter := false | ||
| if tag == "font" { | ||
| result.Warnings = append(result.Warnings, Finding{ | ||
| RuleID: "TAG_FONT_TO_SPAN", Severity: "warning", TagOrAttr: "font", | ||
| Excerpt: "<font>", Hint: "converted to <span>", | ||
| }) | ||
| tag = "span" | ||
| } else if tag == "center" { | ||
| result.Warnings = append(result.Warnings, Finding{ | ||
| RuleID: "TAG_CENTER_TO_DIV", Severity: "warning", TagOrAttr: "center", | ||
| Excerpt: "<center>", Hint: "converted to <div style=\"text-align:center\">", | ||
| }) | ||
| tag = "div" | ||
| convertedCenter = true | ||
| } else if tag == "marquee" || tag == "blink" { | ||
| result.Warnings = append(result.Warnings, Finding{ | ||
| RuleID: "TAG_DEPRECATED_TO_TEXT", Severity: "warning", TagOrAttr: tag, | ||
| Excerpt: "<" + tag + ">", Hint: "kept inner text and removed decorative tag", | ||
| }) | ||
| renderChildren(out, n, result, autoFix) | ||
| return | ||
| } | ||
| attrs := cleanAttrs(n.Attr, result, convertedCenter) | ||
| out.WriteByte('<') | ||
| out.WriteString(tag) | ||
| for _, a := range attrs { | ||
| out.WriteByte(' ') | ||
| out.WriteString(a.Key) | ||
| out.WriteString(`="`) | ||
| out.WriteString(xhtml.EscapeString(a.Val)) | ||
| out.WriteByte('"') | ||
| } | ||
| out.WriteByte('>') | ||
| renderChildren(out, n, result, autoFix) | ||
| out.WriteString("</") | ||
| out.WriteString(tag) | ||
| out.WriteByte('>') | ||
|
Comment on lines
+105
to
+109
There was a problem hiding this comment. htmllint.renderNode unconditionally emits a closing tag for every element. This will produce invalid HTML like |
||
| default: | ||
| for c := n.FirstChild; c != nil; c = c.NextSibling { | ||
| renderNode(out, c, result, autoFix) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| func renderChildren(out *bytes.Buffer, n *xhtml.Node, result *Result, autoFix bool) { | ||
| for c := n.FirstChild; c != nil; c = c.NextSibling { | ||
| renderNode(out, c, result, autoFix) | ||
| } | ||
| } | ||
|
|
||
| func cleanAttrs(attrs []xhtml.Attribute, result *Result, convertedCenter bool) []xhtml.Attribute { | ||
| cleaned := make([]xhtml.Attribute, 0, len(attrs)) | ||
| needsCenterStyle := convertedCenter | ||
| for _, a := range attrs { | ||
| key := strings.ToLower(strings.TrimSpace(a.Key)) | ||
| val := strings.TrimSpace(a.Val) | ||
| if key == "" { | ||
| continue | ||
| } | ||
| if strings.HasPrefix(key, "on") { | ||
| result.Errors = append(result.Errors, Finding{ | ||
| RuleID: "ATTR_EVENT_BLOCKED", Severity: "error", TagOrAttr: key, | ||
| Excerpt: fmt.Sprintf("%s=%q", key, truncate(val)), Hint: "removed event handler attribute", | ||
| }) | ||
| continue | ||
| } | ||
| if (key == "href" || key == "src") && unsafeURL(val) { | ||
| result.Errors = append(result.Errors, Finding{ | ||
| RuleID: "URL_SCHEME_BLOCKED", Severity: "error", TagOrAttr: key, | ||
| Excerpt: fmt.Sprintf("%s=%q", key, truncate(val)), Hint: "removed unsafe URL scheme", | ||
| }) | ||
| continue | ||
| } | ||
| if key == "style" { | ||
| style := cleanStyle(val, result) | ||
| if needsCenterStyle { | ||
| style = mergeStyle("text-align:center", style) | ||
| needsCenterStyle = false | ||
| } | ||
| if style == "" { | ||
| continue | ||
| } | ||
| val = style | ||
| } | ||
| cleaned = append(cleaned, xhtml.Attribute{Key: key, Val: val}) | ||
| } | ||
| if needsCenterStyle { | ||
| cleaned = append(cleaned, xhtml.Attribute{Key: "style", Val: "text-align:center"}) | ||
| } | ||
| return cleaned | ||
| } | ||
|
|
||
| func cleanStyle(raw string, result *Result) string { | ||
| parts := strings.Split(raw, ";") | ||
| kept := make([]string, 0, len(parts)) | ||
| for _, p := range parts { | ||
| if strings.TrimSpace(p) == "" { | ||
| continue | ||
| } | ||
| name, val, ok := strings.Cut(p, ":") | ||
| if !ok { | ||
| continue | ||
| } | ||
| name = strings.ToLower(strings.TrimSpace(name)) | ||
| val = strings.TrimSpace(val) | ||
| if !allowedCSS[name] { | ||
| result.Warnings = append(result.Warnings, Finding{ | ||
| RuleID: "CSS_PROPERTY_REMOVED", Severity: "warning", TagOrAttr: name, | ||
| Excerpt: truncate(p), Hint: "removed unsupported inline style property", | ||
| }) | ||
| continue | ||
| } | ||
| if unsafeURL(val) { | ||
| result.Errors = append(result.Errors, Finding{ | ||
| RuleID: "CSS_URL_BLOCKED", Severity: "error", TagOrAttr: name, | ||
| Excerpt: truncate(p), Hint: "removed unsafe CSS URL", | ||
| }) | ||
| continue | ||
| } | ||
| kept = append(kept, name+":"+val) | ||
| } | ||
| return strings.Join(kept, ";") | ||
| } | ||
|
|
||
| func unsafeURL(raw string) bool { | ||
| s := strings.ToLower(strings.TrimSpace(raw)) | ||
| return strings.HasPrefix(s, "javascript:") || | ||
| strings.HasPrefix(s, "vbscript:") || | ||
| strings.Contains(s, "javascript:") || | ||
| strings.Contains(s, "vbscript:") | ||
| } | ||
|
Comment on lines
+197
to
+203
There was a problem hiding this comment.
Using Suggested fix func unsafeURL(raw string) bool {
s := strings.ToLower(strings.TrimSpace(raw))
- return strings.HasPrefix(s, "javascript:") ||
- strings.HasPrefix(s, "vbscript:") ||
- strings.Contains(s, "javascript:") ||
- strings.Contains(s, "vbscript:")
+ if strings.HasPrefix(s, "javascript:") || strings.HasPrefix(s, "vbscript:") {
+ return true
+ }
+ compact := strings.ReplaceAll(s, " ", "")
+ return strings.Contains(compact, "url(javascript:") ||
+ strings.Contains(compact, "url(vbscript:")
}🤖 Prompt for AI Agents |
||
|
|
||
| func mergeStyle(first, second string) string { | ||
| first = strings.TrimSpace(strings.TrimSuffix(first, ";")) | ||
| second = strings.TrimSpace(strings.Trim(second, ";")) | ||
| if first == "" { | ||
| return second | ||
| } | ||
| if second == "" { | ||
| return first | ||
| } | ||
| return first + ";" + second | ||
| } | ||
|
|
||
| func truncate(s string) string { | ||
| s = strings.TrimSpace(s) | ||
| if len([]rune(s)) <= 80 { | ||
| return s | ||
| } | ||
| r := []rune(s) | ||
| return string(r[:80]) + "..." | ||
| } | ||
|
|
||
| var blockedTags = map[string]bool{ | ||
| "script": true, "style": true, "iframe": true, "object": true, "embed": true, | ||
| "form": true, "input": true, "link": true, "meta": true, "base": true, | ||
| } | ||
|
|
||
| var allowedCSS = map[string]bool{ | ||
| "color": true, "background-color": true, "font-size": true, "font-weight": true, | ||
| "font-style": true, "text-align": true, "text-decoration": true, "line-height": true, | ||
| "padding": true, "margin": true, "border": true, "border-top": true, | ||
| "border-right": true, "border-bottom": true, "border-left": true, "width": true, | ||
| "height": true, "display": true, "text-indent": true, | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,6 +13,7 @@ import ( | |
| "github.com/larksuite/cli/shortcuts/common" | ||
| draftpkg "github.com/larksuite/cli/shortcuts/mail/draft" | ||
| "github.com/larksuite/cli/shortcuts/mail/emlbuilder" | ||
| "github.com/larksuite/cli/shortcuts/mail/htmllint" | ||
| ) | ||
|
|
||
| // draftCreateInput bundles all +draft-create user flags into a single | ||
|
|
@@ -163,6 +164,13 @@ var MailDraftCreate = common.Shortcut{ | |
| if strings.TrimSpace(input.Body) == "" { | ||
| return output.ErrValidation("effective body is empty after applying template; pass --body explicitly") | ||
| } | ||
| var lintReport htmllint.Result | ||
| if !input.PlainText && bodyIsHTML(input.Body) { | ||
| input.Body, lintReport, err = lintHTMLBeforeWrite(input.Body) | ||
| if err != nil { | ||
| return err | ||
|
Comment on lines
+167
to
+171
There was a problem hiding this comment. In +draft-create, linting is only applied when the incoming body is already HTML. However buildRawEMLForDraftCreate upgrades plain text to HTML when a signature is requested (sigResult != nil), which means that write path can bypass linting entirely. Consider moving linting to the point where the final HTML body is assembled (after the plain-text→HTML upgrade and signature injection), or broadening the condition to also lint when sigResult is non-nil. |
||
| } | ||
| } | ||
|
Comment on lines
+167
to
+173
There was a problem hiding this comment. Lint gate misses signature-triggered HTML compose path. On Line 168, lint only runs when 💡 Proposed fix- var lintReport htmllint.Result
- if !input.PlainText && bodyIsHTML(input.Body) {
- input.Body, lintReport, err = lintHTMLBeforeWrite(input.Body)
- if err != nil {
- return err
- }
- }
sigResult, err := resolveSignature(ctx, runtime, mailboxID, runtime.Str("signature-id"), runtime.Str("from"))
if err != nil {
return err
}
+ var lintReport htmllint.Result
+ if !input.PlainText && (bodyIsHTML(input.Body) || sigResult != nil) {
+ input.Body, lintReport, err = lintHTMLBeforeWrite(input.Body)
+ if err != nil {
+ return err
+ }
+ }Please add a regression test for plain-text body + signature ( 🤖 Prompt for AI Agents |
||
| sigResult, err := resolveSignature(ctx, runtime, mailboxID, runtime.Str("signature-id"), runtime.Str("from")) | ||
| if err != nil { | ||
| return err | ||
|
|
@@ -176,7 +184,7 @@ var MailDraftCreate = common.Shortcut{ | |
| if err != nil { | ||
| return fmt.Errorf("create draft failed: %w", err) | ||
| } | ||
| out := addLintReport(map[string]interface{}{"draft_id": draftResult.DraftID}, lintReport) | ||
| if draftResult.Reference != "" { | ||
| out["reference"] = draftResult.Reference | ||
| } | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lintHTMLBeforeWrite currently returns result.CleanedHTML whenever it is non-empty. Since htmllint.Lint(autoFix=true) always renders HTML into CleanedHTML for any non-empty input, this will rewrite/canonicalize all HTML even when there are no findings. Consider only returning CleanedHTML when it differs from the input or when findings require a change, otherwise return the original raw string to avoid unexpected formatting/escaping changes.