feat(jira): convert ADF rich text to markdown instead of plain text - #7543
Conversation
Design Review (Fable 5) — ✅ PASSDesign-level review of Design-Verdict: PASS Root-cause fix at the right layer: markdown into the same payload contract the other providers already use, with the new untrusted surfaces (hrefs, escaping-vs-redaction, expansion) each bounded. Suggestions
[DESIGN-REVIEWED] b231f25 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified: the shared scanner First-Principles-Verdict: PASS Every item traces to a named, mostly measured defect — lost URLs, parser-verified mark corruption, counted exfil leaks — and the two deferred halves are declared with their scope. What this change shipsIntent: a Jira Cloud issue opened in the panel renders with its real formatting instead of a flattened wall of text — a FIX.
Watch
[FIRST-PRINCIPLES-REVIEWED] b231f25 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe single candidate describes No findings. [OPUS-REVIEWED] b231f25 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
f0f4588 to
d6b9a19
Compare
|
Dispositions for round 1 (verdicts on GPT BLOCKING 1 -- nested lists bypass the depth limit: FIXED, and it was worse than a theoretical risk. Confirmed before touching the code. The cap lives in The fix is the dispatch you prescribed, and The test ships parametrized over all five recursing containers rather than pinned to the one that broke, so a container added later cannot quietly bypass the cap either. That generalization is what the Pattern harvest section now names as the rule candidate. GPT BLOCKING 2 -- code spans lose boundary spaces: FIXED. Correct: CommonMark strips one character from each end of a span whose content both begins and ends with a space or newline. First Principles subtraction -- shrink All three of your counts were right: undeclared in the description, zero tests, and it added a network side effect the fix does not need. An external First Principles watch -- the Server/DC v2 branch still feeds wiki markup unescaped: ACCEPTED AND DEFERRED. Agreed, and it is the one unescaped path left. Deferring rather than folding it in: wiki markup is a different grammar, so handling it honestly means a second converter (its own node vocabulary, its own escaping, its own tests), not a line in this diff. Escaping that text without converting it would also be a regression of its own -- Server descriptions would render with visible backslashes where markup used to at least partly work. Issue #2581 names the ADF path, and that is what this PR closes. Recorded locally so it gets re-judged on its own merits rather than riding along here. Self-found in the same span, declared for completeness: a marked empty text node emitted its bare delimiters ( Design Review PASS: noted, no action. |
d6b9a19 to
510e9a4
Compare
|
Round 2 disposition (verdict on GPT BLOCKING -- unsanitized code language enables markdown injection: FIXED, and the class was wider than the one instance. Real, reachable, and self-introduced by this diff -- the old walker emitted no fence at all, so this arrived with the fix. A fence's info string runs to the end of its line, so a Auditing the rest of my own emitters for the same shape found five more: So the fix is two chokepoints rather than one patch:
There are now no direct Five tests pin it: the newline-bearing The PR body's escaping section now describes the attribute path explicitly instead of implying text escaping covered everything, and the Pattern harvest promotes this from the weak review-prompt it was to a confirmed rule candidate: an externally-sourced string interpolated onto the same line as markdown punctuation without a single-line guarantee, with the durable form being one escaper per sink shape and no direct interpolation at call sites. 45 tests pass; flake8, isort and the black baseline gate are clean. |
510e9a4 to
64a728d
Compare
|
Round 3 disposition (verdicts on GPT BLOCKING -- markdown conversion bypasses credential redaction: FIXED, and I could reproduce the leak. The chain checks out structurally and empirically. Both call sites now redact the raw ADF tree before converting, which is what you prescribed: each text node is still contiguous while it is being matched, and escaping happens afterwards on text that no longer holds a secret. The test asserts the asymmetry in both directions rather than just the fixed path -- a GPT FINDING -- literal Correct, and it is the same class as the Backend Tests (3.10, 3): MAIN-OWNED, not this PR. Failing test is Evidence it is pre-existing: main's own CI run 33480742836 on head Design Review PASS and First Principles PASS on 47 tests pass; flake8, isort and the black baseline gate are clean. |
64a728d to
f8d0dcd
Compare
163ed70 to
d6d135d
Compare
|
Round 11 disposition (verdicts on GPT BLOCKING -- table flattening corrupts code-span whitespace: FIXED, and I had already reasoned my way to the right answer once and then not applied it here. Real, and ordinary content again: a Jira table cell containing the code What makes this one worth naming: in round 6 I hit the same question for HEADINGS and chose to collapse newlines only, specifically to protect a code span's spacing and the boundary-space padding added in round 2. I noted the cell had the same exposure and left it, judging the loss marginal. That judgment was wrong -- repeated spaces in code are exactly what a table of shell or regex snippets carries -- and it was also inconsistent with the sibling I had just written. Both now share one rule: First Principles CONCERNS (advisory) -- the fix is Cloud-only and the description implied otherwise: ACCEPTED, description sharpened. Fair, and the imprecision was mine. The body said "every other provider already puts real markdown in that same payload key", which is true of GitHub and GitLab but let the reader infer the field is uniformly markdown after this change. It is not: the The body now states that as the scope boundary rather than as a passing "unchanged" note, including why the two obvious shortcuts are both wrong -- escaping that path without converting it would show visible backslashes where wiki markup used to partly work, and converting it means a second grammar with its own vocabulary and tests. #2581 names the ADF path; the Server path is recorded locally for its own change. I also narrowed "every other provider" to "both other providers", which is what the code supports. 65 tests pass; flake8, isort and the black baseline gate are clean. Design Review PASS and Opus clean on |
d6d135d to
507adb9
Compare
|
Round 12 disposition (verdict on GPT BLOCKING -- nested blocks amplify bounded ADF into an unbounded response: mechanism FIXED, characterisation corrected. The mechanism is real and it is mine: Measured on this branch, same content in both shapes:
Two corrections to the finding. It is not unbounded: The fix removes the quadratic term rather than adding a projected-size check, which is what I would have had to bolt into a formatting helper otherwise. A chain of single-child quotes is now collapsed and marked once; each collapsed level still consumes traversal depth, so the 64-level cap applies exactly as before. Output is byte-identical (24,199,879 characters in both runs) -- only the redundant copying is gone, and nested content now converts in the same time as flat content. A test pins the behaviour the optimisation could break: three nested quotes must still emit three markers, so a collapsed level cannot go missing. Also in this push, from the previous round's numbers: nothing. This round is one change. 66 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS, First Principles PASS (upgraded after the scope-boundary wording), Opus clean -- all on |
|
Round 13 disposition (verdict on GPT BLOCKING -- prefix expansion bypasses the payload memory ceiling: FIXED. GPT was right and my round-12 rebuttal was measured on the wrong shape. Last round I said the growth ratio was "a constant bounded by depth times two characters per line" and put it at 3.61x, so the finding looked like a bounded stall rather than an OOM. That number came from a document built as one paragraph per line, which costs about 58 payload bytes per line. It is not the worst shape. The worst shape puts the newlines INSIDE a single text node, where each costs about three bytes of payload. Same 60 levels of nesting, so the same 120 characters added per line, against 3 bytes instead of 58 -- a 19x denser attack than the one I measured. Re-measured with
The ratio is 40x, not 3.61x, and it is linear, so the 8 MiB fetch cap extrapolates to roughly 325 MiB of output at about a 780 MiB peak. That is the "hundreds of MiB" the finding claimed, and the post-conversion payload gate cannot help because the allocation is what it was supposed to prevent. So my earlier characterisation was wrong on the number that mattered, and the correction is on the record here as well as in the description.
I also guarded the sibling the finding did not name. Two tests: the guard directly (the same text refused at a 120-character expansion, accepted at 2), and end to end through the quote path, where a 100,000-newline text node inside 60 quotes now raises instead of rendering. On this being round 13. The measurement failure is mine twice over -- round 12 I under-measured, and round 5's ragged-table finding was the same class of amplification, so I had already seen this shape once. What I take from it: for an amplification claim, the input shape has to be chosen adversarially for DENSITY, not just built to be large, and peak allocation has to be measured rather than inferred from output length. 68 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS, First Principles PASS, Opus clean on |
|
Round 14 disposition (verdict on All four lanes were clean on GPT FINDING (advisory) -- the block-lead escaper misses Real, and it is a gap in a rule this diff owns.
Why I pushed for an advisory rather than answering in thread. The usual reason not to is that a push re-rolls every lane on a converged PR for no correctness gain. Two things made it the cheaper call here: the PR was not green yet -- seventeen checks including all twelve Backend Tests shards were still in flight, so they had to run again regardless -- and the defect is self-introduced and one character wide. Declaring green while knowingly leaving a real gap in my own escaping rule would also have made the "every concern dispositioned" claim weaker than it reads. 69 tests pass; flake8, isort and the black baseline gate are clean. |
|
Round 15 disposition, addressed in GPT BLOCKING (on Checked against a CommonMark parser rather than taken on the description: Before taking the asterisk I checked it does not trade the bug for the round-6 adjacency problem, since strong already uses Opus BLOCKING (on Note the head: Opus reviewed The mechanism is real and mine, introduced with the round-6 mark merging. Measured rather than assumed, since the claim was "tens of seconds":
So it was superlinear and worth removing, but 1.48s at the reachable bound rather than tens of seconds -- the same overstatement pattern as the memory finding two rounds ago, where the direction was right and the magnitude was not. Each run's text now goes into a list and is joined once, which is linear, and the mark signature is computed once per node instead of twice. A test pins a 100k-node run merging to the right string. One process failure of mine in this round, recorded. I chained the amend and the black gate with 71 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS and First Principles PASS still stand. |
|
Round 16 disposition (verdicts on GPT BLOCKING -- nested unknown containers repeatedly scan the full text: FIXED. The numbers match the finding. Real, and it is the composition of two of my own earlier changes: round 8 routed the unrecognised-container fallthrough through Measured before and after:
So "over 25 seconds" was accurate. A nested container now reuses its ancestor's scan via a The risk in that optimisation is a credential deep inside nested containers no longer being caught, so a test pins exactly that -- a split credential three containers down is still redacted. GPT BLOCKING -- media alt text can bypass credential redaction: REBUTTED, with the output. Not reachable. A media node emits The The finding's premise -- that First Principles CONCERNS (advisory) -- the Server v2 branch keeps the misrender: ACCEPTED AND DEFERRED, unchanged from round 11. Same disposition, and I agree with the framing that a human should know the boundary: the description states it explicitly, naming both sibling sites and why neither shortcut works (escaping without converting shows visible backslashes where wiki markup used to partly work; converting means a second grammar with its own vocabulary and tests). It is recorded locally rather than as an issue. Nothing new to add, but not left silent. 73 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS and Opus clean on this head. Process note, since I reported the same mistake last round. The black gate failed on my first amend again, for the same reason: |
|
Round 17 disposition (verdicts on GPT BLOCKING -- link destinations bypass exfiltration redaction: FIXED, and confirmed with a payload. My first attempt at the fix was inadequate; the second is the one that works. Real, and worse than the one-line summary suggests. The chain, verified at each step rather than reasoned about:
Measured with one blob in a query,
And there is no backstop for this shape. This is new surface from this PR: the old plain-text walker emitted no href at all, so no provider destination reached the payload. The truncation lives in main's shared The correction worth recording: my first fix scanned the raw href and dropped the link if redaction changed it. That does not work -- it calls the same truncating scanner, so the attack URL came back unchanged and the gate passed it through. I caught this only because I checked the rendered output instead of trusting the fix, and the render still showed the full blob. The working version scans a form with the parentheses percent-encoded, which is the character the truncation turns on; the encoded form is used only to DECIDE and is never emitted.
Two tests: the attack renders as bare First Principles CONCERNS (advisory) -- the description overclaims what escaping closes: FIXED in the same push. Correct, and it was my wording, not the code. The body said escaping "closes the second half of the bug" without qualification, which is true only for ADF. It now says so explicitly and states that the Jira Server/DC v2 branches keep the misrender, alongside the existing scope note. The deferral itself is unchanged and still recorded locally. One local red, environment-owned, not folded in. 75 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. Design Review PASS and Opus clean on the previous head. |
|
Round 18 disposition (verdicts on GPT BLOCKING -- ADF layout containers concatenate block content: FIXED, and the class is wider than the two types reported. Real.
So GPT found 1 of 5. Two of the others are worse than concatenation: Markdown has no columns, so a layout flattens to its children in document order; the fix is that they flatten as BLOCKS. The new containers recurse through Design Review CONCERNS 1 -- the Agreed, and this is the more important of the two. I did not fix it here on purpose: Design Review CONCERNS 2 -- the two-layer redaction invariant is conventional, not enforceable: FIXED. Also fair, and the suggested structural test was the right shape. There are now exactly two chokepoints -- The test reads the node types and attribute names back OUT of the module source and tries every combination with a credential in every attribute, so a node type or attribute added later is covered without anyone remembering the test exists. I mutation-checked it rather than trusting it: replacing one 82 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. First Principles is PASS on the previous head after the wording fix. |
|
Round 19 disposition (verdicts on GPT BLOCKING -- overlapping adjacent marks emit literal delimiters: FIXED. Real, and the worst-looking output in this PR so far. Independent per-node wrapping emitted The middle node loses BOTH marks and six asterisks appear as visible text. Two-node cases were fine, so it takes a mark shared and then dropped across three nodes -- bold, bold-italic, italic, which is an ordinary thing to write in Jira. GPT's prescribed direction was right and I took it: a mark shared by adjacent nodes is now emitted ONCE around all of them, greedily taking the mark that spans the longest run. Worth recording why that alone is not sufficient: with
One case has no correct encoding: an italic that both abuts an asterisk run AND is followed by a word needs I introduced a quadratic in the first version of this fix and caught it before pushing. The emitter needs the character before each mark to choose a delimiter, and I passed the accumulated output to get it -- rebuilding the whole prefix at every position. Measured 0.87s / 3.42s / 13.85s for 25k / 50k / 100k nodes, a clean 4x per doubling. It only ever reads one character, so it now carries one character: 0.16s / 0.30s / 0.72s, and the output is byte-identical. I measured only because this PR has already had two superlinear findings; the reflex was worth having. Recursion in the new emitter is bounded by the number of factorable marks -- each level removes one, so at most two deep -- and needs no guard of its own. GPT FINDING (advisory) -- Correct, and it is the same class as the round-2 fence hole, which makes it worth more than its cosmetic impact. 86 tests pass, 46 in the wider Jira slice, 26 in the depth and redaction slice; flake8, isort and the black baseline gate are clean. |
|
Round 20 disposition (verdict on GPT BLOCKING -- code fences duplicate trailing newlines: FIXED, and it was two cases. Real, and it is a content-fidelity bug rather than a cosmetic one: the newline before the closing fence SEPARATES the body from it, so emitting one unconditionally added a newline to the block's CONTENT. Measured on the round trip, where the expected content is what CommonMark reads back between the fences:
GPT named the trailing-newline case. Checking the class rather than the instance turned up the empty body as the same root cause: it produced a block containing one blank line where the source had no content at all. Both come from the same unconditional separator and both are fixed by it being conditional. 87 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. A process observation for the maintainer, offered with numbers rather than as a complaint. This is the twentieth consecutive round in which the GPT lane has returned at least one blocking finding, and I have fixed or rebutted every one on its merits -- no override has been used on this PR. The findings have NOT been noise: by my count 22 of 25 were real and self-introduced, and the three I rebutted I rebutted with a test or a rendered output rather than an assertion. What I cannot tell from inside the loop is whether the count is converging. The last four rounds moved from a security bypass (a redaction gate defeated by a Two things would help, and both are yours to decide rather than mine:
I am not requesting an override and will not post one without your sign-off. |
|
Round 21 disposition (verdict on GPT BLOCKING -- explicit zero-based lists are renumbered: FIXED, plus a worse sibling in the same expression. Real. Auditing the same expression turned up a second defect that GPT did not report and that is worse in effect, because it does not merely renumber -- it stops the list being a list:
Ten digits is not a list marker, so an out-of-range
90 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. On the convergence note I left last round: this finding is the same shape as the previous one -- a real fidelity defect at an edge, found in code the PR introduced, fixed in one expression with its class audited. I am still not requesting an override. The two questions from that comment remain open and are yours: whether the remaining edge work belongs in follow-ups like #7611, and whether a round cap applies here. |
|
Round 22 disposition (verdict on GPT BLOCKING -- apostrophes truncate URL security scanning: FIXED. This one is my own class-audit failure, and it is worth naming as such. Real.
I have spent several rounds correctly widening GPT's findings from the instance to the class, and here I did the opposite in my own code -- worse, I wrote in #7611 that " The scan form now percent-encodes every character that terminates the match, via one table rather than chained Both directions are tested: all ten characters are pinned as sealed, and a benign Confluence URL containing an apostrophe keeps its link, since over-blocking would silently strip ordinary Jira links. 92 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. I have added the measurements to #7611, since they make the shared-scanner case stronger than when I filed it: the bypass is not one character, it is the whole terminating class, and it applies to every caller of that scanner. |
|
Round 23 disposition (verdicts on GPT BLOCKING -- wider body rows lose cells: FIXED. This one is a consequence of my own round-5 change. Real, and it is silent data loss. GFM fixes a table's width at the HEADER row: a row with FEWER cells gets empty ones inserted, but a row with MORE has the excess dropped. Verified against a GFM parser -- under a two-column header, In round 5 I removed the ragged-row padding because it was quadratic, and justified it in the docstring with "GFM already inserts empty cells for a row shorter than the header and ignores a longer row's excess". The first half is true; on the second I wrote "ignores" and reasoned about it as a layout question, when it means the text is gone. The right fix in round 5 was to remove the quadratic, not the width normalisation. The header and separator are now widened to the widest row and body rows are left alone. That keeps both properties: no cell is dropped, and the quadratic does not come back, because only two lines grow rather than every row. A test pins the correctness case and a second pins the shape that caused the original quadratic -- one 400-cell row among 400 single-cell rows now emits under 2500 pipes, against the 40,602 that padding every row produced. Design Review CONCERNS 1 -- the escape set mirrors the frontend renderer's plugins with nothing to catch drift: FIXED, as suggested. This was the right thing to flag: the escape set is DERIVED from that stack -- There is now a test that reads Design Review CONCERNS 2 -- the shadow terminator set will drift when #7611 lands: ACCEPTED, recorded in the code. Correct. The constant now says explicitly that it is a shadow of the scanner's set, exists only while the scanner carries the bug, and should be retired rather than maintained when #7611 lands -- two copies of one set drift, and the copy that matters is the scanner's. Design Review SUGGESTION -- extract the ~830-line converter into its own module: ACCEPTED AND DEFERRED, deliberately not in this PR. I agree with the reasoning and would support it as a follow-up. Declining here is about risk, not disagreement: a pure move of 830 lines out of a 4k-line file would rewrite essentially the whole diff of a PR that has taken 23 review rounds to converge, discard the line-level review context every lane has built up, and make the real changes in this round invisible inside the move. A no-behaviour-change extraction reviews cleanly on its own and badly on top of this. It is also strictly easier after the fact, since the module boundary is now exactly the set of functions this PR added. 95 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. First Principles PASS, Opus clean. |
|
Round 24 disposition (Design Review on Design Review CONCERNS 1 -- no test spans the backend/frontend boundary: DONE, the stronger version you asked for. One correction first, because it changes what was outstanding rather than whether: Your stronger ask is a different and better thing, and I have built it: "a fixture (or shared contract test) that renders converter output through the real plugin stack". Pinning the plugin list detects that the stack changed. Rendering through the stack verifies the escape set actually holds. The second subsumes the first as evidence, so both are now in place.
Six cases: literal emphasis, literal HTML including an Verified rather than assumed, and this needed a detour worth recording: this worktree has no Design Review CONCERNS 2 -- a human should confirm #7611 stays live: NOTED, and agreed that it is a human's to hold. #7611 is open with the measurements from round 22 added, and the shadow set in the code says to retire it when that lands rather than maintain two copies. I cannot guarantee an issue stays alive, which is exactly why it is filed rather than left in a docstring. Design Review SUGGESTION -- extract the converter into its own module: still ACCEPTED AND DEFERRED. Unchanged from last round, and your new framing ("would make the #7611 retire step a one-file change") is a fair argument for it. The reason to keep it out of THIS PR is unchanged: a pure move of ~850 lines would rewrite the whole diff of a PR at 24 review rounds and bury this round's real changes inside the move. It reviews cleanly as its own no-behaviour-change follow-up. 96 backend tests pass, 46 in the wider Jira slice, 7 frontend; flake8, isort and the black baseline gate are clean. |
UX Review (Fable 5) — ✅ PASSUX-level review of All the evidence is in: this is a backend converter change whose output is user-visible in the Issues panel, plus tests and a shared fixture; no screenshots and no new frontend surface. The verdict below is my review. UX-Verdict: PASS Jira issues now render with the same rich formatting as GitHub/GitLab sources; escaping and link-not-image choices are sound and disclosed. Suggestions
[UX-REVIEWED] b231f25 |
|
Round 25 disposition (verdict on GPT BLOCKING -- attribute labels bypass suspicious-URL redaction: FIXED, and it was four sites, not one. Real, and mine. Rounds 17 and 22 normalised the URL before scanning at the link-destination chokepoint only. The truncation is a property of the SCANNER, so it applies wherever provider text is scanned -- and
The two extra classes matter because they are not attribute labels: the inline-run gate scans This is the third round in which the same underlying bug has been reported at a new call site, and the reason is worth stating plainly: I kept fixing the site GPT named instead of the property. The property is "every place this converter scans provider text must scan a form the URL regex cannot truncate", and it is now expressed once, in one function, with the call sites routed through it rather than each holding a copy of the trick. One deliberate behaviour note: when nothing is found the ORIGINAL text is emitted, so the percent-encoding is never visible in the ordinary case -- pinned by a test that an everyday URL containing parentheses, an apostrophe and a 98 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean.
The failure is
I could not rerun the job ( |
|
Round 26 disposition (verdict on GPT BLOCKING -- whole-text encoding corrupts benign Jira prose: FIXED. Reproduced exactly as described. Round 25 encoded the scan form across the WHOLE text, whitespace included, which joined a URL's query to the word after it. A comment citing a URL and a commit SHA: scanned as ONE address whose query carried the SHA, so the entropy heuristic fired and the paragraph was emitted as: Every word after the URL gone, on entirely benign content. That is worse than the leak it was guarding, and I had noted the whitespace risk while writing round 25 before choosing the conservative path anyway -- the conservative choice was the destructive one. The fix is not simply "don't encode whitespace", and the test caught me getting that wrong. My first attempt encoded terminators only inside URL spans, on the reasoning that a space ends a URL. That is true in prose and false for a link DESTINATION: So the rule is now stated properly: scan the form that will actually be EMITTED.
The prose half is verified against the real renderer rather than argued from the spec: for Both directions are pinned: all ten terminators stay sealed on the destination path, and the URL-plus-SHA paragraph now comes back byte-identical with no 99 tests pass, 46 in the wider Jira slice; flake8, isort and the black baseline gate are clean. On the Windows shard: it went green on the previous head, consistent with the round-25 analysis that it is main-owned flake in |
|
Round 27 disposition (verdict on GPT BLOCKING -- code-block URLs bypass complete redaction: REBUTTED with rendered evidence, and the reasoning is GPT's own from last round. The mechanism is accurately described: a code body goes through Rendered through the project's own
So a URL inside code is text, not an address: nothing auto-fetches it and there is nothing to click. This is precisely the reasoning GPT's OWN previous finding established -- whitespace ends a URL in prose, so encoding it to make a blob vanish was destruction without a gain. The same test applies here, and the answer comes out the same way. Adding the scan to code bodies would have a real cost in the same direction as that finding: an API example containing a long opaque token reads exactly like an exfiltration query, so legitimate code samples in Jira issues would be replaced by a redaction marker. Trading working code samples for a threat that cannot be reached is the wrong side of that trade. Credentials in code blocks are NOT unprotected, which is the part worth being explicit about: the payload-level pass is token-shaped, so it catches Two tests record the boundary rather than leaving it in a comment: a code body containing such a URL is emitted verbatim, and the case that DOES linkify -- a bare URL with a 101 tests pass, 46 in the wider Jira slice, 7 frontend; flake8, isort and the black baseline gate are clean. I am not requesting an override: this is a rebuttal on the merits with the renderer's output as evidence, and if the lane re-raises it I will escalate rather than argue it twice. |
Jira Cloud v3 returns a description and each comment body as ADF, and the walker that read them collected text leaf nodes only, so every heading, list, link URL, code fence and table was gone before the Issues panel saw the field. That panel renders the field through MarkdownRenderer, and every other provider already puts real markdown in the same payload key (a GitHub issue body, a GitLab description), so Jira was the one source whose rich text arrived flattened. Replace _adf_to_plain_text with _adf_to_markdown, which maps each ADF node type to its markdown form and escapes literal text so a description cannot smuggle markup into the panel. The Server/DC path (a str of wiki markup, not ADF) is untouched. Closes #2581
|
Round 28 disposition (verdict on GPT BLOCKING -- media alt bypasses run-level credential redaction: FIXED. It looks like a re-raise of round 16 and it is not; my own later change is what made it true. I rebutted this in round 16 with rendered output: a media node emits Round 17 then added a path where a URL failing the destination scan DROPS the link and emits the label alone -- no brackets. GPT's wording names exactly that ("rendered link delimiters disappear"), and it is right. My rebuttal quietly stopped holding two rounds after I made it, and nothing in the test suite noticed, because the test I added in round 16 pinned the bracketed case only. It took a specific payload to isolate, which is worth recording so the disposition is not just an assertion:
Output before: The fix is the one GPT prescribed. Round 16's test is deleted rather than left passing beside its own counterexample, and replaced by one that asserts both the bracketed and the dropped-link cases redact, plus one confirming an ordinary alt still renders as a link so the stricter gate does not cost the normal case. What I am taking from this. Three of the last four findings have been in code the earlier rounds introduced, and this one specifically invalidated a rebuttal I had already given. A rebuttal is only as durable as the code path it describes; when I later changed that path I did not go back and re-check what it had been holding up. The tests I write for a rebuttal need to pin the property, not the one shape I measured. 102 tests pass, 46 in the wider Jira slice, 7 frontend; flake8, isort and the black baseline gate are clean. |
bolichen97
left a comment
There was a problem hiding this comment.
Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.
Problem / Motivation
Jira Cloud v3 returns an issue description and every comment body as ADF
(Atlassian Document Format), a JSON document tree. The walker that read it,
_adf_to_plain_text(), collected text leaf nodes only, so all structure wasgone before the field left the backend: headings became unstyled text,
bold/italic/code spans were stripped, links kept their anchor text and lost
the URL entirely, code blocks lost their fences, list items lost their
bullets, and tables lost every cell boundary.
Why it matters
IssuePanel.tsxrenderssource.descriptionand eachcomment.bodythroughMarkdownRenderer, and both other providers already put real markdown in thatsame payload key -- a GitHub issue
body, a GitLabdescription. Jira Cloud wasthe one source whose rich text arrived pre-flattened, so a Jira issue opened in
the panel was a wall of undifferentiated text while the same panel rendered a
GitHub issue properly.
One loss is worse than cosmetic. A link's URL was unrecoverable from the
panel, because the walker kept only the anchor text, so an issue whose
reproduction steps are a list of links arrived as a list of bare phrases.
Feeding plain text into a markdown renderer also had it both ways: text that
merely looked like markup was still re-parsed as markup, so a literal
**ina Jira description turned bold and a literal
<b>was dropped by the HTMLsanitizer -- while the real structure beside it was already gone.
What changed (motivation -> approach -> change)
Symptom: a Jira description renders as one undifferentiated block. Root
cause: the traversal was lossy by construction. It returned
node["text"]for a leaf and appended a newline after a known block type, and that was the
entire use it made of a node's type; marks were never read at all. No amount
of post-processing recovers formatting from that output, so the traversal
itself has to emit per node type.
_adf_to_markdown()replaces it and dispatches on node type:_adf_block_to_markdown): heading level to#repeated, code blockto a fence carrying the
languageattribute, bullet and ordered lists to-/N.markers with a hanging indent so a nested list stays nested, anADF task list to a GFM checklist, table to a GFM table, blockquote and panel
to
>, rule to---, expand to its bold title plus its content. A chain ofsingle-child quotes is collapsed and marked in one pass rather than re-marking
at every level, which is quadratic in the nesting depth: a 6.7MB document
nested 60 deep measured 2.57s of blocking work against 0.47s for the same
content unnested, and 0.51s after the collapse, with byte-identical output. A
heading
collapses its internal newlines, since only its first line carries the
#anda hardBreak would otherwise leave a second line whose leading
-renders as alist.
_adf_inline_to_markdown): marks to**,*/_, backticks and~~.A mark shared by adjacent nodes is emitted ONCE around all of them, because
wrapping each node on its own produced
**a*****b****c*for strong, strong+em,em -- which a CommonMark parser reads as strong(a), a LITERAL
***b***, thenem(c): the delimiters became visible text and the middle node lost both marks.
Italic takes
*normally, since underscore will not open emphasis intraword anda_b_closes the italic, and_where a*would touch another asterisk run.Where neither spelling is safe at both ends the mark is dropped and the text
kept, which loses an italic instead of showing a delimiter as content;
a link mark and an
inlineCardto[text](url); a mention to@name; ahard break to a two-space line break, which is what the panel needs since it
renders with CommonMark soft-break collapse. An external
medianode (onecarrying a public
url) becomes a plain link as well, not an image: thatkeeps the address recoverable without the panel auto-fetching a
provider-controlled URL when someone opens the issue. A
medianode thatonly references an attachment by id has no fetchable address and contributes
nothing, exactly as before.
handling the top level uses, so an ADF type Atlassian adds later still
contributes its text -- and gets the same mark merging and credential check its
children would get at the top level.
_adf_to_markdown()is also the ONE guarded entry to the recursion: it holdsthe 64-level depth cap, and every descent -- a container's children, a list
item's nested list, a table cell -- re-enters through it rather than calling a
renderer directly. Without that, a list nested a few hundred deep re-entered
its own renderer past the cap and exhausted the stack, turning one hostile or
machine-generated Jira description into an HTTP 500 on issue fetch.
Literal text is escaped on the way out (
_md_escape_inlinefor the inlinesyntax openers,
_md_escape_block_leadsfor a line-leading-,+,#,1.or a setextunderline of
=).That is what closes the second half of the bug on the ADF path -- and only
there: the old walker emitted raw provider text into a markdown renderer, so the
panel could not tell a description's real formatting from text that happened to
look like formatting. The Jira Server/DC v2 branches keep that misrender, so for
those issues this half of the bug stays open; the scope note below says why.
Block containers are classified as blocks.
layoutSection/layoutColumn,bodiedExtensionanddecisionListgroup other blocks, and reaching the inlinefallthrough concatenated them: a two-column layout of a paragraph and a heading
rendered as
firstsecond, losing both the separator and the##.blockCardand
embedCardcarry their URL in an attribute with no content, so the samefallthrough rendered them as the empty string and the URL was lost outright --
the same unrecoverable loss this change exists to fix. Markdown has no columns,
so a layout flattens to its children in document order.
A link destination is scanned before it is emitted, and a URL that fails is
dropped to plain text rather than linked. The old walker emitted no href at all,
so a provider-controlled destination reaching the panel is new here, and the
payload-level scan cannot be relied on to clean one up:
_URL_RE's path groupexcludes whitespace,
),",'and>, so ANY of those in the path truncatesthe match and puts the entire query outside every check that follows. Measured: a high-entropy query blob is redacted
without the paren and not redacted with it, and no credential-pattern pass covers
a bare blob. Every site that emits provider text runs its redaction through one primitive,
_md_redact_untruncated-- the gap is per-CALL-SITE, not per-node-type, so whenonly the link destination was covered an expand title, a mention label, an inline
card and a media URL each still leaked. The rule is to scan the form that will
actually be EMITTED, which is why whitespace differs per site: a link DESTINATION
is one address and the angle-bracket form emits a space as
%20, so the scanencodes it too, while in PROSE a space genuinely ends the URL (the renderer's
autolinker stops there) and encoding it treated a URL followed by a commit SHA as
one address, firing the entropy heuristic and replacing the whole paragraph. The
scan form is only a scan form:
used only to decide, never emitted -- and a URL with parentheses that passes, such
as a wiki or Confluence page, keeps its link in the angle-bracket form. The
truncation itself is in the shared scanner and affects its other callers, so it
is filed as #7611 rather than recorded only in a docstring here.
A table's header is widened to the widest row. GFM fixes the width at the header
and DROPS a longer row's excess -- the text is gone, not wrapped -- so a narrow
first row silently lost the later rows' cells. Only the header and separator grow,
which is linear in the width; padding every row is what made this quadratic
before.
An ordered list keeps its own start number, including an explicit
order: 0,which ADF allows and CommonMark honours as
<ol start="0">. The start is boundedso the LAST item's marker still fits in nine digits: ten digits is not a list
marker at all, so an out-of-range order would render the whole list as paragraphs
carrying visible numbers, and a start of 999999999 overflows on its second item.
The run gate reads a media node's ALT, not its URL. When a URL fails the
destination scan the link is dropped and the label is emitted with no brackets, so
the alt is what can join a neighbour's text into one credential -- measured as a
recoverable 32-character token before this, with the backslash from escaping
ghp_then defeating the payload pass. Reading the alt errs toward MOREcontiguity than the output has when the brackets do survive, which is the safe way
to be wrong. An inline card keeps the URL, since its label IS the redacted URL.
A code body is deliberately NOT URL-scanned. Verified against the real renderer:
a fenced block and an inline code span both render with zero anchors and zero
images, so a URL in code is text rather than a fetchable address -- the same
reason whitespace ends a URL in prose. Running the entropy heuristic there would
replace legitimate code samples (an API example with a long opaque token reads
exactly like an exfiltration query) for no reachable gain, and credentials in code
are still caught by the token-shaped payload pass.
A code block's body keeps its own trailing newlines. The newline before the
closing fence separates the body from it, so emitting one unconditionally changed
the CONTENT: a source ending in one newline came back with two, and an empty body
became a block holding a blank line.
A
codemark is exclusive -- a code span is literal by definition, so nothingis escaped inside one, the fence widens past any backticks in the content, and
content that would lose a boundary space to CommonMark's own strip rule is
padded so it survives.
!is in the escape set for its own reason: thisconverter emits a real
[for a link, an inline card and a media node, so aliteral
!landing immediately before one would splice into image syntax andthe panel would auto-fetch the URL -- the beacon the media-as-link form exists
to avoid.
$is there because the same renderer runs remark-math, so a literal$$x$$would render as KaTeX rather than as the characters someone typed. Bothwere checked against the project's own parser rather than assumed.
Node ATTRIBUTES take a stricter path than text does, because they are labels
rather than prose and every one of them is provider-controlled. A newline
inside an attribute would end the construct the attribute sits in and let the
remainder become document structure, so
_adf_attr_labelcollapses whitespacebefore escaping (a mention's name, an expand's title, a media alt text, an
inline card's URL label). A code block's
languageis stricter still: afence's info string runs to the end of its line, so only a single highlighter
token is admitted (
_MD_CODE_LANGUAGE_RE-- letters, digits and thepunctuation real names carry, as in
c++orshell_session) and anything elsedrops to a bare fence, costing syntax highlighting and nothing else.
Escaping also forces a REDACTION contract, because
_redact_provider_datarunsover the finished payload and matches contiguous secrets (
ghp_...) thatmarkdown punctuation would break apart -- something the old unescaped, seamless
plain-text walk could not do. Redaction therefore happens exactly where this
converter inserts characters, and in both cases inside its own depth-capped
traversal:
_adf_inline_sequencechecks each inline run ONCE, against the plain-textrendition a seamless walk would produce -- every node's own text in order with
no markup between any of it, which is exactly the string the payload pass used
to see. When it fires, the run is emitted as that redacted string: it loses its
formatting, but no node's text is lost with it. Checking the whole run rather
than some span of it is deliberate: any narrower boundary has to answer "which
nodes contribute text seamlessly", and that answer kept turning out wider than
the last one -- a bold sibling, an unrecognised container, then a mention or
emoji label, each contributing text with no delimiter of its own. The run has
no such boundary to get wrong. A nested container reuses its ancestor's scan
rather than repeating it, since
_adf_plain_textrecurses and so the outerscan already covered every descendant: rescanning per level is depth-times-text
work, measured at 7.0s for 1MiB under 60 unrecognised containers and 27.8s for
4MiB, against 0.12s and 0.50s after.
_adf_attr_labelredacts, collapses whitespace, then escapes, and owns everyattribute label. That covers a credential in a free-form attribute (a media
alt text, an expand title), which is not part of any inline run's text.
The same span handling fixes a display bug that has nothing to do with
redaction: adjacent text nodes carrying identical marks are merged before those
marks are applied. Wrapping each separately emits
**a****b**, which CommonMarkrenders as a bold
a****b-- the delimiters become visible content -- and a codemark is worse, since two adjacent code-marked nodes collapse into one span
holding literal backticks. Verified against a CommonMark parser, not assumed.
Each run's text is joined once rather than rebuilt per node: only traversal
DEPTH is capped, so a provider can put 300k adjacent text nodes inside the 8MiB
fetch cap, which measured 1.48s of blocking work against 0.23s after the change.
Everything else the converter emits verbatim -- a link destination, a code
block's body -- stays contiguous and is caught by the payload pass as before.
Deliberately NOT a pre-pass over the raw ADF tree:
_redact_provider_datarecurses without a depth cap, so redacting a provider-controlled document before
converting it raises
RecursionErrora few hundred levels down (measured: fineat 400, raising at 800 against the default limit of 1000) and turns a valid
issue fetch into a 500. Redacting inside the converter is bounded by the same 64
levels as everything else.
Per-line expansion is bounded by projection. Marking a quote and indenting a
list item both add characters to EVERY line, and a provider controls the line
count as well as the nesting depth that sets the per-line cost: newlines embedded
in a single text node cost about three bytes of payload each, while sixty levels
of nesting adds a hundred and twenty characters to each of them. The payload gate
runs only after conversion, so
_md_guard_line_expansionprojects the expandedsize first and refuses the document instead of allocating it -- measured before
the guard, a 2.3MiB payload rendered 93MiB of markdown with a 224MiB peak, and
the 8MiB fetch cap extrapolated to roughly 780MiB. The check lives inside both
expanders rather than at their call sites, so a new caller cannot forget it, and
it refuses the way an oversized response is already refused.
Unchanged, and this is the scope boundary of the fix: the
isinstance(raw_desc, str)branch carries Jira Server/DC v2, which returns wikimarkup rather than ADF, and it still passes that string to the renderer
untouched. So the "literal text re-parsed as markup" half is fixed for Cloud v3
only -- a literal
**in a Server description still turns bold there, and itswiki markup still misrenders. Escaping that path without converting it would be
a regression of its own (visible backslashes where markup used to partly work),
and converting it means a second grammar with its own node vocabulary and tests.
Issue #2581 names the ADF path; the Server path is recorded for its own change.
Both fetch call sites keep their
.strip()._adf_to_plain_textis removed rather than left beside the new function: itsonly two callers are the two converted here, so keeping it would leave an
unreachable helper and a test class pinning output nothing consumes.
Tests
test/test_source_providers.py:TestAdfToPlainTextbecomesTestAdfToMarkdown, 102 tests. Beyond the four cases carried over from the oldclass, they lock in:
marks,
inlineCard, fenced code with a language, bullet and ordered lists, anordered list's explicit
orderstart, nested-list indentation, task list,blockquote, panel, rule, GFM table, mention, and external
mediaas a link(plus an id-only
mediacontributing nothing).DIFFERENT marks do not, a hardBreak between two equally marked nodes keeps them
apart, both hold one level down inside an unrecognised container, an italic
between two plain neighbours keeps its emphasis, and a 100k-node run merges to
the right string.
collapse cannot lose a level; and a document whose projected expansion
exceeds the payload ceiling is refused rather than rendered, checked both
through the quote path and against the guard directly.
narrow rows stays linear in cell count rather than padding to rows x width.
**/<b>/_survive as text, a literal©is not decoded to a copyright sign, a literal$$x$$is not renderedas math, a line-leading
-/1.stays text, a===or---line cannotpromote the line above it into a heading, a pipe inside a table cell is
escaped -- including one inside a code span, which is emitted literally and
would otherwise split the cell -- a cell folds to one line by collapsing
newlines only, so a code span's repeated spaces survive, a literal
!before a media node cannotsplice the pair into an auto-fetching image, and a heading with a hardBreak
stays on one line.
$forremark-math,
!for the image rehypeRaw admits,~for GFM strikethrough), sothat coupling is pinned from both sides rather than described in a comment.
test/fixtures/adf_markdown_safety.jsonis shared: the backend test asserts theconverter turns each
adfinto exactly thatmarkdown, andwebsite/src/test/MarkdownRenderer.adfSafety.test.tsxrenders the same markdownthrough the REAL plugin stack and asserts no
<strong>, no<b>, no<img>, noKaTeX, and no recoverable credential. A second backend test pins the plugin list
itself, so adding a plugin goes red with a message to re-derive the escapes.
split across a plain and a bold sibling, split either side of an unknown inline
container, split inside such a container, split deep inside nested containers
(which is what the reused scan could have broken), split across a label
boundary (an emoji's text), and sitting in a free-form attribute. Plus one
recording why a media alt is NOT such a shape: the node emits
[alt](url), soits label is always bracketed and cannot continue a credential from the text
before it. Plus one asserting the
fallback keeps every node's text, so a mention and a card in a redacted
paragraph do not disappear.
languagecannot close its ownfence and inject markdown, a real token like
c++survives, a multi-tokenvalue drops to a bare fence, and an expand title and a mention name each stay
on one line when the attribute carries newlines.
spaces padded so CommonMark's strip rule cannot eat them, whitespace-only and
one-sided-space content deliberately NOT padded, and a marked empty text node
emitting nothing rather than bare
****.bulletList, orderedList, taskList, table, and an unrecognised inline
container): content nested 350 deep is dropped and nothing raises, while
content nested 3 deep survives. This is the test that pins the
single-guarded-entry invariant -- it fails with
RecursionErroron the threelist containers if a renderer is re-entered directly.
parentheses switching to the angle-bracket destination form.
test_realistic_description_round_trips_to_markdownpins one whole document-- heading, code mark, strong mark, a list containing a link, fenced code,
table, rule, mention -- against its exact expected markdown.
Mutation-verified, each reverted after: making
_md_escape_inlinereturn itsinput fails exactly the two escaping tests; replacing the hanging-indent
padding with
""fails exactly the nested-list test; making_md_code_languageaccept any value fails the fence-injection and multi-tokentests; dropping the whitespace collapse from
_adf_attr_labelfails the expandand mention tests. The depth test was written first and observed to fail with
RecursionErroronbulletList,orderedListandtaskListbefore theguarded-entry fix, and to pass after.
Targeted runs only (the full suite was not run locally; that is what CI is
for):
pytest test/test_source_providers.py -k AdfToMarkdown -q -n 4-> 102 passedpytest test/test_source_providers.py -k "jira or Jira" -q -n 4-> 46 passedflake8andisortclean on both files;scripts/check_black_formatting.pypasses;mypy src/kiro_crew/reportsnothing for this file.
Manual verification
N/A -- reaching this path needs a live Jira Cloud v3 instance and credentials.
The converter is a pure function of the ADF payload, and the whole-document
test above asserts the exact string the panel would be handed, so the unit
coverage is the same evidence a manual pass would produce.
Related Issues
Closes #2581
Pattern harvest
Rule candidate: semgrep
Pattern: a recursive walker whose depth cap lives in ONE entry function, with a
sibling in the same family calling an inner renderer directly and re-entering
the cycle past the cap. Here
_adf_to_markdownheld the guard while thelist-item helper called
_adf_block_to_markdownstraight, so three of the fivenesting containers exhausted the stack while the two that happened to route
through the entry were fine. The greppable shape is a mutually recursive
function group where the guard predicate appears in a strict subset of its
members; a checker can flag a call that closes a cycle without passing through
a guarded member. The test form generalizes too, and is what this PR ships:
parametrize the depth test over EVERY container that can nest, so a container
added later cannot quietly bypass the cap.
Second pattern, confirmed rather than hypothetical: a provider-controlled
string interpolated into a LINE-ORIENTED markdown construct -- a fence's info
string, a heading, an emphasis run -- without collapsing newlines first. The
construct ends at the newline, so the remainder of the attribute becomes
document structure. This diff had it in six places (a code block's
languageplus five attribute labels) and closes all six through two chokepoints,
_adf_attr_labeland_md_code_language. The greppable shape is an f-stringthat places an externally-sourced value on the same line as markdown
punctuation without a single-line guarantee; the durable fix is having exactly
one escaper per sink shape and no direct interpolation at the call sites, which
is what a checker should assert.
Third pattern, the deepest one this change surfaced: a transform that INSERTS
characters into text which a downstream security scanner must still match. Both
credential findings here are that shape -- escaping put a backslash inside
ghp_, and marks put**between a secret's halves, in each case defeating aredactor that ran afterwards and had matched fine before. The rule generalizes
past markdown to any encoder, escaper or formatter placed upstream of a
pattern-matching gate: when you add one, the gate either has to move upstream of
it or be told what the transform can break. A checker cannot see that coupling,
but a reviewer can be told to look for it whenever a diff adds an escaping step
to a value that already flows through a scanner.
Checklist