Fix/review p1 p3 - #18
Merged
Merged
Conversation
The guard added for "tabbing to Cancel and pressing Enter ran the confirm action" bailed out for any focused BUTTON anywhere on the page. The listener sits on the document, an alert is portaled to the body with no focus trap, and only the input variant autofocuses - so an alert opened by a click leaves focus on the button that opened it. Declining the key there does not hand it to Cancel. It hands it to the browser, which turns an unclaimed Enter on a focused button into a click, so the trigger fires again and pushes a second copy of the alert being answered. Enter no longer dismissed an alert at all, and a held key stacked them. areaRef answers which buttons are the alert's own, which is the distinction the rule was always about. The test stub carried no `contains`, so it could not tell the two cases apart and passed for either rule; it models containment now and covers both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
migrateStoredPassword is awaited at the top level of index.jsx, so whether it settles decides whether React mounts. A .catch() was put on it for the rejecting case, which covers a refusal and a dropped connection and misses the one it has to survive: a request accepted and then never answered - a reverse proxy holding the call while the backend restarts, which is when an upgrading user reloads. A fetch that never settles is neither fulfilled nor rejected, so no catch can run, the await never returns and root.render below it never executes. The page is blank, which is the symptom the catch was added to remove. login() was the only fetch in the file with no AbortController on it; the other three armed one each, and the copies are what let this one drift. One timedFetch wrapper now, used by all four. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The provider PATCH sat above the preview branch rather than inside it, so it ran first and previewReadOnly refused it 403 - which throws, toasts, and returns before the line that records the wizard as shown. ConfigContext reopens it whenever previewMode is set and welcomeShown is absent, and nothing else writes that key, so every visitor to a public demo met an unclosable box over the whole dashboard, on every load, in every browser. There is nothing to save on a demo in any case: its configuration is fixed by the operator who published it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…h it withoutSecrets blanks exactly the fields a module flags, and ntfy flagged only its optional token. send() posts to `<url>/<topic>` and attaches an Authorization header only when a token is set - so on the ntfy.sh default the topic name is the whole publish and subscribe control, exactly as the unguessable webhook path discord, webhook and healthChecks each mark secret. Both paths that redact were handing it out: GET /api/integrations/active on a public demo, and the config export that stamps itself secretsRedacted and is the file people attach to bug reports. The test now names, per integration, what has to be withheld to stop a stranger reaching it, and refuses a module that has not had that decided - the same rule the read routes are held to in previewReadOnly.test.js. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hoose only its bytes Two things the proxy took from the far end that were not its to give. 304 sits in the 3xx range and is not a redirect: it carries no Location and means the caller's copy is still good. Classified by range alone it became 502 "The node redirected the request" - and Express puts an ETag on every proxied 200, so the browser revalidates on its next poll and a node whose answer had stopped changing turned the node view into an error. The content type was copied through unchecked, which let a node decide what the *parent* origin serves. text/html renders as a document there, and the CSP that would stop it says script-src self - which the node's own scripts satisfy, because they are proxied through the parent too. From there a script reads /api/storage/config?includeSecrets=true with the operator's session attached. The body still passes through untouched; it is served as bytes unless the type is one the proxy is actually used for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Clearing the parent's stored credential was checked the way setting one is: the sentinel was presented to the node as a password. Nothing accepts it, and the node whose own password has just been removed refuses hardest - that being the only reason anyone sends this. So the parent refused the clear 400 and kept the stale credential, while the dialog, which never looked at the response, toasted "Password removed" in green. Every poll afterwards authenticated with a password the node no longer had, and the re-prompt that followed could not be satisfied. The route skips the round trip when clearing, and the dialog checks the answer like its save path does. A password that is genuinely wrong is still refused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
promptUntilAccepted catches nothing and both loops are started from a bare call, so a rejection in the attempt was an unhandled rejection and the prompt simply disappeared: no toast, no retry, and a node left half-configured. Both halves of the first line can reject - baseRequest on a dropped connection or its own ten second abort, and .json() on any body that is not JSON, a reverse proxy error page being the ordinary one. Neither is a wrong password, so the loop now ends with the reason on screen rather than asking again. Both prompts also marked their input required, as the admin login prompt is: an empty answer reads as a cancel, so a stray Enter closed them the same silent way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A backup carries the ids of the instance that wrote it, and they mean nothing on the one reading it. Written through as-is, every id already taken raised a UNIQUE violation - each caught, counted into a console-only tally, and reported to the operator as "Tests imported" with a 200. The shape that costs the most is the ordinary one: a disk dies, MySpeed is reinstalled and runs for a week before anyone gets to the backup, and the restore then silently discards exactly the overlapping week. Left to the database to number, nothing collides and the whole file lands. What was skipped travels back with the message too, so a file that was half refused no longer answers the same bare sentence as one that restored whole. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
listStatistics reads every row in the range - findEvery carries no limit - so "all time" on a year of five-minute tests materialises around a hundred thousand rows per call. It sat behind the general 300/min backstop alone, while /export, which reads the same rows to answer the same page, sat behind 20. On a demo the password middleware admits everyone, so the heaviest read in the app was the one anonymous callers could ask for most often. The page loads statistics on demand rather than on a poll, so the limit is well clear of ordinary use. The test names which reads are costly and holds the mounts to that list in both directions. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…d adapter Two problems in one round. The probes were awaited one at a time, and each is a network round trip ending on an answer or on a five second timeout. requestInterfaces is awaited before app.listen(), so a Windows host carrying Hyper-V, VPN and Docker adapters that answer nothing spent that time with the port shut and nothing on screen to say why. They are independent by construction; they run together now. The fallback wrote to the configuration the first round the pinned adapter was missing, and that write is one-way - nothing else sets the key, and the guard reads the replacement as present ever after. One round is what a tunnel being restarted looks like: WireGuard removes the interface outright, so a wg-quick down landing on the hourly refresh permanently repointed every later measurement off the VPN onto the bare WAN link, with the dialog showing the new choice as though the operator had made it. Three consecutive rounds now, and the old value is named in the log so the change is recoverable. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
console.error runs util.inspect over an Error, which prints its own enumerable properties beside the frames - and sequelize copies a failed statement''s bind parameters onto its DatabaseError. The integrations table''s data column is where every downstream credential lives, so a database failure on an integration write printed the telegram bot token, the webhook URL and the influx token into the log. On the Windows service that log is a file on disk, and it is the first thing anyone attaches to a bug report. util/errorHandler.js already records reported.stack for this reason, which is why nothing ever leaked through that path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The description was trimmed inside send() so a message added later could not be the one sent whole and refused - and username, one property away in the same object, was not. Discord validates the override at 1-80 characters and answers a longer one with a 400, delivering nothing: worse than an over-long description, because it is the same value on every request and so kills the finished and the failed notification unconditionally. Nothing upstream bounds it either - display_name declares no regex, so the only gate was validateInput''s generic 250-character cap, three times what discord takes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
safeUrl called itself "the one place the server fetches a URL the user typed: adding a remote node", and that was not true - webhook, healthChecks, gotify, ntfy and influxdb each fetch a stored one too, and all five go through util/http.js, which called the global fetch with no address filtering at all. The only gate was the field''s own regex, which matches http://169.254.169.254/ as happily as anything else. The outbound check is deliberately narrower than the node one. Loopback is allowed: a node is another machine by definition, but an integration endpoint on the same host is ordinary - InfluxDB on 127.0.0.1:8086, a gotify container beside MySpeed - and refusing it would break more working installs than it protected. Link-local is refused, because nothing legitimate notifies it and 169.254.169.254 is the one destination in that space with anything to gain. No DNS lookup either: this runs inside the run lock on every finished test. The residual gaps - a name resolving to link-local, and a redirect chosen after the check - are named in the docstring rather than left implied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
findMounts needs app. at the start of a line and a quoted path. A mount that is indented - inside an if, a loop, a helper - or one whose path is a const matches nothing, so it is absent from the scans built on it: not unguarded, not exempt, simply not there. Every preview-mode assertion is derived from that list, so none of them can see it, and their only defence was a floor on the count with four or five of slack in it. That is the one direction a security scan must not fail in, and it is the same failure previewReadOnly exists to end - a rule reproduced by hand is a rule something forgets, and a scan that quietly drops what it cannot parse forgets the same way. Nothing in the tree escapes it today; the check is there so that stays a fact rather than an assumption. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
wget -O opens and truncates its output file before the transfer starts, so pointing it at the live binary destroyed the installation the moment it ran. Nothing checked the result, and there is no set -e, so a download that died mid-transfer fell through to writing the service, restarting it, and printing "Installation completed" over a zero-byte executable in a Restart=always loop - exiting 0, so a pipeline read it as a successful upgrade. Downloaded beside the binary and moved over it only once it has arrived and is not empty. The cd is checked too: unchecked, an unwritable path put the binary in whatever directory the caller was in while the unit still pointed elsewhere. This is the conclusion the AVX2 check above already reaches - a permanent crash loop announced as a finished installation is worse than stopping. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ut of shell bodies Two things the Windows job got right and the rest of the chain did not. build-windows boots its binary between the compile and the upload, because a green compile says nothing about whether it runs. Every word of that applies to MySpeed-linux-x64-baseline, which exists for the same CPUs - and install.sh picks it automatically and silently on any x86_64 host without avx2, under a Restart=always unit. The artifact most likely to reach someone who cannot debug it was the one shipped without ever having been started. Both x64 legs verify now; arm64 says in the matrix why it does not. And a workflow expression is substituted into the shell source before bash parses it, so create_release.yml took the raw dispatch input as code - eleven lines above the regex that exists because the value is not trusted, in a job holding a contents:write token checkout has already written to disk. Routed through env:, along with the two later steps that did the same with an already-validated value, so staying safe does not depend on remembering which were checked upstream. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Reading window.localStorage throws a SecurityError rather than answering null when the store is blocked - Chrome and Edge with third-party cookies off, which Incognito is by default, and any browser set to block all site data. The dashboard is put in cross-origin iframes on purpose: FRAME_ANCESTORS exists so it can sit in Homepage or Heimdall, and that is exactly where a blocked store turns up. The throw landed during module evaluation - i18n.js seeded the language at the top level, ThemeContext read the theme in a useState initialiser - so nothing rendered at all. Not a degraded page: a blank one, with no error boundary above it to say why, because the router carrying the error element had not been built. One guarded wrapper, used by all eight call sites, falling back to memory rather than nothing - so a blocked store costs the reader their preferences surviving a reload and nothing else. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ttable Putting statistics on the expensive limit was too tight: twenty a minute is reachable by clicking around a page that re-reads on every timeframe and range change, and being told to slow down for using the page costs more than the load it saves. Sixty is a request a second sustained - nobody does that by hand, and it still cuts the worst case by five. Adding it also turned nineteen aggregation tests into tests of the limiter, which is the general problem: a suite drives one endpoint dozens of times from one address in seconds, a shape no caller produces. Every limiter is built through one helper now and resetRateLimits puts them back, so a route can be given a limit without quietly rewriting what its tests measure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The card trims by container query so an enlarged view is "simply a wide container" - but flex-wrap stayed on @media (max-width: 1500px) when the rest was converted. That is the worst of both: in the modal the list is wide, so no trim matches and every row keeps its icon and its whole description, while the viewport is under 1500px, so the row may still wrap. The longest row then put its figure on a line of its own, left-aligned under the icon - space-between leaves a lone item on a wrapped line at the start - while every other row kept its value on the right. Seen at 950px and again at 644px, on Ping: the longest description the card carries, and the only one of those values wearing a delta. The wrap moves to the tightest container step, where the icon is already gone and the label already takes two lines - the point at which a row genuinely has nowhere left to take the room from. Above it the description ellipsises, which is what the shared row was built to do. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1030 is where three cards stop fitting, not where they stop reading. The band just above it was the problem: at around 1050 the page is still three across, each card near 330px, and the summary - five labelled rows, the widest thing on the page - is dressed in a form tuned for a third of a much wider row. A paragraph in a box meant for a figure. The summary takes its own row from 1250 now, while there is still room for it to read as one. Measured across the change: at 1260 the page is three across with the summary at 467px, at 1250 it spans 1218px, and nothing on the page clips at any width between 1000 and 1300. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
One line is right on a wide row: the sentence nearly always fits, and the title attribute finishes the rare one that does not. On a stacked card it is not. Measured at 375px the cell holds about 40 characters of a 108-character reason, so the summary read "Could not open a connection to the test..." - which names no cause at all - and the title that would finish it is a hover, which a phone does not have. Clamped at two lines rather than left to wrap: the first thing this stylesheet pins down is a list that does not change row height as it scrolls. Two lines double what is readable and still reserve a height the card is built around, and at 640px the whole sentence now fits inside them. The full reason was never actually lost - the detail panel wraps it in full, one tap away - which is what makes this a better summary rather than a rescue. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Its columns were capped at 32rem, and on a shrink-to-fit dialog a cap is also the width the dialog settles at: measured at every viewport from 768px to 1920px the track came out at exactly 512px, so "Average latency, between 4.8 ms and 21 ms" was cut by the same 22px on a 4K display as on a laptop, with 798px of screen sitting empty beside the dialog. German loses three rows the same way, and reported first - but English is cut on that row at every width there is. The cap is now what the widest row actually reads at, measured across the fifteen languages: 534px in English, 601 in German, the only other language whose overview strings are translated at all. And it is clamped against the viewport rather than left as a bare length. Written as one it was taken whole in the pass that has no dialog width to answer with yet, and the dialog's overflow: hidden then cut whatever fell past its edge - on a phone a 512px grid inside a 351px dialog, with every figure the panel is read for drawn outside it and unreachable, the body scrolling vertically only. That was there before this change and would have widened with it. The columns now measure from the screen: 317px at 375, 442 at 500, the full 608 from 768 up. Where a column still cannot hold a description, a second line rather than an ellipsis. The shared row cuts its sub-lines because a card has a height to keep; the enlarged view scrolls, so the line costs it nothing where the ellipsis costs the reader the end of the sentence. Two columns arrive where two of them fit as they read - 1220px - rather than where two of the old tracks fit, and share what the screen leaves after the gap between them up to the same cap. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Found while checking the change above across the nine cards: the opened value cards still cut theirs. Measured at 375px, "Maximum" by 16px and "Minimum" by 2, so a phone read "Maximu..." over a number - a figure whose measurement has gone missing, where a cut description only loses the end of a sentence. Wrapping is what the enlarged view can offer a description, and it is offered here too, but a one-word label has nowhere to break. So the room comes from the figure instead. Those cards already state it one step down and exempt this view, on the grounds that "opened, the card has the width of the dialog" - which holds while the dialog is the wider of the two, and stops holding at the bottom of the range: at 375px the enlarged card is 317px inside its padding against the ~330 the card itself gets. Below 500px it takes the same step the card takes, rather than a third size of its own. Swept afterwards at 1280px and 375: nothing cut and nothing outside the dialog in any of the nine enlarged views. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…draws Four blocks in the dialog's stylesheet sized the svg and the fonts of .consistency-item, .overview-item, .test-container and .value-item - the row each statistics panel drew by hand before all four were unified onto PanelRow. No component has rendered any of those class names since, so the fourteen rules they compiled to could not match a node in the document, and deleting them changes nothing a reader sees: every surviving rule in the sheet compiles byte-identical. The parents stay. .consistency-container, .info-container, .value-container and .overview-items are all still rendered and all still carry rules of their own; it is only the retired row inside each that goes. panelRow.test.js already guarded against these names coming back, and it passed the whole time they were here: it asks each panel's own stylesheet, and each of those four was clean. The shared sheet was simply not on the list. Naming one more file would leave the same hole one file over, so the guard now puts the question to every stylesheet the client has - which is what makes it a guard against the class returning rather than against four files changing. Rules nothing can match are not inert. They are the next reader's evidence that the class is still in use, and the enlarged view is exactly where someone would look to learn how these rows are meant to grow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The row cuts a label that will not fit, which keeps the figure whole and is the right trade on a card - except that a label cut to "Maximu..." or "Téléchargem..." leaves a number with no measurement attached, and unlike a description there is nothing left to infer it from. Measured across ten languages at every stage of the page, the cut happens in exactly two states, both a list of about 300px: the three-across grid, where a card holds 324px of list, and a phone, where it holds 293. Between them - the two-column stage, and any display past 1600px - a list is 460px or wider and nothing is cut in any language. So this is a step and not a smaller icon everywhere. What those 300px hold besides the label is the icon and the gap after it, 52px, which is what the overview card already gives up at its own tightest step for exactly this reason. 22rem is the measurement: at 324px of list Ukrainian's "Завантаження" beside a gigabit speed overflowed by 27px, so it wants 351px to sit beside both its figure and its icon. It closes all five of the desktop cuts and fifteen of the eighteen on a phone. The three left are Ukrainian's two upload and download labels, over by 4px and 6px at 375px, and French's "Téléchargement", which lands either side of the edge depending on how wide the figure beside it is that day. Keyed on the list rather than the viewport, like the overview card's steps: the same card is drawn at a third of a wide row, at the whole width of a phone, and again inside a dialog, and one viewport figure cannot tell those apart. The value card establishes that container everywhere but the dialog. A size container contains its inline axis, so it offers an intrinsic pass nothing of its contents - and the dialog it opens into is shrink-to-fit, which is nothing but that pass. Measured with the container left on: the enlarged card fell to 402px against the 426 its rows want, and to 282 against 351 on a phone, where the floor is min-content rather than the dialog's own 400px minimum. The step is not wanted there anyway - opened, the labels wrap and the figure steps down on a phone. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The run was cleared on the round the threshold was reached, whether or not anything was written - and on a host with no usable adapter at all there is nothing to write. So the count went back to zero while the pinned adapter was still gone, and a machine that lost every adapter and brought them back one at a time began the three rounds afresh from the moment there was finally something to move to: three more hours pointed at an adapter that had by then been missing for six rounds rather than three. The count means how long it has been missing, and it has been missing on the rounds where nothing could be done about it too, so it carries on. Not carried across a change of adapter, though, which is what makes this more than moving one line. The old code folded "nothing pinned" into the same path by forcing the wait to its full length, so preserving the count there would hand a freshly chosen interface a spent one: the operator picks it, it blinks once - which is all `wg-quick down` looks like - and it is rewritten immediately, the exact failure the wait exists to prevent. The two cases are separate now, and there is a test for each. The progress line splits with them: a count that may now stand past the threshold cannot be printed as "(4/3). Keeping it for now", which describes neither still waiting nor having nowhere to go. Found by the cloud review of this branch, and independently in reading it back. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…lyph The icon was the wrong thing to give up first. It is the only part of the row that carries the grade - the glyph wears it, the figure beside it is white whatever the reading - so a row without its icon states a number with no verdict on it. Taken at the three-across stage that cost the reader more than it bought: looked at on a 1280px screen, the latest test stood glyphless between two cards that kept theirs on exactly the same 324px of list, which reads as a fault rather than as a tighter card, and English was never cut at that width to begin with. So the figure goes first, at 22rem of list. It is stated a size larger than anything else in the card, one step down frees 34px where the icon frees 52, and it costs nothing anyone can read: that band is where French, Ukrainian and Russian are cut and English is not. The icon goes last, at 20rem, which is where the overview card already gives its own up - so on a phone the cards lose their glyphs together and the page reads as one thing rather than one card at a time. Measured after: no label cut anywhere, in ten languages across six viewports from 375px to 1600. The value cards take the icon step only - their figure is already stated a size down at every width, so there is none of it left to give. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.