fix: render portal contact-form html instead of escaping it

.agents/skills/pr-ready/SKILL.md

@@ -1,12 +1,12 @@
 ---
 name: pr-ready
-description: Pre-PR flight check on the current branch. Reviews the diff against stated intent, flags scope creep, regression risks, and commit hygiene problems, and drafts a compact PR title (conventional-commit style) and description. Requires a clean working tree. Does not run tests, lint, or type-check.
+description: Pre-PR flight check on the current branch. Re-anchors on the intended change and reviews the diff against it at a macro level — scope, completeness, drift — flags risky or sensitive changes for the reviewer, and drafts a compact PR title (conventional-commit style) and description. Requires a clean working tree. Does not run tests, lint, or type-check, and is not a substitute for line-level code review.
 disable-model-invocation: true
 ---
 
 # PR Pre-Flight Reviewer
 
-A focused review pass before opening a pull request. The goal is a PR that is easy on the reviewer: tight scope, no bloat, surprises called out up front.
+A focused macro pass before opening a pull request, run *after* ordinary code review. It assumes the lines are already reviewed; its job is to re-anchor on what the change was *meant* to do and check the diff against that — catching a session that drifted, even when the code it produced looks fine. Three outputs: a faithful **summary** (the PR title and description are the source of truth for release notes), a **scope check** (is the intent fully and proportionately delivered, no drift?), and a **look-forward** pass (what should the reviewer focus on; what could bite users or maintainers?).
 
 ## When to use
 
@@ -59,39 +59,35 @@ git diff --stat "$base..HEAD"                  # file-level overview
 git diff "$base..HEAD"                          # full content diff
 ```
 
-`--no-merges` drops commits like `Merge branch 'main'` that only pulled the default branch back into this one — they aren't this branch's own work and shouldn't be analyzed under commit hygiene or intent. The diff itself is already scoped via the merge-base, so changes brought in by those merges don't appear in `git diff "$base..HEAD"` either; only edits actually made on this branch (including any conflict resolutions in the merge commit) show up.
+`--no-merges` drops `Merge branch 'main'`-style commits that only pulled the default branch back in — not this branch's own work. The merge-base already scopes the diff to this branch's edits (including conflict resolutions). Since merges are squashed here, don't review commits for hygiene — the commit list only helps infer intent and scope.
 
 Read the full diff. For larger changes, use `--stat` first to plan where to look closest.
 
-## Phase 4 — Analyze against five vectors
+## Phase 4 — Analyze
 
-For each vector, note specific findings with `file:line` references when applicable.
+Re-read the locked intent, then judge the diff against it at a **macro level** — not a second line-level review (that's already done), but a check on whether this is still the right change, whole and no more. Note `file:line` for concrete, locatable items.
 
-1. **Intent alignment** — Does every hunk serve the stated intent? Flag scope creep, accidental edits, refactors that snuck in, drive-by formatting.
-2. **Code smells / bloat** — `console.log`, `print`, leftover `TODO`/`FIXME` from this change, commented-out code, debug branches, unused imports, over-verbose logic, unnecessary new abstractions.
-3. **Artifacts** — Files that shouldn't be in a PR: editor configs, local env files, build outputs, generated files that should be gitignored, lockfile changes unrelated to dependency edits.
-4. **Regression risks** — Behavior changes in shared code, removed or renamed exports, signature changes on public APIs, changes to defaults, error-handling changes, anything subtle that could break callers. Each item must be specific enough that the reviewer and the developer can decide whether it's intentional.
-5. **Commit hygiene** — Fix-up / "wip" / vague-message commits, commits that should be squashed, messages that don't match what the commit actually changes. If the chain is messy, suggest a concrete squash/rewrite plan.
+1. **Scope** — Does every hunk serve the intent? Flag scope creep, accidental edits, refactors that snuck in, drive-by formatting. A diff that doesn't fit a single intent is a signal the PR is doing too much.
+2. **Completeness & proportion** — Measured against the intent, not as standalone code quality:
+   - *Under-delivered* — is the intent actually finished, or left half-built or stubbed?
+   - *Over-delivered / drifted* — did it grow past the intent (gold-plating, speculative generality, unrelated refactors)?
+   - *Companions* — does the change carry what makes it complete for this intent: tests for the new behavior, and docs/types/comments that describe it?
+3. **Loose ends & artifacts** — Diff-local things that shouldn't ship: leftover `console.log`/debug branches/commented-out code/stray `TODO` from this change, unused imports; and files that don't belong (editor configs, local env files, build outputs, unrelated lockfile churn).
+4. **Risks / heads-up** — What should the reviewer look at hardest, and what could bite users or maintainers later? Behavior changes in shared code, removed or renamed exports, signature or default changes on public APIs, error-handling changes, data migrations — anything subtle that could break callers or surprise users.
 
 ## Phase 5 — Report
 
-Output a single compact report in this order. If a section has nothing to say, write "No issues" rather than omitting it.
+Output a single compact report in this order.
 
 ```
 ## Summary
-<two sentences max: what this branch does>
+<what this branch does — the basis for the PR description>
 
-## Intent check
-<aligned, or list deviations>
+## Scope & completeness
+<intent in a phrase; aligned or deviations; under/over-delivery; missing tests or docs; loose ends — with file:line>
 
-## Cleanup
-- <bullets with file:line where useful>
-
-## Regression flags
-- <each one specific and actionable>
-
-## Commit hygiene
-- <bullets; include a squash plan if needed>
+## Risks / heads-up
+- <what to review hardest, or a one-line "low risk" note>
 
 ## Suggested PR title
 <one line, ready to paste>
@@ -100,6 +96,8 @@ Output a single compact report in this order. If a section has nothing to say, w
 <markdown block, ready to paste>
 ```
 
+Be honest about emptiness: "Scope: aligned and complete" or "Low risk — isolated change" is a complete section. A short, true section beats a padded one; never invent findings to make a section look substantial.
+
 ### Suggested PR title: format
 
 Follow Conventional Commits. Pick the prefix that best matches the dominant change in the diff:
@@ -119,21 +117,22 @@ Optional scope in parentheses when the repo already uses it (look at recent `git
 Style:
 - Imperative mood, lowercase after the colon, no trailing period.
 - Aim for ≤ 72 characters so it doesn't truncate in GitHub.
-- If the branch already has a single, well-formed conventional commit, reuse its subject as the title rather than inventing a new one.
-- If the changes don't fit a single prefix, that's a signal the PR is doing too much — flag it under "Intent check" rather than papering over it with `chore:`.
+- If the changes don't fit a single prefix, that's a signal the PR is doing too much — flag it under "Scope & completeness" rather than papering over it with `chore:`.
 
 ### Suggested PR description: format
 
-The PR description is the most important output. Reviewers read it first; if it's good, the rest of the review goes faster.
+The most important output: reviewers read it first, and it becomes the release note.
 
 Required content:
 - **What changed**, in plain terms (one sentence to a few bullets, scaled to the change).
 - **Why** — the intent, in the user's own words from Phase 2.
-- **Regression risks** — the items from Phase 4 vector 4, restated for the reviewer. This is the most critical section; it lets the reviewer focus their attention where it matters.
+
+Optional:
+- **Risks / heads-up** — include only when there's something the reviewer or a future maintainer should know (the items from Phase 4 vector 4). Omit the section entirely for low-risk changes rather than writing "none".
 
 Length guidance: as compact as the change allows. A typo fix is one sentence. A multi-part feature is a short summary plus a tight bullet list. **No filler, no test-plan boilerplate, no marketing tone.** If you find yourself padding, stop.
 
-Line breaks: **no superfluous line-breaks.** Only insert double line breaks where it marks a genuine section delimitation (e.g. between the summary and a `**Why:**` block, or before a bullet list). Do not separate every sentence with a line-break, do not pad between bullets, and do not add leading or trailing blank lines.
+Line breaks: **no superfluous blank lines.** Insert one only at a genuine section break (before a `**Why:**` block or a bullet list) — not between every sentence, not as padding, not leading or trailing.
 
 Example, for a small feature:
 
@@ -144,8 +143,7 @@ Add `--dry-run` flag to the import command so users can preview changes without
 
 **Why:** requested by ops to validate large imports before committing.
 
-**Regression risks:**
-- Default behavior unchanged, but the new flag short-circuits before the DB transaction — double-check no downstream code assumed the transaction always runs.
+**Heads-up:** the new flag short-circuits before the DB transaction — double-check no downstream code assumed the transaction always runs.
 ```
 
 ### Persist to files
@@ -174,4 +172,5 @@ Then tell the user, in one short message, the **resolved** file paths and the pl
 - Tree not clean → stop, do not review.
 - Intent unclear after inference → ask, do not guess silently.
 - Long list of "nice to have" cleanup → keep it tight; only call out things that actually matter for this PR.
-- Regression section empty on a non-trivial diff → re-check. Most non-trivial diffs have at least one risk worth flagging.
+- Manufacturing findings to make a section look substantial → stop. "Scope: aligned" and "Low risk" are valid results.
+- Sliding into line-level code review → stop. That pass is already done; stay at the scope-and-intent altitude.

api/src/mails/router.ts

@@ -135,7 +135,12 @@ router.post('/contact', async (req, res) => {
     path: site?.path,
     subject: req.body.subject,
     text,
-    htmlMsg: textToSafeHtml(text),
+    // the portal contact form builds an HTML body client-side (lists, bold
+    // labels, line breaks, optional bodyTemplate_html); sanitize rather than
+    // escape so that structure renders while scripts/dangerous hrefs are
+    // stripped. The body is partly anonymous-visitor-controlled, so the
+    // sanitizer (not raw passthrough) stays the trust boundary.
+    htmlMsg: sanitizeMailHtml(text),
     htmlCaption: ''
   })
   res.send(req.body)

docs/architecture/emails.md

@@ -89,9 +89,17 @@ HTML from reaching the recipient.
   proof) plus IP-based rate limit (1 req / 60s, mongo-backed via
   `RateLimiterMongo`).
 - The `text` is wrapped with a "Message transmitted by the contact form of
-  …" prefix and sent both as plain-text and as
-  `textToSafeHtml(...)` → `htmlMsg`. The schema accepts text only, so the
-  caller never gets to inject HTML.
+  …" prefix and sent both as plain-text and as `sanitizeMailHtml(...)` →
+  `htmlMsg`. The schema names the field `text`, but the portal contact form
+  (the only known caller) composes an **HTML** body client-side — lists,
+  bold labels, line breaks, an optional `bodyTemplate_html` — and posts it
+  in that field. So `sanitizeMailHtml` (the same allow-list sanitizer the
+  `/api/mails` route applies to caller html), not `textToSafeHtml`, is what
+  guards this value: structure renders, scripts and non-http(s)/mailto hrefs
+  are stripped. The body is partly anonymous-visitor-controlled (the portal
+  interpolates the visitor's typed text into the HTML without escaping), so
+  the sanitizer is the trust boundary — raw passthrough would re-open the
+  injection path.
 
 ### Direct `sendMailI18n` (internal, no HTTP)
 
@@ -109,7 +117,7 @@ HTML from reaching the recipient.
 
 | Service | Endpoint | Caller (file:line) | What reaches the template |
 |---------|----------|---------------------|---------------------------|
-| portals | `POST /api/mails/contact` | `portal/app/components/page-element/basic/page-element-contact.vue:354` | `text` (the form body — schema rejects html, server escapes) |
+| portals | `POST /api/mails/contact` | `portal/app/components/page-element/basic/page-element-contact.vue:354` | `text` (an HTML body composed client-side — schema names it `text`, server sanitizes it with `sanitizeMailHtml`) |
 | events  | `POST /api/mails`         | `events/api/src/notifications/service.ts:65` | `html` (notification `htmlBody`, third-party-supplied) — sanitized by `sanitizeMailHtml` |
 | sd internal | `sendMailI18n` direct | auth / users / invitations / organizations routers, users worker | i18n template HTML; substituted params come from validated URLs and trusted config |
 
@@ -156,10 +164,15 @@ posture in [`./email-trust-and-site-isolation.md`](./email-trust-and-site-isolat
 2. Caller-supplied `html` to `POST /api/mails` is run through
    `sanitizeMailHtml` (a strict tag allow-list, `href` schemes restricted
    to `http`/`https`/`mailto`) before reaching the MJML substitution.
-3. Caller-supplied `text` to either endpoint reaches `htmlMsg` only via
-   `textToSafeHtml` (HTML-escape + `\n`→`<br>`).
-4. The contact-form schema (`api/contract/contact-mail.ts`) admits `text`
-   only; the server never reads an html field from the contact-form caller.
+3. Caller-supplied `text` to `POST /api/mails` (the internal route, when no
+   `html` is given) reaches `htmlMsg` only via `textToSafeHtml`
+   (HTML-escape + `\n`→`<br>`).
+4. The contact-form `text` value reaches `htmlMsg` only via
+   `sanitizeMailHtml` (strict tag allow-list, `href` schemes restricted to
+   `http`/`https`/`mailto`). The schema (`api/contract/contact-mail.ts`)
+   admits no separate html field, but the `text` it accepts is itself a
+   client-composed HTML body, so the sanitizer — not raw passthrough — is
+   the boundary that keeps anonymous-controlled markup safe.
 5. `sendMailI18n` rejects a `params.link` whose protocol is not `http:` or
    `https:`, so `{link}` (button `href`) and the derived `{origin}` /
    `{host}` cannot carry a `javascript:` / `data:` payload into an i18n

skills-lock.json

@@ -5,7 +5,7 @@
       "source": "data-fair/lib",
       "sourceType": "github",
       "skillPath": "skills/pr-ready/SKILL.md",
-      "computedHash": "9fc1b00c6c180d8b46205d713328136c1101107c1991db7d8ab59bc14d0adfa2"
+      "computedHash": "dcf190b1d7fb792318b50fa4515a912fdaee01822c6ef158319f8866f5b97655"
     }
   }
 }

tests/features/mails.api.spec.ts

@@ -124,24 +124,25 @@ test.describe('mails', () => {
     assert.ok(email.html.includes('https://example.com/ok'), 'http(s) href should survive')
   })
 
-  test('Contact form: user text is HTML-escaped in the html part', async () => {
+  test('Contact form: portal-built html structure renders, dangerous tags stripped', async () => {
     await testEnvAx.patch('/config', { anonymousContactForm: true })
     const ax = await axios()
     const token = (await ax.get('/api/auth/anonymous-action')).data
+    // the portal contact form composes an HTML body client-side; reproduce a
+    // representative payload mixing safe structure with an injected script
     const res = await ax.post('/api/mails/contact', {
       token,
       from: 'visitor@test.com',
       subject: 'contact-injection',
-      text: 'hello <script>alert(1)</script>\nsecond line'
+      text: '<ul><li><strong>Subject</strong> : hi</li></ul><p>body<script>alert(1)</script></p>'
     })
     assert.equal(res.status, 200)
     const email = await findEmail('contact-injection')
     assert.ok(email)
-    assert.ok(email.html.includes('&lt;script&gt;alert(1)&lt;/script&gt;'),
-      'html should contain escaped <script>')
-    assert.ok(!email.html.includes('<script>alert(1)</script>'),
-      'html must not contain a raw <script> tag')
-    assert.ok(email.html.includes('<br>second line'), 'newline should be turned into <br>')
+    assert.ok(email.html.includes('<strong>Subject</strong>'), 'safe structural tags should render')
+    assert.ok(email.html.includes('<li>'), 'list structure should survive')
+    assert.ok(!email.html.includes('<script'), '<script> tag must be stripped')
+    assert.ok(!email.html.includes('alert(1)'), 'script content must be stripped')
   })
 
   test('Send email to address and with attachments', async () => {