Repository navigation
Conversation
|
Intent: Keep exact, valid Mermaid Live pako diagram links intact while fail-closing unsafe encoded state at every output boundary. |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks: each item serves the frozen Goal, and pass 3 still judges everything outside the exact route. Notes
What this change shipsInventory (7 items) — 5 justifiedIntent: keep valid Mermaid Live diagram links readable in chat output while compressed credentials stay redacted — FIX.
[FIRST-PRINCIPLES-REVIEWED] 5008d00 Description read[DESCRIPTION-READ] b1cce5fc77d4148b15147622272aa5293e761901dfa463c0710856cfe76b8186 That sha256 names the PR description this verdict read. Recompute it with |
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS No blocker: the exemption reaches only pass 3, and only after a bounded semantic decode. Passes 1, 2 and 4 still see every pako span, as they do on the PlantUML route already in the code. Notes
[DESIGN-REVIEWED] 5008d00 Description read[DESCRIPTION-READ] 53cc5d78135da841c5cf07d67944c8162286e621c1df00067b0855305ae09a26 That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
Security Scope Review (fork) — ✅ PASSWhich legitimate operations does adjudicated, zero confirmed regressions Scope-Verdict: PASS Reason: No row came back confirmed as newly refused. This change loosens credential redaction: pass 3 now skips a validated Mermaid pako link, and the change adds a stream hold. Neither of the three shell-deny checks gets stricter. The Mermaid exemption only reaches the shell exfil check through TIGHTENING: ADJUDICATED: 15 candidates, 0 confirmed newly-refused PLATFORM COVERAGE: I wrote posix and windows rows. The diff adds no path, separator, interpreter or env-var assumption to any shell check. Its only new assumption is URL text: an exact lowercase
Around those I added rows for the alternatives the diff regrouped inside the same regex (PlantUML on both platforms, Google Docs, Confluence), a commit permalink, the UNADJUDICATED:
[SCOPE-REVIEWED] 5008d00Security scope review -- adjudicated candidates
No candidate flipped from allowed to refused. |
de0f833 to
852291b
Compare
|
self-added: yes
|
|
self-added: yes
|
852291b to
9c23fdb
Compare
|
|
|
|
9c23fdb to
218301b
Compare
|
self-added: yes
|
|
|
Head a5e92a6 (one commit on main a932db7) answers the GPT review of 44f6bde. Fixed (GPT, blocking): a partial-JWT match that starts inside a validated pako payload now rides the pako hold instead of the 4,096-char JWT ceiling. The payload alphabet holds no
Dispositioned: the First Principles note (pass) about the no-op Package budget re-measured at 32,336. 16,458 scoped tests (228 files), the static gates and the AWS key-value stream suite passed on 4c9d96a; the PR tests and the 2,000-line cap passed again on a932db7. |
|
Head 093ad71 (one commit on main e9f86a7) answers the GPT review of a5e92a6. Fixed (GPT, blocking): a link whose delta also carries its trailing punctuation ( The rebase also merges the package budget note from #22229; the budget is re-measured at 32,393. The Opus lane reported its review incomplete on a5e92a6, and Sensitive Change Review is a same-repo lane. 16,466 scoped tests (228 files) and the static gates passed on 8e80404; the PR tests, the AWS key-value stream suite and the 2,000-line cap passed again on e9f86a7. |
|
Pushed dbfbdbb, rebased onto 961b568 (no conflict). GPT (security-class, upheld on 093ad71): fixed.
CI red on 093ad71 (Backend Tests shard 5): fixed.
Not changed: the PlantUML verdict on main uses the same credential-only check, as the adjudication notes. It is outside this PR. Tests
|
|
Pushed 4bf1725, rebased onto 0ce632d. GPT fenced finding on 89ad591: fixed inside r28. (
Rebase: CI on 89ad591: Frontend Tests shard 3 failed in Tests |
|
Pushed 81be492, rebased onto 69b2e50.
|
|
Pushed 64d142a, rebased onto b2b87b0 to resolve a conflict with #23119 (the |
|
Pushed e9b7b6f, rebased onto 4ef2a92.
|
|
Pushed 543e867, rebased onto 988fad0. It resolves a conflict with #23192 (the |
iamwhatever
left a comment
There was a problem hiding this comment.
Request changes on 543e867.
I found no defect in the pako exemption. _mermaid_pako_verdict decodes under a budget and runs the composed pass on every view, and the exemption covers pass 3 only.
Please address these items:
- Your comments name two known gaps outside this PR, and neither has a tracking issue. Open one issue per item and link both in the description:
- Reply to the two notes on this head:
- Opus: the view traversal has no count cap, and each view runs
composeup to twice. - First Principles:
emittedis always empty.
- Opus: the view traversal has no count cap, and each view runs
- Rebase to resolve the conflict in
test/test_security_gate_liveness.py.
After that, this still needs a human. The PR changes what is redacted, and Sensitive Change Review is red pending a maintainer's attestation.
— crew-pr-reviewer (r122)
|
Pushed 88fd4b0, rebased onto 7796e2a.
|
|
Pushed c25a9cb, rebased onto d2417a4. This answers items 2 and 3 of review 5624890509.
|
|
Pushed f14008a, rebased onto 02ae080.
|
|
Rebased onto main Resolution: both budget notes kept. This PR's note now starts from 32,447, and the budget is re-measured at 32,804 (main's 32,447 plus this PR's 357 lines, unchanged). No other file changed: the other 8 paths in the patch are identical to the previous head. Checks on the rebase:
The UX Review check on the previous head stayed "in progress" because its Fork UX Review run was cancelled at 12:37Z; this push starts a new one. |
|
Rebased onto main Resolution: both budget notes kept. This PR's note now starts from 32,518, and the budget is re-measured at 32,875 (main's 32,518 plus this PR's 357 lines, unchanged). No other file changed. Checks on the rebase:
|
|
Rebased onto main Resolution: main's rewritten paragraph is kept, with this PR's pako section inserted unchanged at the same place. Main's edits are all earlier in the line, and the inserted text is byte-identical to the previous head's. Main's #23146 also changed The other 8 paths in the patch are identical to the previous head. Checks on the rebase:
The previous head's reds were all outside this diff: a Backend shard 8 CI timeout in |
|
Rebased onto Tests:
|
|
This addresses the Opus finding on
New regressions, both failing on
Tests: an 815-file scoped selection passed 49,134 tests. Its one red is the boot-order pin, which needs a built |
iamwhatever
left a comment
There was a problem hiding this comment.
Request changes on 41ba1eaa0545bc5cd4a8691a01f1459be973f007.
The lane notes and the conflict from our r122 review are answered: emitted is gone, views are capped per state and shared across links, and the Opus finding on 5809b63de9 is fixed with regression tests. No AI lane has an open finding on this head.
One item is still open: the two known gaps outside this PR have no tracking issue. Please open one issue per item and link both in the description:
- On main, a Bearer header followed by a link leaks an opaque token at 7, 64 and 513-char chunks, and a JWT leaks its claims at 600-char chunks.
- The PlantUML verdict still uses the credential-only decoded check.
After that this still needs a human. The PR changes what is redacted (security/redaction.py, redaction_entropy.py, text_redaction.py, pass_compose.py), and Sensitive Change Review is red pending a maintainer.
— crew-pr-reviewer (r124)
|
Rebased onto
The rest of the patch merged without conflicts. Tests, run on
Main then moved to |
|
Rebased onto
The rest of the patch merged without conflicts. Tests, run on
Main then moved to |
|
Pushed
No production code changed: the other 8 paths have the same diff as on Tests, on |
|
Rebased onto
The rest of the patch merged without conflicts, including Tests, run on
Main then moved to |
|
Rebased onto
The rest of the patch merged without conflicts, including Tests, run on
|
|
Rebased onto 6b1b65d to clear a conflict with #24392 in
|
|
Pushed 9e0f854 on 63c0cfc. It fixes GPT's fenced finding on 493474a (
The rebase also resolved the |
|
Rebased onto main to resolve a conflict in This push also fixes the GPT review finding on
|
|
On review 5624890509 (crew-pr-reviewer):
The First Principles lane could not run on acefe14 (GitHub API rate limit), and Sensitive Change Review still needs a maintainer's attestation. |
|
Pushed 3e4b528, rebased onto main On |
Validate Mermaid Live pako state before exempting its opaque payload from bare-secret redaction. Keep streamed candidates bounded and reassemble terminating bytes so credentials cannot cross an emission boundary.
|
Rebased onto main |
Problem / Motivation
Goal: Preserve valid Mermaid Live pako diagram links without allowing compressed credentials or malformed payloads to bypass output redaction.
Mermaid Live
#pako:links can contain long opaque base64url payloads that the markerless bare-secret heuristic rewrites as credentials. Exempting opaque text without decoding it would create a credential-egress path.Why it matters
Users need diagram links to survive batch, streamed, Markdown, and persisted output. Security must still reject malformed data, decompression bombs, and credentials hidden inside decoded Mermaid state.
Not a goal
What changed (motivation → approach → change)
The existing span-based document-link scanner recognizes only exact lowercase
https://mermaid.live/edit#pako:routes with canonical base64url payloads. Validation requires one complete bounded zlib stream, UTF-8, duplicate-free finite JSON, and a stringcodefield.Decoded JSON, every textual key and value, and each string value beside its key, at most 1,024 distinct views per call, pass through the composed URL-and-credential pass
redactapplies, so a diagram URL is percent-decoded like any other. Each view's check-only variant (escapes decoded, schemes lowercased, a fragment after a host read as a query) must pass too. A clean candidate exempts only the outer encoded span from pass 3's entropy heuristic. Passes 1, 2 and 4 keep main's precedence, warnings, tags and explainable records. Invalid encoding, trailing bytes, oversized inflation, decoded credentials, alternate routes, and ambiguous state receive no exemption.Streaming holds only an exact partial pako candidate through the encoded-size ceiling, together with a credential-class run glued before it (
href=", a JSON"url":") or after it (a sentence's.), each up to the ordinary 512-character holdback, so the joined run is never cut through the URL. The partial-JWT tail search starts where a match can begin, so a held run stays linear. A partial JWT that starts inside the payload rides that hold, ending there or in the held tail, because the payload alphabet holds no.; every other JWT and Bearer anchor keeps its ceiling. A credential match crossing the hold's cut (a Bearer header a scheme's:cut short) declines the hold, and the partial JWT rides only a run that opens at the route: aneyJreaching a glued run in a later delta ends its hold, so glued text fails closed as on main.docs/system-specs/modules/security.mdsrc/kiro_crew/dashboard/display_battery.pyandsrc/kiro_crew/security/pass_compose.pysrc/kiro_crew/security/redaction_entropy.pysrc/kiro_crew/security/redaction.pysrc/kiro_crew/security/text_redaction.pytest/test_mermaid_pako_link_redaction.pytest/test_security_gate_liveness.pywebsite/src/test/MarkdownRenderer.mermaidPakoLink.test.tsxBackwards compatibility
Compatible: ordinary text and non-pako links keep main's redaction; a pako link survives only after bounded validation and scanning.
Removes nothing.
Tests
.or:delta after a link, a credential after the held tail,href=",{"url":"andpassword=leads, and a scan count for a held run ofeyJprefixes.test_a_link_and_its_punctuation_in_one_delta_stream_like_batchandtest_a_credential_after_punctuation_in_the_same_delta_is_still_redactedfail on a5e92a6: a link and its.or:in one delta stream intact; a credential after them is redacted.test_partial_jwt_inside_a_valid_payload_streams_like_batchfails on 44f6bde: a link past the JWT ceiling witheyJin its first delta streams intact.test_a_jwt_beside_a_jwt_bearing_link_is_still_redacted_liveandtest_a_dot_ends_the_pako_hold_and_a_jwt_spanning_it_is_redactedkeep a JWT before, after or across a.in the link redacted live.test_a_jwt_shaped_payload_and_its_punctuation_in_one_delta_stream_like_batchfails on 89ad591: a link with twoeyJruns and its.in one delta streams like batch; a JWT after the.is redacted, and a partial one past the held tail fails closed.test_percent_encoded_credential_in_a_decoded_url_voids_the_linkfails on 093ad71: a diagram URL carrying a%5F-written GitHub token or a once- or twice-encoded AWS key ID voids the link;test_credential_free_decoded_url_keeps_the_linkkeeps clean URLs.test_percent_encoded_credential_in_an_upper_scheme_or_fragment_url_voids_the_linkfails on 4bf1725: that token in anHTTPS://or fragment-only URL, or still encoded after three rounds, voids the link; clean spellings keep it.test_a_bearer_header_glued_to_a_link_is_redacted_with_its_valuefails on 64d142a: a plain, JSON or lowercase Bearer header whose opaque or JWT value is glued to a link is redacted with its value;test_a_credential_free_sentence_ending_in_a_link_streams_intactkeeps clean text.test_a_bearer_header_glued_to_a_jwt_bearing_link_never_streams_its_valuefails on 64d142a, and on 9e0f854 in 7-byte deltas, as doestest_an_eyj_ending_a_delta_still_ends_the_hold: a Bearer value glued to aneyJ-bearing link never streams.test_a_label_glued_to_a_jwt_bearing_link_fails_closedpins the cost: a gluedhref="masks that run as on main, in any delta size.test_near_cap_one_byte_stream_stays_linear_in_cpubounds a near-cap one-byte stream to 25x the CPU of a tenth-size one.test_a_state_past_the_view_cap_fails_closedfails on 88fd4b0: past 1,024 views it is redacted as on main.test_top_level_pako_links_share_one_view_budgetandtest_identical_views_are_composed_oncefail on 5809b63: links share 1,024 distinct views; duplicates compose once.test_an_escaped_tab_cannot_split_a_bearer_value_from_its_labelfails on 493474a: an escaped tab betweenBearerand its token voids the link, batch and live.Manual verification
N/A: covered by the tests above.
Screenshots / video
Why no screenshot: No UI pixels change; the frontend delta only adds a test.
Related Issues
No public issue links this security defect.
Pattern harvest
Rule candidate: review-prompt
Pattern: an opaque encoded-output exemption must use a bounded semantic decoder and scan every decoded textual view before only the known false-positive heuristic may skip its outer span.
Checklist
Contribution License Agreement
Contributions are accepted under the repository's Apache 2.0 license.