github/zigbee-OTA
2
0
Fork 0
mirror of https://github.com/Koenkk/zigbee-OTA.git synced 2026-10-07 19:45:12 +00:00
Code Issues Packages Projects Releases Wiki Activity

Reject in-place image replacements and same-name overwrites (#1328)

Browse Source
This commit is contained in:
TheJulianJES
2026-10-06 20:10:58 +02:00
committed by GitHub
parent 663cf231ba
commit 10797024f0
5 changed files with 192 additions and 4 deletions
Download Patch File Download Diff File Expand all files Collapse all files
README.md
+3 -1
View File
@@ -13,7 +13,7 @@ Create a pull request with the image(s) in their proper subdirectories (manufact
The pull request automation will validate the image. If any error occur, a comment will be posted in the pull request. If the validation succeed, a comment will be posted to inform of the changes that merging the pull request will commit (in a following commit).
> [!IMPORTANT]
> Do NOT delete current images and do NOT submit images in `images1` directory. The pull request automation will take care of archiving as appropriate for each file.
> Do NOT delete, rename or replace current images and do NOT submit images in `images1` directory. The pull request automation will take care of archiving as appropriate for each file.
### Example using Github
@@ -119,6 +119,8 @@ Example:
If a manual modification of the manifests is necessary, it should be done in a PR that does not trigger the `update_ota_pr` workflow (no changes in `images/**` directory). As a last resort, the label `ignore-ota-workflow` can be added to prevent the workflow from running.
A file name must identify a single image: the workflows refuse to add or archive an image where the target manifest already has a different image (by `sha512`) under the same file name, and refuse PRs that modify, rename or delete existing images. Auto download may legitimately re-download a newer version under the same file name, since it archives the current file before writing the new one.
The metadata structure for images is as below (see above for details on extra metas):
```typescript
src/common.ts
+41 -2
View File
@@ -337,6 +337,28 @@ export function getValidMetas(metas: Partial<ExtraMetas & ExtraMetasWithFileName
return validMetas;
}
/**
* Throw if the manifest has a different image (by sha512) under the given file name (for the given manufacturer, in the given images dir).
* Adding/archiving an image under that file name would overwrite its binary while leaving its manifest entry (sha512, fileVersion...) behind.
* @param ignoredImage the entry about to be removed (replaced by the image being added/archived), if any
*/
function assertNoFileNameConflict(
manifest: RepoImageMeta[],
manufacturer: string,
fileName: string,
imagesDir: string,
sha512: string,
ignoredImage: RepoImageMeta | undefined,
): void {
const url = getRepoFirmwareFileUrl(manufacturer, fileName, imagesDir);
if (manifest.some((i) => i !== ignoredImage && i.url === url && i.sha512 !== sha512)) {
throw new Error(
`${imagesDir === PREV_IMAGES_DIR ? "Prev" : "Base"} manifest already has a different image with file name '${fileName}' for '${manufacturer}'. Cannot add/archive without overwriting it, use a different file name`,
);
}
}
export function addImageToPrev(
logPrefix: string,
isNewer: boolean,
@@ -371,6 +393,9 @@ export function addImageToPrev(
throw new Error("Image already present for manufacturer");
}
// check before removing anything, so a failure leaves files and manifests untouched
assertNoFileNameConflict(prevManifest, manufacturer, firmwareFileName, PREV_IMAGES_DIR, newMetas.sha512, isNewer ? prevMatch : undefined);
if (isNewer) {
console.log(`${logPrefix} Removing prev image.`);
prevManifest.splice(prevMatchIndex, 1);
@@ -419,6 +444,9 @@ export function addImageToBase(
throw new Error("Image already present for manufacturer");
}
// check before removing anything, so a failure leaves files and manifests untouched
assertNoFileNameConflict(baseManifest, manufacturer, firmwareFileName, BASE_IMAGES_DIR, newMetas.sha512, isNewer ? baseMatch : undefined);
if (isNewer) {
console.log(`${logPrefix} Base manifest has older version ${baseMatch.fileVersion}. Replacing with ${parsedImage.fileVersion}.`);
@@ -429,6 +457,19 @@ export function addImageToBase(
console.warn(`${logPrefix} Base image is new/newer but prev image is not older/non-existing.`);
}
// make sure fileName exists for migration from old system
const baseFileName = baseMatch.fileName ? baseMatch.fileName : baseMatch.url.split("/").pop()!;
// check before removing anything, so a failure leaves files and manifests untouched
assertNoFileNameConflict(
prevManifest,
manufacturer,
baseFileName,
PREV_IMAGES_DIR,
baseMatch.sha512,
prevStatus !== ParsedImageStatus.New ? prevMatch : undefined,
);
if (prevStatus !== ParsedImageStatus.New) {
console.log(`${logPrefix} Removing prev image.`);
prevManifest.splice(prevMatchIndex, 1);
@@ -440,8 +481,6 @@ export function addImageToBase(
}
// relocate base to prev
// make sure fileName exists for migration from old system
const baseFileName = baseMatch.fileName ? baseMatch.fileName : baseMatch.url.split("/").pop()!;
const baseFilePath = path.join(baseOutDir, baseFileName);
// if for some reason the file is no longer present (should not happen), don't add it to prev since link is broken
src/ghw_get_changed_ota_files.ts
+16
View File
@@ -24,6 +24,22 @@ export async function getChangedOtaFiles(
core.info(`Changed files: ${compare.data.files.map((f) => f.filename).join(", ")}`);
const fileList = compare.data.files.filter((f) => f.filename.startsWith(`${BASE_IMAGES_DIR}/`));
// existing images must not be replaced in place (same file name for a different image), renamed or deleted:
// the manifest entry would keep pointing at the old file name with the old sha512/version, and archiving would move the wrong binary
const alteredFiles = fileList.filter(
(f) =>
f.status === "modified" ||
f.status === "removed" ||
f.status === "changed" ||
// a rename from outside `images` (e.g. a retracted image added back) is a new image
(f.status === "renamed" && f.previous_filename?.startsWith(`${BASE_IMAGES_DIR}/`)),
);
if (alteredFiles.length > 0) {
throw new Error(
`Detected modified/renamed/deleted existing images: ${alteredFiles.map((f) => `${f.filename} (${f.status})`).join(", ")}. Existing images must not be altered, add new images under a different file name instead.`,
);
}
if (throwIfFilesOutsideOfImages && fileList.length !== compare.data.files.length) {
if (context.payload.pull_request) {
tests/ghw_check_ota_pr.test.ts
+115 -1
View File
@@ -1,4 +1,4 @@
import {existsSync, readFileSync, rmSync, writeFileSync} from "node:fs";
import {copyFileSync, existsSync, mkdirSync, readFileSync, rmSync, writeFileSync} from "node:fs";
import path from "node:path";
import type * as CoreApi from "@actions/core";
import type {Octokit} from "@octokit/rest";
@@ -9,6 +9,7 @@ import type {Context, RepoImageMeta} from "../src/types.js";
import {
BASE_IMAGES_TEST_DIR_PATH,
getAdjustedContent,
getImageOriginalDirPath,
IMAGE_GLEDOPTO,
IMAGE_INVALID,
IMAGE_LUMI,
@@ -798,4 +799,117 @@ Text after end tag`);
}).rejects.toThrow(expect.objectContaining({message: expect.stringContaining(error)}));
}
});
it("failure archiving base image over different prev image with same file name", async () => {
// base v13 and prev v12 share the same file name, prev v12 is restricted so it does not match base v13
const prevSameName = withExtraMetas(IMAGE_V12_1_METAS, {
// @ts-expect-error override
fileName: IMAGE_V13_1,
url: `${common.BASE_REPO_URL}${common.REPO_BRANCH}/${common.PREV_IMAGES_DIR}/${IMAGES_TEST_DIR}/${IMAGE_V13_1}`,
maxFileVersion: 11,
});
setManifest(common.BASE_INDEX_MANIFEST_FILENAME, [structuredClone(IMAGE_V13_1_METAS_MAIN)]);
setManifest(common.PREV_INDEX_MANIFEST_FILENAME, [structuredClone(prevSameName)]);
useImage(IMAGE_V13_1);
mkdirSync(PREV_IMAGES_TEST_DIR_PATH, {recursive: true});
copyFileSync(getImageOriginalDirPath(IMAGE_V12_1), path.join(PREV_IMAGES_TEST_DIR_PATH, IMAGE_V13_1));
// PR adds v14, which archives base v13 to prev under the file name of prev v12
filePaths = [useImage(IMAGE_V14_1)];
await expect(async () => {
// @ts-expect-error mock
await checkOtaPR(github, core, context);
}).rejects.toThrow(
expect.objectContaining({
message: expect.stringContaining(`Prev manifest already has a different image with file name '${IMAGE_V13_1}'`),
}),
);
expect(writeManifestSpy).toHaveBeenCalledTimes(0);
// prev v12 binary must not have been overwritten by base v13
expect(common.computeSHA512(readFileSync(path.join(PREV_IMAGES_TEST_DIR_PATH, IMAGE_V13_1)))).toStrictEqual(IMAGE_V12_1_METAS.sha512);
});
it("failure archiving base image over prev image with same file name leaves files untouched", async () => {
// prev has a matching v12 entry (to be removed) and a conflicting restricted entry under the same file name (corrupt state)
const prevUrl = `${common.BASE_REPO_URL}${common.REPO_BRANCH}/${common.PREV_IMAGES_DIR}/${IMAGES_TEST_DIR}/${IMAGE_V13_1}`;
// @ts-expect-error override
const prevMatching = withExtraMetas(IMAGE_V12_1_METAS, {fileName: IMAGE_V13_1, url: prevUrl});
// @ts-expect-error override
const prevConflicting = withExtraMetas(IMAGE_V12_1_METAS, {fileName: IMAGE_V13_1, url: prevUrl, sha512: "other", maxFileVersion: 11});
setManifest(common.BASE_INDEX_MANIFEST_FILENAME, [structuredClone(IMAGE_V13_1_METAS_MAIN)]);
setManifest(common.PREV_INDEX_MANIFEST_FILENAME, [prevMatching, prevConflicting]);
useImage(IMAGE_V13_1);
mkdirSync(PREV_IMAGES_TEST_DIR_PATH, {recursive: true});
copyFileSync(getImageOriginalDirPath(IMAGE_V12_1), path.join(PREV_IMAGES_TEST_DIR_PATH, IMAGE_V13_1));
filePaths = [useImage(IMAGE_V14_1)];
await expect(async () => {
// @ts-expect-error mock
await checkOtaPR(github, core, context);
}).rejects.toThrow(
expect.objectContaining({
message: expect.stringContaining(`Prev manifest already has a different image with file name '${IMAGE_V13_1}'`),
}),
);
expect(writeManifestSpy).toHaveBeenCalledTimes(0);
expect(existsSync(path.join(PREV_IMAGES_TEST_DIR_PATH, IMAGE_V13_1))).toStrictEqual(true);
expect(existsSync(path.join(BASE_IMAGES_TEST_DIR_PATH, IMAGE_V13_1))).toStrictEqual(true);
});
it("success archiving base image when prev has the same binary under the same file name for another modelId", async () => {
// same file declared for two models in base, model_a already upgraded (its v13 entry archived to prev, same binary)
const baseA = withExtraMetas(IMAGE_V13_1_METAS_MAIN, {modelId: "model_a"});
const baseB = withExtraMetas(IMAGE_V13_1_METAS_MAIN, {modelId: "model_b"});
const prevA = withExtraMetas(IMAGE_V13_1_METAS, {modelId: "model_a"});
setManifest(common.BASE_INDEX_MANIFEST_FILENAME, [baseA, baseB]);
setManifest(common.PREV_INDEX_MANIFEST_FILENAME, [prevA]);
useImage(IMAGE_V13_1);
useImage(IMAGE_V13_1, PREV_IMAGES_TEST_DIR_PATH);
filePaths = [useImage(IMAGE_V14_1)];
const newContext = withBody(`\`\`\`json [{"fileName": "${IMAGE_V14_1}", "modelId": "model_b"}] \`\`\``);
// @ts-expect-error mock
await checkOtaPR(github, core, newContext);
expect(writeManifestSpy).toHaveBeenCalledWith(common.BASE_INDEX_MANIFEST_FILENAME, [
baseA,
withExtraMetas(IMAGE_V14_1_METAS, {modelId: "model_b"}),
]);
expect(writeManifestSpy).toHaveBeenCalledWith(common.PREV_INDEX_MANIFEST_FILENAME, [
prevA,
withExtraMetas(IMAGE_V13_1_METAS, {modelId: "model_b"}),
]);
});
it.each([["modified"], ["renamed"], ["removed"]])("failure with existing image %s in PR", async (status) => {
setManifest(common.BASE_INDEX_MANIFEST_FILENAME, [structuredClone(IMAGE_V13_1_METAS_MAIN)]);
filePaths = [
useImage(IMAGE_V14_1),
{...useImage(IMAGE_V13_1), status, previous_filename: `${BASE_IMAGES_TEST_DIR_PATH}/renamed-${IMAGE_V13_1}`},
];
await expect(async () => {
// @ts-expect-error mock
await checkOtaPR(github, core, context);
}).rejects.toThrow(
expect.objectContaining({
message: expect.stringContaining(
`Detected modified/renamed/deleted existing images: ${BASE_IMAGES_TEST_DIR_PATH}/${IMAGE_V13_1} (${status})`,
),
}),
);
expectNoChanges(false);
});
it("success with image renamed from outside images directory in PR", async () => {
filePaths = [{...useImage(IMAGE_V14_1), status: "renamed", previous_filename: `retracted-images/${IMAGE_V14_1}`}];
// @ts-expect-error mock
await checkOtaPR(github, core, context);
expect(addImageToBaseSpy).toHaveBeenCalledTimes(1);
expect(writeManifestSpy).toHaveBeenCalledWith(common.BASE_INDEX_MANIFEST_FILENAME, [IMAGE_V14_1_METAS]);
});
});
tests/process_firmware_image.test.ts
+17
View File
@@ -1,4 +1,5 @@
import {existsSync, mkdirSync, readFileSync, rmSync} from "node:fs";
import path from "node:path";
import {afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, type MockInstance, vi} from "vitest";
import * as common from "../src/common.js";
import {ProcessFirmwareImageStatus, processFirmwareImage} from "../src/process_firmware_image.js";
@@ -306,6 +307,22 @@ describe("Process Firmware Image", () => {
expect(writeManifestSpy).toHaveBeenCalledWith(common.PREV_INDEX_MANIFEST_FILENAME, []);
});
it("failure adding image with same file name as a different base image", async () => {
// existing base image declared for model_a, a different binary is downloaded under the same file name for model_b (not a match, so not an upgrade)
setManifest(common.BASE_INDEX_MANIFEST_FILENAME, [withExtraMetas(withOriginalUrl(IMAGE_V13_1, IMAGE_V13_1_METAS), {modelId: "model_a"})]);
useImage(IMAGE_V13_1);
const status = await processFirmwareImage(IMAGES_TEST_DIR, IMAGE_V13_1, IMAGE_V14_1, {modelId: "model_b"});
expect(status).toStrictEqual(ProcessFirmwareImageStatus.Error);
expect(consoleErrorSpy).toHaveBeenCalledWith(
expect.stringContaining(`Base manifest already has a different image with file name '${IMAGE_V13_1}'`),
);
expect(writeManifestSpy).toHaveBeenCalledTimes(0);
// existing binary untouched
expect(common.computeSHA512(readFileSync(path.join(BASE_IMAGES_TEST_DIR_PATH, IMAGE_V13_1)))).toStrictEqual(IMAGE_V13_1_METAS.sha512);
});
it("success with extra metas", async () => {
const status = await processFirmwareImage(IMAGES_TEST_DIR, IMAGE_V14_1, IMAGE_V14_1, {manufacturerName: ["lixee"]});