Add Markdown text escaping - #166
Conversation
|
🔍 OpenCodeReview found 19 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| return "".join( | ||
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | ||
| for char in escaped | ||
| ) |
There was a problem hiding this comment.
The ordinal comparison char > " " relies on implicit string-to-ordinal coercion. While correct for ASCII, consider using ord(char) > 32 for clarity. Additionally, \x7f (DEL, 127) is explicitly excluded from percent-encoding but not documented. The docstring should clarify that DEL is intentionally preserved.
There was a problem hiding this comment.
Thanks for the review. DEL is in fact percent-encoded here: the passthrough condition requires char != "\x7f", so \x7f takes the encoding branch — the docstring's "control characters" covers it. On the char > " " idiom: agreed ord() would be more explicit, but the comparison is correct for single characters and we're keeping it as-is.
| """ | ||
| return lambda props: block([prefix, props["children"]]) | ||
| return lambda props: block( | ||
| [mark_safe(prefix, block_prefix=block_prefix), props["children"]] |
There was a problem hiding this comment.
Type mismatch: mark_safe passes block_prefix as a Python bool, but engines/markdown.py:157 compares it to the string "true". This means block_prefix=True will never match, and at_line_start will never be set to True after a mark_safe element with block_prefix=True. This breaks the nested block behavior for blockquotes and list indentation. Fix by comparing to the boolean True instead.
Suggestion:
| [mark_safe(prefix, block_prefix=block_prefix), props["children"]] | |
| c.attr.get("block_prefix") is True |
There was a problem hiding this comment.
This would be a real bug if attributes weren't stringified, but DOM.create_element coerces True to the string "true" before create_tag sees it, and the behavior is pinned end-to-end by test_line_start_in_list_item and test_line_start_in_blockquote in tests/markdown/test_escaping.py (both fail if the comparison stops matching). Not a defect.
| else: | ||
| out.append(escape_text(c, at_line_start)) | ||
| if c: | ||
| at_line_start = c.endswith(("\n", "\r")) |
There was a problem hiding this comment.
Empty string children don't update at_line_start, causing consecutive empty strings to inherit stale state. This could lead to incorrect escaping for empty strings that should be at line start but aren't marked as such. Consider using 'if not c or c.endswith(('\n', '\r'))' to handle empty strings correctly (they don't change line start state but shouldn't break tracking).
Suggestion:
| else: | |
| out.append(escape_text(c, at_line_start)) | |
| if c: | |
| at_line_start = c.endswith(("\n", "\r")) | |
| else: | |
| out.append(escape_text(c, at_line_start)) | |
| if c.endswith(('\n', '\r')): | |
| at_line_start = True |
There was a problem hiding this comment.
Inheriting state after an empty string is intentional: nothing was emitted, so line-start state must not change. The suggested change would set at_line_start=True after empty strings mid-line (e.g. right after a "**" marker), producing false-positive escapes. Current behavior is correct.
| def test_renders_verbatim(self): | ||
| self.assertEqual(DOM.render(inline([mark_safe("# "), "x"])), "# x") |
There was a problem hiding this comment.
This test passes coincidentally because 'x' has no Markdown metacharacters. Consider using a value with special characters (e.g., 'bold' or 'italic') to more robustly verify that: (1) mark_safe content renders verbatim, and (2) plain strings are still properly escaped after mark_safe content.
There was a problem hiding this comment.
Fair point — addressed in ae50109: the test now uses "x", so it verifies both verbatim mark_safe rendering and escaping of the following plain string.
| class TestMarkdownEscaping(unittest.TestCase): | ||
| def setUp(self): | ||
| self.exporter = HTML(MARKDOWN_CONFIG) | ||
|
|
||
| def render(self, *args: Any, **kwargs: Any) -> str: | ||
| return self.exporter.render(content_state(*args, **kwargs)) |
There was a problem hiding this comment.
In render_children, block_prefix is compared as a string (== "true"), but mark_safe passes the boolean directly to DOM.create_element. If DOM.create_element does not coerce booleans to lowercase strings, the condition c.attr.get("block_prefix") == "true" would never match, causing line-start escaping to fail after block prefixes.
Evidence: test_helpers.py tests that elt.attr["block_prefix"] equals the string "true" (line 22) and "false" (line 26), confirming the expected stored type. The implementation should either: (1) ensure mark_safe stores block_prefix as a string (str(block_prefix)), or (2) use a boolean comparison in render_children (c.attr.get("block_prefix") is True).
Suggestion:
| class TestMarkdownEscaping(unittest.TestCase): | |
| def setUp(self): | |
| self.exporter = HTML(MARKDOWN_CONFIG) | |
| def render(self, *args: Any, **kwargs: Any) -> str: | |
| return self.exporter.render(content_state(*args, **kwargs)) | |
| if ( | |
| c.type == "mark_safe" | |
| and c.attr | |
| and str(c.attr.get("block_prefix")) == "true" | |
| ): |
There was a problem hiding this comment.
Same as the other block_prefix comment: DOM.create_element stringifies booleans to "true"/"false", which the helper tests assert directly. Note the suggested str(...) comparison would actually break if a raw bool ever reached the attr, since str(True) is "True", not "true".
| "element": make_code_element(), | ||
| "wrapper": make_code_wrapper(fence), |
There was a problem hiding this comment.
The make_code_element() function no longer accepts a fence parameter (line 151), but the default export code_element is still constructed via make_code_element() with no arguments, which is consistent. This change removes the deprecated fence parameter from the element level, with fence handling now centralized in make_code_wrapper.
Suggestion:
| "element": make_code_element(), | |
| "wrapper": make_code_wrapper(fence), | |
| "element": make_code_element(), | |
| "wrapper": make_code_wrapper(fence), |
There was a problem hiding this comment.
This suggestion matches the code as written — nothing to change.
| "wrapper": list_wrapper, | ||
| }, | ||
| BLOCK_TYPES.BLOCKQUOTE: prefixed_block("> "), | ||
| BLOCK_TYPES.BLOCKQUOTE: prefixed_block("> ", block_prefix=True), |
There was a problem hiding this comment.
The block_prefix=True parameter for blockquotes is correctly added in both BLOCK_MAP and build_markdown_config. However, verify that the escaping behavior after blockquote prefixes handles nested constructs correctly (e.g., a line like # Title inside a blockquote should be escaped to \# Title).
Suggestion:
| BLOCK_TYPES.BLOCKQUOTE: prefixed_block("> ", block_prefix=True), | |
| BLOCK_TYPES.BLOCKQUOTE: prefixed_block("> ", block_prefix=True), |
There was a problem hiding this comment.
Already the case as written, and the nested-construct behavior is pinned by test_line_start_in_blockquote in tests/markdown/test_escaping.py.
| def escape_link_destination(url: str) -> str: | ||
| """Escape a URL for use as an inline link destination inside ``](…)``. | ||
|
|
||
| Backslash-escapes backslashes and parentheses (which would otherwise | ||
| break out of the destination), and percent-encodes ASCII whitespace and | ||
| control characters, which inline destinations may not contain. URL | ||
| scheme validation is out of scope: it is the integrator's | ||
| responsibility (see ``docs/SECURITY.md``). | ||
|
|
||
| Parameters: | ||
| url: The URL to escape. | ||
|
|
||
| Returns: | ||
| The escaped destination. | ||
| """ | ||
| escaped = url.replace("\\", "\\\\").replace("(", "\\(").replace(")", "\\)") | ||
| return "".join( | ||
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | ||
| for char in escaped | ||
| ) |
There was a problem hiding this comment.
The function does not escape backticks (`). While CommonMark permits literal backticks in link destinations (](url)), there is no isolation between URL content and code span delimiter computation. A URL like https://example.com/a`````b could cause code_span_delimiters to generate unexpectedly long delimiters. Consider escaping backticks here alongside ( and ) to ensure URL content cannot influence delimiter sizing.
Suggestion:
| def escape_link_destination(url: str) -> str: | |
| """Escape a URL for use as an inline link destination inside ``](…)``. | |
| Backslash-escapes backslashes and parentheses (which would otherwise | |
| break out of the destination), and percent-encodes ASCII whitespace and | |
| control characters, which inline destinations may not contain. URL | |
| scheme validation is out of scope: it is the integrator's | |
| responsibility (see ``docs/SECURITY.md``). | |
| Parameters: | |
| url: The URL to escape. | |
| Returns: | |
| The escaped destination. | |
| """ | |
| escaped = url.replace("\\", "\\\\").replace("(", "\\(").replace(")", "\\)") | |
| return "".join( | |
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | |
| for char in escaped | |
| ) | |
| def escape_link_destination(url: str) -> str: | |
| """Escape a URL for use as an inline link destination inside ``](…)``. | |
| Backslash-escapes backslashes, parentheses, and backticks (which would | |
| otherwise break out of the destination or interfere with code span | |
| delimiter sizing), and percent-encodes ASCII whitespace and control | |
| characters, which inline destinations may not contain. URL scheme | |
| validation is out of scope: it is the integrator's responsibility | |
| (see ``docs/SECURITY.md``). | |
| Parameters: | |
| url: The URL to escape. | |
| Returns: | |
| The escaped destination. | |
| """ | |
| escaped = ( | |
| url.replace("\\", "\\\\") | |
| .replace("(", "\\(") | |
| .replace(")", "\\)") | |
| .replace("`", "\\`") | |
| ) | |
| return "".join( | |
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | |
| for char in escaped | |
| ) |
There was a problem hiding this comment.
Link destinations never pass through code_span_delimiters — code span delimiters are sized from the code span's own text content only, so a backtick in a URL cannot influence delimiter sizing. Not a defect.
| return "".join( | ||
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | ||
| for char in escaped | ||
| ) |
There was a problem hiding this comment.
The condition char > " " uses lexicographic string comparison. For single ASCII characters this is equivalent to ord(char) > 32, but the intent is implicit. Using ord(char) > 32 makes the comparison explicit and is the more common idiom in this codebase's sibling modules (e.g., ord(char):02X on the same line).
Suggestion:
| return "".join( | |
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | |
| for char in escaped | |
| ) | |
| return "".join( | |
| char if ord(char) > 32 and char != "\x7f" else f"%{ord(char):02X}" | |
| for char in escaped | |
| ) |
There was a problem hiding this comment.
Same note as the earlier round's comment on this line: keeping char > " " — it is correct for single characters.
| A fragment containing the children and a trailing blank line. | ||
| """ | ||
| return DOM.create_element("fragment", {}, children + ["\n\n"]) | ||
| return DOM.create_element("fragment", {}, children + [mark_safe("\n\n")]) |
There was a problem hiding this comment.
The block function always appends a mark_safe("\n\n") element to children. If children already contains a trailing mark_safe with newlines (e.g., from a code block), this would result in multiple blank lines. Consider checking if the last child already ends with a mark_safe containing trailing newlines to avoid duplication.
There was a problem hiding this comment.
block() is only used by paragraph-like components; code blocks render via the code_block node and never pass through block(), so this duplication cannot occur with the current component set. A hypothetical misuse would produce a cosmetic extra blank line at worst.
| @@ -0,0 +1,258 @@ | |||
| """Integration tests for Markdown escaping at the exporter level.""" | |||
There was a problem hiding this comment.
The file is listed as test_escape.py in the modified files list but the diff shows test_escaping.py. Verify the file was renamed intentionally and the test file exists at the correct location.
There was a problem hiding this comment.
Both files exist intentionally: tests/markdown/test_escape.py covers the escaping primitives, tests/markdown/test_escaping.py covers exporter-level integration.
| class TestCodeElement(unittest.TestCase): | ||
| def test_works(self): | ||
| def test_renders_line_with_newline(self): | ||
| self.assertEqual( | ||
| DOM.render( | ||
| code_element( | ||
| { | ||
| "block": {}, | ||
| "children": "test", | ||
| } | ||
| ) | ||
| ), | ||
| DOM.render(code_element({"block": {}, "children": "test"})), | ||
| "test\n", | ||
| ) |
There was a problem hiding this comment.
The removed tests test_block_end and test_tilde_fence_block_end verified that closing fences are rendered when a code block ends. While the closing fence is still rendered (now handled by the code_block node in the Markdown engine), the unit test coverage for this behavior in code_element and make_code_element has been removed. The new integration tests (TestCodeBlockExport) cover the full rendering but don't directly test the component functions themselves. If the component-to-engine contract is accidentally broken (e.g., content not reaching code_block), this would not be caught by unit tests.
There was a problem hiding this comment.
Coverage moved levels deliberately: the component contract is pinned by test_renders_line_with_newline, and closing-fence rendering is covered at exporter level by TestCodeBlockExport, including the multi-block grouping case.
| def test_simple_code_block(self): | ||
| self.assertEqual(render_code_block("foo"), "```\nfoo\n```\n\n") | ||
|
|
||
| def test_content_not_escaped(self): |
There was a problem hiding this comment.
The test name test_content_not_escaped is ambiguous - it could mean either 'verifying content is not escaped in code blocks' (intended) or 'content was previously escaped but should not be now'. Consider renaming to test_content_preserved_in_code_block or test_special_chars_not_escaped_in_code_block for clarity.
There was a problem hiding this comment.
Keeping the name — within TestCodeBlockExport, test_content_not_escaped reads as intended alongside test_simple_code_block and test_fence_sized_to_content.
| def test_renders_verbatim(self): | ||
| self.assertEqual(DOM.render(inline([mark_safe("# "), "*x*"])), "# \\*x\\*") |
There was a problem hiding this comment.
The test uses DOM.render(inline([mark_safe("# "), "x"])) expecting "# *x*". This correctly validates that a heading marker rendered verbatim via mark_safe allows the subsequent user text to be escaped at line start. The expected string "# \x\" contains the literal #, space, backslash, *, x, backslash, *. This is the correct expected output — the heading prefix is preserved and the asterisks are escaped. No issue.
There was a problem hiding this comment.
Thanks for confirming — this test was strengthened in ae50109 following the earlier round's feedback.
| fence_char = fence[0] | ||
| return lambda props: DOM.create_element("code_block", {"fence": fence_char}) |
There was a problem hiding this comment.
Accessing fence[0] without validating that fence is non-empty will raise an IndexError if an empty string is passed. Although unlikely in practice (the default is "```"), adding a guard or asserting non-emptiness would improve robustness.
Suggestion:
| fence_char = fence[0] | |
| return lambda props: DOM.create_element("code_block", {"fence": fence_char}) | |
| if not fence: | |
| raise ValueError("fence must be a non-empty string") | |
| fence_char = fence[0] | |
| return lambda props: DOM.create_element("code_block", {"fence": fence_char}) |
There was a problem hiding this comment.
🤖 Not a defect: fence is typed as a literal set of the two supported fences in build_markdown_config, so type-checked callers cannot pass an empty string, and a runtime misuse fails loudly (IndexError) rather than silently. Per docs/SECURITY.md the exporter config is trusted developer code, not attacker input. Adding a guard + test to rephrase an already-loud failure is not worth it.
| if c.type == "mark_safe": | ||
| parts.append(c.attr["markup"] if c.attr else "") |
There was a problem hiding this comment.
In flatten_text, the mark_safe branch accesses c.attr['markup'] without using .get() for safe access. If c.attr is None, the guard catches it. But if c.attr is a dict missing the 'markup' key (possible with direct create_element calls), KeyError is raised. Use .get("markup", "") for consistency with the code_block case.
Suggestion:
| if c.type == "mark_safe": | |
| parts.append(c.attr["markup"] if c.attr else "") | |
| if c.type == "mark_safe": | |
| parts.append(c.attr.get("markup", "") if c.attr else "") |
There was a problem hiding this comment.
🤖 Deliberate design choice: a mark_safe element without a markup key is a component contract violation, and a loud KeyError surfaces it immediately at render time. The suggested .get("markup", "") would silently drop structural syntax from the output, turning a developer bug into invisible output corruption. Loud beats silent here.
| @given(escapable_text) | ||
| def test_escape_text_never_raises(self, text): | ||
| escape_text(text, at_line_start=True) | ||
| escape_text(text, at_line_start=False) |
There was a problem hiding this comment.
This test only verifies that escape_text doesn't raise an exception — it doesn't assert any property about the returned value. While this checks robustness, it's essentially a no-op assertion. Consider adding an assertion about the output, such as verifying the returned type is str, or rename to make the lack of output verification explicit.
There was a problem hiding this comment.
🤖 Not a defect — crash safety without output assertions is the established property-based test pattern in this project (see docs/contributing/test-strategy.md; the pre-existing test_markdown_engine_never_raises has the same shape). An isinstance(str) assertion could never fail, so it would add noise, not signal.
| @given(escapable_text) | ||
| def test_escape_text_never_raises(self, text): | ||
| escape_text(text, at_line_start=True) | ||
| escape_text(text, at_line_start=False) |
There was a problem hiding this comment.
This test only verifies that escape_text doesn't raise an exception — it doesn't assert any property about the returned value. While this checks robustness, it's essentially a no-op assertion. Consider adding an assertion about the output, such as verifying the returned type is str, or rename to make the lack of output verification explicit.
There was a problem hiding this comment.
🤖 Duplicate of the comment above (same finding posted twice) — see the reply there. Not a defect.
|
Triage of the automated review's summary finding ( |
| from draftjs_exporter.markdown.helpers import mark_safe | ||
| from draftjs_exporter.types import Block, Element, Props |
There was a problem hiding this comment.
The list_item function now wraps structural Markdown syntax (indent, prefix, suffix) with mark_safe() to ensure proper escaping of user content in list items. The block_prefix=True for indent and prefix correctly indicates that these markers can start nested blocks, while suffix (typically just \n) does not need this flag. This is consistent with how mark_safe is used in other modules like blocks.py and styles.py. The change is well-tested with existing tests like test_list_item_marker_uses_mark_safe and test_line_start_in_list_item.
| BLOCK_TYPES.BLOCKQUOTE: prefixed_block("> ", block_prefix=True), | ||
| BLOCK_TYPES.CODE: { | ||
| "element": make_code_element(fence), | ||
| "element": make_code_element(), |
There was a problem hiding this comment.
The fence parameter was dead code in make_code_element() - the function body never used it. Removing it from the signature is correct and improves clarity.
| def escape_link_destination(url: str) -> str: | ||
| """Escape a URL for use as an inline link destination inside ``](…)``. | ||
|
|
||
| Backslash-escapes backslashes and parentheses (which would otherwise | ||
| break out of the destination), and percent-encodes ASCII whitespace and | ||
| control characters, which inline destinations may not contain. URL | ||
| scheme validation is out of scope: it is the integrator's | ||
| responsibility (see ``docs/SECURITY.md``). | ||
|
|
||
| Parameters: | ||
| url: The URL to escape. | ||
|
|
||
| Returns: | ||
| The escaped destination. | ||
| """ | ||
| escaped = url.replace("\\", "\\\\").replace("(", "\\(").replace(")", "\\)") |
There was a problem hiding this comment.
The escape_link_destination function does not escape angle brackets (<, >). While ANYWHERE_ESCAPES in the same module includes < for mid-line text escaping, escape_link_destination only escapes backslashes and parentheses. URLs with angle brackets (e.g., https://example.com/<path>) are emitted verbatim into inline link destinations ](URL). In some markdown contexts or processors, unescaped angle brackets could have special meaning. Consider adding < and > to the character replacements in escape_link_destination.
Suggestion:
| def escape_link_destination(url: str) -> str: | |
| """Escape a URL for use as an inline link destination inside ``](…)``. | |
| Backslash-escapes backslashes and parentheses (which would otherwise | |
| break out of the destination), and percent-encodes ASCII whitespace and | |
| control characters, which inline destinations may not contain. URL | |
| scheme validation is out of scope: it is the integrator's | |
| responsibility (see ``docs/SECURITY.md``). | |
| Parameters: | |
| url: The URL to escape. | |
| Returns: | |
| The escaped destination. | |
| """ | |
| escaped = url.replace("\\", "\\\\").replace("(", "\\(").replace(")", "\\)") | |
| def escape_link_destination(url: str) -> str: | |
| """Escape a URL for use as an inline link destination inside ``](…)``. | |
| Backslash-escapes backslashes, parentheses, and angle brackets | |
| (which would otherwise break out of the destination or have special | |
| meaning), and percent-encodes ASCII whitespace and control characters, | |
| which inline destinations may not contain. URL scheme validation is out | |
| of scope: it is the integrator's responsibility (see ``docs/SECURITY.md``). | |
| Parameters: | |
| url: The URL to escape. | |
| Returns: | |
| The escaped destination. | |
| """ | |
| escaped = ( | |
| url.replace("\\", "\\\\") | |
| .replace("(", "\\(") | |
| .replace(")", "\\)") | |
| .replace("<", "\\<") | |
| .replace(">", "\\>") | |
| ) |
| def _escape_underscores(line: str) -> str: | ||
| """Escape underscore runs that could form emphasis. | ||
|
|
||
| A run is left unescaped only when both adjacent characters are | ||
| alphanumeric, where CommonMark's flanking rules guarantee it can | ||
| neither open nor close emphasis. | ||
|
|
||
| Parameters: | ||
| line: The line to process, with other escapes already applied. | ||
|
|
||
| Returns: | ||
| The line with escapable underscore runs backslash-escaped. | ||
| """ | ||
|
|
||
| def replace(match: re.Match[str]) -> str: | ||
| run = match.group(0) | ||
| before = line[match.start() - 1] if match.start() > 0 else "" | ||
| after = line[match.end()] if match.end() < len(line) else "" | ||
| if before.isalnum() and after.isalnum(): | ||
| return run | ||
| return "\\_" * len(run) | ||
|
|
||
| return UNDERSCORE_RUN.sub(replace, line) |
There was a problem hiding this comment.
The _escape_underscores function uses a closure that captures line from the outer scope. While this works correctly for the current call pattern (single pass through UNDERSCORE_RUN.sub(replace, line)), the stale closure makes the code fragile and harder to reason about. Consider passing the full line as a parameter to the callback or using a different approach that doesn't rely on closure capture.
Suggestion:
| def _escape_underscores(line: str) -> str: | |
| """Escape underscore runs that could form emphasis. | |
| A run is left unescaped only when both adjacent characters are | |
| alphanumeric, where CommonMark's flanking rules guarantee it can | |
| neither open nor close emphasis. | |
| Parameters: | |
| line: The line to process, with other escapes already applied. | |
| Returns: | |
| The line with escapable underscore runs backslash-escaped. | |
| """ | |
| def replace(match: re.Match[str]) -> str: | |
| run = match.group(0) | |
| before = line[match.start() - 1] if match.start() > 0 else "" | |
| after = line[match.end()] if match.end() < len(line) else "" | |
| if before.isalnum() and after.isalnum(): | |
| return run | |
| return "\\_" * len(run) | |
| return UNDERSCORE_RUN.sub(replace, line) | |
| def _escape_underscores(line: str) -> str: | |
| """Escape underscore runs that could form emphasis. | |
| A run is left unescaped only when both adjacent characters are | |
| alphanumeric, where CommonMark's flanking rules guarantee it can | |
| neither open nor close emphasis. | |
| Parameters: | |
| line: The line to process, with other escapes already applied. | |
| Returns: | |
| The line with escapable underscore runs backslash-escaped. | |
| """ | |
| def replace(match: re.Match[str], line_text: str) -> str: | |
| run = match.group(0) | |
| before = line_text[match.start() - 1] if match.start() > 0 else "" | |
| after = line_text[match.end()] if match.end() < len(line_text) else "" | |
| if before.isalnum() and after.isalnum(): | |
| return run | |
| return "\\_" * len(run) | |
| return UNDERSCORE_RUN.sub(lambda m: replace(m, line), line) |
| def escape_link_destination(url: str) -> str: | ||
| """Escape a URL for use as an inline link destination inside ``](…)``. | ||
|
|
||
| Backslash-escapes backslashes and parentheses (which would otherwise | ||
| break out of the destination), and percent-encodes ASCII whitespace and | ||
| control characters, which inline destinations may not contain. URL | ||
| scheme validation is out of scope: it is the integrator's | ||
| responsibility (see ``docs/SECURITY.md``). | ||
|
|
||
| Parameters: | ||
| url: The URL to escape. | ||
|
|
||
| Returns: | ||
| The escaped destination. | ||
| """ | ||
| escaped = url.replace("\\", "\\\\").replace("(", "\\(").replace(")", "\\)") | ||
| return "".join( | ||
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | ||
| for char in escaped | ||
| ) |
There was a problem hiding this comment.
Test coverage for escape_link_destination is missing key edge cases: (1) angle brackets in URLs (<, >) - currently not escaped, (2) null bytes and DEL character handling verification, (3) URLs containing both parentheses and whitespace/control characters to verify the mixed escaping/percent-encoding behavior is correct.
Suggestion:
| def escape_link_destination(url: str) -> str: | |
| """Escape a URL for use as an inline link destination inside ``](…)``. | |
| Backslash-escapes backslashes and parentheses (which would otherwise | |
| break out of the destination), and percent-encodes ASCII whitespace and | |
| control characters, which inline destinations may not contain. URL | |
| scheme validation is out of scope: it is the integrator's | |
| responsibility (see ``docs/SECURITY.md``). | |
| Parameters: | |
| url: The URL to escape. | |
| Returns: | |
| The escaped destination. | |
| """ | |
| escaped = url.replace("\\", "\\\\").replace("(", "\\(").replace(")", "\\)") | |
| return "".join( | |
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | |
| for char in escaped | |
| ) | |
| class TestEscapeLinkDestination(unittest.TestCase): | |
| def test_plain_url_unchanged(self): | |
| self.assertEqual( | |
| escape_link_destination("https://example.com/a?b=1&c=2"), | |
| "https://example.com/a?b=1&c=2", | |
| ) | |
| def test_parentheses_escaped(self): | |
| self.assertEqual( | |
| escape_link_destination("https://example.com/a_(b)"), | |
| "https://example.com/a_\\(b\\)", | |
| ) | |
| def test_backslash_escaped(self): | |
| self.assertEqual(escape_link_destination("a\\b"), "a\\\\b") | |
| def test_space_percent_encoded(self): | |
| self.assertEqual(escape_link_destination("a b"), "a%20b") | |
| def test_control_char_percent_encoded(self): | |
| self.assertEqual(escape_link_destination("a\nb"), "a%0Ab") | |
| def test_non_ascii_unchanged(self): | |
| self.assertEqual(escape_link_destination("/café"), "/café") | |
| def test_angle_brackets_escaped(self): | |
| # Angle brackets can have special meaning in markdown contexts. | |
| self.assertEqual( | |
| escape_link_destination("https://example.com/<path>"), | |
| "https://example.com/\\<path\\>", | |
| ) | |
| def test_null_byte_percent_encoded(self): | |
| self.assertEqual(escape_link_destination("a\x00b"), "a%00b") | |
| def test_del_percent_encoded(self): | |
| self.assertEqual(escape_link_destination("a\x7fb"), "a%7Fb") | |
| def test_mixed_parentheses_and_whitespace(self): | |
| # Verify that parentheses are backslash-escaped and whitespace | |
| # is percent-encoded, without interference. | |
| self.assertEqual( | |
| escape_link_destination("https://example.com/a (b) c"), | |
| "https://example.com/a%20\\(b\\)%20c", | |
| ) |
| def mark_safe(markup: str, block_prefix: bool = False) -> Element: | ||
| """Create an element holding structural Markdown syntax. |
There was a problem hiding this comment.
Type coercion between Python bool and DOM string attribute: mark_safe(block_prefix=True) produces {"block_prefix": "true"} (string), while the Markdown engine checks c.attr.get("block_prefix") == "true" (string comparison). This is consistent throughout the codebase (blocks.py, lists.py, markdown/init.py all use block_prefix=True), but the implicit bool-to-string conversion could be clarified. Consider either documenting this behavior or adding explicit conversion in mark_safe using str(block_prefix).lower() for consistency.
| @given(escapable_text) | ||
| def test_no_unescaped_angle_bracket(self, text): | ||
| escaped = escape_text(text, at_line_start=True) | ||
| self.assertIsNone(re.search(r"(?<!\\)<", escaped)) |
There was a problem hiding this comment.
The negative lookbehind (?<!\\)< checks only whether the character immediately before < is a single backslash. It cannot distinguish an escaped < (preceded by an odd number of backslashes) from an un-escaped < preceded by an even number. For example, input "a\\\\<b" (three backslashes + <) would after str.translate become "a\\\\\\\\<b" — the < IS preceded by a backslash (satisfying (?<!\\)) but is NOT escaped (even count before it). The test silently passes on broken escaping. Consider rewriting the assertion to iterate through matches and count preceding backslashes, or use a different property (e.g. idempotence: escape_text(escape_text(s)) == escape_text(s)).
| return block( | ||
| [ | ||
| mark_safe("!["), | ||
| props.get("alt", ""), |
There was a problem hiding this comment.
The alt text is passed as a plain string and will be escaped by escape_text(). While this prevents XSS-like issues with special characters, it may produce unexpected output when alt contains characters like *, _, [, ], `, etc. These characters should be rendered verbatim since they are valid in Markdown alt text. Consider wrapping alt with mark_safe() if the alt text is known to be pre-sanitized, or document that alt text will be escaped.
| return inline( | ||
| [ | ||
| mark_safe("["), | ||
| props["children"], |
There was a problem hiding this comment.
The children text is passed as a plain string and will be escaped by escape_text(). Link text content can legitimately contain special Markdown characters like *, _, backticks, etc. that should render correctly. However, escaping may be intentional as a safety measure. Consider documenting whether link children should be escaped or rendered verbatim, and whether this is a security-conscious default.
| return block( | ||
| [ | ||
| mark_safe("!["), | ||
| props.get("alt", ""), |
There was a problem hiding this comment.
The alt text is passed as a plain string and will be escaped by escape_text(). While escaping prevents XSS-like issues, it may produce unexpected output when alt contains Markdown-valid characters like *, _, [, ], `, etc. These characters are safe within image alt text and should render verbatim. Consider wrapping alt with mark_safe() if the alt text is known to be pre-sanitized, or document that alt text will be escaped.
| return inline( | ||
| [ | ||
| mark_safe("["), | ||
| props["children"], | ||
| mark_safe("]("), | ||
| link_destination(props["url"]), | ||
| mark_safe(")"), | ||
| ] | ||
| ) |
There was a problem hiding this comment.
The children text is passed as a plain string and will be escaped by escape_text(). Link text content can legitimately contain Markdown characters like *, _, backticks, etc. that should render correctly. However, escaping may be intentional as a safety measure. Consider documenting whether link children should be escaped or rendered verbatim.
| return "".join( | ||
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | ||
| for char in escaped | ||
| ) |
There was a problem hiding this comment.
The condition char > " " uses string comparison rather than ordinal comparison. While this works correctly for ASCII characters, it is fragile and potentially confusing. For example, ord('\x80') > ord(' ') would encode 0x80, but "\x80" > " " in Python relies on lexicographic ordering that happens to work the same way here. Using ord(char) > 32 would be clearer and more explicit.
Suggestion:
| return "".join( | |
| char if char > " " and char != "\x7f" else f"%{ord(char):02X}" | |
| for char in escaped | |
| ) | |
| return "".join( | |
| char if ord(char) > 32 else f"%{ord(char):02X}" | |
| for char in escaped | |
| ) |
| @given(escapable_text) | ||
| def test_no_unescaped_angle_bracket(self, text): | ||
| escaped = escape_text(text, at_line_start=True) | ||
| self.assertIsNone(re.search(r"(?<!\\)<", escaped)) |
There was a problem hiding this comment.
This test is testing the wrong escape mode. When at_line_start=False, angle brackets should still be escaped because < is in ANYWHERE_ESCAPES. The test should either: (1) check both modes, or (2) be renamed to clarify it only tests at_line_start=True. This is a medium-severity issue because the invariant should hold for both modes but isn't being verified.
| if c.type == "mark_safe": | ||
| parts.append(c.attr["markup"] if c.attr else "") |
There was a problem hiding this comment.
Potential KeyError if mark_safe element has attr dict without 'markup' key. While the mark_safe helper always includes 'markup', direct calls to create_element could pass incomplete attributes. Use .get() for defensive access.
Suggestion:
| if c.type == "mark_safe": | |
| parts.append(c.attr["markup"] if c.attr else "") | |
| if c.type == "mark_safe": | |
| parts.append(c.attr.get("markup", "") if c.attr else "") |
| else: | ||
| parts.append(DOMMarkdown.flatten_text(c.children)) |
There was a problem hiding this comment.
The flatten_text method uses recursion for nested elements. Deeply nested structures in complex Draft.js content could cause RecursionError. Consider using an iterative approach with a stack.
Suggestion:
| else: | |
| parts.append(DOMMarkdown.flatten_text(c.children)) | |
| # Consider iterative approach for deep nesting |
| @staticmethod | ||
| def flatten_text(children: list["str | Elt"]) -> str: |
There was a problem hiding this comment.
The flatten_text method is used internally by code_span and code_block but has no dedicated unit tests. Adding direct tests would improve maintainability.
Suggestion:
| @staticmethod | |
| def flatten_text(children: list["str | Elt"]) -> str: | |
| # Add unit tests for flatten_text directly |
| if ( | ||
| c.type == "mark_safe" | ||
| and c.attr | ||
| and c.attr.get("block_prefix") == "true" | ||
| ): | ||
| at_line_start = True |
There was a problem hiding this comment.
Missing integration tests for block_prefix behavior with nested blocks (e.g., heading inside blockquote, list item with heading-like text). The render_children logic is correct but untested at integration level.
Suggestion:
| if ( | |
| c.type == "mark_safe" | |
| and c.attr | |
| and c.attr.get("block_prefix") == "true" | |
| ): | |
| at_line_start = True | |
| # Add integration tests for nested block scenarios: | |
| # - Heading inside blockquote: "> # x" should not escape | |
| # - List item with heading-like text: "- # x" should escape |
Markdown escaping designDate: 2026-08-04 ContextThe Markdown exporter ( Per the threat model in
Goals
Non-goals
ArchitectureEscaping lives in the Markdown engine at render time. The invariantAfter this change, every tree rendered by
Over-escaping from a forgotten wrapper fails safe (cosmetic noise, not a vulnerability). The invariant is documented in New node kinds in the Markdown engine
Line-start detection
Every level of Escaping rulesNew module:
|
| Char | Reason |
|---|---|
\ |
escape character itself |
` |
code span / code fence |
* |
emphasis, thematic break |
_ |
emphasis, thematic break |
[ |
link/image open (also neutralizes ![ — no need to escape !) |
] |
link close, link-text breakout |
< |
inline HTML (XSS), autolinks |
& |
entity/character reference decoding |
Additionally escaped at line start (per the detection above, and after embedded \n):
| Char | Reason |
|---|---|
# |
ATX heading |
- |
bullet list, thematic break, setext underline |
+ |
bullet list |
> |
blockquote |
= |
setext underline |
| |
GFM-style tables |
~ |
tilde code fence (and GFM strikethrough) |
. / ) after leading digits (^(\d{1,9})([.)])) |
ordered list item — escape the delimiter, e.g. 1\. item |
escape_link_destination(url)
Applied to LINK URLs and IMAGE src inside ](…):
- Backslash-escape
\,(,)(prevents destination breakout). - Percent-encode ASCII whitespace and control characters (inline destinations may not contain them).
Scheme validation stays out of scope (integrator duty, already documented).
Code span delimiter sizing
Flatten the node's text content, then:
- delimiter =
`repeated (longest backtick run in content + 1), minimum 1. - if the content starts or ends with a backtick, pad with one space after the opening / before the closing delimiter (CommonMark rule).
Code block fence sizing
- fence = configured fence char (
`or~) repeated (longest run of that char in content + 1), minimum 3.
Flattening inside code contexts
Children of code_span / code_block nodes are flattened to their text content (nested elements contribute only their text, structure discarded) before delimiter sizing and verbatim emission.
Component changes (draftjs_exporter/markdown/)
All literal syntax strings move into mark_safe:
helpers.py:block()wraps its trailing"\n\n"inmark_safe.blocks.py:prefixed_blockwraps the prefix;>(blockquote) getsblock_prefix=True, headings do not. List items (list_item): indent, marker prefix (-,1.) and suffixes (\n,\n\n) wrapped; marker prefix and indent getblock_prefix=True.styles.py:inline_style(mark)wraps both marks; theCODEstyle maps to a new component emitting a"code_span"node instead of backtick marks.code.py:code_wrapper/code_element(and theirmake_*factories) are replaced by a single component emitting a"code_block"node carrying the configured fence char inattr.entities.py:link→[mark_safe("["), children, mark_safe("]("), link_destination(url), mark_safe(")")];imagesimilarly, withaltleft as a plain string (text-escaped,]-safe by the engine) andsrcthroughlink_destination;horizontal_rulewraps its marker inmark_safe.fallbacks.py: unchanged (children pass through).
escape_link_destination results are wrapped verbatim (a link_destination helper returns mark_safe(escape_link_destination(url))).
Security mapping (docs/SECURITY.md)
| Threat | Mitigation |
|---|---|
| Stored XSS via inline HTML in block text | < and & escaped everywhere |
| Entity-reference spoofing | & escaped |
| Structure injection (headings, lists, quotes, thematic breaks, fences) | line-start escaping |
| Attacker-crafted links in prose | [, ] escaped everywhere |
Link destination breakout via ) |
escape_link_destination |
| Code fence/delimiter breakout | engine-side delimiter sizing |
| ReDoS/DoS in escaping | linear-time translate/anchored regex; covered by property-based tests |
docs/SECURITY.md gains a short note that the Markdown engine escapes text and link destinations, and that scheme validation remains integrator duty.
Error handling
No new exceptions. Unknown node types keep the existing catch-all HTML rendering. Escaping never fails on malformed input (worst case: cosmetic noise) — matching the project's crash-safety property tests.
Testing
- Unit (
tests/markdown/test_escape.py, new): each metacharacter; backslash-first ordering; line-start vs. mid-line for#,-,>,=,~,+,1.; embedded\n;escape_link_destination(parens, backslash, spaces, control chars); code span sizing (backtick runs, leading/trailing backtick padding); fence sizing (``` and~~~). - Engine (
tests/engines/test_engines_markdown.py):mark_safepassthrough,block_prefixbehavior, plain-string escaping,code_span/code_blockunescaped content with sized delimiters,<sup>-style HTML elements unaffected (tags verbatim, children escaped). - Integration (
tests/test_output.py): heading-like text, list-like text, blockquote-like text,1.text,<script>in text,[x](javascript:…)in prose, link URL containing), image alt containing], code block containing fences, inline code containing backticks, soft break followed by#. - Snapshots (
tests/test_exports.json): update existing Markdown cases whose output changes; add cases for the above. - Property-based (
tests/test_properties.py,tests/strategies.py): metacharacter-heavy text strategies; assert crash safety and that output contains no unescaped line-start constructs derived from text. - Docs:
docs/markdown.md— remove the "No HTML escaping in text" limitation, document escaping behavior and the leading-whitespace limitation; migration guide entry for the breaking output change; CHANGELOG entry.
Breaking change
Any Markdown output containing metacharacters in text changes. Called out in the migration guide and changelog; acceptable because Markdown support is documented as experimental.
Markdown Escaping Implementation Plan
Goal: Add context-aware CommonMark escaping of user-controlled text and link destinations to the Markdown exporter, and make code span/block delimiters breakout-proof. Architecture: Escaping lives in the Markdown engine ( Tech Stack: Python 3.10+, pytest/unittest, Hypothesis, ruff, mypy. Commands via Global Constraints
Task 1:
|
1 participant
Adds context-aware CommonMark escaping of user-controlled text and link destinations to the experimental Markdown exporter, closing several content-injection vectors described in the project's threat model (
docs/SECURITY.md): ContentState is attacker-controlled, and previously any block text or entity data reached the Markdown output verbatim.What changes
strchild is always user text; structural syntax is wrapped inmark_safeelements by components. Anywhere-escaped:\,`,*,_,[,],<,&. Line-start-escaped:#,-,+,>,=,|,~, and ordered-list markers (1.), where line start means start of block content, after any line ending (\n,\r\n,\r), after any leading run of spaces, or after a list/blockquote marker.\,(,)are backslash-escaped, whitespace/control characters percent-encoded (prevents](…)breakout). URL scheme validation remains the integrator's responsibility, as documented inSECURITY.md.```gets a longer fence; inline code containing a backtick gets double backticks). This also fixes pre-existing output corruption for such content.How to verify
just test— 543 tests passing, including new unit, engine, integration (tests/markdown/test_escaping.py), and Hypothesis property tests.just test-coverage— 100% line and branch coverage.just lint— clean (ruff, mypy, ty).# not a headingnow exports as\# not a heading;<script>alert(1)</script>as\<script>alert(1)\</script>; a link entity URLhttps://example.com/a(b)as[click](https://example.com/a\(b\)).Notes for reviewers
docs/SECURITY.md; a dedicated adversarial review round verified escaping cannot be bypassed via line endings, leading spaces, entities spanning line starts, code delimiter content, or list/blockquote nesting.docs/markdown.mdgains an "Escaping" section documenting the rules and residual limitations (leading 4+-space/tab indentation can still render as an indented code block — a fidelity edge case, not an injection vector).just test-coverage)just lint)