Fix markdown-to-storage rendering on Confluence Cloud#75
Conversation
pchuri
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @eschoeller! This is a well-structured PR with clear explanations for each fix. Really appreciate you tackling multiple rendering issues together with proper test coverage.
Overall
The changes look solid — the isCloud() helper, code block entity decoding, trailing newline trim, and the removal of the global entity decode are all sensible fixes. The Cloud vs Server/DC link branching preserves backward compatibility nicely, and the test additions cover both paths.
One issue: entity decoding order
The current decoding chain has a potential double-decoding bug:
const decodedCode = code.replace(/\n$/, '')
.replace(/"/g, '"')
.replace(/&/g, '&') // &lt; → <
.replace(/</g, '<') // < → < (double-decoded!)
.replace(/>/g, '>');If the original source contains a literal < (i.e. the user wrote < in their code), markdown-it would encode the & to produce &lt;. With the current order, & is decoded first → <, then < is decoded again → <. This silently corrupts the content.
Moving & to the end fixes this:
const decodedCode = code.replace(/\n$/, '')
.replace(/"/g, '"')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/&/g, '&'); // decode & lastMinor note
The isCloud() heuristic (checking for .atlassian.net) is pragmatic and works for the vast majority of cases. Worth noting that custom-domain Cloud instances won't be detected, but that's an edge case we can address later with a config flag if needed.
Could you fix the decoding order? Everything else looks good to merge. 🙏
|
One more thing worth considering: CDATA injection defense. If user code contains a literal const decodedCode = code.replace(/\n$/, '')
.replace(/"/g, '"')
.replace(/</g, '<')
.replace(/>/g, '>')
.replace(/&/g, '&'); // & last to avoid double-decoding
const safeCode = decodedCode.replace(/]]>/g, ']]]]><![CDATA[>');
return `<ac:structured-macro ac:name="code"><ac:parameter ac:name="language">${language}</ac:parameter><ac:plain-text-body><![CDATA[${safeCode}]]></ac:plain-text-body></ac:structured-macro>`;This is a defensive measure — unlikely to hit in normal usage, but it prevents breakage when someone pastes XML/CDATA snippets into a code block. Not a blocker, but nice to have while we're touching this code. |
- Escape ]]> in code block content to prevent premature CDATA termination when user code contains XML/CDATA snippets - Move & decode last to avoid double-decoding entities - Add test for CDATA terminator escaping in code blocks Addresses review feedback from pchuri on PR pchuri#75.
|
Added in 1ce15ca — CDATA injection defense plus moved |
pchuri
left a comment
There was a problem hiding this comment.
Looks great — the decoding order fix and CDATA injection defense are exactly what I had in mind. Tests cover all the new paths well. Approving!
One thing: there are merge conflicts on this branch. Could you rebase on main and resolve them when you get a chance? Once that's sorted, this is good to merge.
- Use smart links (data-card-appearance="inline") for Cloud instances instead of ac:link/ri:url which Cloud no longer renders. Server/DC instances continue using the ac:link format. - Decode HTML entities (" & < >) inside code blocks before wrapping in CDATA, so code renders with literal characters instead of escaped entities. - Trim trailing newline that markdown-it appends to code block content, which caused an extra blank line in rendered Confluence code macros. - Remove global HTML entity decode (< > &) that was stripping angle-bracket placeholders (e.g. <tenant>) from inline text. Code blocks now handle their own entity decoding before CDATA insertion.
- Escape ]]> in code block content to prevent premature CDATA termination when user code contains XML/CDATA snippets - Move & decode last to avoid double-decoding entities - Add test for CDATA terminator escaping in code blocks Addresses review feedback from pchuri on PR pchuri#75.
1ce15ca to
5c132c7
Compare
|
Rebased on main (v1.27.3), no conflicts. All 145 tests passing. Ready to merge. |
|
CI is failing due to an ESLint Line 86 (existing on isCloud() {
return this.isScopedToken() || (this.domain && this.domain.trim().toLowerCase().endsWith('.atlassian.net'));
}Line 95 (added by this PR): isCloud() {
return this.domain && this.domain.trim().toLowerCase().endsWith('.atlassian.net');
}The existing one at line 86 already covers the |
The rebase onto main (v1.27.3) introduced a duplicate isCloud() at line 95. The upstream version at line 86 is preferred as it also handles scoped tokens via isScopedToken(). Removes the duplicate to fix the ESLint no-dupe-class-members error.
|
Good catch — removed the duplicate isCloud() in c168239. The upstream version at line 86 is the right one since it handles scoped tokens too. ESLint clean, all 145 tests passing. Thanks a lot for accepting this PR and I also really appreciate the work you've done on this cli tool it's been incredibly useful to me!! |
Summary
Fixes several issues with
markdownToStorage()/htmlToConfluenceStorage()when targeting Confluence Cloud instances:Links not rendering: Cloud no longer renders the
ac:link+ri:urlstorage format for external URLs. This PR adds anisCloud()helper (checks if domain ends with.atlassian.net) and uses smart links (<a href="..." data-card-appearance="inline">) for Cloud instances while preservingac:linkformat for Server/Data Center.HTML entities in code blocks:
markdown-itHTML-encodes characters inside<code>blocks ("→",&→&, etc.). These entities were being passed verbatim into CDATA sections, rendering as literal"in Confluence code macros. Now decoded before CDATA insertion.Trailing newline in code blocks:
markdown-itappends\nto code block content during HTML rendering, causing an extra blank line at the end of every Confluence code macro. Now trimmed.Angle-bracket placeholders stripped: A global
</>/&decode at the end ofhtmlToConfluenceStorage()was converting<placeholder>back to<placeholder>, which Confluence then stripped as an unknown HTML tag. Removed the global decode — code blocks handle their own entity decoding separately.Test plan
ac:link) to ensure backward compatibility