Remediate project audit findings - #342
Conversation
Harden tool parsing, async result handling, external previews, and service worker cache privacy. Update production dependencies, deployment scripts, generated metadata, and compressed translation payloads with regression coverage.
Deploying byteflow with
|
| Latest commit: |
143b216
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://d34a9f33.byteflow-c58.pages.dev |
| Branch Preview URL: | https://fix-project-audit-remediatio.byteflow-c58.pages.dev |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 143b216245
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| import { existsSync } from "node:fs"; | ||
| import { createServer } from "node:http"; | ||
| import path from "node:path"; | ||
| import serveHandler from "serve-handler"; |
There was a problem hiding this comment.
When a self-hosting or container deployment installs with npm ci --omit=dev (the checked npm ci --help lists --omit <dev|optional|peer>), this import throws ERR_MODULE_NOT_FOUND before the server starts because serve-handler remains only in devDependencies. Since npm start now always executes this file, move serve-handler into dependencies or avoid requiring a development-only package at runtime.
Useful? React with 👍 / 👎.
[Blocking / High] Production-only installs cannot run
|
[High] Returning from Decode mode leaves an enabled PNG action attached to a blank canvasStatus: Confirmed on PR head Affected code
User reproduction
Browser evidenceA fresh headless Chromium run against {
before: {
width: 256,
height: 256,
dataUrlLength: 5322,
nonTransparentPixels: 65536
},
after: {
width: 300,
height: 150,
dataUrlLength: 2118,
nonTransparentPixels: 0
}
}After returning to Generate mode, both PNG and Copy reported Expected behaviorReturning to Generate mode should immediately show the QR that corresponds to the current form state. Export/copy actions must refer to that same rendered artifact; they must be disabled if no current artifact exists. Actual behavior and impactThe visible canvas is replaced, but the state variable used to enable actions survives. This creates two inconsistent artifacts:
The operation reports success even though the PNG is unusable, making this a silent data-integrity failure rather than a cosmetic preview issue. Root causeChanging This mode/remount gap appears to predate the offscreen-render change, but it remains in a tool touched by this project-wide correctness audit and interacts directly with the new render lifecycle. Recommended fixMake the render lifecycle aware of the canvas remount. Viable approaches include:
In all cases, invalidate or gate export state while the visible canvas does not contain the current committed render. Do not rely on a historical Regression coverageAdd a component/browser test that:
Merge assessment: blocking for the QR workflow because a normal tab sequence produces a successful-looking but blank export. |
[High] PNG and Copy remain enabled for the previous QR while a new asynchronous render is pendingStatus: Confirmed by the current state flow on PR head Affected code
Deterministic reproductionThis is easiest to make deterministic by delaying
At step 3, the form displays B but the visible canvas and The same window can occur during the lazy QR module load, QR encoding, logo image decoding, or any future asynchronous renderer work. Slow devices and larger logo images make it easier for a user to hit. Expected behaviorAn export action must either:
A stale result must never be presented as a successful export of the current input. Actual behavior and impactThe request ID correctly prevents an older asynchronous request from overwriting a newer one, but it does not invalidate the last successful artifact when a new request starts. Users can therefore silently distribute a QR code containing the previous URL/text. This is especially risky when the old and new inputs differ only in a token, destination, environment, or campaign identifier: the image looks structurally valid, so the mismatch may not be noticed before publication. SVG export is computed from current React state while PNG/Copy are based on the last committed raster render, so different export buttons can also produce different payloads during the same pending interval. Root causeThe PR introduced a two-phase offscreen render ( Recommended fixTrack render validity explicitly. For example:
Keeping the old preview visible during rendering is acceptable, but it should be visibly pending and must not be exportable as the new result. Missing test coverage
Add tests that:
Merge assessment: blocking because the UI can successfully export a valid-looking QR for the wrong payload. |
[High] The new Crontab pre-validator rejects common expressions that the tool's parser supportsStatus: Reproduced on PR head Affected code
Confirmed examplesThe current validator returns
Commands used for confirmation: cronstrue.toString(expression, { throwExceptionOnParseError: true })
validateCronExpression(expression, invalid)Regression from
|
[Medium] Service Worker install fetches the same app-shell assets once per language routeStatus: Confirmed on PR head Affected code
Measured build evidenceParsing the seven generated localized pages with the same {
perPage: {
en: 16,
zh-CN: 16,
zh-TW: 16,
ja: 16,
ko: 16,
de: 16,
fr: 16
},
totalPerPageAssets: 112,
globallyUniqueAssets: 16,
duplicateFetchInvocations: 96
}All seven home pages currently reference the same 16 Next.js static assets. The implementation therefore schedules 112 calls to Browser HTTP caching or request coalescing may reduce transferred response bodies in some environments, but it does not make the algorithm correct: the Service Worker still creates duplicate fetch promises and duplicate Expected behaviorEach unique app-shell resource should be requested and cached at most once per Service Worker installation, regardless of how many localized HTML routes reference it. Actual behavior and impact
The issue is more than a performance budget concern because failed installation leaves users on the old worker or without the intended fresh offline shell. Root causeDeduplication scope is local to Recommended fixUse installation-wide deduplication. Two reasonable designs are:
Open the destination caches once per install where practical, and preserve explicit failure handling for genuinely missing required resources. Do not merely swallow asset failures to hide the duplicate-request symptom. Regression coverageAdd a behavior-level Service Worker unit test with mocked
The current tests in Merge assessment: should be fixed in this PR because the duplicate graph is introduced by the new localized app-shell precaching implementation and directly affects PWA installation reliability. |
[Low / Compatibility] SVG generators silently replace valid CSS colors with defaultsStatus: Confirmed on PR head Affected code
Reproduction / confirmed outputsEnter any of these valid SVG/CSS colors into the text field: Direct calls to the current builders produced the following behavior for every value above: <!-- Blob output -->
<path ... fill="#22d3ee" stroke="#0f172a" ... />
<!-- Pattern output -->
<rect ... fill="#020617" />
<circle ... fill="#22d3ee" />The input still displays Regression from
|
[High] Env Parser silently drops parsed variables from every export formatStatus: Confirmed on PR head Affected code
Reproduction / confirmed outputInput: BAD-KEY=no
DATABASE.URL=postgresDirect execution of the current parser/exporters produced: {
"parsed": [
{ "key": "BAD-KEY", "value": "no", "line": 1 },
{ "key": "DATABASE.URL", "value": "postgres", "line": 2 }
],
"uiVariableCount": 2,
"json": "{}",
"yaml": "",
"dockerArgs": ""
}On the page, the header reports 2 variables and the table displays both rows, but selecting JSON produces Regression from
|
[High] The default sample and ordinary Open Graph image URLs can never be previewedStatus: Confirmed on PR head Affected code
Default-state browser reproduction
Fresh Chromium result: {
"image": "https://example.com/og/release-2048.png",
"previewDisabledBeforeConfirmation": true,
"previewDisabledAfterConfirmation": true,
"disabledReason": "Add input before running this action."
}Direct utility execution confirms the internal contradiction: {
"candidatePreviewImage": null,
"metaContainsImage": true,
"imageTag": "<meta property=\"og:image\" content=\"https://example.com/og/release-2048.png\" />"
}Replacing the sample with a normal project/CDN image such as The Instagram rule has another compatibility gap: Expected behaviorThe shipped default/sample must exercise the primary workflow successfully. A user should be able to enter the same valid image URL that is emitted into If the product intentionally supports only a fixed host set, the sample must use a real allowed image and the UI must explicitly report ?unsupported preview host,? including the supported hosts. It must not claim that a populated field is missing input. Actual behavior and impactThe page advertises a live social-card preview, but its initial state, Sample action, and most real-world OG image URLs cannot invoke that feature. Metadata generation still includes those URLs, so users see an image tag in the output while being unable to validate it in the adjacent preview. The generic disabled reason sends them toward editing an already populated field without identifying the actual policy restriction. Root causeThe PR correctly added explicit external-image consent, but coupled it to a platform-specific thumbnail allowlist copied from media tools. That policy does not match a general Open Graph generator, and the sample/default was not updated to satisfy the new rule. Eligibility failure is represented as ?no candidate,? losing the distinction between empty, invalid, insecure, credentialed, unsupported-host, and offline cases. Recommended fixFirst decide and document the intended preview threat model:
In either design, Regression coverageAdd component/browser tests that:
The current component test replaces the default with an allowed fictional YouTube URL before confirming, so it cannot detect the broken default/sample workflow. Merge assessment: blocking for this tool because the PR disables the primary preview workflow in its own default state and for typical user inputs. |
[Medium] Open Graph Preview reports success before the remote image has loadedStatus: Confirmed on PR head Affected code
Browser reproduction / evidence
In fresh Chromium, 100 ms after the click the application had already mounted a success toast containing: At that same moment the image state was: {
"src": "https://i.ytimg.com/robots.txt",
"complete": false,
"naturalWidth": 0,
"naturalHeight": 0
}After the request completed/failed to decode, The same behavior occurs for an allowlisted 404, corrupt image, blocked response, CSP failure, DNS/network interruption after the click, or a URL that returns HTML/text instead of image bytes. Expected behaviorThe action should enter a pending/loading state when the URL is assigned. Success should be announced only from the image's actual Actual behavior and impactThe UI equates ?React accepted a new Root cause
Recommended fixModel the preview lifecycle explicitly, for example
Regression coverageExtend the component test with controlled image events:
Merge assessment: should be fixed in this PR because the new explicit-preview action currently makes a false correctness claim about external network work. |
1 participant
Summary
Change Type
Release Checklist
Commands Run
npm run check:audit:prodnpm run generate:tool-indexnpm run check:tool-indexnpm run generate:client-tool-lookupnpm run check:client-tool-lookupnpm run lintnpm run testnpm run check:typesnpm run validatenpm run buildnpm run test:e2e:smokenpm run test:e2e:pwaRoute Metadata / Export Checklist
Validation Evidence
npm startclean-route probe.Rollback Plan
143b216as one unit, then rebuild so generated registries and the injected Service Worker build ID stay aligned.