Differentiate between ipa and other zip file types in ExtractInstallerMetadata#48802
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #48802 +/- ##
==========================================
+ Coverage 68.02% 68.04% +0.02%
==========================================
Files 3681 3684 +3
Lines 233961 234061 +100
Branches 12416 12416
==========================================
+ Hits 159146 159263 +117
+ Misses 60498 60482 -16
+ Partials 14317 14316 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
WalkthroughThis change addresses misdetection of MSIX files as IPA files. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as ExtractInstallerMetadata
participant Extractor as ExtractZIPMetadata
participant Zip as ZIP Entries
Caller->>Extractor: ExtractZIPMetadata(tfr)
Extractor->>Zip: scan entries
Zip-->>Extractor: Info.plist found?
Extractor->>Extractor: set hasInfoPlist
Extractor->>Extractor: check LSRequiresIPhoneOS, set isIPA
alt missing Info.plist or not IPA
Extractor-->>Caller: ErrInvalidType
else valid IPA
Extractor-->>Caller: InstallerMetadata
end
Related issues: Suggested labels: bug, go, low-risk Suggested reviewers: none identified from provided context 🐰 A zip disguised, an MSIX in vain, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
pkg/file/ipa_test.go (1)
91-103: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLatch test doesn't assert the returned bundle identifier.
This case only checks that detection isn't fooled by the framework's plist, but doesn't verify
meta.BundleIdentifier. Given the app'sInfo.plistis processed before the framework's (which also sets a differentCFBundleIdentifier), asserting the value here would catch the identity-field-clobbering issue flagged inpkg/file/ipa.go.✅ Suggested strengthening
meta, err = file.ExtractZIPMetadata(latchTfr) require.NoError(t, err) require.NotNil(t, meta) + require.Equal(t, "com.example.ios", meta.BundleIdentifier) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pkg/file/ipa_test.go` around lines 91 - 103, The latch test in ExtractZIPMetadata should also verify the returned BundleIdentifier, not just that ipa detection stays enabled. Update the test case in ipa_test.go that builds the ZIP with the app Info.plist followed by the framework plist to assert meta.BundleIdentifier still matches the app’s CFBundleIdentifier, so the framework plist cannot clobber the identity fields handled by file.ExtractZIPMetadata and the IPA parsing logic in pkg/file/ipa.go.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/file/ipa.go`:
- Around line 29-38: The IPA metadata selection in the plist parsing logic is
still using the last Info.plist seen, which can let nested framework or
extension plists overwrite the app’s real BundleID/Name/Version. Update the
parsing in the IPA handling code around the Info.plist scan so that when the
plist with LSRequiresIPhoneOS is found, the identity fields are latched from
that plist and preserved, or restrict the read to the root
Payload/*.app/Info.plist only. Use the existing plistData, hasInfoPlist, and
isIPA flow to ensure the app’s metadata cannot be replaced by later nested
plists.
---
Nitpick comments:
In `@pkg/file/ipa_test.go`:
- Around line 91-103: The latch test in ExtractZIPMetadata should also verify
the returned BundleIdentifier, not just that ipa detection stays enabled. Update
the test case in ipa_test.go that builds the ZIP with the app Info.plist
followed by the framework plist to assert meta.BundleIdentifier still matches
the app’s CFBundleIdentifier, so the framework plist cannot clobber the identity
fields handled by file.ExtractZIPMetadata and the IPA parsing logic in
pkg/file/ipa.go.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 997e6006-dc9b-4766-b5ad-8e77acbe6e75
⛔ Files ignored due to path filters (1)
pkg/file/testdata/software-installers/README.mdis excluded by!**/*.md
📒 Files selected for processing (7)
changes/48102-fix-msix-detecting-as-ipaee/server/service/software_installers.gopkg/file/file.gopkg/file/ipa.gopkg/file/ipa_test.gopkg/file/testdata/software-installers/ipa_test.ipapkg/file/testdata/software-installers/msix_test.msix
💤 Files with no reviewable changes (1)
- ee/server/service/software_installers.go
| BundleID string `plist:"CFBundleIdentifier"` | ||
| Name string `plist:"CFBundleName"` | ||
| Version string `plist:"CFBundleShortVersionString"` | ||
| RequiresIPhoneOS bool `plist:"LSRequiresIPhoneOS"` | ||
| } | ||
| var hasInfoPlist, isIPA bool | ||
|
|
||
| for _, f := range r.File { | ||
| // Matches any Info.plist and the last wins, so a nested framework or | ||
| // extension plist can override the app's own plist. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and test names, then read only the needed slices.
git ls-files | rg '(^|/)pkg/file/ipa\.go$|(^|/)pkg/file/ipa_.*\.go$|(^|/)pkg/file/.*_test\.go$' || true
echo '--- pkg/file/ipa.go (lines around the loop and return) ---'
nl -ba pkg/file/ipa.go | sed -n '1,220p'
echo '--- tests mentioning IPA / Info.plist / RequiresIPhoneOS ---'
rg -n 'RequiresIPhoneOS|Info\.plist|BundleIdentifier|ExtractZIPMetadata|latch|framework' pkg/file -g '*_test.go'Repository: fleetdm/fleet
Length of output: 476
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- pkg/file/ipa.go ---'
sed -n '1,220p' pkg/file/ipa.go | cat -n
echo '--- pkg/file/ipa_test.go matches ---'
rg -n 'RequiresIPhoneOS|Info\.plist|BundleIdentifier|ExtractZIPMetadata|latch|framework' pkg/file/ipa_test.go
echo '--- pkg/file/ipa_test.go relevant ranges ---'
sed -n '1,260p' pkg/file/ipa_test.go | cat -nRepository: fleetdm/fleet
Length of output: 8107
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the implementation around the plist parsing loop and return path.
sed -n '1,220p' pkg/file/ipa.go | cat -n
# Read the focused test cases.
sed -n '1,260p' pkg/file/ipa_test.go | cat -nRepository: fleetdm/fleet
Length of output: 6887
Latch the bundle metadata when the IPA plist is found.
isIPA is latched, but plistData.BundleID/Name/Version still come from the last Info.plist seen, so a nested framework or extension plist can overwrite the app’s real metadata. Snapshot the identity fields from the plist that sets LSRequiresIPhoneOS, or only read the root Payload/*.app/Info.plist.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/file/ipa.go` around lines 29 - 38, The IPA metadata selection in the
plist parsing logic is still using the last Info.plist seen, which can let
nested framework or extension plists overwrite the app’s real
BundleID/Name/Version. Update the parsing in the IPA handling code around the
Info.plist scan so that when the plist with LSRequiresIPhoneOS is found, the
identity fields are latched from that plist and preserved, or restrict the read
to the root Payload/*.app/Info.plist only. Use the existing plistData,
hasInfoPlist, and isIPA flow to ensure the app’s metadata cannot be replaced by
later nested plists.
There was a problem hiding this comment.
I decided to leave this out of the scope of this PR, since it was already getting kind of big for what the fix is.
There was a problem hiding this comment.
@jkatz01, that makes sense — keeping the fix focused is a good call. Would you like me to open a follow-up issue to track the nested Info.plist metadata latching concern so it doesn't get lost? Just let me know and I'll create it, referencing this PR and comment.
🧠 Learnings used
Learnt from: iansltx
Repo: fleetdm/fleet PR: 42393
File: server/service/integration_enterprise_test.go:23209-23209
Timestamp: 2026-04-02T21:46:15.560Z
Learning: This codebase targets Go 1.26. In Go 1.26, the built-in `new` accepts an expression operand (e.g., `new(true)` is valid and evaluates to a `*bool` initialized to `true`). Do not flag usages of `new(<value>)` as syntax errors in this repository; only treat `new` as invalid when it’s not conforming to the Go 1.26 expression form.
| Version string `plist:"CFBundleShortVersionString"` | ||
| RequiresIPhoneOS bool `plist:"LSRequiresIPhoneOS"` | ||
| } | ||
| var hasInfoPlist, isIPA bool |
There was a problem hiding this comment.
isIPA isn't necessary for the fix but I thought it would be useful there in case we decide to support .zip for macOS apps in the future and will need to differentiate between those and .ipa.
2 participants
Related issue: Resolves #48102
Changes:
ExtractIPAMetadatatoExtractZIPMetadatabecause the magic bytes for zip based installers (.ipa, .msix, .zip, etc) are the same so any zip file reaches it. If the zip does not contain anInfo.plistfile it will now fail withErrInvalidType.The actual error message is still just "invalid file type".
Checklist for submitter
If some of the following don't apply, delete the relevant line.
Changes file added for user-visible changes in
changes/,orbit/changes/oree/fleetd-chrome/changes.See Changes files for more information.
Input data is properly validated,
SELECT *is avoided, SQL injection is prevented (using placeholders for values in statements), JS inline code is prevented especially for url redirects, and untrusted data interpolated into shell scripts/commands is validated against shell metacharacters.Timeouts are implemented and retries are limited to avoid infinite loops
If paths of existing endpoints are modified without backwards compatibility, checked the frontend/CLI for any necessary changes
Testing
Added/updated automated tests
Where appropriate, automated tests simulate multiple hosts and test for host isolation (updates to one hosts's records do not affect another)
QA'd all new/changed functionality manually
.ipa, a macos FMA that uses a .zip file (alt-tab/darwin), and a windows FMA that uses a .zip file (vnc-server/windows).Summary by CodeRabbit