From 245fe8eccec796866b412180e9d1a4954078a41a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Oliv=C3=A9r=20Falvai?= Date: Tue, 22 Sep 2026 13:08:12 +0200 Subject: [PATCH] Reject patchedFiles.patch values outside __hcp_patches/ manifestFromJSON: (iOS) and parseDiffManifest (Android) validated only that `patch` was a non-empty string, not that it stayed under the reserved patches folder prefix. A manifest pointing `patch` elsewhere left a stray file behind after install (the cleanup step only deletes __hcp_patches by name), which verifyFolderHash still caught, but as a generic integrity-check failure instead of a clear manifest error. RA-4925 Co-Authored-By: Claude Sonnet 5 --- .../react/diffpatch/BinaryDiffPatcher.kt | 5 ++- .../codepush/react/diffpatch/DiffManifest.kt | 8 +++- .../react/diffpatch/BinaryDiffPatcherTest.kt | 30 +++++++++++++++ .../react/diffpatch/DiffManifestTest.kt | 38 +++++++++++++++++++ ios/CodePush/CodePushDiffManifest.h | 3 ++ ios/CodePush/CodePushDiffManifest.m | 11 +++++- ios/CodePush/CodePushPackage.m | 4 +- .../CodePushDiffManifestTests.swift | 36 ++++++++++++++++++ 8 files changed, 128 insertions(+), 7 deletions(-) diff --git a/android/app/src/main/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcher.kt b/android/app/src/main/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcher.kt index 756a4079..433d3da9 100644 --- a/android/app/src/main/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcher.kt +++ b/android/app/src/main/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcher.kt @@ -43,11 +43,12 @@ fun applyBinaryDiffPatches( // Manifest-supplied paths come from the update's JSON, so we treat them as untrusted. // Resolve them strictly under `base` and reject anything ("../../etc", an absolute path) that would otherwise -// let a manifest entry read or write outside the package/patch folders. +// let a manifest entry read or write outside the package/patch folders. Callers treat the result as a file +// inside `base`, so `base` itself (e.g. from ".") does not pass either. private fun resolveWithin(base: File, relativePath: String): File { val baseCanonical = base.canonicalFile val resolved = File(base, relativePath).canonicalFile - if (resolved != baseCanonical && !resolved.path.startsWith(baseCanonical.path + File.separator)) { + if (!resolved.path.startsWith(baseCanonical.path + File.separator)) { throw BinaryDiffApplyException(relativePath, "path escapes expected directory") } return resolved diff --git a/android/app/src/main/java/com/microsoft/codepush/react/diffpatch/DiffManifest.kt b/android/app/src/main/java/com/microsoft/codepush/react/diffpatch/DiffManifest.kt index b45f9a3d..64b023d6 100644 --- a/android/app/src/main/java/com/microsoft/codepush/react/diffpatch/DiffManifest.kt +++ b/android/app/src/main/java/com/microsoft/codepush/react/diffpatch/DiffManifest.kt @@ -1,5 +1,6 @@ package com.microsoft.codepush.react.diffpatch +import com.microsoft.codepush.react.CodePushConstants import org.json.JSONException import org.json.JSONObject @@ -36,15 +37,20 @@ fun parseDiffManifest(json: JSONObject): DiffManifest { emptyList() } + val reservedPatchesFolderPrefix = "${CodePushConstants.DIFF_PATCHES_FOLDER_NAME}/" val patchedFilesJson = json.optJSONObject("patchedFiles") val patchedFiles = if (patchedFilesJson != null) { patchedFilesJson.keys().asSequence().associateWith { relativePath -> val entry = patchedFilesJson.getJSONObject(relativePath) + val patch = entry.getString("patch") + if (!patch.startsWith(reservedPatchesFolderPrefix)) { + throw JSONException("Diff manifest patchedFiles[\"$relativePath\"] field \"patch\" must be under the reserved \"$reservedPatchesFolderPrefix\" prefix, but is \"$patch\".") + } PatchedFileEntry( algo = entry.getString("algo"), baseHash = entry.getString("baseHash"), targetHash = entry.getString("targetHash"), - patch = entry.getString("patch"), + patch = patch, ) } } else { diff --git a/android/app/src/test/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcherTest.kt b/android/app/src/test/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcherTest.kt index d094f293..6149b1c5 100644 --- a/android/app/src/test/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcherTest.kt +++ b/android/app/src/test/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcherTest.kt @@ -296,6 +296,36 @@ class BinaryDiffPatcherTest { assertEquals(0, applier.invocationCount) } + @Test + fun applyBinaryDiffPatches_patchFieldResolvesToUnzippedFolderItself_throwsWithoutInvokingApplier() { + // Given + val currentPackageFolder = tempFolder.newFolder("current") + val oldFile = File(currentPackageFolder, "index.android.bundle").apply { writeText("old hermes bytecode contents") } + val unzippedFolder = tempFolder.newFolder("unzipped") + val newUpdateFolder = tempFolder.newFolder("newUpdate") + + val manifest = manifestOf( + mapOf( + "index.android.bundle" to PatchedFileEntry( + algo = "bsdiff", + baseHash = sha256Hex(oldFile), + targetHash = "irrelevant", + patch = "__hcp_patches/..", + ) + ) + ) + val applier = FakePatchApplier { _, _, _ -> DiffPatch.PatchResult.OK } + + // When / Then + try { + applyBinaryDiffPatches(manifest, currentPackageFolder, unzippedFolder, newUpdateFolder, applier) + fail("expected BinaryDiffApplyException") + } catch (e: BinaryDiffApplyException) { + assertEquals("__hcp_patches/..", e.relativePath) + } + assertEquals(0, applier.invocationCount) + } + @Test fun applyBinaryDiffPatches_realBsdiffFixtureShape_appliesSuccessfully() { fun fixture(name: String) = diff --git a/android/app/src/test/java/com/microsoft/codepush/react/diffpatch/DiffManifestTest.kt b/android/app/src/test/java/com/microsoft/codepush/react/diffpatch/DiffManifestTest.kt index f363c4be..26eeabe5 100644 --- a/android/app/src/test/java/com/microsoft/codepush/react/diffpatch/DiffManifestTest.kt +++ b/android/app/src/test/java/com/microsoft/codepush/react/diffpatch/DiffManifestTest.kt @@ -3,6 +3,7 @@ package com.microsoft.codepush.react.diffpatch import org.json.JSONObject import org.junit.Assert.assertEquals import org.junit.Assert.assertTrue +import org.junit.Assert.fail import org.junit.Test class DiffManifestTest { @@ -124,6 +125,43 @@ class DiffManifestTest { parseDiffManifest(json) } + private fun assertPatchFieldRejectedByParser(patch: String) { + // Given + val json = JSONObject( + """ + { + "version": 2, + "patchedFiles": { + "relative/path.js": { + "algo": "bsdiff", + "baseHash": "base-hash-1", + "targetHash": "target-hash-1", + "patch": "$patch" + } + } + } + """.trimIndent() + ) + + // When / Then + try { + parseDiffManifest(json) + fail("expected JSONException") + } catch (e: org.json.JSONException) { + assertTrue(e.message, e.message!!.contains("__hcp_patches/")) + } + } + + @Test + fun parseDiffManifest_patchedFileEntryPatchOutsideReservedPrefix_throws() { + assertPatchFieldRejectedByParser("relative/path.js.bsdiff") + } + + @Test + fun parseDiffManifest_patchedFileEntryPatchWithPrefixButNoSlash_throws() { + assertPatchFieldRejectedByParser("__hcp_patchesX/relative/path.js.bsdiff") + } + @Test(expected = org.json.JSONException::class) fun parseDiffManifest_patchedFileEntryMissingRequiredField_throws() { // Given diff --git a/ios/CodePush/CodePushDiffManifest.h b/ios/CodePush/CodePushDiffManifest.h index ed623f30..efe28276 100644 --- a/ios/CodePush/CodePushDiffManifest.h +++ b/ios/CodePush/CodePushDiffManifest.h @@ -2,6 +2,9 @@ NS_ASSUME_NONNULL_BEGIN +// Folder within the update ZIP that contains the diff patches. Must be in sync with server-side impl. +extern NSString *const CodePushDiffPatchesFolderName; + @interface CodePushPatchedFileEntry : NSObject // The only value this client understands at the moment is "bsdiff". diff --git a/ios/CodePush/CodePushDiffManifest.m b/ios/CodePush/CodePushDiffManifest.m index 1488e6d6..b00015c0 100644 --- a/ios/CodePush/CodePushDiffManifest.m +++ b/ios/CodePush/CodePushDiffManifest.m @@ -14,6 +14,8 @@ return [CodePushErrorUtils errorWithMessage:[NSString stringWithFormat:@"Diff manifest %@ field \"%@\" must be a string, but is %@", context, fieldName, NSStringFromClass([value class])]]; } +NSString *const CodePushDiffPatchesFolderName = @"__hcp_patches"; + static BOOL isAbsent(id value) { return value == nil || [value isKindOfClass:[NSNull class]]; @@ -170,10 +172,17 @@ + (nullable instancetype)manifestFromJSON:(NSDictionary *)json error:(NSError ** } } + NSString *patch = entryJSON[@"patch"]; + NSString *reservedPatchesFolderPrefix = [CodePushDiffPatchesFolderName stringByAppendingString:@"/"]; + if (![patch hasPrefix:reservedPatchesFolderPrefix]) { + if (error) *error = [CodePushErrorUtils errorWithMessage:[NSString stringWithFormat:@"Diff manifest %@ field \"patch\" must be under the reserved \"%@\" prefix, but is \"%@\"", context, reservedPatchesFolderPrefix, patch]]; + return nil; + } + patchedFiles[relativePath] = [[CodePushPatchedFileEntry alloc] initWithAlgo:entryJSON[@"algo"] baseHash:entryJSON[@"baseHash"] targetHash:entryJSON[@"targetHash"] - patch:entryJSON[@"patch"]]; + patch:patch]; } } diff --git a/ios/CodePush/CodePushPackage.m b/ios/CodePush/CodePushPackage.m index 43088bec..a3859292 100644 --- a/ios/CodePush/CodePushPackage.m +++ b/ios/CodePush/CodePushPackage.m @@ -13,8 +13,6 @@ @implementation CodePushPackage #pragma mark - Private constants static NSString *const DiffManifestFileName = @"hotcodepush.json"; -// Folder within the update ZIP that contains the diff patches. -static NSString *const DiffPatchesFolderName = @"__hcp_patches"; static NSString *const DownloadFileName = @"download.zip"; static NSString *const RelativeBundlePathKey = @"bundlePath"; static NSString *const StatusFile = @"codepush.json"; @@ -78,7 +76,7 @@ + (BOOL)applyDiffManifest:(CodePushDiffManifest *)diffManifest // The patches folder must not stay in the installed package: it is // not part of the released contents, so it changes the folder hash // and surfaces later as a misleading integrity-check failure. - NSString *patchesFolderPath = [newUpdateFolderPath stringByAppendingPathComponent:DiffPatchesFolderName]; + NSString *patchesFolderPath = [newUpdateFolderPath stringByAppendingPathComponent:CodePushDiffPatchesFolderName]; if ([[NSFileManager defaultManager] fileExistsAtPath:patchesFolderPath]) { NSError *removeError = nil; BOOL patchesFolderRemoved = [[NSFileManager defaultManager] removeItemAtPath:patchesFolderPath diff --git a/ios/CodePushTests/CodePushDiffManifestTests.swift b/ios/CodePushTests/CodePushDiffManifestTests.swift index 1e3820ce..e9d31af8 100644 --- a/ios/CodePushTests/CodePushDiffManifestTests.swift +++ b/ios/CodePushTests/CodePushDiffManifestTests.swift @@ -102,6 +102,42 @@ final class CodePushDiffManifestTests: XCTestCase { XCTAssertThrowsError(try CodePushDiffManifest(json: jsonWithPatchedFiles)) } + func testManifest_patchedFilesEntryPatchOutsideReservedPrefix_throws() { + let json: [AnyHashable: Any] = [ + "version": 2, + "patchedFiles": [ + "main.jsbundle": [ + "algo": "bsdiff", + "baseHash": "aaaa", + "targetHash": "bbbb", + "patch": "main.jsbundle.bsdiff", + ] + ], + ] + + XCTAssertThrowsError(try CodePushDiffManifest(json: json)) { error in + XCTAssertTrue(error.localizedDescription.contains("__hcp_patches/"), error.localizedDescription) + } + } + + func testManifest_patchedFilesEntryPatchWithPrefixButNoSlash_throws() { + let json: [AnyHashable: Any] = [ + "version": 2, + "patchedFiles": [ + "main.jsbundle": [ + "algo": "bsdiff", + "baseHash": "aaaa", + "targetHash": "bbbb", + "patch": "__hcp_patchesX/main.jsbundle.bsdiff", + ] + ], + ] + + XCTAssertThrowsError(try CodePushDiffManifest(json: json)) { error in + XCTAssertTrue(error.localizedDescription.contains("__hcp_patches/"), error.localizedDescription) + } + } + func testManifest_patchedFilesWithoutVersionTwo_throws() { let json: [AnyHashable: Any] = [ "version": 1,