Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changes/42735-fix-inconsistent-error-message
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Added a software package maximum size error message in the UI to fix inconsistent errors across different browsers.
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,7 @@
}
}
},
"max_software_package_size": 537919488,
"gitops": {
"gitops_mode_enabled": false,
"repository_url": "",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ spec:
max_backups: 0
max_size: 500
plugin: filesystem
max_software_package_size: 537919488
org_info:
org_logo_url: ""
org_logo_url_light_background: ""
Expand Down
1 change: 1 addition & 0 deletions frontend/__mocks__/configMock.ts
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ const DEFAULT_CONFIG_MOCK: IConfig = {
secrets: true,
},
},
max_software_package_size: 10 * 1024 * 1024 * 1024,
};

export const createMockConfig = (overrides?: Partial<IConfig>): IConfig => {
Expand Down
1 change: 1 addition & 0 deletions frontend/interfaces/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,7 @@ export interface IConfig {
mdm: IMdmConfig;
gitops: IGitOpsModeConfig;
partnerships?: IFleetPartnerships;
max_software_package_size: number;
Comment thread
cdcme marked this conversation as resolved.
}

interface IFleetPartnerships {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import {
} from "pages/SoftwarePage/helpers";
import { ensurePeriod } from "pages/SoftwarePage/SoftwareAddPage/helpers";

const EDIT_SOFTWARE_ERROR_PREFIX = "Couldn't edit software.";
export const EDIT_SOFTWARE_ERROR_PREFIX = "Couldn't edit software.";
const DEFAULT_ERROR_MESSAGE = `${EDIT_SOFTWARE_ERROR_PREFIX} Please try again.`;

// eslint-disable-next-line import/prefer-default-export
Expand Down
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import React from "react";
import { screen } from "@testing-library/react";
import userEvent from "@testing-library/user-event";

import { createCustomRenderer } from "test/test-utils";
import { createMockSoftwarePackage } from "__mocks__/softwareMock";
import { notify } from "components/ToastNotification";
import { IConfig } from "interfaces/config";

import PackageForm from "./PackageForm";

Expand All @@ -14,20 +17,33 @@ const BASE_PROPS = {
};

const renderForm = (
overrides: Partial<React.ComponentProps<typeof PackageForm>> = {}
overrides: Partial<React.ComponentProps<typeof PackageForm>> = {},
config?: Partial<IConfig>
) => {
const render = createCustomRenderer({
withBackendMock: true,
context: {
app: {
isPremiumTier: true,
isGlobalAdmin: true,
config,
},
},
});
return render(<PackageForm {...BASE_PROPS} {...overrides} />);
};

const ONE_GIB = 1024 * 1024 * 1024;

// The form reads File.size, so fake the size rather than allocating a real
// multi-gigabyte buffer.
const selectFileOfSize = async (container: HTMLElement, size: number) => {
const file = new File(["installer"], "test.pkg");
Object.defineProperty(file, "size", { value: size });
const input = container.querySelector("#upload-file") as HTMLInputElement;
await userEvent.upload(input, file);
};

const TARGET_BANNER_COPY = /If multiple packages of the same software target the same host, Fleet will install the one that was added first\./i;

describe("PackageForm", () => {
Expand Down Expand Up @@ -63,4 +79,53 @@ describe("PackageForm", () => {
expect(screen.getByLabelText("Custom")).toBeInTheDocument();
});
});

describe("Maximum package size", () => {
afterEach(() => {
jest.restoreAllMocks();
});

it("rejects a package over the configured maximum before uploading", async () => {
const errorSpy = jest.spyOn(notify, "error");
const { container } = renderForm(
{},
{ max_software_package_size: ONE_GIB }
);

await selectFileOfSize(container, ONE_GIB + 1);

expect(errorSpy).toHaveBeenCalledWith(
"Couldn't add. The maximum file size is 1GiB."
);
// The rejected file never reaches form state, so the Target section
// stays hidden.
expect(screen.queryByLabelText("All hosts")).not.toBeInTheDocument();
});

it("rejects any package when the limit is zero", async () => {
// A zero limit is a real setting, not a missing one, and the server
// refuses every upload under it.
const errorSpy = jest.spyOn(notify, "error");
const { container } = renderForm({}, { max_software_package_size: 0 });

await selectFileOfSize(container, 1);

expect(errorSpy).toHaveBeenCalledWith(
"Couldn't add. The maximum file size is 0B."
);
});

it("accepts a package at the configured maximum", async () => {
const errorSpy = jest.spyOn(notify, "error");
const { container } = renderForm(
{},
{ max_software_package_size: ONE_GIB }
);

await selectFileOfSize(container, ONE_GIB);

expect(errorSpy).not.toHaveBeenCalled();
expect(screen.getByLabelText("All hosts")).toBeInTheDocument();
});
});
});
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
// Used in AddPackageModal.tsx and EditSoftwareModal.tsx
import React, { useState, useEffect, useCallback } from "react";
import React, { useState, useEffect, useCallback, useContext } from "react";
import classnames from "classnames";

import { AppContext } from "context/app";
import useGitOpsMode from "hooks/useGitOpsMode";
import { LEARN_MORE_ABOUT_BASE_LINK } from "utilities/constants";
import {
formatFileSize,
getExtensionFromFileName,
getFileDetails,
} from "utilities/file/fileUtils";
Expand Down Expand Up @@ -33,11 +35,17 @@ import {
import { DropdownTargetLabelSelector } from "components/TargetLabelSelector";
import SoftwareOptionsSelector from "pages/SoftwarePage/components/forms/SoftwareOptionsSelector";
import { GitOpsCustomPackageBanner } from "pages/SoftwarePage/SoftwareAddPage/SoftwareCustomPackage/SoftwareCustomPackage";
import { ADD_SOFTWARE_ERROR_PREFIX } from "pages/SoftwarePage/SoftwareAddPage/helpers";
import { EDIT_SOFTWARE_ERROR_PREFIX } from "pages/SoftwarePage/SoftwareTitleDetailsPage/EditSoftwareModal/helpers";
import InfoBanner from "components/InfoBanner";
import CustomLink from "components/CustomLink";

import PackageAdvancedOptions from "../PackageAdvancedOptions";
import { createTooltipContent, generateFormValidation } from "./helpers";
import {
createTooltipContent,
estimateUploadSize,
generateFormValidation,
} from "./helpers";
import SoftwareDeploySlider from "../SoftwareDeploySelector";

export const baseClass = "package-form";
Expand Down Expand Up @@ -181,6 +189,8 @@ const PackageForm = ({
initialTargetType,
}: IPackageFormProps) => {
const { gitOpsModeEnabled, repoURL } = useGitOpsMode("software");
const { config } = useContext(AppContext);
const maxSoftwarePackageSize = config?.max_software_package_size;

const initialFormData: IPackageFormData = {
// `formData.software` is typed as `File | null` (its shape once a user
Expand Down Expand Up @@ -209,10 +219,30 @@ const PackageForm = ({
software: { isValid: false },
});

const notifyTooLarge = () => {
const errorPrefix = isEditingSoftware
? EDIT_SOFTWARE_ERROR_PREFIX
: ADD_SOFTWARE_ERROR_PREFIX;
notify.error(
`${errorPrefix} The maximum file size is ${formatFileSize(
maxSoftwarePackageSize || 0
)}.`
);
};

const onFileSelect = (files: FileList | null) => {
if (files && files.length > 0) {
const file = files[0];

// Reject before uploading if file size is too big
if (
maxSoftwarePackageSize !== undefined &&
file.size > maxSoftwarePackageSize
) {
notifyTooLarge();
return;
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
// Only populate default install/uninstall scripts when adding (but not editing) software
if (isEditingSoftware) {
const newData = { ...formData, software: file };
Expand Down Expand Up @@ -250,6 +280,16 @@ const PackageForm = ({

const onFormSubmit = (evt: React.FormEvent<HTMLFormElement>) => {
evt.preventDefault();

// The server caps the whole request body, and not just the file.
if (
maxSoftwarePackageSize !== undefined &&
estimateUploadSize(formData) > maxSoftwarePackageSize
) {
notifyTooLarge();
return;
}

onSubmit(formData);
};

Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import React from "react";

import { validateQuery } from "components/forms/validators/validate_query";
import { listNamesFromSelectedLabels } from "services/entities/labels";
import { getExtensionFromFileName } from "utilities/file/fileUtils";
import { encodeScriptBase64 } from "utilities/scripts_encoding";
import { getGitOpsModeTipContent } from "utilities/helpers";
import { IPackageFormData, IPackageFormValidation } from "./PackageForm";

Expand Down Expand Up @@ -236,4 +238,47 @@ export const createTooltipContent = (
);
};

/** 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;

// Names are user written, so count bytes. Anything outside ASCII takes more
// than one.
const encoder = new TextEncoder();

formData.categories.forEach((category) => {
fieldsSize += encoder.encode(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 += encoder.encode(label).length;
});
}

return (formData.software?.size || 0) + scriptsSize + fieldsSize;
Comment on lines +241 to +281

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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 calling onSubmit.
📍 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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (req.Method == http.MethodPost && strings.HasSuffix(req.URL.Path, "/fleet/software/package")) ||
(req.Method == http.MethodPatch && strings.HasSuffix(req.URL.Path, "/package") && strings.Contains(req.URL.Path,
"/fleet/software/titles/")) ||
(req.Method == http.MethodPost && strings.HasSuffix(req.URL.Path, "/bootstrap")) ||
(req.Method == http.MethodPost && strings.HasSuffix(req.URL.Path, "/fleet_maintained_apps")) ||
(req.Method == http.MethodGet && strings.Contains(req.URL.Path, "/package/token")) ||
(req.Method == http.MethodPost && strings.Contains(req.URL.Path, "orbit/software_install/package")) {

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"and it counts category and label strings by JavaScript code units instead of encoded byte length" seems like a valid finding though.

};

export default generateFormValidation;
27 changes: 27 additions & 0 deletions frontend/utilities/file/fileUtils.tests.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import {
formatFileSize,
getExtensionFromFileName,
getFileDetails,
getPlatformDisplayName,
Expand Down Expand Up @@ -120,4 +121,30 @@ describe("fileUtils", () => {
description: "macOS",
});
});

describe("fileUtils - formatFileSize", () => {
// Expectations verified against the server's installersize.Human, which is
// what writes the same limit into its own too-large error
const testCases = [
{ bytes: 0, expectedSize: "0B" },
{ bytes: 999, expectedSize: "999B" },
{ bytes: 1000, expectedSize: "1kB" },
{ bytes: 1024, expectedSize: "1KiB" },
{ bytes: 1000000, expectedSize: "1MB" },
{ bytes: 1048576, expectedSize: "1MiB" },
{ bytes: 536870912, expectedSize: "512MiB" },
{ bytes: 1073741824, expectedSize: "1GiB" },
{ bytes: 1500000000, expectedSize: "1.5GB" },
{ bytes: 10737418240, expectedSize: "10GiB" },
{ bytes: 5497558138880, expectedSize: "5TiB" },
{ bytes: 1000000000000000, expectedSize: "1PB" },
{ bytes: 1125899906842624, expectedSize: "1PiB" },
];

testCases.forEach(({ bytes, expectedSize }) => {
it(`should return "${expectedSize}" for ${bytes} bytes`, () => {
expect(formatFileSize(bytes)).toEqual(expectedSize);
});
});
});
});
48 changes: 48 additions & 0 deletions frontend/utilities/file/fileUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -102,3 +102,51 @@ export interface IFileDetails {
name: string;
description?: React.ReactNode;
}

// Both tables match the ones go-units gives the server, so the two agree at
// every magnitude rather than only up to terabytes.
const DECIMAL_ABBREVIATIONS = [
"B",
"kB",
"MB",
"GB",
"TB",
"PB",
"EB",
"ZB",
"YB",
];
const BINARY_ABBREVIATIONS = [
"B",
"KiB",
"MiB",
"GiB",
"TiB",
"PiB",
"EiB",
"ZiB",
"YiB",
];

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

const formatWithBase = (
bytes: number,
base: number,
abbreviations: string[]
) => {
let size = bytes;
let abbreviationIndex = 0;
while (size >= base && abbreviationIndex < abbreviations.length - 1) {
size /= base;
abbreviationIndex += 1;
}
// 4 significant digits with trailing zeros dropped, matching Go's "%.4g"
return `${Number(size.toPrecision(4))}${abbreviations[abbreviationIndex]}`;
};

// Returns a human readable size, like the server's installersize.Human function
export const formatFileSize = (bytes: number) => {
const decimal = formatWithBase(bytes, 1000, DECIMAL_ABBREVIATIONS);
const binary = formatWithBase(bytes, 1024, BINARY_ABBREVIATIONS);

return binary.length < decimal.length ? binary : decimal;
};
11 changes: 6 additions & 5 deletions server/fleet/app.go
Original file line number Diff line number Diff line change
Expand Up @@ -1054,11 +1054,12 @@ type EnrichedAppConfig struct {

// enrichedAppConfigFields are grouped separately to aid with JSON unmarshaling
type enrichedAppConfigFields struct {
UpdateInterval *UpdateIntervalConfig `json:"update_interval,omitempty"`
Vulnerabilities *VulnerabilitiesConfig `json:"vulnerabilities,omitempty"`
License *LicenseInfo `json:"license,omitempty"`
Logging *Logging `json:"logging,omitempty"`
Email *EmailConfig `json:"email,omitempty"`
UpdateInterval *UpdateIntervalConfig `json:"update_interval,omitempty"`
Vulnerabilities *VulnerabilitiesConfig `json:"vulnerabilities,omitempty"`
License *LicenseInfo `json:"license,omitempty"`
Logging *Logging `json:"logging,omitempty"`
Email *EmailConfig `json:"email,omitempty"`
MaxSoftwarePackageSize int64 `json:"max_software_package_size"`
}

// UnmarshalJSON implements the json.Unmarshaler interface to make sure we serialize
Expand Down
Loading
Loading