diff --git a/README.md b/README.md index b587c0c..7f5765d 100644 --- a/README.md +++ b/README.md @@ -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 diff --git a/src/common.ts b/src/common.ts index 0884c88..ac80136 100644 --- a/src/common.ts +++ b/src/common.ts @@ -337,6 +337,28 @@ export function getValidMetas(metas: Partial 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 diff --git a/src/ghw_get_changed_ota_files.ts b/src/ghw_get_changed_ota_files.ts index d917b3c..a3b6649 100644 --- a/src/ghw_get_changed_ota_files.ts +++ b/src/ghw_get_changed_ota_files.ts @@ -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) { diff --git a/tests/ghw_check_ota_pr.test.ts b/tests/ghw_check_ota_pr.test.ts index 81b90bc..1252b22 100644 --- a/tests/ghw_check_ota_pr.test.ts +++ b/tests/ghw_check_ota_pr.test.ts @@ -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]); + }); }); diff --git a/tests/process_firmware_image.test.ts b/tests/process_firmware_image.test.ts index 472e204..a9baa3b 100644 --- a/tests/process_firmware_image.test.ts +++ b/tests/process_firmware_image.test.ts @@ -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"]});