Skip to content

iOS: add CodePushDiffManifest for parsing diff manifests - #58

Merged
ofalvai merged 1 commit into
ios-errorutil-refactorfrom
ios-diff-manifest-parsing
Sep 22, 2026
Merged

ofalvai merged 1 commit into
ios-errorutil-refactorfrom
ios-diff-manifest-parsing

Conversation

@ofalvai

@ofalvai ofalvai commented Sep 9, 2026 •

Copy link
Copy Markdown
Collaborator

Why

Before landing #48, we need a sane way of parsing and reading diff manifest values. Existing code does does ad-hoc NSDictionary access (example: NSArray *deletedFiles = manifestJSON[@"deletedFiles"];).

Let's have a dedicated and type-safe manifest parser that we can easily extend later as we add more information to the manifest. This code can also be easily rewritten to Swift later.

What

  • Add CodePushDiffManifest type and JSON deserialization code.
  • Add path resolution helpers that make sure the untrusted (relative) paths in the diff manifest don't resolve to something unexpected at runtime. This turned out to be significantly more complex than on Android, there is no equivalent of java.io.File.getCanonicalFile().

@ofalvai
ofalvai added this pull request to stack #57 September 9, 2026 06:44
@ofalvai ofalvai changed the title ios diff manifest parsing iOS: add CodePushDiffManifest for parsing diff manifests Sep 9, 2026
@ofalvai
ofalvai requested a balanced review from Copilot September 9, 2026 06:54

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The parser silently accepts several malformed version and collection values, potentially interpreting manifests incorrectly.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Adds iOS support for parsing versioned diff manifests and safely resolving manifest paths.

Changes:

  • Adds diff-manifest models, parsing, and secure path resolution.
  • Adds unit tests for parsing and path traversal.
  • Registers implementation and tests with Xcode targets.
File summaries
File Description
ios/CodePushTests/CodePushTests-Bridging-Header.h Exposes the manifest API to Swift tests.
ios/CodePushTests/CodePushDiffManifestTests.swift Tests parsing and path safety.
ios/CodePush/CodePushDiffManifest.m Implements manifest parsing and path resolution.
ios/CodePush/CodePushDiffManifest.h Defines manifest models and APIs.
ios/CodePush.xcodeproj/project.pbxproj Adds sources and tests to Xcode targets.
Review details

Suppressed comments (2)

ios/CodePush/CodePushDiffManifest.m:118

  • A present but non-array deletedFiles value is currently treated exactly like an omitted field, so malformed input such as {"deletedFiles":"old.js"} parses successfully and silently retains files that the manifest intended to remove. Reject non-null values of the wrong container type before applying the optional-field default.
    NSArray *deletedFilesJSON = json[@"deletedFiles"];
    NSMutableArray<NSString *> *deletedFiles = [NSMutableArray array];
    if ([deletedFilesJSON isKindOfClass:[NSArray class]]) {

ios/CodePush/CodePushDiffManifest.m:130

  • A present patchedFiles value with the wrong container type is silently converted to an empty dictionary. For a version 2 update this skips every declared binary patch rather than reporting a malformed manifest, which can leave copied old-package bytes in the candidate update. Reject non-null, non-dictionary values before parsing entries.
    NSDictionary *patchedFilesJSON = json[@"patchedFiles"];
    NSMutableDictionary<NSString *, CodePushPatchedFileEntry *> *patchedFiles = [NSMutableDictionary dictionary];
    if ([patchedFilesJSON isKindOfClass:[NSDictionary class]]) {
  • Files reviewed: 5/5 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread ios/CodePush/CodePushDiffManifest.m Outdated
@ofalvai
ofalvai force-pushed the ios-diff-manifest-parsing branch 4 times, most recently from f1e5738 to ed626e1 Compare September 14, 2026 13:17
@ofalvai
ofalvai marked this pull request as ready for review September 14, 2026 13:21
@ofalvai
ofalvai force-pushed the ios-diff-manifest-parsing branch from ed626e1 to 8d83482 Compare September 14, 2026 14:21

@miklosboros miklosboros left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked this out against ios-errorutil-refactor and compiled CodePushDiffManifest.m + CodePushErrorUtils.m standalone on macOS to probe the resolver and parser directly, rather than reason about NSString path semantics from memory. Notes inline.

The path-resolution approach is sound and the layering is right: reject absolute/.., canonicalize the base, canonicalize the deepest existing ancestor (so a symlinked prefix resolves and gets caught by the prefix check), then lstat each re-appended component to catch dangling symlinks. I probed symlink-out, dangling-leaf, dangling-intermediate and inside-symlink, and they all behave as documented. The CodePushPackage.m change also closes two real pre-existing gaps: the NSJSONSerialization error was never checked, and deletedFiles entries were appended with no traversal check at all.

Three verified behavioral findings (resolvePath accepting the folder root, version accepting 2.7/true, and wrong-typed fields reported as missing), plus a few test gaps and nits.

Comment thread ios/CodePush/CodePushDiffManifest.m Outdated
Comment thread ios/CodePush/CodePushDiffManifest.m
Comment thread ios/CodePush/CodePushDiffManifest.m
Comment thread ios/CodePush/CodePushDiffManifest.m
Comment thread ios/CodePush/CodePushDiffManifest.m
Comment thread ios/CodePush/CodePushDiffManifest.m Outdated
Comment thread ios/CodePushTests/CodePushDiffManifestTests.swift
Comment thread ios/CodePushTests/CodePushDiffManifestTests.swift Outdated
Comment thread ios/CodePushTests/CodePushDiffManifestTests.swift
@ofalvai
ofalvai force-pushed the ios-diff-manifest-parsing branch 6 times, most recently from 4fe3dbe to 89b413a Compare September 21, 2026 13:27
@ofalvai
ofalvai force-pushed the ios-diff-manifest-parsing branch from 89b413a to b83a502 Compare September 22, 2026 08:10
@ofalvai
ofalvai merged commit 99329f4 into master Sep 22, 2026
6 checks passed
@ofalvai
ofalvai deleted the ios-diff-manifest-parsing branch September 22, 2026 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants