Fix/panel row steps - #19
Merged
Merged
Conversation
…d entry The MSI registered "Timo" as its Manufacturer, which is what Add/Remove Programs shows as Publisher, and offered no link at all from there back to the project - the same standing a repackage of the installer would have. None of this is the "Unknown publisher" line on the UAC prompt. That line is read out of the file's Authenticode signature and nowhere else, so it stays until the MSI is signed; the comment beside the new properties says so, since the two are otherwise easy to mistake for each other. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The stability card printed "0.8 M" in German where every other locale prints "0.8 ms". The label is right and the figure is right; latest.jitter_unit is what is wrong, and it is wrong in three of the sixteen translations - "M" in German, "m" in Spanish and "Mms" in French. Reported from a real card in German. They read as a unit that has been through a translator: "ms" is a symbol, not a word, so nothing should have happened to it. latest.ping_unit sits two lines above in the same file and survived in all three, which is what makes this a slip in those entries rather than a policy about how the app names milliseconds. en.json is the Crowdin source and the rest are pulled from it, so the same three strings want correcting there as well or the next sync brings them back. Ukrainian carries no jitter_unit at all and falls back to the English "ms" while its ping reads "мс". That one is a missing translation rather than a broken one, so it is left to Crowdin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… everywhere
Three reports from one window, and they are three views of the same fault: at
1263px the latest test stood glyphless between two cards holding exactly the
same 319px of list; at 859px it alone stated a smaller figure than the card
beside it; at 795px it lost its glyphs while the page was still two columns
wide. Each of those is a card being dressed differently from its line-mates on
the same room, which reads as a fault in that card rather than as a tight page.
The icon was the wrong thing to give up, for the reason it was given up last:
it is the only part of a row that carries the grade - the glyph wears it, the
figure beside it is white whatever the reading - so a row without it states a
number with no verdict on it. It goes nowhere now, on any panel, at any width.
And the figure step was in the wrong place. Four cards each stated their own,
and the four had already come apart: the latest test stepped at 22rem of list,
the summary at 30rem along with its descriptions, the stability card at no width
at all, and the value cards were simply a size smaller everywhere. So it moves
into the shared row as an unnamed container query, which steps on whichever list
the row is drawn in. Every panel makes its list a container, so the threshold is
the same for all of them by construction, and cards drawn side by side hold the
same width and step together at every stage of the page.
Two steps rather than one, because the room the glyph used to free has to come
from somewhere: 1.75rem down to 1.4 at 22rem of list, and to 1.2 at 20rem, which
is the width the icon used to go at. Measured across fifteen languages with the
glyph kept, as the widest list that still cuts a label:
1.75rem 1.4rem 1.2rem
summary 380px 360px 350px
latest test 350px 316px 290px
stability 270px 250px 240px
value cards 360px 328px 300px
22rem clears the latest test's 350 and 20rem clears the 316 the first step
leaves it. Re-measured after, over 6930 rows in fifteen languages at 22 widths:
no glyph hidden anywhere, no two cards of equal list disagreeing on a size, and
76 rows cut where there were icons missing from five rows of three cards. Of
those 76, 58 are at a 320px viewport and 11 more at 360 and 375, where a phone
holds 238-293px of list and "Téléchargement" does not sit beside a glyph and a
speed at any size a figure can be stated at. The rest is "Самый высокий" short
2-5px in two 8px-wide bands, which a step of its own would cost every panel a
size across the whole band above to close.
The summary closes its own band differently: its labels are two words where
these are one, so it wraps rather than cuts, from the width its descriptions go
rather than only at the width its rows stack. The line the description just gave
up is the line the wrapped label takes.
The stability card's list becomes a container so it can take the steps at all,
and carries the same :not(.chart-modal-body *) guard the value cards do - a size
container offers an intrinsic sizing pass nothing of its contents, and the
dialog it opens into is shrink-to-fit, which is nothing but that pass. Checked
opened at 1600, 900 and 390px: full-size figures, every glyph, nothing cut.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The left column is 40% because of the summary: its labels and descriptions are the page's only sentences, and a row of them needs 473px of list to sit beside its figure. Below the width where that stops fitting the card gives the sentences up - and from there it is holding 100px it has nothing to put in. That was not only waste. A panel steps its figure down on its own list width, which is the only rule that can hold at all three stages, since the cards' shares of the row change between them. So a card 100px wider than its line-mates steps later than they do, and the summary stood beside two cards stating a smaller figure than its own for the whole band. Measured at 1263px: 28px against 19.2 on exactly the room the rule says each should have. Every alternative pays for it somewhere. Giving the summary a wider threshold of its own fixes the three-across stage and breaks the one-column one, where all three cards hold the same width and it would then step first. Viewport figures per stage break the enlarged view and want re-deriving every time a stage moves. Equal thirds at every width costs the summary its sentences outright - an equal third of even this page is 470px against the 473 they need. Scaling the figure smoothly instead of stepping it makes the cards differ at every width rather than in one band. So the width goes back instead. 1417px is where the summary's list falls to $overview-trim, so the card gives up the room in the same movement as the thing the room was for, rather than twice at two widths - measured, the descriptions already go there, and this adds no second reflow of its own. Every card, not only the three that show it: the left column runs the whole page, the summary over the latency chart over the hourly one, so squaring the top row alone would put a kink in a column three rows deep. Measured across fifteen languages at ten widths, against the same page before: 1400px 22.4/28/28 -> 28/28/28 on 398px each 1300px 22.4/22.4/28 -> 28/28/28 on 365px each 1263px 22.4/22.4/28 -> 28/28/28 on 352px each Nothing cut in any language either side of the change, and the figures come out larger rather than smaller: the two tight cards gain 36px of list each, which is enough to hold full size down to the two-column stage instead of dropping two steps to 19.2px. It costs the summary a second line on one to three of its five labels between 1252 and 1340px, where its descriptions are already gone - so the line they gave up is the line the wrapped label takes - and the page measured the same 1246px tall at 1300px before and after. statisticsReflow.test.js found its stages by taking the widest ceiling in the stylesheet, which held only while the two-column stage was the widest thing on the page. Four of its suites retargeted onto a stage that pairs nothing the moment this was added, so they key on the constants that name them now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ecked
Two values reached a column with no validator standing in front of them, and
each was let through by the same shape of gap: a check written where the value
is not.
The thresholds asked `/[^0-9.]/`, which is not "is this a number" but "is every
character a digit or a dot" - so "1.2.3", ".." and "." were all accepted. No
server code reads ping, download or upload, so the value was stored behind a 200
and handed to the client, where Number("1.2.3") is NaN and getIconBySpeed
answers neutral for a threshold it cannot read: every speed on the page goes
grey and stays grey, with nothing on screen naming the value that did it. "."
was worse, because the ping branch splits on the dot and stored the empty
string. retentionDays already anchors its own check, with a comment saying why -
this is the same rule, two hundred lines up.
The display name is the one value in an integration payload that no module
declares, so the loop holding every type and length cap never saw it. It went
onto the result unread and into displayName, a bare Sequelize.STRING: on MySQL
that is VARCHAR(255), where an over-long name was ER_DATA_TOO_LONG - a 500 and a
stack in the operator's log - while sqlite stored it whole behind a 201. The two
supported backends answered the same request differently. A non-string reached
sequelize's own validator and was a 500 on both. It now wears the cap a declared
text field wears, which is also what keeps it inside the column; undefined stays
allowed, because create() leaves the name to the column default and patch()
reads undefined as "leave it alone".
The two field caps are named while they are being read twice.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three of the app's own controls could be operated with a pointer and with nothing else, and each was the only route to what it opened. The date range decides what every figure on /statistics is about, and its trigger was a div carrying an onClick - so Tab walked past it and Enter did nothing if focus were forced onto it. The presets live inside the popover it opens, so a keyboard-only reader was left with whatever range the page loaded with, on both toolbars that draw one. The four month and year arrows behind it were buttons already, and announced as nothing: each holds one FontAwesome glyph, which renders aria-hidden, so all four had an empty accessible name and which one stepped a year rather than a month was carried entirely by how many chevrons could be seen. The settings menu is the only route to nine dialogs - optimal values, the provider, storage, the password, the schedule, pause, integrations, the language and the preferences - and every entry was the same click-only div. The export menu's two formats were as well, behind a trigger that was already named and already said whether it was open: a keyboard could reach the menu, open it, and then had nothing to press. The trigger and the export formats become real buttons, which is the answer wherever the control is a control - the pagination and the integration menu are both buttons for that reason - and each takes the styling resets a button arrives with. The settings entries take `clickable` instead, for the reason it documents: they hold an <h3>, which a button may not contain. Every button in the picker also states its type, since one left untyped defaults to submit. And the delta states its direction in words. The whole of it was the glyph, in a span marked aria-hidden - right for the glyph, wrong for the annotation around it, because what was announced was a bare magnitude. "5%" is not a reading, and on ping and packet loss the two directions are opposite verdicts, so the part that says which one was the part being withheld. Six new strings, in the source locale and in German. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Neither overlay managed focus at all. Opening a settings dialog from the header gear left focus on the gear, behind the backdrop: nothing announced that a modal had opened, Tab walked the whole page underneath before reaching the dialog, and closing it dropped focus to the top of the document instead of back to the control that opened it. The close control was worse than unlabelled. It was a bare FontAwesome svg carrying an onClick, and FontAwesome renders its svg aria-hidden - so it announced as nothing and, being an svg, was not in the tab order either. A reader was never told a close control existed and had to know to press Escape. Both take one hook, for the reason `clickable` is one function: written out per component is how the shape comes to be missing from one of them. The judgement inside it - where Tab should land - is exported apart from the hook and tested without a DOM, the split useClickOutside already makes. Returning null for a Tab in the middle is the case that matters: that key is the browser's own to answer, and claiming it would mean re-implementing tab order rather than closing it into a loop. The listener sits on the dialog rather than on the document, so the overlay on top is whichever one holds focus and no isTopmostOverlay question arises - unlike Escape, which the document has to arbitrate because it is pressed at the page level. Only the alert on top takes focus; one stacked under another has given up its turn, and when the one above closes it becomes top again and pulls focus back into itself. Focus already inside is left alone, because the input variant of an alert autoFocuses its field and moving that to the first button would put the caret nowhere. An alert names itself with its own title. The Dialog cannot: its heading is written by whoever renders DialogHeader, so the id travels between the two halves through a context, generated per dialog rather than fixed - two of these can be mounted at once, and a repeated id names the wrong one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing MySpeed does needs a privilege. It listens on 5216, which is above the reserved range, and it writes its database and its logs inside its own installation directory. What it also does is download a third-party speedtest CLI at first boot and then spawn it - so running the whole of that as uid 0 meant a replaced upstream asset, or any remote-code flaw in the server, executed with full access to the host filesystem. The Docker path for the same code already drops to an unprivileged user; the native installer did not. The account is created before the unit names it and the installation is handed over before the service is started. That order is the whole of the upgrade case: on an existing install every file is owned by root because that is what put it there, so a service that starts first meets a database it cannot open. The chown names the user and not user:group, because useradd --system creates a matching group on Debian and RHEL but not everywhere, and a chown naming a group that does not exist changes nothing. The home directory is the installation path rather than nothing, because the Ookla CLI writes its licence acceptance under $HOME and an account with no writable home would be asked for it again on every run. A system with no useradd falls back to root and says so. A unit naming an account that was never created starts nothing at all, which is a worse outcome than the privilege this drops - and it would be discovered as "the service stopped working after an update" rather than as anything to do with this. The sandboxing is the standard baseline, and none of it restricts a userspace HTTP server that spawns a CLI and writes inside its own directory: ProtectSystem=full leaves /opt alone, and ProtectHome covers /home and /root, which is not where this account's home is. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two guards and a set of comments that had stopped being true.
The jitter unit is held against the ping unit, in every locale that carries
both. They are the same unit two lines apart, so one being mangled while the
other survives is the whole shape of the fault: latest.jitter_unit read "M" in
German, "m" in Spanish and "Mms" in French, and nothing could catch it.
germanLocale.test.js already names the key in its SHARED set - the values a
German reader sees in the same shape an English one does - but that set is only
ever used to *exempt* keys from the copied-English check, so the one assertion
built on it fires when de equals en, which is the case that is correct.
Read out of the same file rather than against a literal "ms", because ru says
"мс" and zh says "毫秒" and both are right. This matters as CI rather than as
tidiness: crowdin.yml maps every non-English file here as a translation of
en.json, so the next sync can bring all three back.
It found a fourth. ru.json states ping in Latin "ms" and jitter in Cyrillic
"мс" - the same reading twice in one card, in two scripts. Russian names
milliseconds "мс" in the three other places this file does it, so the ping unit
is the outlier, and it is now what the rest of the file says.
The MSI's third link is held too. ARPURLUPDATEINFO was written and left
unasserted, which is the state a property reaches just before someone tidies it
away as unused, and all three are now checked to point at this project rather
than only to exist.
And the comments. Four claims had gone stale, three of them in the same commit
that introduced them:
- "no card can step at a width of its own" was contradicted by a live rule.
ChartModal states the first step for the enlarged value card at 500px of
viewport, which is the one figure size outside the shared row and the one
keyed on a viewport - necessarily, because that card withholds its container
in the dialog and there is no list left to key on. Both ends now say so.
- "a card drawn inside a dialog takes no step at all" holds for three of the
four panels and for three different reasons, none of them the one given. The
summary keeps its container in the dialog on purpose, so it is the panel that
does step there.
- "frees comparable room" was half: the second step buys 26px where the icon
it replaced was 52. The trade is still worth making; it is not a swap.
- "a phone narrower than 375px" put the residual cut band a phone size too low.
A card holds the viewport less 82px at the one-column stage, so the value
cards' floor is a viewport of ~382 and the latest test's ~372 - the common
375px phone is already inside the first.
Two more were invalidated by the equal-thirds stage rather than by their own
commit: the latest test's "~324px of list in the three-across grid", which no
three-across width produces any more, and layout's claim that the new stage holds
every figure at full size, which is true across all but its bottom eleven pixels.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An adversarial pass over the five commits before this one found that three of them had broken something while fixing something else. Each is the same shape: a change that is right on its own terms and wrong about what it sits next to. Enter on a confirmation had started cancelling it. The focus trap seats focus on the first focusable in the dialog, and the close X the same commit added is the first thing in the header - so a confirmation opened with focus on the one control that resolves it with null. The document handler declines a key aimed at a button inside the alert, on purpose, so the browser turned Enter into a click on the X: "delete this test", "remove the password" and the node delete all answered Enter by doing nothing, where before they confirmed. An alert now opens on its primary button, which is what it should have been either way, and the hook takes the target rather than guessing it. Focus was also given back to the wrong element. The hook read document.activeElement in a passive effect, which runs after React has applied autoFocus - so an alert that opens on an input recorded its own field as the place to return focus to, and returned it to an element that had just been unmounted. The alert records the control it was opened from at the moment it is asked for. The welcome dialog was announced as an unnamed dialog. Every Dialog emitted aria-labelledby pointing at the id DialogHeader stamps, and the welcome dialog is the one consumer that draws a banner instead of a header - so the reference resolved to nothing, which names it less than no role would have. It says its name outright now, and that is also the one modal nobody can dismiss. The delta's label was on a bare span, where ARIA prohibits naming: role=generic cannot be named from the author, so conforming readers drop the label and fall back to the visible text - the bare magnitude, with the arrow still hidden. The fix would have looked present and delivered nothing. role="img" is what carries a name on a non-interactive element. And two in the installer, both from -d taking any path. ProtectSystem=full with no ReadWritePaths made the one directory the service writes read-only whenever that path was under /usr, and ProtectHome=true made it unreachable whenever it was under /home or /root - each a permanent restart loop behind a banner saying the install completed, which is what the AVX2 and download guards exist to prevent. The sandbox now names $INSTALLATION_PATH, and ProtectHome is gone: a dedicated unprivileged account already cannot read other users' homes. The recursive chown was worse. It ran over whatever -d was given, and "-d /opt" is one slip from "-d /opt/myspeed" - which would have handed every other application under /opt to an unprivileged account, silently, with nothing that reverses it. Before this the installer changed no ownership at all, so a mistyped path cost a stray binary. It now refuses any directory that does not hold the binary this script has just written. The threshold regex was tightened past the defect it was for. ".5" and "1." are 0.5 and 1, the check this replaced took both, and importConfig runs every stored key back through the validator and abandons the whole restore on the first refusal, naming no key - so a threshold saved as ".5" on an existing instance would have taken the nodes and the integrations down with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The two constants landed between validateInput's doc comment and validateInput, so the @PARAM isPatch block documented a number and the function was left documented as "the caps a declared text field wears" - which says nothing about the patch contract a reader hovers it for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Focus could leave a trapped overlay and never be brought back. The Tab listener sits on the dialog, so it hears only keys pressed inside it - which is enough while focus stays in, and focus can leave without any key at all: a mousedown on the backdrop blurs to the body, and so does a control that unmounts itself. On the welcome wizard, whose backdrop click is a no-op and which has no Escape either, that state was permanent - the trap went inert and Tab walked the page behind a backdrop advertising aria-modal. A focusout recovery seats focus again whenever it lands outside, and an overlay opened over this one is not counted as an escape, because pulling focus out of a stacked alert would fight it for it. Being open and holding focus were one state, and for an alert they are two. The restore was keyed on the second, so a second alert stacking over the first handed focus back to the page underneath two backdrops - scrolling the page to get there - and the alert above then recorded that page control as its own place to return to. They are two effects now: `open` owns the restore, `holdsFocus` owns the seating and the trap. An alert that takes its turn back returns focus to the control it was last on rather than to its first. The date picker announced a pop-up dialog it does not open. Behind the trigger is a popover with no role, no name and nothing that moves focus into it, so a reader was told they had opened a dialog, pressed Enter, heard "expanded", and was then told nothing. The markup is a disclosure - content immediately after its trigger, dismissed with Escape - and aria-expanded states that correctly on its own. And the uninstaller now removes the account the installer creates. It stopped the service, deleted the unit and the whole directory, then reported MySpeed uninstalled while leaving a `myspeed` entry in /etc/passwd whose home directory was the path it had just deleted - and it survived every later uninstall, since install.sh only runs useradd when the account is missing. Not under --keep-data, which is why that is a condition and not a line: the flag exists to leave the database for a later reinstall, and those files belong to this account. Delete it and they belong to a free uid, which the next account created on the host may be given. Data that is kept keeps its owner. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rule kept Two loose ends from the focus work, both found by reviewing it rather than by running it. The close button added to the alert header declared its type; the three beside it did not. A button left untyped defaults to submit, and typing one of four is the state where the next reader cannot tell which way the file means to go. All four now say it, and the rule is held against both overlays rather than against the one control this range happened to add. And the Enter guard's rationale had outlived itself. It explained the rule by saying that "an alert is portaled to the body with no focus trap and only the input variant autofocuses, so one opened by a click leaves focus on the page button that opened it" - which was true when it was written and was made false by the focus trap two commits ago. The case it describes is now unreachable, and the guard is still right, for a reason the comment did not give: Enter on Cancel or on the close X belongs to that control and not to handleSubmit. Which is also how the same trap came to answer a confirmation by cancelling it, so the two are worth reading together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The seating rule took the first focusable in document order, and in both overlays the header comes first - so the first focusable is the X. That is how an alert came to answer Enter by cancelling a confirmation, fixed two commits ago by naming its primary button; the Dialog has no such name to give and was left opening on its dismiss control. Milder there and still wrong in two ways: Enter closes a settings dialog the moment it opens, and a keyboard reader has to Tab past the dismiss button to reach the first field of every dialog in the app - the password, the optimal values, the schedule, the integrations. So the rule skips it rather than each overlay working around it. The control is marked with an attribute rather than recognised by its class, so what the rule means is written where the rule is and does not follow the styling around; and the X is still taken when it is the only control there, which is what an alert with no buttons of its own leaves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…s it
The server's copy of this rule was anchored two hundred lines of review ago;
the client's was left asking `/[^0-9.]/`, which is not "is this a number" but
"is every character a digit or a dot" - so "1.2.3", ".." and "." all satisfied
it. That is the same defect, in the same shape, in the other half of the app.
What it cost is small and specific. The dialog patches its three fields one
after another, so a bad third value was refused only after the first two had
been written: the operator saw an error over a change that had partly happened,
and the message came from the server rather than from the field they typed it
in. The client's own "invalid" toast could not fire for exactly the values it
exists for.
The rule goes beside getIconBySpeed, which is what breaks without it - it
divides by this value, and Number("1.2.3") is NaN, so an unreadable threshold
takes every speed on the dashboard to the colour for a figure nobody measured.
isFailedTest two hundred lines above is the precedent: a judgement the server
also makes, neither side able to import the other, the two pinned to one table
by a test. This one reads the server's regex out of its source rather than
restating it, so the pair cannot drift again without failing.
And withoutComments, because the negative assertion needed it. A scan saying a
construct is *absent* is answered by prose, and the house style names what a
change replaced - so the comment beside this fix, which says what `/[^0-9.]/`
asked, is precisely what an assertion that `/[^0-9.]/` is gone finds. Left
alone, the rule holds the explanation hostage: reword the comment to keep the
test green. It is worth having as a helper rather than as a reworded sentence,
since every `doesNotMatch` over a source file in this repository has the same
exposure.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every regression this hook has had lived in the one part of it nothing executes. modalFocus.test.js covers the four exported judgements - which control Tab should land on, whether focus has escaped - and they are the easy half. The other half is two effects, one keyed on being open and one on holding focus, what their cleanups do, the order React runs them in, and a recovery scheduled from a focusout. Enter answering a confirmation by cancelling it, focus restored to a field that had just been unmounted, an alert stacking over another and handing focus back to the page beneath two backdrops, an overlay opening on its own close button: four bugs, all in that seam, all of which shipped past a green suite. There is no jsdom here and no renderer, and neither is worth a dependency for one hook. So the DOM is the dozen methods the hook actually calls and React is its two, with the deps comparison and the guarantee that matters - on a re-render every cleanup runs before any effect. The timers are real: the recovery is scheduled with setTimeout(…, 0) and the cleanup clears it, and waiting a turn is the only thing that tells those two apart. The hook is loaded by rewriting its react import, so the file under test is the file on disk and nothing about the import graph is arranged for the test. The substitution is asserted, so a hook that stops importing those two fails here rather than quietly testing something else. Held to mutation testing rather than to its own green tick, because a scenario that cannot fail proves nothing: each of the ten defects above and beside them was reintroduced into the hook in turn and the suite had to catch it. It caught nine. The tenth - a trap that claims a Tab in the middle of the dialog instead of leaving it to the browser - was survived, because the assertion asked whether this control had been focused again rather than whether focus had moved at all, and a trap claiming that key moves it somewhere else entirely. That assertion now asks the right question, and the tenth is caught. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Added two commits ago so that an assertion about absent code would not be answered by a comment describing it. Reviewing it found that it gets the question wrong on 13 of this repository's 548 source files, and gets it wrong silently - the direction where a scan quietly stops seeing what it is about. Both causes are the same missing piece: it does not track regex literals, because telling one from a division needs the previous token, and in JSX "</" and "/>" are neither. A pattern carrying a quote - /['"]/ - opened a string state that no later quote closed. A pattern carrying a backtick, which server/util/markdown.js has, opened a template literal, and those really do span lines, so every comment after it in the file was read as string and left standing. One ending in an escaped slash before its delimiter puts two slashes together and reads as a comment opening. Scoping the string state to a line fixed the first - a quoted literal cannot contain a raw newline, so the damage stops at the end of it - and did nothing for the other two. What is left needs a lexer, and a lexer here needs a parser, which is a dependency this repository does not otherwise carry for one assertion. So the assertion goes back to reading the file whole, and the comment beside the fix names the character class in words instead of writing it out. That is the smaller thing to give up: one sentence phrased around a scan, against a shared helper that would go on being reached for and would go on being wrong in the direction nothing notices. The other half of that commit stands and is untouched - the client's rule is still anchored, still held against the server's own regex, and the assertion that it is called once per field is tightened, since asking only whether the name appears is answered by the import line while the guard below it uses something else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The guard on the recursive chown could not do what its own comment claimed. It asked whether $INSTALLATION_PATH held a `myspeed` file - but the script writes that file itself fifty lines earlier, `cd`ing into the path and `mv`ing the downloaded binary to that name. So on any host without a prior install the test passed for every path it could be given: "-d /opt" re-owned every other application under /opt to the service account, and "-d /" the filesystem. It fired in exactly one case, where `myspeed` was a *directory* rather than a file - which is a genuine earlier install one level down. Backwards in both directions. Reproduced before the fix: with a nextcloud config beside it, `-d /opt` left /opt, /opt/nextcloud and its config.php owned by `myspeed`, and the account could then overwrite them. The installer printed that the installation had completed. There is nothing to guard now. The server writes `data` and `bin` and nothing else at the installation root - the database, the logs, the server lists, the certs and the CLI it downloads all sit under those two - so the installer makes them itself and hands over those, which are directories it created. A mistyped path costs a stray binary and two empty directories again, as it did before any of this. The binary and the root stay with root, which is worth having on its own: the account that runs the binary cannot rewrite it. And "-d /root/myspeed" was made permanently unstartable by the same commit that dropped the privilege. The comment beside ProtectHome names that path as one a root user reaches for, and the reasoning stopped at the sandbox: /root is 0700 root:root, handing over the installation never touches /root itself, and systemd chdirs and execs *after* dropping to User= - so the unit failed chdir with EACCES and Restart=always made a loop of it, behind the same completion banner. Confirmed by running the two operations systemd performs, as the account: `can't cd to /root/myspeed`, and `Permission denied` on the exec. So the path is checked for what it actually needs, traversal by that account, and an installation it cannot reach falls back to root and says why - the shape the missing-useradd case already had. Two ways to end up as root, one sentence each, rather than a service that never starts. The three tests over this were all satisfied by source text. One asserted the `if` and the `exit 1` were present, which they were, while the protection did not exist; one asserted the chown named $INSTALLATION_PATH, which is the defect itself. They now pin what has to hold: that no chown names anything but the two directories, that the installer creates them, and that reachability decides the account. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hidden entry The focus restore two commits back works for every overlay opened from a real button and does nothing at all for the nine opened from this menu - which is most of them: optimal values, the provider, storage, the password, the schedule, pause, integrations, the language and the preferences. The menu is never unmounted. DropdownComponent toggles `dropdown-invisible`, which is `visibility: hidden`, and every entry is a `clickable` div that takes focus when it is activated. So the element focus sits on when an entry is chosen is one the same call has just hidden - and a hidden element keeps `isConnected` true while `focus()` on it does nothing. The dialog therefore recorded, passed the guard with, and handed focus back to an element that cannot take it, leaving the reader on <body>: the exact outcome the restore was written to prevent, on the commonest route to it. Answered where the menu is rather than in the hook. A modal has nothing sensible to do with a hidden element, and a menu owes its trigger the focus back when it closes - so the gear takes it, and what the dialog then records is a control that is visible and can be returned to. Only when the menu holds focus: the same function runs for a click outside, where focus belongs to whatever was clicked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Anchoring the client's check made it refuse a value it never sends. `update` patches a field only when it differs from what is stored, but the guard ran over all three regardless - so an instance already holding a malformed threshold, and every MySpeed up to 1.3.4 stored whatever the unanchored check let through, could no longer save a change to either of the other two. Before the anchoring that same value passed the guard and was never sent, so the save worked. Worse than a refusal, it was an unattributable one. The field holding the culprit reads empty on screen, because a number input drops a value it cannot parse - so the toast named nothing and the field it was about looked blank. The guard now asks the same question the send does. A value that is being sent and is not a number is still refused, which is the whole point of the change; one that is only sitting there is left alone, exactly as the server leaves it until something writes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y that matters now The comment justified --home-dir with the Ookla CLI re-prompting for its licence acceptance without a writable $HOME. It never could: server/util/speedtest.js spawns it with --accept-license and --accept-gdpr on every run, so there is no acceptance to store. Worth correcting rather than leaving, because the commit before this one stops handing the installation root to the account - so $HOME is now genuinely unwritable by the account that names it, and a reader going by the old comment would conclude that speedtests are about to start prompting. What the coupling actually costs is named instead: useradd only runs when the account is missing, so a reinstall at a different -d path leaves the home pointing at the old one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The harness ran the hook and could not see four of the things it was written to
check. Each was proved by reintroducing a defect and watching the suite stay
green.
Every listener got its own copy of the event, so preventDefault set a flag on
something discarded the moment the listener returned. A trap that claimed *every*
Tab - which makes the key dead inside every dialog in the app, so a reader can
never leave the control it opened on - was indistinguishable from one that
claimed none, because both leave focus where it was. One event now propagates,
as a browser dispatches it, press() hands it back, and the two cases are asserted
apart: claimed where the trap wraps, left alone in the middle.
Every render built a fresh `{current: dialog}`, so the trap effect's dependency
list compared unequal every time and re-ran whatever else had changed. Both real
callers hold a useRef, so in the app `holdsFocus` is what re-runs it - and with
it dropped from the dependencies the whole stacked-alert suite still passed,
including the case that exists for it. The ref is held across renders now, which
is what the callers do.
Nothing exercised the cleanup's detach. An alert stacked over gives the trap up
and takes it back, and without the detach it keeps the old listeners and adds a
second set - so a later focusout schedules two recoveries while the effect tracks
one, and the other fires into a dialog that has closed. The listeners are counted
after a turn is given up and taken back.
And a closed overlay was never actually closed: unmounting ran the hook's
cleanups while leaving the nodes in the tree, and removing a focused element left
document.activeElement pointing at it - a state no browser produces, and the one
that hid the recovery's `isConnected` re-check. remove() now moves focus to the
body as a browser does, and the recovery is held to it.
The other half of that re-check turns out to decide nothing: seat() goes through
initialFocusTarget, which answers null for focus already inside, so
`!dialog.contains(...)` is an early-out rather than a guard and no test can tell
it from its absence. Said where it is, so it is not later read as a gap.
Fifteen defects reintroduced one at a time, fourteen caught, the fifteenth being
that equivalence.
Also: the patched module went to a guessable path under a shared /tmp that
writeFileSync would follow a symlink to, and was never removed - this machine had
eighty-two of them. It goes in an mkdtemp directory, removed when the run ends.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
None of these change behaviour; each is a sentence a reader would act on. The display name's history overstated the bug in one direction and understated the fix in the other. The create route answers 200, not the 201 the comment and the test both named. And a non-string did not simply fail: sequelize's STRING validator - checked against the version in the lockfile - refuses a boolean, an object and an array, and lets a *number* through, so `42` was stored as the text "42" with nothing said. Which makes the type half of that check the only thing standing between a numeric display name and a silently coerced one, rather than belt-and-braces beside the length cap it sits with. The date picker's trigger names two of the styles a button arrives with that its rule does not state. There are four: font-size and font-weight are unstated too, and as a div the trigger computed 16px/600 against a button's 13.3px/400. Nothing shows it, because both children state their own size and every box measurement there is in rem - so the comment now says which four and why the two it does not set are safe, because the next thing put in that trigger is what would find out. A bare text node, or a padding in em, takes the 13.3px. An assertion in the overview's modal styles failed with "a label wraps while its icon still shows". The icon always shows now - that is this branch's central change, asserted three lines above and again across all four panels - so the message named the intended state as the fault, and would send whoever tripped it looking for a `display: none` rule that was deliberately deleted. The comment beside it had been updated; the message had not. And the calendar arrow parity check was one rename away from testing nothing. It sliced between two class names, and a missing first marker makes indexOf answer -1, which slice reads as one character from the end - an empty window, a parity of nothing against nothing, and a fifth unnamed arrow invisible. Both markers are checked before the slice, and the window has to hold the four arrows that are there. Renaming the container now fails it, where it passed before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both are the same shape: a cap added on this branch is right, and the data already on an instance predates it. A backup carrying an unreadable threshold could not be restored at all. ping, download and upload were guarded by a negated character class until this branch anchored it, so "1.2.3", ".." and a lone "." were all stored behind a 200 by `PATCH /api/config/:key`. importConfig runs every stored key back through the validator and abandons the whole import on the first refusal, naming none - so the nodes, the integrations and the recorded history were all lost to a display preference no server code even reads, and the operator got "Error importing config" with nothing to act on. Those three now fall back to the default and the restore completes. It is the trade the rule already makes for ".5" and "1." - a value that was legal when it was saved must not take a restore down with it - except that these cannot be kept as they are, because Number() cannot read them and a threshold it cannot read greys every speed on the dashboard. Only these three: anything else refused there is a value the server acts on, and guessing at one would restore an instance that is not the one that was backed up. The three keys are named once now rather than spelled out at each of the two places that treat them apart. And an over-long display name locked its integration out of editing. It is the one value on that form no module declares, so it fell outside the client's field validation as well as the server's - and the card resends it on every save whether or not it was touched. An integration created against sqlite before the cap existed could hold a longer name, and every later save of it was a 400 whose only sign was the card's generic error state: no field marked, nothing naming the length, and the offending value one the operator never edited. It wears the same 250 a declared text field wears, on both sides, and the field is marked like every other. The same dead end met anyone typing a 251st character. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`chmod +x` is masked by the file mode creation mask - POSIX says so - and wget creates its output at 666 less that mask, so on a host where root runs with 077 the installed binary came out 700. That was harmless while the installation was handed over whole: the service account owned the binary, so 700 still let it run. It does not own the binary any more, by design - root keeping it is what stops the account rewriting the thing it executes - and 700 root:root is a binary the service can neither read nor execute, under a unit with Restart=always. The install directory is created under the same mask, so a fresh install on such a host is caught by the reachability check and runs as root anyway. The way in is an upgrade: the directory already exists from an install made under a laxer mask, so it is traversable, nothing falls back, and only the new binary comes out unreadable. 755 outright. It is the mode this file has always ended up with in practice, and the one a binary owned by root and run by another account has to have. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every button the keyboard work touched states its type, and the rule is held against the pagination, the date picker, the export menu and the settings menu. The three dialogs those overlays open were never in it: two buttons in the optimal values dialog, two in the integration card and one in the welcome wizard were left untyped, so a rule the branch introduced applied to four components out of seven. Nothing renders a form in this client, so an untyped button submits nothing today. It is worth closing while it is five attributes rather than meeting the exception on the day a form appears. The rule itself asked for `type="button"` immediately after `<button`, so it was really a rule about attribute order - a `ref` or a `key` written first failed a component that had lost nothing, which is exactly what happened once already when the alert's submit button gained a ref. It now looks anywhere in the tag. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…g it Three menus, one shape. Each renders its contents only while open, so closing unmounts whatever has focus - and the reader is left on <body>, with the next Tab restarting at the top of the document. All of it is reachable only because of this branch. The export formats and the date picker's trigger and presets were click-only divs before it: a pointer has no focus to lose, so making them answer a keyboard is what created the state where focus can be dropped. The export menu drops it when a format is chosen - setIsOpen(false) takes away the button the choice was made on. The date picker drops it three ways: Escape and the trigger toggle both run closePicker, a preset closes the popover from inside itself, and so does the second click of a day range. Escape is the one that matters most, since returning focus to the trigger is the whole of what a disclosure owes when it is dismissed. Both give it back to their own trigger, which each already held in a ref for its click-outside. Guarded on the popover actually holding focus, because closing is not always the menu's doing: a click outside closes these too, and there focus belongs to whatever was clicked - and the date picker's trigger closes itself, where focus is already in the right place. This is the third menu of this shape; the settings menu was the first, and it needed a different answer because it hides its entries rather than unmounting them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The focus trap added on this branch shut the integration dialog's create menu out of the dialog it belongs to. DropdownSelect portals its menu to the body, and has to: the dialog it opens inside carries a backdrop-filter, which makes that dialog a containing block for anything positioned fixed within it, so a menu rendered in place would be positioned against the dialog rather than the viewport. The menu is therefore a sibling of the backdrop, not a descendant of the dialog - and every question the trap asks it is a question about containment. So the trap answered "outside" for a control the reader had just deliberately opened. Tab wrapped within the dialog and never reached the options, and focus that did land on one was read as an escape and recovered back out. That menu is the only way to add an integration at all, so a keyboard could open it and then had nothing to press - which is the export menu's own bug, arriving from the other direction, in the same release that fixed it. Two halves. The popover says it belongs to an overlay, and focusEscaped reads it as part of the one that owns it rather than as the page behind. And the menu places focus on its first option when it opens, which is how a menu behaves anyway and is the only way in, since no Tab the trap allows can get there. Choosing an item also gives focus back to the button, which Escape has always done and selection never did: the option it was chosen on is unmounted by the same call, so focus went to the document. Checked for others - the alert and the dialog portals are themselves backdrops and were already covered, and the two on the home page sit under one. handlerIn took the parameter name of the handler it lifts out for granted, so it could only run ones that happen to call it `e`. It reads the real one now, which is what lets the select handler be run rather than pattern-matched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A restore is abandoned whole on the first value the validator turns down - the nodes, the integrations and the recorded history with it - and the answer was "Error importing config" and nothing else. The operator is holding a file that will not go back and sixteen stored values to bisect by hand, with no hint that the problem is one of them rather than the file, the database or the version. This branch made that more reachable before it made it less: anchoring the threshold check added a way for an old backup to be refused, and the fallback added for those three keys does not cover the other thirteen. A cron expression an older version accepted, or a hand-edited password that is not a bcrypt hash, still stops everything. So the refusal carries the key. importConfig answers with a shape rather than a boolean, because the useful half of a refusal is which value it was; the two paths with nothing to blame - a payload that is not a backup, and a write the database turned down - keep the plain wording, since naming a key there would be inventing one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing in the test workflow built it. The suite reads the client's modules as text, because node cannot parse JSX, and lint parses each file on its own - so neither resolves an import. A component importing a sibling whose filename differs only in case builds on a case-insensitive filesystem and fails on CI, and the failure names the generated service worker rather than the import that caused it. This repository has met that one. The only workflow that compiled the client was build-binaries, which runs on `release`. So the first build of the thing every user installs happened after the release was published, and a build-only failure was something the person installing it found first. After the suite rather than before it, because the tests are the cheaper signal and the one that says more about what broke. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three things this branch's own focus work got wrong, and a doc comment
that ended up over the wrong symbol.
The export menu focused its trigger and disabled it in the same commit.
`disabled={exporting}` is set by the very call that closes the menu, and
a disabled element is not a focusable area - the browser's focus fixup
rule takes focus off it and puts it on the viewport. Focusing and
disabling together is exactly as good as never focusing at all, so the
reader still ended on <body>: the bug the fix was written for. The debt
is recorded when the menu closes and paid when the export ends, which is
the first moment the button can hold focus again.
The create menu seated focus from the effect that positions it, which
also depends on `items` - and that array is built inline by the dialog
above, so it is a new one on every render of that dialog. Any of them
while the menu is open dragged focus back to the first option. Seating
is its own effect now, keyed on `isOpen` alone.
And Tab off the last option walked out of the modal. The menu is
portalled to the body, so the dialog's trap - a keydown listener on the
dialog - never hears a key pressed in it: Tab went past the end of the
menu to the page underneath, the blur closed the menu, and the reader
was left on a control behind a backdrop still announcing aria-modal. Tab
cycles the options now, using the trap's own nextFocus so a step in the
middle is still the browser's to answer. Escape remains the way out.
importConfig's own documentation had REFUSED inserted between it and the
comment, leaving each described by the wrong block.
handlerIn takes the last bracket before the arrow rather than the first,
so a handler wrapped in useCallback lifts out the same way a bare one
does. Every one of the ten mutations these tests are written against
fails at least one of them.
Measured in Chrome 148 rather than assumed: removing a focused element fires no event at all, and neither does disabling one. Focus becomes <body> in silence, and re-enabling does not bring it back. So the modal trap's recovery - a focusout listener - cannot see either case, and the component doing the removing is the only place that knows. Adding the *first* integration is that case. The dialog's empty state and its list branch hold two different DropdownSelects, so the trigger the menu has just handed focus to is removed in the same commit that draws its replacement. Every later add keeps the same menu and was already fine. The dialog now records the debt when the branch is about to flip and puts focus on the create button in its new place, so the first add ends where every later one does - which is the one person this menu exists for, adding their first integration without a mouse. The menu no longer claims Tab when it holds no options: nextFocus answers for an empty container by handing back the container, which is what a dialog wants and not what this menu wants - it has no tabIndex, so the key would be swallowed against an element that cannot take focus and Tab would do nothing at all. And useModalFocus no longer names an unmounting control among the cases its recovery answers. Focus does go to the body there; nothing tells this hook about it. Six more mutations, each caught.
Both forward arrows carry disabled={isCurrentMonthView()}, and pressing
one is how that view is reached: stepping forward from the month before
the current one turns off the very control that was just activated.
Chrome fires nothing when a focused element is disabled, so nothing
answers it - the reader is dropped out of an open popover onto <body>,
with the next Tab at the top of the document, and re-enabling the arrow
later does not bring focus back.
Reachable because this branch made the popover openable without a
pointer at all; before it, a keyboard never got in here to lose anything.
Focus goes to the step back: at the boundary it is the only direction
left, and it is the one arrow that is never disabled. Guarded on <body>,
which is exactly the state the browser's focus fixup leaves behind -
Safari does not focus a clicked button, so there the step never held
focus and there is nothing to give back.
Six mutations, each caught.
The trap could only ever hear focusout, and neither of the two ways a dialog loses focus fires one. Chrome dispatches nothing when the focused element is removed and nothing when it is disabled, moving focus to the body in silence both times - and re-enabling the control does not bring it back. That is the common case, not an edge: the retention save, the schedule, the pause window and the create-node dialog all disable their primary button while it saves, so the usual thing to do inside a dialog is also what empties it of focus, behind a backdrop still announcing aria-modal and with the next Tab at the top of the document. So the dialog watches itself. Only a mutation says anything, and the watcher acts only when focus has landed on the body - focus that moved somewhere real, a stacked alert or the portalled create menu, is not something to take back, and focusout already answers for that. seat() no longer offers the seat back to a control that has since been disabled: it is still connected, cannot take focus, and focus() on it is a silent no-op that left the reader exactly where the recovery found them. The harness grew what the case needs: a MutationObserver that honours childList, attributes, attributeFilter and subtree - ignoring them would pass a watcher that watches neither of the two things it is for - and a focus() that refuses, as a browser does, to land on an element that is disconnected or disabled. IntegrationDialog's own restore became a layout effect, so it settles before the watcher's microtask and a reader hears one control named rather than two. Nine mutations; eight caught and the ninth documented where it stands.
The create menu's key handler sits on its container, which is mounted whether or not the menu is - so with focus on the trigger it claimed Escape and closed a menu that was already closed. Dialog's own Escape declines a key whose default has been prevented, which is what stops a stacked alert taking the dialog under it down as well, so what this swallowed was the one key that dismisses the dialog: on the create button, inside the only dialog that draws one, Escape did nothing. Not new here, but this branch is what makes it easy to reach - the trap seats focus inside the dialog and keeps it there, so that button is part of the ordinary cycle rather than a long Tab away. The export menu answers Escape now, which is what the settings menu, the context menu, the date picker and the create menu all already do and the only one of the five that had nothing. A reader who opened it and wanted neither format could Tab past both or reach for the pointer. Reachable because this branch made the two formats answer a keyboard at all. Both handlers are now asserted to be wired to something, which neither was: removing the onKeyDown left every key test passing.
Three of the assertions covering the privilege drop were satisfied by a command being mentioned rather than used, and one function was not covered at all. reachable_by_service is what decides whether the service account can get to the installation - "-d /root/myspeed" is the case, where /root is 0700, the chown never touches it, and systemd fails chdir under Restart=always. Every assertion written against the script's text still passed with its permission check replaced by `:`, which is the whole function saying yes to everything. It is executed now, with `find` answered from a list rather than from the filesystem: the mode bits do not exist on the machine the suite runs on, and what is being tested is the walk up the tree. Bounded, because what it runs is a `while :` loop. A walk that stops climbing does not fail the suite, it hangs it - which is the one failure nobody reads, and it is what happened while checking this. useradd and userdel were both asserted as words, and `command -v useradd` and `command -v userdel` carry those words in the guards beside them - so an installer that only ever asked whether the host had useradd, and an uninstaller that never removed the account, both passed. Nine mutations across the two scripts, each caught.
The reason a step is spent on the figure rather than on the label is that the figure is what the eye lands on, and $panel-figure-floor says so - "1.2rem is still larger than the label beside it, so the figure is what the eye lands on at every width the card is ever drawn at". Nothing checked it: either step could be taken below $panel-title-size and the suite stayed green. The two sizes themselves stay a judgement and are left as one. Pinning them to their exact rem would be a change detector, and the relationships that matter - the tighter step stated second, the wider step taken before a label is cut - are already held.
A Dialog holds focus for as long as it is open - it has no idea an alert has stacked over it - so both watchers are live together, and a mutation behind the alert can arrive while focus is momentarily on the body. The watcher is a microtask and the focusout recovery is a timeout, so the dialog underneath does win the first move. It does not keep it: the alert's own recovery finds focus outside itself and takes it back within the turn. Reasoned through and then run, because the alternative was a comment claiming a race resolves without anything demonstrating that it does. Both halves are mutation-checked - removing the focusout recovery, or the watcher's guard on where focus actually is, each leave the reader behind the alert's backdrop.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
CodeQL is right, and the regex is one this branch introduced. Anchoring the threshold check replaced a linear negated class with `[0-9]+\.?[0-9]*`, which puts two digit runs on either side of something optional: a run of digits that fails at the end can be divided between them in as many ways as it is long, and the engine tries every one. Measured here at 1.4ms for 2,000 characters, 7.2 for 5,000, 27.6 for 10,000 and 113.6 for 20,000 - doubling the input quadruples the work. Reachable with nothing but a request, and worse through the import than through the PATCH it was written for: importConfig runs every stored key back through this validator and its body is parsed at a 50mb limit, so one restore carrying a long enough threshold blocks the event loop for as long as the caller cares to make it. A default install has no password to stop anyone sending it. The dot moves inside the optional group, which leaves the two runs unable to trade characters. The accepted language is unchanged - the table both copies are pinned to is what says so, and all 29 values in it still answer the same on both sides. The client's copy goes with it. Only the server's is reachable with an input worth the cost, but the two are one rule and drift is what that file's history is about. Timed rather than reasoned about, on both copies, with four orders of magnitude of margin. Reverting either regex fails its test.
…n it Found sweeping for anything of the same shape as the ReDoS CodeQL caught. `field.regex` was compiled and run against the raw request value, and the only bound on that value is app.js's 100kb body parser - so every pattern a module declares was handed up to 100,000 characters, while the 250/2000 caps that exist for exactly that value ran afterwards. Nothing is wrong today: all eleven shipped patterns were parsed out and timed to 80,000 characters and every one is flat, worst case 0.3ms. This is so the next module to declare a pattern cannot be handed more than its column takes, rather than that being left to whoever writes it - which is the assumption the threshold check just broke. Every branch in that block answers `false`, so the move changes no answer, only which check reaches it first. It stays ahead of the number branch, which ends by writing the coerced value back: past that it would be testing a number rather than what arrived. The test is an ordering test and says so. A functional one cannot see this, because both orders refuse the same values with the same answer.
The fix stands; the reach I wrote beside it does not. Both the comment in the controller and the one in the test said a default install has no password to stop anyone sending it. That has not been true since the public-exposure work: handleUnconfigured admits only a loopback caller, ALLOW_NO_PASSWORD, or the per-boot setup token, and previewReadOnly refuses the method on a demo. So the two write routes are behind the password, and the bug was a post-authentication stall rather than an anonymous one. Stated as what it is instead, and why it was still worth fixing: restoring a backup somebody handed you is the ordinary way to hold a value nobody typed, both routes are reachable from a session as well as a header, and a single-threaded server stalled by one request is stalled for every caller. Caught by an audit that checked the premise I had given it rather than taking it on trust.
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
2 participants
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.
No description provided.