Add software installer file size check before upload - #50475
Conversation
Adds max_software_package_size and max_software_package_size_human to GET /config, and checks a selected file against the limit in PackageForm so the add, multi-package add, and edit flows all fail fast with the same wording the server uses.
Drops max_software_package_size_human so GET /config gains only the one new key. PackageForm formats the byte count with formatFileSize, which mirrors installersize.Human and is pinned to it by tests. Also populates the limit on the PATCH /config response, which several pages feed straight into AppContext. Without it the context value went to zero after any config save and the size check stopped firing.
Adds max_software_package_size to EnrichedAppConfig so fleetctl get config --include-server-config reports it, and updates the two golden files. The client silently dropped the unknown key before, so the flag that exists to dump server config was leaving this one out. Also covers the PackageForm size check with tests.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #50475 +/- ##
==========================================
+ Coverage 68.13% 68.30% +0.17%
==========================================
Files 3947 3950 +3
Lines 251501 252686 +1185
Branches 13465 13350 -115
==========================================
+ Hits 171349 172603 +1254
+ Misses 64833 64648 -185
- Partials 15319 15435 +116
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe server includes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx`:
- Around line 222-234: Update the package-size validation in PackageForm to
distinguish a missing limit from a configured zero limit, so
maxSoftwarePackageSize of 0 still rejects any nonempty file before upload.
Preserve the existing error notification and return behavior, and add a test
covering the zero-limit case.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e7d8854-80f3-4e8e-ad82-aaea27be7e79
📒 Files selected for processing (15)
changes/42735-fix-inconsistent-error-messagecmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigJson.jsoncmd/fleetctl/fleetctl/testdata/expectedGetConfigIncludeServerConfigYaml.ymlfrontend/__mocks__/configMock.tsfrontend/interfaces/config.tsfrontend/pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/helpers.tsxfrontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tests.tsxfrontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsxfrontend/utilities/file/fileUtils.tests.tsxfrontend/utilities/file/fileUtils.tsxserver/fleet/app.goserver/fleet/service.goserver/mock/service/service_mock.goserver/service/appconfig.goserver/service/integration_core_test.go
| const file = files[0]; | ||
|
|
||
| // Reject before uploading if file size is too big | ||
| if (maxSoftwarePackageSize && file.size > maxSoftwarePackageSize) { |
There was a problem hiding this comment.
Looks like the server caps the entire request body at this value, not the file size. The multipart body adds boundaries plus the script fields. Does a file at exactly the max still fail after uploading in full? If so, should this compare against a slightly lower margin, and should the test change too?
There was a problem hiding this comment.
Thanks, I see that now. I think it makes sense to try to calculate the size of the whole request.
The only problem I can see is that maybe the error message would be confusing in this case because it's actually the size of the whole request and not just the file, but the error says "maximum file size".
|
|
||
| const DECIMAL_ABBREVIATIONS = ["B", "kB", "MB", "GB", "TB"]; | ||
| const BINARY_ABBREVIATIONS = ["B", "KiB", "MiB", "GiB", "TiB"]; | ||
|
|
There was a problem hiding this comment.
The server's installersize.Human uses go-units tables that run to YB and YiB. These stop at TB and TiB, so the two disagree above 1 PiB. The server returns 1PiB where this returns 1126TB. Worth adding PB/PiB and EB/EiB?
cdcme
left a comment
cdcme
left a comment
There was a problem hiding this comment.
Couple of questions, but looks great!
…e sizes The server caps the whole request body, so the submit handler now measures the installer plus the encoded scripts and the other fields instead of the file on its own. Dropping omitempty means a configured zero reaches clients as a real value rather than a missing key, so the update endpoint sets it too and the browser checks treat zero as a limit instead of as unset.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@frontend/pages/SoftwarePage/components/forms/PackageForm/helpers.tsx`:
- Around line 241-277: The current estimateUploadSize approximation does not
measure the raw multipart request body or encoded byte lengths. In
frontend/pages/SoftwarePage/components/forms/PackageForm/helpers.tsx at lines
241-277, replace it with a shared serialized multipart-body measurement or
proven byte-accurate upper bound covering framing, fields, scripts, labels,
categories, and the file. In
frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx at
lines 284-291, use that shared request-body size when comparing against the
configured limit, before invoking onSubmit.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8bfa9027-7135-44c4-aa01-a7d09135ba46
📒 Files selected for processing (7)
frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tests.tsxfrontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsxfrontend/pages/SoftwarePage/components/forms/PackageForm/helpers.tsxfrontend/utilities/file/fileUtils.tests.tsxfrontend/utilities/file/fileUtils.tsxserver/fleet/app.goserver/service/appconfig.go
🚧 Files skipped from review as they are similar to previous changes (4)
- server/fleet/app.go
- frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tests.tsx
- server/service/appconfig.go
- frontend/utilities/file/fileUtils.tests.tsx
| /** Calculates the size of the payload, because the server limits the whole | ||
| * request and not just the installer file. Not all fields are accounted for | ||
| * in this calculation so if the final payload sent is over the size limit, | ||
| * the server will reject it. | ||
| */ | ||
| export const estimateUploadSize = (formData: IPackageFormData) => { | ||
| const scripts = [ | ||
| formData.installScript, | ||
| formData.uninstallScript, | ||
| formData.preInstallQuery, | ||
| formData.postInstallScript, | ||
| ]; | ||
|
|
||
| // The scripts are base64 encoded on the way out, so encode them to get the | ||
| // length that actually goes over the wire. | ||
| let scriptsSize = 0; | ||
| scripts.forEach((script) => { | ||
| scriptsSize += encodeScriptBase64(script)?.length || 0; | ||
| }); | ||
|
|
||
| // The two flags are sent as "true" or "false". | ||
| let fieldsSize = | ||
| String(formData.selfService).length + | ||
| String(formData.automaticInstall).length; | ||
|
|
||
| formData.categories.forEach((category) => { | ||
| fieldsSize += category.length; | ||
| }); | ||
|
|
||
| // Labels are only sent when the target is Custom, and only the selected ones. | ||
| if (formData.targetType === "Custom") { | ||
| listNamesFromSelectedLabels(formData.labelTargets).forEach((label) => { | ||
| fieldsSize += label.length; | ||
| }); | ||
| } | ||
|
|
||
| return (formData.software?.size || 0) + scriptsSize + fieldsSize; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Measure the multipart body that the upload sends.
The server limits the raw request body before multipart parsing. estimateUploadSize omits multipart framing and fields, and it counts category and label strings by JavaScript code units instead of encoded byte length. A request can pass this check but exceed the server limit, so the user still uploads the file and receives the server error.
frontend/pages/SoftwarePage/components/forms/PackageForm/helpers.tsx#L241-L277: Replace the approximation with a shared serialized multipart-body measurement, or a proven byte-accurate upper bound.frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx#L284-L291: Compare the configured limit against that shared request-body size before callingonSubmit.
📍 Affects 2 files
frontend/pages/SoftwarePage/components/forms/PackageForm/helpers.tsx#L241-L277(this comment)frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx#L284-L291
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/pages/SoftwarePage/components/forms/PackageForm/helpers.tsx` around
lines 241 - 277, The current estimateUploadSize approximation does not measure
the raw multipart request body or encoded byte lengths. In
frontend/pages/SoftwarePage/components/forms/PackageForm/helpers.tsx at lines
241-277, replace it with a shared serialized multipart-body measurement or
proven byte-accurate upper bound covering framing, fields, scripts, labels,
categories, and the file. In
frontend/pages/SoftwarePage/components/forms/PackageForm/PackageForm.tsx at
lines 284-291, use that shared request-body size when comparing against the
configured limit, before invoking onSubmit.
There was a problem hiding this comment.
Decided to only go with estimating the size of the form and not the whole multipart request. It is always more permissive, there should never be a case where the frontend rejects something the backend would accept. The backend check is still there so it's not a big deal.
If we want to improve this in the future, it should probably be done for all endpoints affected by max_software_package_size
fleet/cmd/fleet/http_middleware.go
Lines 43 to 49 in 92aaf1d
There was a problem hiding this comment.
"and it counts category and label strings by JavaScript code units instead of encoded byte length" seems like a valid finding though.
Both are user written, so a name outside ASCII takes more bytes than the string has characters and the estimate came in short.
2 participants
Related issue: Resolves #42735
GET /api/v1/fleet/configendpointChecklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
Summary by CodeRabbit
New Features
Tests