fix(legal): drop img from the allowed markdown tags - #30
Merged
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The functional change is a straightforward allowlist narrowing with corresponding docs and tests, and the remaining feedback is minor wording accuracy.
Pull request overview
This PR aligns the legal-document Markdown allowlist and documentation with what instances can realistically render by removing image support end-to-end (parser allowlist, docs, and styling), and adds a regression test to ensure images are dropped.
Changes:
- Remove
imgfrom the legal Markdown allowed tag list and drop the now-unusedallowDataImagessetting. - Update self-hosting documentation to no longer claim images are allowed in legal pages.
- Remove legal-document image styling and add a test asserting images are dropped.
File summaries
| File | Description |
|---|---|
| apps/docs/content/2.self-hosting/5.legal-pages.md | Updates docs to remove images from the stated allowed Markdown set. |
| apps/app/tests/legal-markdown.test.ts | Adds a regression test ensuring image syntax doesn’t produce <img> nodes. |
| apps/app/shared/utils/legal-markdown.ts | Removes img from the Markdown sanitizer/tag allowlist and simplifies security plugin options. |
| apps/app/app/components/LegalDocument.vue | Removes CSS targeting images inside legal documents. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What this changes
Closes #29
Removes
imgfrom the tag allowlist inshared/utils/legal-markdown.ts, along with the now-pointlessallowDataImages: falseand the image rule in the legal document stylesheet. The documentation stops listing images among the allowed Markdown.Why
An operator has nowhere to host an image. The instance serves no user-supplied assets, and the CSP sets
img-src 'self' data: blob:, so an external URL is refused by the browser and adata:URI was already rejected by the parser. Everything an operator could realistically write rendered as a broken icon, in the editor preview and on the published page alike, with a silent CSP refusal as the only explanation.Dropping the tag makes the allowlist describe what actually works. Nothing is lost: no shipped template uses an image, and a document that contains one now loses the image rather than showing it broken.
This came out of a Copilot review on #28, which framed it as a security mismatch. It is not one — the CSP refusing external images is the intended layered defence, and the parser is the second layer. The reason to remove the tag is that it cannot work, not that it is unsafe.
Checks
pnpm lint,pnpm typecheckandpnpm testpasspnpm db:generate)No schema change, no crypto, no authorization, no read counter. It narrows an existing allowlist, so the change can only reject more than before. A test covers both a relative and a remote image being dropped.
AI assistance
See CONTRIBUTING.md. Disclosure is not held against you: it tells
the reviewer where to look hardest.