Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Filtering only the downloaded source can leave a reserved patch directory copied from the current installed package on both platforms.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Adds filtered directory copying so binary patch payloads are excluded from installed CodePush packages.
Changes:
- Adds top-level exclusion support to Android and iOS copy utilities.
- Identifies binary manifests and rejects patched targets inside the reserved patch directory.
- Adds Android and iOS coverage for filtering and manifest validation.
| File | Description |
|---|---|
ios/CodePushTests/CodePushDiffManifestTests.swift |
Tests reserved patch-directory validation. |
ios/CodePush/CodePushUpdateUtils.m |
Adds filtered copy support. |
ios/CodePush/CodePushPackage.m |
Excludes patches during package merging. |
ios/CodePush/CodePushDiffManifest.m |
Adds binary-diff detection and reserved-path validation. |
ios/CodePush/CodePushDiffManifest.h |
Exposes binary-diff status. |
ios/CodePush/CodePush.h |
Declares the filtered copy overload. |
android/app/src/test/java/com/microsoft/codepush/react/FileUtilsTest.kt |
Tests top-level exclusion behavior. |
android/app/src/test/java/com/microsoft/codepush/react/diffpatch/DiffManifestTest.kt |
Tests reserved-path validation. |
android/app/src/test/java/com/microsoft/codepush/react/CodePushUpdateManagerTest.kt |
Tests package hashing without patch payloads. |
android/app/src/main/java/com/microsoft/codepush/react/FileUtils.java |
Adds filtered directory copying. |
android/app/src/main/java/com/microsoft/codepush/react/diffpatch/DiffManifest.kt |
Adds binary-diff status and path validation. |
android/app/src/main/java/com/microsoft/codepush/react/CodePushUpdateManager.java |
Filters patch payloads during installation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+279
to
+281
| boolean isBinaryDiffUpdate = isDiffUpdate && diffManifest.isBinaryDiff(); | ||
| FileUtils.copyDirectoryContents(unzippedFolderPath, newUpdateFolderPath, | ||
| isBinaryDiffUpdate ? CodePushConstants.DIFF_PATCHES_FOLDER_NAME : null); |
Collaborator
Author
There was a problem hiding this comment.
I think it's acceptable, and a bit more correct than the old behavior. I don't think any app would ship a __hcp_patches folder on purpose, but this new behavior preserves that dir.
Comment on lines
287
to
290
| [CodePushUpdateUtils copyEntriesInFolder:unzippedFolderPath | ||
| destFolder:newUpdateFolderPath | ||
| excludingEntry:(diffManifest.isBinaryDiff ? CodePushDiffPatchesFolderName : nil) | ||
| error:&error]; |
Collaborator
Author
There was a problem hiding this comment.
Same as the above comment about the Android impl, see my response there. I think it's acceptable.
ofalvai
force-pushed
the
push-trysqrlsvyss
branch
from
September 25, 2026 08:48
05f27e5 to
54b486b
Compare
ofalvai
marked this pull request as ready for review
September 25, 2026 13:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
At the moment, the install flow for a binary diff update is:
__hcp_patches/folder with the patch files.__hcp_patches/from the new package.The copy in step 1 is unnecessary I/O, because the patcher reads the patches from the unzipped folder. Step 3 is also a failure point. If the delete fails or the process stops before it, the patches folder stays in the installed package. It is not part of the release contents, so it changes the folder hash. The result is an integrity-check failure that does not point to the real cause.
What
__hcp_patches/. Full updates and non-binary diff updates copy the same files as before.patchedFileskeys in the diff manifest. The delete step was also a guard against a manifest that creates or patches a file inside__hcp_patches/, so the parser now does that check:..component is rejected. Without this, a key such asa/../__hcp_patches/xcould get past the top-level check.__hcp_patchesis rejected. This includes./__hcp_patches/xand/__hcp_patches/x.version == 2checks become anisBinaryDiffproperty on the manifest, on both platforms.I do not think a real server ever sends a manifest like this, but after the delete step is removed, nothing else stops it, so the parser must reject it.