Fix: Security + robustness hardening: Markdown render sanitization, folder path validation, regex perf, and event listener cleanup

[worldlyjohn]

Summary

While reviewing the plugin codebase, I noticed a few areas that would benefit from security/robustness hardening:

1. Rendering user-provided comment text via MarkdownRenderer.renderMarkdown(...) without an explicit “no raw HTML” / sanitization guarantee.
2. Free-text folder path setting for “Markdown comments folder” without normalization/validation (potential path traversal / unexpected write locations depending on Vault API behavior).
3. Multi-line regex over potentially large note content (possible performance degradation on large files / frequent updates).
4. CodeMirror click listener uses .bind(this) in both add/remove, which prevents proper cleanup and can lead to listener accumulation.

None of these appear to be active remote RCE issues, but together they can increase risk (XSS-like injection in rendered panes) and stability/perf issues.

Affected code

• Markdown rendering of comment bodies:
  • src/main.ts (MarkdownRenderer.renderMarkdown(comment.comment || "", ...))
• Folder path setting:
  • src/main.ts (Setting: “Markdown comments folder”)
• Regex-based markdown comment block manipulation:
  • src/core/markdownCommentBlocks.ts (replaceMarkdownCommentBlock, removeMarkdownCommentBlock)
• Event listener cleanup bug:
  • src/main.ts (CodeMirror ViewPlugin: addEventListener('click', this.handleClick.bind(this)) and removeEventListener('click', this.handleClick.bind(this)))

Details / Risks

1) Markdown rendering sanitization

Comments are rendered through Obsidian’s MarkdownRenderer. If raw HTML is allowed or sanitization is not guaranteed in this context (or impacted by other plugins), a maliciously crafted comment could inject unsafe HTML into the sidebar view.

Recommendation

• If Obsidian supports it, render with raw HTML disabled (or sanitize prior to rendering).
• Alternatively, sanitize rendered output (e.g., DOMPurify) before inserting into the DOM (if compatible with Obsidian).

Acceptance criteria

• Comment rendering path either:
  • explicitly disallows raw HTML, OR
  • sanitizes before insertion, OR
  • documents with a clear rationale why Obsidian sanitization is sufficient and links to the relevant Obsidian API behavior.

2) Validate/normalize “Markdown comments folder”

The “Markdown comments folder” setting is a free-text string. If later used for reads/writes, it should be normalized and validated to prevent unexpected traversal (e.g., ../) or invalid/absolute paths.

Recommendation

• Normalize and validate the path:
  • reject absolute paths
  • remove or reject .. segments
  • ensure the resolved folder stays under the vault root
  • ensure it exists or create it safely via Vault APIs

Acceptance criteria

• Folder path is normalized and validated before use.
• Invalid paths are rejected with a helpful error message.

3) Regex over large markdown content (perf / DoS-by-size)

replaceMarkdownCommentBlock / removeMarkdownCommentBlock use multi-line regex patterns that can scan large files ([^]*? etc.). This can be expensive on large notes and may cause noticeable lag if triggered frequently.

Recommendation

• Replace multi-line regex with a more deterministic parser:
  • scan for start/end markers with indexOf and slice, OR
  • line-based state machine
• Add a size guard for extremely large files if needed.

Acceptance criteria

• Operations on large notes (e.g., >1–5MB) remain responsive.
• No heavy multi-line regex across entire documents in tight loops.

4) Event listener cleanup bug (.bind creates a new function)

The click listener is attached with this.handleClick.bind(this) and removed with this.handleClick.bind(this) again. Since .bind returns a new function each time, the remove call won’t match, leaving orphan listeners.

Recommendation

• Store the bound handler once:
  • this.boundHandleClick = this.handleClick.bind(this)
  • use this.boundHandleClick for both add/remove

Acceptance criteria

• Listener is correctly removed on plugin unload/view destroy (no accumulation).

Proposed implementation sketch

Fix listener cleanup

class CommentClickPlugin {
  private boundClick: (e: MouseEvent) => void;

  constructor(view: EditorView) {
    this.boundClick = this.handleClick.bind(this);
    view.dom.addEventListener("click", this.boundClick);
  }

  destroy() {
    this.view.dom.removeEventListener("click", this.boundClick);
  }
}

Folder path validation

• Implement a helper:

  • normalizeCommentsFolderPath(input: string): string | null

• Reject invalid values and show a Notice.

Regex replacement

• Implement marker-based slicing (start marker index + end marker index) and replace the content between them.

[mofukuru]

All four items have been addressed in v1.0.6:

1. Markdown rendering sanitization — Reviewed. MarkdownRenderer.renderMarkdown() is Obsidian's official API and applies Obsidian's own content policy to all rendered Markdown. No additional sanitization was added, as the rendering relies on the same guarantees Obsidian applies throughout the app. This will be revisited if Obsidian exposes an explicit option to disable raw HTML.

2. Folder path validation — Fixed. Added normalizeCommentsFolderPath() which:

• Rejects absolute paths
• Rejects paths containing .. segments
• Falls back to the default folder (side-note-comments) with a Notice if the input is invalid

3. Multi-line regex — Fixed. Replaced with indexOf-based parsing (locate start/end markers, then slice). No regex over the entire document body.

4. Event listener cleanup — Fixed. The bound handler is now stored as a class property and the same reference is used for both addEventListener and removeEventListener, so the listener is correctly removed on destroy().

Thanks for the thorough audit — this was very helpful!