fix(admin): draw the control-byte class at interpreted against composing (#402) - #419
Merged
Merged
Conversation
…ing (#402) The class stopped at what a terminal EXECUTES: C0, DEL and C1, with U+009B in it because a CSI introducer needs no ESC in front. It excluded bidi and zero-width code points on the ground that a terminal "does not INTERPRET" them, and that was the wrong reading of its own rule. A terminal does interpret a bidi override. It reorders everything after it, so `repo/safe` plus U+202E plus `gnp.txt` is drawn as a name ending in `.png`. The job id, the target and the branch are all attacker- or model-writable, and a run record is what an operator reads before deciding what to do about it. The other half is invisibility rather than reordering. `deploy-prod` and `deploy` plus U+200B plus `-prod` draw as eleven columns each and are not the same trigger, so a picker that selects by the string acts on the row the operator did not mean. The dialog gate's own ambiguity note, from #404, is about exactly this and relies on `#N` prefixes to stay safe. IN: C0, DEL, C1, the bidi controls and isolates, U+200B, the word-joiner range, U+FEFF, U+00AD, U+180E, the interlinear annotation marks, and U+2028 / U+2029. DELIBERATELY OUT: a code point that COMPOSES the character beside it. U+200D joins an emoji sequence into one glyph, U+200C is orthography in Persian and the Indic scripts, and a variation selector chooses a character's form and carries a column with it under #401. Substituting those changes a CHARACTER where substituting the rest reveals a CONTROL, and a gate that cannot tell them apart corrupts the text it was added to protect. Pinned from that side too: a class that swept up every zero-width code point would split a family emoji into three. ONE CLASS AND ONE OPERATION, not a second named operation, which is the shape the issue left open. The worker escapes for the same question, and that is not an inconsistency to reconcile: `endpointShown`'s gate is an allowlist of printable ASCII, correct for a DNS name or a socket path, while this panel renders a CJK repository name as a matter of course. The same allowlist here would escape the content #401 had just taught it to measure. The width half of this issue was already answered by #401 rather than pending. Its table says these code points are counted as a column and so break the width math; they measure zero now, which is what the renderer draws. Only the reading was left, which is why the class moves and the width table does not. Stated rather than implied: substituting makes a difference VISIBLE, it does not say which of two look-alike strings was intended, because the panel cannot know. Ten mutations checked, every one red, in BOTH directions: each group removed from the class one at a time, and the class widened to swallow the joiner, the non-joiner and the variation selectors. Full suite in the CI posture: 4219 tests, 0 fail, 1 skipped. Same under the +399 day clock shift. All four guards pass and a run under a fresh TMPDIR leaves nothing behind. DES-ADMIN-VIA-PI-EXTENSION amended. OQ-035 amended with it, because `scrubReason` shares the class, so this widens that belt again: nothing observable moves, since a failedReason is a worker throw's message decoded as UTF-8 and this project produces none carrying a bidi control, but a contract that moves without a row is the gap that rule exists to close. The worker's validator is UNCHANGED, checked, for the reason DES-ONE-SHOT-DISARM-IN-THE-FILE already records: a refusal at that writer leaves a one-shot armed and costs a second paid run. No version moved. Signed-off-by: Rob Boerman <robboerman@live.nl>
…es (#402) A review pass refused the first version of this, and the reason is the one this file has now been wrong about three times: the class was a hand-written LIST of the shapes someone thought of. It covered 98 code points and left 167. Every one of them reproduced the issue's own selection hazard verbatim through the real panes: the Hangul and halfwidth FILLERS, the TAG block (a whole hidden ASCII message at zero columns), the Arabic and Egyptian format controls, the musical controls, and every blank that is not U+0020. `deploy-prod` against `deploy` plus U+3164 plus `-prod` is eleven columns either way. The regex also had no `u` flag, so 116 of those could not be expressed in it at all. The sharpest part: this project's own `env-file.mjs` already names U+3164 as a deception character. One surface had decided the question and the other missed it. So the class is DERIVED now: every format character, line separator, paragraph separator and blank-that-is-not-a-space, plus C0, DEL, C1 and the four fillers, which are `Lo` and reachable by no property. The test states the rule independently and sweeps every code point there is, requiring the two to agree. A list is what the #382 carve-out was, and the test file already says that was wrong twice. THE TAG BLOCK IS BOTH THINGS, so it is the one settled by SEQUENCE rather than by code point: after U+1F3F4 a tag builds a subdivision flag, and anywhere else it is invisible text. It carries the one lookbehind, measured both ways. A REGRESSION THIS CHANGE INTRODUCED, found by the same pass and fixed here: the LIVE_TAIL search could no longer find a line it was drawing. The pane substitutes the class, the search box deletes it, and the haystack was raw, so for a log line carrying an invisible byte there was NO query that matched. Typing what was on screen missed the raw byte; pasting the original missed because the box had dropped it. Both readings are compared now, so both ways of asking find it. Six false statements corrected, three of them mine from the first version: - "every one of these code points measures zero columns now" appeared in the code, the spec entry and the PR. U+2028 and U+2029 measure ONE, on both sides. The conclusion survives, the reason did not. - SUBSTITUTION IS NO LONGER COLUMN-PRESERVING, which used to be free: every member of the old class measured one, so replacing it with a space changed no geometry, and most members of this one measure zero. Nothing breaks because every measurement site scrubs BEFORE it measures, but that is an ordering those sites keep rather than an identity the counts gave. OQ-035's Position bullet said "one column each" and is corrected. - Four live sentences in `specs/` and `dashboard.ts` still said the class was C0 + DEL + C1, including one in the file that owns `scrubReason`. WHAT IT COSTS, stated rather than waved at: a correctly ISOLATED right-to-left name now displays worse, because the isolate that was making it read correctly beside an LTR path is substituted. The panel cannot tell that isolate from an attacker's, since they are the same code point doing the same thing, so this is the price of closing the deception rather than an oversight. The gate also closes the EXPLICIT deception only: the bidi algorithm reorders neutrals beside a strong RTL character with no control present, which nothing here can reach without refusing Hebrew and Arabic outright. THREE CLASSES FOR ONE QUESTION, now stated as deliberate rather than left as drift. `triggers.mjs`'s validator is C0 + DEL and decides whether an operator's file is acceptable. `env-file.mjs`'s `QUOTED_CONTROL` is this rule almost exactly and its docblock says so, and it also holds U+200C, U+200D and the variation selectors, because a `.env` value has no legitimate emoji sequence in it. This panel renders a CJK repository name and a container's log output, so it keeps what composes. Fourteen mutations checked, zero survivors, in both directions: each property branch removed, the fillers, the tag block and its flag exemption, the `u` flag, the class widened to swallow the joiner and the non-joiner, and both readings of the tail search. Full suite in the CI posture: 4221 tests, 0 fail, 1 skipped. Same under the +399 day clock shift. All four guards pass and a run under a fresh TMPDIR leaves nothing behind. No version moved. Signed-off-by: Rob Boerman <robboerman@live.nl>
…g is a flag (#402) A second review pass found the third instance of this round's own pattern, in the construct the previous repair added. THE FLAG EXEMPTION WAS NOT A VALIDITY CHECK. It read "a tag preceded by the base and any number of tags", which exempts every tag FOREVER AFTER a flag, cancel tag included. One legitimate flag emoji anywhere in a model-writable field restored the whole hidden-message hazard, and the pass recovered `rm -rf /` verbatim from a drawn line. It was also a variable-length lookbehind, which rescans backwards at every position: 200 KB of tags in one `.log` line took 2.3 seconds, and `readLogTail` bounds the number of lines but not their length, so that is a reachable freeze of the operator's terminal from the content this file calls the most untrusted it renders. A whole valid sequence is matched now, base plus one to six tag letters plus the cancel tag, with no lookbehind. The same 20,000-tag run went from 296 ms to 6 ms, and the mutation reverting to the unbounded shape is pinned. THE CLASS ASKS THE RENDERER NOW, rather than four Unicode categories. The previous version derived it from `\p{Cf}|\p{Zl}|\p{Zp}|\p{Zs}`, which is a different rule wearing this one's clothes: it still left U+FFF0-U+FFF8, which this file's own width table already calls noncharacters the renderer draws as nothing, and U+2800, which draws a blank cell. Membership asks `columnsOf`, which issue #401 bolts to the pinned renderer over every code point there is, so the question it answers is the one the class actually asks. THAT CHANGE FOUND A DEFECT OF ITS OWN, which is the part worth reading. Asking "does it draw nothing" put the INDIC VOWEL SIGNS into the class, because #401 had made a spacing mark measure zero to agree with the renderer, and the composing test was `\p{Mn}|\p{Me}`. Devanagari `\u0915\u093f` was being substituted to `\u0915 ` -- not revealing a control, deleting a vowel. Every mark composes, so the test is `\p{M}`. U+3000 IS DELIBERATELY OUT, which the previous version got wrong by including it: an ideographic space draws TWO columns and is ordinary Japanese text, so substituting it rewrites content and narrows the line. The collision it can still make is a stated residual. THE SEARCH NO LONGER RE-MERGES WHAT THE CLASS SEPARATES. Comparing the drawn reading OR the deleted one meant a search for `deploy-prod` matched both it and `deploy` plus a zero-width space plus `-prod`: the pane tells those rows apart and the search put them back together. The search box substitutes like the pane now, so one reading serves both, and typing what is on screen and pasting the original produce the same query. `makeLineInput` substitutes rather than deletes, which is safe because the cursor sentinels are added at render. Claims corrected, several of them from the previous commit: - "every one of these measures zero columns now" survived in the spec entry and the revision row. Of the 168 members outside the tag block, 85 draw as nothing, 16 draw a blank, 65 are what a terminal executes and 2 are line breaks. The conclusion held, the reason did not. - "`QUOTED_CONTROL` is this rule almost exactly" implied an 18-code-point difference. It holds 18 this class does not and this class holds about 160 it does not. It reaches for the same idea by LISTING, which is where this class was two revisions ago, and the docblock says that now. - "left 167" was not reproducible: the old class was 98 and this is 264. Twelve mutations checked, zero survivors, including the unbounded flag shape, both composing directions, each arm of the predicate, and both halves of the search. Full suite in the CI posture: 4221 tests, 0 fail, 1 skipped. Same under the +399 day clock shift. All four guards pass and a run under a fresh TMPDIR leaves nothing behind. No version moved. Signed-off-by: Rob Boerman <robboerman@live.nl>
…ble guesses (#402) A third review round found the same shape once more, one level down: asking `columnsOf` alone READ like "ask the renderer" and is not. Issue #401's sweep pins that table as never NARROWER than the renderer, and its docblock says it deliberately answers ONE for an unassigned code point, because guessing wider is the safe direction for a WIDTH. For MEMBERSHIP that guess is a MISS, and it left 3,760 code points outside the class that the pinned renderer draws at zero columns: U+2065 and the special-purpose plane. That is a hidden ASCII channel 39 times the size of the tag block this file builds a whole sequence matcher for, and `deploy-prod` against `deploy` plus one of them plus `-prod` was eleven columns either way after the gate. Round 2 rejected the previous version for leaving ten code points by the same substitution of one question for another. Rejecting for ten and shipping 3,760 would not have been consistent. Both places the table guesses are now named, measured against the pin rather than reasoned from the standard, and the class is 4,024 code points where the first attempt listed 98. That list is a strict subset of this one. ALSO FIXED, and it is a cost this change introduced: `interpreted` is called per character by every caller, and the tail search scrubs the whole held tail on every render. A review pass measured a 200-line tail of 100 KB lines at 2.1 SECONDS per render. Printable ASCII is answered before anything else now, and the same 20 MB takes 113 ms. Newly pinned, each having survived the previous commit: - the cancel tag is REQUIRED, not optional. Making it optional left a six-character hidden message whole, and the assertion that caught nothing was an inequality on an eight-character one, which a partial substitution satisfies. Every tag outside a complete sequence is now counted. - a flag is matched AT the position being walked, not further along the slice. Without that, `base tags base tags cancel` came back as the second flag twice with the bytes between them gone. - the two arms where the width table guesses. Corrected, several of them mine from earlier commits in this PR: - the live `DES-ADMIN-VIA-PI-EXTENSION` sentence still said "every one of these code points measures zero columns now". The previous commit said it had corrected that and had corrected the revision row and the code instead. 83 members measure one. - the revision row's `IN:` list was still the round-1 class and contradicted the breakdown three sentences later. - `CONTROL_CHARS` has not existed since the class became a predicate, and was still named in `dashboard.ts`, a dashboard test and both revision rows. - "left 158" and "left 167" were both wrong and both unnecessary: the old class was 98 and this is 4,024. - U+FFF0-U+FFF8 were called noncharacters. They are RESERVED; the real noncharacters draw a glyph and are not in the class. - a dead `FLAG_BASE`, and an eslint suppression for a control regex that no longer exists. - the claim that `stripControls` "removes C0 and C1, not half a character": it removes the whole class, and `dropOrphans` on that path is belt rather than the braces its comment claimed. Seventeen mutations checked, two survivors, both equivalent and both recorded in the code: the ASCII fast path and the explicit space check each make the other's mutation equivalent, one kept for cost and one for legibility. Full suite in the CI posture: 4222 tests, 0 fail, 1 skipped. Same under the +399 day clock shift. All four guards pass and a run under a fresh TMPDIR leaves nothing behind. No version moved. Signed-off-by: Rob Boerman <robboerman@live.nl>
…rrect the numbers (#402) The final review pass returned no blocker. What it did return was a list of sentences that are false and two guards nothing holds, and neither is something to ship knowingly, so they are fixed here rather than filed. TWO UNPINNED GUARDS, both found by mutating them: - `FLAG_SEQUENCE`'s lower bound. `{0,6}` survived the whole suite with a real change: a base followed only by the cancel tag kept that tag instead of substituting it. The commit that introduced the bound pinned the upper one and the cancel tag and not the minimum. - The matcher's bounded search window. Widening it to the rest of the string is output-identical and QUADRATIC: 100,000 bare flag bases went from 30 ms to over five seconds, which is the same shape as the lookbehind it replaced. A timing assertion would flake, so what is pinned is the behaviour that makes the bound safe: the longest valid sequence is recognised, at the start and away from it. THE SWEEP ASKS THE RENDERER for the arm the previous commit added, where it had restated the implementation's own range. That is the shape this test's own comment forbids -- it catches a typo and never a wrong bound -- and it was reintroduced for the one arm that most needed an independent oracle. `visibleWidth` is loaded with its version asserted, as `width.test.mjs` does. NUMBERS CORRECTED, and the shape of the error is worth recording: the width sentences carried figures from when the class was 264 members. It is 4,024. By this module's own table 3,843 of them measure ONE and only 181 measure zero; the renderer reads it the other way round and draws all but 19 as nothing. The gap between those two readings is exactly what the predicate exists for, so stating either number alone was never going to be right. Also corrected: a docblock still crediting a lookbehind that was removed two commits ago; "noncharacters" at six sites, where U+FFF0-U+FFF8 are RESERVED and the real noncharacters draw a glyph and are not in the class; and the claim that `stripControls` "removes C0 and C1, not half a character", which the previous commit listed as corrected and had only added a paragraph beside. TWO RESIDUALS NOW STATED rather than left implied: - The composing carve-out is also a channel, and it is the one this file argues hardest for keeping. 265 default-ignorable code points stay out because they compose, and the renderer draws every one as nothing, so `deploy` plus U+E0100 plus `-prod` is eleven columns either way -- the same collision the tag block gets a sequence matcher for, at about 2.8 times its size. Closing it needs the same sequence-awareness, per script rather than per block. - The ASCII fast path only rescues ASCII: a 200-line tail of 100 KB lines costs about 200 ms a render at 0% non-ASCII and about 4 seconds at 100% CJK. Not a regression, since the branch only saves work, but this panel's own argument is that it renders CJK as a matter of course. Full suite in the CI posture: 4223 tests, 0 fail, 1 skipped. Same under the +399 day clock shift. All four guards pass and a run under a fresh TMPDIR leaves nothing behind. No version moved. Signed-off-by: Rob Boerman <robboerman@live.nl>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #402.
The class stopped at what a terminal EXECUTES: C0, DEL and C1, with U+009B in it because a CSI introducer
needs no ESC in front. It excluded bidi and zero-width code points on the ground that a terminal "does not
INTERPRET" them, and that was the wrong reading of its own rule.
A terminal does interpret a bidi override. It reorders everything after it, so
repo/safeplus U+202Eplus
gnp.txtis drawn as a name ending in.png. The job id, the target and the branch are all attacker-or model-writable, and a run record is what an operator reads before deciding what to do about it.
The other half is invisibility rather than reordering.
deploy-prodanddeployplus U+200B plus-proddraw as eleven columns each and are not the same trigger, so a picker that selects by the stringacts on the row the operator did not mean. The dialog gate's own ambiguity note, from #404, is about
exactly this and relies on
#Nprefixes to stay safe.Where the line is drawn, and how it got there
The rule is INTERPRETED against COMPOSING, and membership is asked of the renderer rather than listed:
columnsOfis 0: the format characters, the bidi controls, the fillers, thenoncharacters),
That took three attempts, and the two rejected ones are the interesting part:
A hand-written list of the shapes I thought of. It covered 98 code points where this holds 4,024,
including the Hangul and halfwidth fillers, the entire tag block, and every blank that is not U+0020.
This repo's own
env-file.mjsalready names U+3164 as a deception character, so one surface haddecided the question and mine missed it. It also had no
uflag, so 116 of the misses were notexpressible in it.
A category version derived from
\p{Cf}|\p{Zl}|\p{Zp}|\p{Zs}. That is a different rule wearingthis one's clothes: it still left U+FFF0-U+FFF8, which this file's own width table already calls
noncharacters the renderer draws as nothing, and U+2800, which draws a blank cell.
Asking
columnsOfalone, which reads like "ask the renderer" and is not. admin: every width promise in the panel is a UTF-16 count, so non-ASCII content breaks the frame #401 pins that table asnever NARROWER than the renderer and it deliberately answers one column for an unassigned code point,
because guessing wider is the safe direction for a width. For membership that guess is a MISS, and it
left 3,760 code points outside the class that the renderer draws at zero: U+2065 and the
special-purpose plane, a hidden-ASCII channel 39 times the size of the tag block this change builds a
whole sequence matcher for. Round 2 had rejected the previous version for leaving ten by the same
substitution of one question for another, so shipping 3,760 would not have been consistent.
Both places the table guesses are now named, measured against the pin. The class holds 4,024 code
points where the first attempt listed 98, and that list is a strict subset of this one.
That change found a defect of its own. Asking "does it draw nothing" put the Indic vowel signs into
the class, because #401 had made a spacing mark measure zero to agree with the renderer while the composing
test was
\p{Mn}|\p{Me}. Devanagari\u0915\u093fwas substituted to\u0915-- not revealing acontrol, deleting a vowel. Every mark composes, so the test is
\p{M}.U+3000 is deliberately out: an ideographic space draws two columns and is ordinary Japanese text, so
substituting it rewrites content and narrows the line. The collision it can still make is a stated
residual.
The tag block is both things
After U+1F3F4 a tag composes a subdivision flag; anywhere else it is invisible text, and a run of them is a
whole ASCII message at zero columns. So it is settled by SEQUENCE.
A first version of that exemption read "a tag preceded by the base and any number of tags", which is a
prefix test rather than a validity check: it exempted every tag forever after a flag, cancel tag
included, so one legitimate flag emoji anywhere in a model-writable field restored the whole hazard, and a
review pass recovered
rm -rf /verbatim from a drawn line. It was also a variable-length lookbehind,which rescans at every position: 200 KB of tags in one
.logline took 2.3 seconds, andreadLogTailbounds the number of lines but not their length. A whole valid sequence is matched now, with no lookbehind:
the same 20,000-tag run went from 296 ms to 6 ms.
One class and one operation
This is the shape the issue left open, and it resolves against a second named operation. The worker escapes
for the same question, and that is not an inconsistency to reconcile:
endpointShown's gate is anallowlist of printable ASCII, right for a DNS name or a socket path, while this panel renders a CJK
repository name as a matter of course.
env-file.mjs'sQUOTED_CONTROLreaches for the same idea but byLISTING, which is where this class was two revisions ago.
A regression this change introduced, and fixed
The LIVE_TAIL search could no longer find a line it was drawing: the pane substitutes the class, the search
box deleted it, and the haystack was raw, so for a log line carrying an invisible byte no query matched.
A first repair compared the drawn reading OR the deleted one, which re-merged the very collision the class
exposes --
deploy-prodmatched both rows the pane had just learned to tell apart. The search boxsubstitutes like the pane now, so one reading serves both and typing what is on screen and pasting the
original produce the same query.
What it costs
Two residuals beyond the costs below are stated in the code rather than left implied: the composing
carve-out is also a hidden-text channel (265 default-ignorable code points the renderer draws as nothing,
about 2.8x the tag block, which closing needs the same per-script sequence-awareness the flag has), and
the ASCII fast path only rescues ASCII, so a 200-line tail of 100 KB lines costs about 200 ms a render at
0% non-ASCII and about 4 seconds at 100% CJK.
A correctly isolated right-to-left name now displays worse: the isolate that was making it read
correctly beside an LTR path is substituted, and the panel cannot tell it from an attacker's because they
are the same code point doing the same thing. That is the price of closing the deception, not an oversight.
A soft-hyphenated word and a BOM-led log line each gain a space. And the gate closes the EXPLICIT deception
only: the bidi algorithm reorders neutrals beside a strong RTL character with no control present, which
nothing here can reach without refusing Hebrew and Arabic outright.
Substituting makes a difference visible; it does not say which of two look-alike strings was intended,
because the panel cannot know.
Checked
Mutation-checked at every round; the final set covers the unbounded flag shape, the cancel tag, the tag
minimum, the bounded search window, both composing directions, each arm of the predicate, both places the
width table guesses, and both halves of the search. Two survivors remain and both are genuinely
equivalent, recorded in the code as a pair: the ASCII fast path and the explicit space check each mask the
other's mutation.
The class sweep walks every code point there is and asks the pinned renderer what a code point draws,
with its version asserted, rather than restating the implementation.
Full suite in the CI posture: 4223 tests, 0 fail, 1 skipped. Same under the +399 day clock shift. The
temp-dir, dated-fixture, test-count and revision-row guards all pass, and a run under a fresh TMPDIR leaves
nothing behind.
DES-ADMIN-VIA-PI-EXTENSION amended. OQ-035 amended with it, because
scrubReasonshares the class. Theworker's validator is UNCHANGED, checked, for the reason DES-ONE-SHOT-DISARM-IN-THE-FILE already records.
No version moved: root and worker 1.10.3, receiver 1.5.0, admin 1.10.2.
Carried from earlier in this round:
receiver/src/start.mjsimports@edgehero/pi-dispatch/watch-closer,which the published worker 1.10.3 does not export, so the next release PR must move the receiver's worker
range to the version that ships it.