feat(mobileapp): Update mobile app command for ipa uploads #2619

noahsmartin · 2025-07-16T02:22:52Z

Add support for uploading iOS .ipa files to the mobile-app command by converting them to xcarchive format.

Co-authored-by: noah.martin <noah.martin@sentry.io>

…ploads-e699

szokeasaurusrex

Requesting changes primarily because of the use of ZipFile::name, which is potentially dangerous here.

I've left some other suggestions, too

src/commands/mobile_app/upload.rs

szokeasaurusrex · 2025-07-17T10:09:07Z

src/commands/mobile_app/upload.rs

+    // Extract .app from Payload/ directory
+    for i in 0..ipa_archive.len() {
+        let mut file = ipa_archive.by_index(i)?;
+        let file_path = file.name().to_string();


[change request]

Using ZipFile::name directly is dangerous, as it could break out of the current directory (e.g. if the path is Payload/../../../something). We could then end up overwriting this path later.

Please use ZipFile::enclosed_name instead, as the enclosed name is validated to ensure it does not break out of the current directory.

As a side note, we should probably make sure we don't use ZipFile::name anywhere else in the codebase.

src/commands/mobile_app/upload.rs

szokeasaurusrex

I forgot to review the other files in my original review, so here is some feedback on those

src/utils/mobile_app/validation.rs

tests/integration/mobile_app/upload.rs

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

noahsmartin · 2025-07-17T15:32:06Z

Thanks @szokeasaurusrex just updated the PR with those changes

# Steps to reproduce 1. Checkout this PR locally 2. Run `cargo run -Funstable-mobile-app -- mobile-app upload tests/integration/_fixtures/mobile_app/unexpected.ipa --log-level=debug` 3. Observe following the following error, which occurs despite the presence of `Payload/DemoApp.app/Info.plist`, because `Payload/s/demo.app/Info.plist` is shorter in terms of number of characters. ``` error: Failed to convert IPA to XCArchive for file tests/integration/_fixtures/mobile_app/unexpected.ipa Caused by: No .app found in IPA ```

src/commands/mobile_app/upload.rs

src/utils/mobile_app/ipa.rs

src/commands/mobile_app/upload.rs

src/utils/mobile_app/ipa.rs

szokeasaurusrex · 2025-07-18T10:28:08Z

src/utils/mobile_app/ipa.rs

+    Ok(xcarchive_dir)
+}
+
+fn extract_app_name_from_ipa(archive: &ZipArchive<Cursor<&[u8]>>) -> Result<String> {


I am still waiting for an answer to this question from my previous review

src/utils/mobile_app/ipa.rs

szokeasaurusrex · 2025-07-18T10:37:46Z

src/utils/mobile_app/validation.rs

    Ok(has_bundle_config && has_base_manifest)
 }

+pub fn is_ipa_file(bytes: &[u8]) -> Result<bool> {


Just wondering, why do we have this validation code in the first place? Why not just try to parse the file as all the different file types directly, then if we fail to, we assume that the file is invalid?

Having the separate validation logic seems like it is leading to duplicated code paths. We already check this structure in ipa.rs, in the extract_app_name_from_ipa method.

Fixing this (especially for the other file types) is of course out of scope for this PR, but hoping you can shed some light on this architectural decision.

Discussed on a call, it's mostly because this was ported from our typescript code, we probably could refactor this and make it at least just file extension checking

szokeasaurusrex · 2025-07-18T10:59:48Z

tests/integration/mobile_app/upload.rs

 }
+
+#[test]
+#[cfg(target_os = "macos")]


Why only run the test on macOS? The code we are testing is available on all platforms from what I can tell.

Suggested change

#[cfg(target_os = "macos")]

I updated the ipa support to be macOS only

[question] do we expect that in the future users may want to be able to upload IPA files from a non-macOS platform, e.g. from a (non-macOS) CI runner? If so, would it be possible to add such support?

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

noahsmartin · 2025-07-21T14:57:52Z

Thanks @szokeasaurusrex just made some more updates. I also made the IPA support macOS only so people don't accidentally upload without the asset catalog logic running (the Swift code). I intend to make XCArchive support macOS only too but figured I would do that in a follow up PR to keep this one focused.

szokeasaurusrex

Hey, this looks good! I left some suggestions, please address those, and then feel free to merge

src/utils/mobile_app/mod.rs

src/commands/mobile_app/upload.rs

szokeasaurusrex · 2025-07-21T15:57:08Z

src/commands/mobile_app/upload.rs

+    #[cfg(target_os = "macos")]
+    let format_list = "APK, AAB, XCArchive, or IPA";
+    #[cfg(not(target_os = "macos"))]
+    let format_list = "APK, AAB, or XCArchive";


As we have to conditionally print this list in several places, it might eventually make sense to extract this into a constant. I don't think we have to do it now necessarily but could make the code more easily maintainable, especially if we add more file formats in the future

szokeasaurusrex · 2025-07-21T16:18:08Z

src/utils/mobile_app/apple.rs

+}
+
+fn extract_app_name_from_ipa(archive: &ZipArchive<Cursor<&[u8]>>) -> Result<String> {
+    let pattern = Regex::new(r"^Payload/([^/]+)\.app/Info\.plist$")?;


As compiling regex is an expensive-ish operation, it is better to only do it once. We can achieve this by storing the pattern in a constant LazyLock, which is initialized on first access, like so:

Suggested change

let pattern = Regex::new(r"^Payload/([^/]+)\.app/Info\.plist$")?;

const PATTERN: LazyLock<Regex> = LazyLock::new(|| {

Regex::new(r"^Payload/([^/]+)\.app/Info\.plist$").expect("regex is valid")

});

You will need to update the usages of the variable, and also add an import of LazyLock

src/utils/mobile_app/apple.rs

szokeasaurusrex · 2025-07-21T16:32:58Z

src/utils/mobile_app/validation.rs

+    let is_ipa = archive.file_names().any(|name| {
+        name.starts_with("Payload/")
+            && name.ends_with(".app/Info.plist")
+            && name.matches('/').count() == 2
+    });


we could reuse the regex here, but this is also fine

szokeasaurusrex · 2025-07-21T16:33:32Z

tests/integration/_cases/mobile_app/mobile_app-upload-help.trycmd

Please rename this file to indicate this is macOS specific

szokeasaurusrex · 2025-07-21T16:34:40Z

tests/integration/mobile_app/upload.rs

 }
+
+#[test]
+#[cfg(target_os = "macos")]


[question] do we expect that in the future users may want to be able to upload IPA files from a non-macOS platform, e.g. from a (non-macOS) CI runner? If so, would it be possible to add such support?

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

noahsmartin · 2025-07-21T22:06:35Z

@szokeasaurusrex we wouldn't be able to run the asset catalog logic on non-macos which would result in incomplete features when the rest of our backend runs on the upload. I'd prefer to fail early here and require that the upload be on a Mac rather than possible show incorrect results later on, that's why I don't think we'd add non-macos support for these

Updates the mobile app command to make sure apple apps aren't uploaded from non-macos ARM, follow up from #2619

cursoragent and others added 2 commits July 16, 2025 01:38

Add support for uploading iOS IPA files in mobile-app command

2ba5dff

Co-authored-by: noah.martin <noah.martin@sentry.io>

Changes from background composer bc-bea23efb-7a3f-4687-8444-c50797c152d5

46ebd0f

noahsmartin requested a review from szokeasaurusrex as a code owner July 16, 2025 02:22

noahsmartin marked this pull request as draft July 16, 2025 02:23

noahsmartin changed the title ~~Update mobile app command for ipa uploads~~ feat(mobileapp): Update mobile app command for ipa uploads Jul 16, 2025

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch 2 times, most recently from c4fb721 to 6650034 Compare July 16, 2025 14:35

Fix

9586ada

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch from 6650034 to 9586ada Compare July 16, 2025 14:37

noahsmartin marked this pull request as ready for review July 16, 2025 14:39

Merge branch 'master' into cursor/update-mobile-app-command-for-ipa-u…

6c078ce

…ploads-e699

szokeasaurusrex requested changes Jul 17, 2025

View reviewed changes

szokeasaurusrex reviewed Jul 17, 2025

View reviewed changes

src/utils/mobile_app/validation.rs Outdated Show resolved Hide resolved

tests/integration/mobile_app/upload.rs Show resolved Hide resolved

noahsmartin and others added 2 commits July 17, 2025 09:23

Update src/commands/mobile_app/upload.rs

b9cf3ed

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

Update src/utils/mobile_app/validation.rs

0b088cb

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch 2 times, most recently from 910259b to 06a5da0 Compare July 17, 2025 14:57

PR feedback

e8cb4a3

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch from 06a5da0 to e8cb4a3 Compare July 17, 2025 15:22

szokeasaurusrex reviewed Jul 18, 2025

View reviewed changes

noahsmartin and others added 5 commits July 20, 2025 16:50

Update src/commands/mobile_app/upload.rs

5b8fd2e

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

Update src/commands/mobile_app/upload.rs

e050988

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

Update src/utils/mobile_app/ipa.rs

a26b6ad

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

Update src/utils/mobile_app/ipa.rs

b2cef47

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

Update src/utils/mobile_app/ipa.rs

9daae6d

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch 3 times, most recently from 7dd4e86 to ac74d43 Compare July 20, 2025 21:46

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch 8 times, most recently from 3f63cb9 to ad4e1ca Compare July 21, 2025 02:53

PR feedback

620bce8

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch from ad4e1ca to 620bce8 Compare July 21, 2025 15:04

szokeasaurusrex approved these changes Jul 21, 2025

View reviewed changes

noahsmartin and others added 4 commits July 21, 2025 17:39

Update src/commands/mobile_app/upload.rs

cd04205

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

Update src/commands/mobile_app/upload.rs

d681185

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

Update src/commands/mobile_app/upload.rs

0eeaa67

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

Update src/utils/mobile_app/apple.rs

8ff0a95

Co-authored-by: Daniel Szoke <7881302+szokeasaurusrex@users.noreply.github.com>

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch 2 times, most recently from fd40572 to 335ea98 Compare July 21, 2025 21:54

PR feedback

b8518fb

noahsmartin force-pushed the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch from 335ea98 to b8518fb Compare July 21, 2025 21:56

noahsmartin merged commit 1e67c3f into master Jul 21, 2025
26 checks passed

noahsmartin deleted the cursor/update-mobile-app-command-for-ipa-uploads-e699 branch July 21, 2025 22:08

noahsmartin mentioned this pull request Jul 22, 2025

fix: XCArchive upload on macOS ARM only #2635

Merged

noahsmartin added a commit that referenced this pull request Jul 31, 2025

fix: XCArchive upload on macOS ARM only (#2635)

937ff98

Updates the mobile app command to make sure apple apps aren't uploaded from non-macos ARM, follow up from #2619

-    let pattern = Regex::new(r"^Payload/([^/]+)\.app/Info\.plist$")?;
+    const PATTERN: LazyLock<Regex> = LazyLock::new(|| {
+        Regex::new(r"^Payload/([^/]+)\.app/Info\.plist$").expect("regex is valid")
+    });

Uh oh!

feat(mobileapp): Update mobile app command for ipa uploads #2619

feat(mobileapp): Update mobile app command for ipa uploads #2619

Uh oh!

Conversation

noahsmartin commented Jul 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

noahsmartin commented Jul 17, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahsmartin commented Jul 21, 2025

Uh oh!

szokeasaurusrex left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noahsmartin commented Jul 21, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

noahsmartin commented Jul 16, 2025 •

edited

Loading