Fix Windows 0-byte Tailwind binary downloads & integrity checks - #133
Conversation
|
Thanks for recreating it! 👍🏼 I see we need to fix some merge conflicts here. Could you proceed with it? |
|
@bocharsky-bw Thanks for the heads up! Merged
Should be conflict-free and ready for another look. Let me know if anything needs adjusting! 🙂 |
There was a problem hiding this comment.
Hey @Taminoful, thanks for this PR!
It seems to me this PR is doing two things:
- detecting and failing when a 0-byte binary is downloaded
- integrity checking the downloads
Can we simplify this PR by just focusing on 1 - the bug fix? Just fail if the downloaded binary is 0 bytes. If the user is on windows, we could adjust the error message to hint at the possible problem as described.
The integrity check is a cool feature but could we do that in a follow-up PR? I think this can be simplified also. For instance, there's a sha256sums.txt attached with every release which would, I think save on API calls.
|
Hello @kbond, Thanks for the review! On splitting the PR, I don't think that buys us much here. The 0-byte check and the integrity check aren't really two separate things, a 0-byte file is just one symptom of the same corrupted-download problem from #115. A partial download from the same AV-quarantine scenario would just fall through a Regarding the That said, there's a solid case for keeping the API too. It gives us the More on my reasoning in 57f27d8 and 962b30b.
The GitHub API gives us the full asset list in one shot, so we're iterating over that instead of fetching assets one by one, the content-type check is what filters Personally I find it adds unnecessary noise to the function for legacy versions that most likely aren't used for new projects. The goal was to keep the PR as clean as possible while making the entire download process more robust and easier to manage than it was beforehand. I will go ahead and snip the |
Fair enough, but I feel the solution here is too complex. I don't think the model classes are required. I'd prefer to just do the check right in the existing
We've been having issues with this in the CI, yes. |
Adds some model classes that represent the important parts of the GitHub tag endpoint data that makes it easier to rework the download process. Also allows for easy extension if more fields should become relevant in the future. `TailwindBinaries` represents a release of a version which holds the downloadable assets. The class also contains a helper function as part of it's model too which allows to search for assets of a release by their tag name. `TailwindBinary` represents the details of each executable that is pushed to GitHub as part of a release. Important to note is, that the digest field only gets filled after Tailwind v4.1.9 but gets filled after, since, realistically people will use v4 from now on more than v3, I decided to not make the field nullable or go the extra route of comparing against the contents of the `sha256sums.txt`. This approach should keep the code more clean going forward as each binary has their digest attached directly as a field.
This commit changes the download method to use the GitHub API endpoint instead. It's requesting the information of the API about the tag and temporarily saves it in the `Model` classes for further use. From there the actual file gets downloaded over the API provided link. The commit also starts using the `Path::canonicalize()` method to eliminate any potential pathing issues. This does have the downside of sending two requests instead of one but allows for a more robust download process. E.g., it's now possible to check the file integrity with the provided SHA256 hash to determine if the file got corrupted as described in SymfonyCasts#115. In the future this change also allows for removing any hardcoded lists within the code that contain the platform executable names, as it's possible to just get the list off GitHub, which helps in maintaining if TailwindLabs decides to build for other platforms or removes platforms from their builds in the future. The main functionality for this lives in `requestBinariesByVersion()` which I might move to the `TailwindBinary` Model during cleanup, depending on where it feels right.
…asts#115) - Detect and delete 0-byte files before re-downloading, fixing the core Windows antivirus interference bug where a corrupt file blocked recovery - Validate SHA256 digest after download; delete file and throw a clear RuntimeException on mismatch so the next run triggers a clean retry - Skip integrity check for versions <=4.1.9 where no digest is available - Replace dd() debug call with a RuntimeException listing available assets - Fix double "Expected file hash" label (second was the actual hash) - Fix awkward TailwindBinaries construction (build assets array first, pass to constructor directly, removing the setAssets() workaround) - Simplify model classes: make properties readonly, remove unused setters, rename getFileSize() to getSize(), add return type to getAssetByBinaryName() - Remove dead downloadExecutableOld() method - Add tailwindcss-linux-armv7 to mock fixture so armv7 test case resolves - Add tests for 0-byte re-download and integrity failure scenarios
Replace the `api.github.com` release-metadata call with a fetch of `sha256sums.txt` from the release download host. The API endpoint is rate limited (60/hour, shared by IP) and was reintroducing the CI throttling the `ScopingHttpClient` auth work removed; `sha256sums.txt` is served from the same non-rate-limited host as the binary itself. The expected hash is now looked up inline in `TailwindBinary`, so the `Model\TailwindBinaries` / `Model\TailwindBinary` DTOs are removed. The check is best-effort: when `sha256sums.txt` or the binary's entry is unavailable (older releases) the check is skipped and the download still succeeds; only a genuine hash mismatch deletes the file and throws. This drops the hardcoded `4.1.9` version gate. Also revert the unrelated `Path::canonicalize` hardening (which pulled `symfony/filesystem` into `src`) and the `\Exception` type change.
Throw a descriptive exception when a download produces a 0-byte file (a common Windows antivirus symptom, see SymfonyCasts#115) instead of retrying silently. Drop the now-redundant unlink in `getBinaryPath()` since `downloadExecutable()`'s `fopen(..., 'w')` truncates any leftover file.
|
Thanks @Taminoful! |
3 participants
This PR fixes an issue on Windows where TailwindCSS binaries could be downloaded as empty (0-byte) executables due to antivirus interference, leading to SmartScreen errors and failed command execution. A detailed description of the issues occuring can be found in #115.
In some environments (notably with Windows Defender or third-party antivirus), the downloaded executable may be blocked or quarantined during download. This results in a 0-byte file being saved, which the bundle incorrectly treats as a valid binary. Subsequent execution attempts then fail with Windows SmartScreen / access denied errors.
What's Fixed
Improvements
Impact
Recreated from #121, which GitHub won't allow reopening since its base branch (
main) was renamed to1.x(see #121 (comment)).