Reject diff manifest patch paths outside __hcp_patches/ - #66
Merged
Merged
Conversation
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 <noreply@anthropic.com>
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The validation is consistent across platforms and adequately covered by focused tests.
Review effort: Balanced
Findings: None
What changed in this PR
Validates binary-diff patch paths against the reserved patch folder and aligns Android path containment with iOS.
Changes:
- Rejects manifest patch paths lacking the
__hcp_patches/prefix. - Shares the iOS patch-folder constant.
- Prevents Android paths from resolving to the base directory itself.
| File | Description |
|---|---|
ios/CodePushTests/CodePushDiffManifestTests.swift |
Tests invalid patch prefixes. |
ios/CodePush/CodePushPackage.m |
Uses the shared folder constant. |
ios/CodePush/CodePushDiffManifest.m |
Adds prefix validation and shared constant. |
ios/CodePush/CodePushDiffManifest.h |
Declares the shared constant. |
android/app/src/test/java/com/microsoft/codepush/react/diffpatch/DiffManifestTest.kt |
Tests parser rejection behavior. |
android/app/src/test/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcherTest.kt |
Tests base-directory resolution rejection. |
android/app/src/main/java/com/microsoft/codepush/react/diffpatch/DiffManifest.kt |
Validates the reserved prefix. |
android/app/src/main/java/com/microsoft/codepush/react/diffpatch/BinaryDiffPatcher.kt |
Tightens containment validation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
miklosboros
approved these changes
Sep 24, 2026
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
The server puts every bsdiff patch file in a reserved
__hcp_patches/folder of the update ZIP. The install flow already depends on this. After patching, it removes that folder from the new package. Until now, nothing checked that thepatchpaths in a diff manifest actually point into that folder.What
patchedFilesentry whosepatchvalue does not start with__hcp_patches/. The error names the entry and the expected prefix.resolveWithinrejects a path that resolves to the base folder itself, for example__hcp_patches/... iOS already did this, so the two platforms now agree.Decisions
unzipped/comes from the ZIP. A patch path that leaves__hcp_patches/but stays insideunzipped/can only read bytes that the ZIP could also have put under the prefix. The real protection is still the existing containment check againstunzipped/, plus the folder hash and signature checks. My first version also enforced the subfolder in the patchers. I removed it, because it needed a new iOS API and symlink handling and it did not make the install any safer.Out of scope
patchedFileskeys ordeletedFilesentries that point inside__hcp_patches/. This only happens if an app ships its own folder with that name, and the server should reject such a release.🤖 Generated with Claude Code