Skip to content
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 35 additions & 11 deletions src/commands/mobile_app/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,9 @@ use crate::utils::fs::get_sha1_checksums;
use crate::utils::fs::TempFile;
#[cfg(target_os = "macos")]
use crate::utils::mobile_app::handle_asset_catalogs;
use crate::utils::mobile_app::{is_aab_file, is_apk_file, is_apple_app, is_zip_file};
use crate::utils::mobile_app::{
ipa_to_xcarchive, is_aab_file, is_apk_file, is_apple_app, is_ipa_file, is_zip_file,
};
use crate::utils::progress::ProgressBar;
use crate::utils::vcs;

Expand All @@ -36,7 +38,7 @@ pub fn make_command(command: Command) -> Command {
.arg(
Arg::new("paths")
.value_name("PATH")
.help("The path to the mobile app files to upload. Supported files include Apk, Aab or XCArchive.")
.help("The path to the mobile app files to upload. Supported files include Apk, Aab, XCArchive, or IPA.")
Comment thread
noahsmartin marked this conversation as resolved.
Outdated
.num_args(1..)
.action(ArgAction::Append)
.required(true),
Expand Down Expand Up @@ -95,12 +97,29 @@ pub fn execute(matches: &ArgMatches) -> Result<()> {

let normalized_zip = if path.is_file() {
debug!("Normalizing file: {}", path.display());
normalize_file(path, &byteview).with_context(|| {
format!(
"Failed to generate uploadable bundle for file {}",
path.display()
)
})?

// Handle IPA files by converting them to XCArchive
if is_zip_file(&byteview) && is_ipa_file(&byteview)? {
debug!("Converting IPA file to XCArchive structure");
let temp_dir = crate::utils::fs::TempDir::create()?;
Comment thread
noahsmartin marked this conversation as resolved.
Outdated
let xcarchive_path =
ipa_to_xcarchive(path, &byteview, &temp_dir).with_context(|| {
format!(
"Failed to convert IPA to XCArchive for file {}",
path.display()
)
})?;
normalize_directory(&xcarchive_path).with_context(|| {
format!("Failed to normalize XCArchive for file {}", path.display())
})?
Comment thread
noahsmartin marked this conversation as resolved.
Outdated
} else {
normalize_file(path, &byteview).with_context(|| {
format!(
"Failed to generate uploadable bundle for file {}",
path.display()
)
})?
}
} else if path.is_dir() {
debug!("Normalizing directory: {}", path.display());
normalize_directory(path).with_context(|| {
Expand Down Expand Up @@ -186,9 +205,9 @@ fn validate_is_mobile_app(path: &Path, bytes: &[u8]) -> Result<()> {
return Ok(());
}

// Check if the file is a zip file (then AAB or APK)
// Check if the file is a zip file (then AAB, APK, or IPA)
if is_zip_file(bytes) {
debug!("File is a zip, checking for AAB/APK format");
debug!("File is a zip, checking for AAB/APK/IPA format");
Comment thread
noahsmartin marked this conversation as resolved.
if is_aab_file(bytes)? {
debug!("Detected AAB file");
return Ok(());
Expand All @@ -198,11 +217,16 @@ fn validate_is_mobile_app(path: &Path, bytes: &[u8]) -> Result<()> {
debug!("Detected APK file");
return Ok(());
}

if is_ipa_file(bytes)? {
debug!("Detected IPA file");
return Ok(());
}
}

debug!("File format validation failed");
Err(anyhow!(
"File is not a recognized mobile app format (APK, AAB, or XCArchive): {}",
"File is not a recognized mobile app format (APK, AAB, XCArchive, or IPA): {}",
path.display()
))
}
Expand Down
132 changes: 132 additions & 0 deletions src/utils/mobile_app/ipa.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,132 @@
#![cfg(feature = "unstable-mobile-app")]

use anyhow::{anyhow, Result};
use log::debug;
use std::io::Cursor;
use std::path::{Path, PathBuf};
use zip::ZipArchive;

use crate::utils::fs::TempDir;

/// Converts an IPA file to an XCArchive directory structure. The provided IPA must be a valid IPA file.
///
/// # Format Overview
///
/// ## IPA (iOS App Store Package)
/// An IPA file is a compressed archive containing an iOS app ready for distribution.
/// It has the following structure:
/// ```
/// MyApp.ipa
/// └── Payload/
/// └── MyApp.app/
/// ├── Info.plist
/// ├── MyApp (executable)
/// ├── Assets.car
/// └── ... (other app resources)
/// ```
///
/// ## XCArchive (Xcode Archive)
/// An XCArchive is a directory structure created by Xcode when archiving an app for distribution.
/// It has the following structure:
/// ```
/// MyApp.xcarchive/
/// ├── Info.plist
/// ├── Products/
/// │ └── Applications/
/// │ └── MyApp.app/
/// │ ├── Info.plist
/// │ ├── MyApp (executable)
/// │ ├── Assets.car
/// │ └── ... (other app resources)
/// └── ... (other archive metadata)
/// ```
///
/// # Transformation Process
///
/// This function performs the following steps:
/// 1. Creates the XCArchive directory structure (`archive.xcarchive/Products/Applications/`)
/// 2. Extracts the app name from the IPA by finding the shortest path ending with `.app/Info.plist`
/// 3. Extracts all files from the IPA's `Payload/` directory into the XCArchive structure
/// 4. Creates an `Info.plist` file for the XCArchive with the app path reference
/// 5. Returns the path to the XCArchive directory structure
Comment thread
noahsmartin marked this conversation as resolved.
Outdated
pub fn ipa_to_xcarchive(ipa_path: &Path, ipa_bytes: &[u8], temp_dir: &TempDir) -> Result<PathBuf> {
debug!(
"Converting IPA to XCArchive structure: {}",
ipa_path.display()
);

let xcarchive_dir = temp_dir.path().join("archive.xcarchive");
let products_dir = xcarchive_dir.join("Products");
let applications_dir = products_dir.join("Applications");

debug!("Creating XCArchive directory structure");
std::fs::create_dir_all(&applications_dir)?;

// Extract IPA file
let cursor = Cursor::new(ipa_bytes);
let mut ipa_archive = ZipArchive::new(cursor)?;

let app_name = extract_app_name_from_ipa(&ipa_archive)?;

// Extract all files from the archive
for i in 0..ipa_archive.len() {
let mut file = ipa_archive.by_index(i)?;

if let Some(name) = file.enclosed_name() {
if let Ok(stripped) = name.strip_prefix("Payload/") {
Comment thread
noahsmartin marked this conversation as resolved.
Outdated
if !file.is_dir() {
// Create the file path in the XCArchive structure
let target_path = applications_dir.join(stripped);

// Create parent directories if necessary
if let Some(parent) = target_path.parent() {
std::fs::create_dir_all(parent)?;
}

// Extract file
let mut target_file = std::fs::File::create(&target_path)?;
std::io::copy(&mut file, &mut target_file)?;
}
}
}
}

// Create Info.plist for XCArchive
let info_plist_path = xcarchive_dir.join("Info.plist");

let info_plist_content = format!(
r#"<?xml version="1.0" encoding="UTF-8"?>
<!DOCTYPE plist PUBLIC "-//Apple//DTD PLIST 1.0//EN" "http://www.apple.com/DTDs/PropertyList-1.0.dtd">
<plist version="1.0">
<dict>
<key>ApplicationProperties</key>
<dict>
<key>ApplicationPath</key>
<string>Applications/{app_name}.app</string>
</dict>
<key>ArchiveVersion</key>
<integer>1</integer>
</dict>
</plist>"#
);

std::fs::write(&info_plist_path, info_plist_content)?;

debug!(
"Created XCArchive Info.plist at: {}",
info_plist_path.display()
);
Ok(xcarchive_dir)
}

fn extract_app_name_from_ipa(archive: &ZipArchive<Cursor<&[u8]>>) -> Result<String> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

archive
.file_names()
.filter(|name| name.starts_with("Payload/") && name.ends_with(".app/Info.plist"))
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Have you tested that this works as expected if running this command on Windows? Since on Windows, the paths might have \ instead of / (though I am not sure how the zip crate handles this)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It wouldn't really make sense to upload an IPA from windows, they are always on Macs and other things like the asset catalog support would not work on windows. So I will just add the code to avoid ever calling this path so users don't get confused about why thinks like asset catalogs stop working

.min_by_key(|name| name.len())
Comment thread
noahsmartin marked this conversation as resolved.
Outdated
.and_then(|name| name.strip_prefix("Payload/"))
.and_then(|name| name.split('/').next())
.and_then(|name| name.strip_suffix(".app"))
.map(|name| name.to_owned())
.ok_or_else(|| anyhow!("No .app found in IPA"))
}
Comment thread
noahsmartin marked this conversation as resolved.
Outdated
4 changes: 3 additions & 1 deletion src/utils/mobile_app/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,10 @@

#[cfg(target_os = "macos")]
mod apple;
mod ipa;
mod validation;

#[cfg(target_os = "macos")]
pub use self::apple::handle_asset_catalogs;
pub use self::validation::{is_aab_file, is_apk_file, is_apple_app, is_zip_file};
pub use self::ipa::ipa_to_xcarchive;
pub use self::validation::{is_aab_file, is_apk_file, is_apple_app, is_ipa_file, is_zip_file};
13 changes: 13 additions & 0 deletions src/utils/mobile_app/validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,19 @@ pub fn is_aab_file(bytes: &[u8]) -> Result<bool> {
Ok(has_bundle_config && has_base_manifest)
}

pub fn is_ipa_file(bytes: &[u8]) -> Result<bool> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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

let cursor = std::io::Cursor::new(bytes);
let archive = zip::ZipArchive::new(cursor)?;

let is_ipa = archive.file_names().any(|name| {
name.starts_with("Payload/")
&& name.ends_with(".app/Info.plist")
&& name.matches('/').count() == 2
});
Comment on lines +45 to +49
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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


Ok(is_ipa)
}

pub fn is_xcarchive_directory<P>(path: P) -> bool
where
P: AsRef<Path>,
Expand Down
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please rename this file to indicate this is macOS specific

Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,8 @@ $ sentry-cli mobile-app upload --help
Usage: sentry-cli[EXE] mobile-app upload [OPTIONS] <PATH>...

Arguments:
<PATH>... The path to the mobile app files to upload. Supported files include Apk, Aab or
XCArchive.
<PATH>... The path to the mobile app files to upload. Supported files include Apk, Aab,
XCArchive, or IPA.

Options:
-o, --org <ORG>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
```
$ sentry-cli mobile-app upload tests/integration/_fixtures/mobile_app/ipa.ipa --sha test_sha
? success
Successfully uploaded 1 file to Sentry
- tests/integration/_fixtures/mobile_app/ipa.ipa

```
1 change: 1 addition & 0 deletions tests/integration/_fixtures/mobile_app/invalid.ipa
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
invalid ipa content
Binary file added tests/integration/_fixtures/mobile_app/ipa.ipa
Binary file not shown.
66 changes: 66 additions & 0 deletions tests/integration/mobile_app/upload.rs
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,18 @@ fn command_mobile_app_upload_invalid_xcarchive() {
.run_and_assert(AssertCommand::Failure);
}

#[test]
fn command_mobile_app_upload_invalid_ipa() {
Comment thread
noahsmartin marked this conversation as resolved.
TestManager::new()
.assert_cmd(vec![
"mobile-app",
"upload",
"tests/integration/_fixtures/mobile_app/invalid.ipa",
])
.with_default_token()
.run_and_assert(AssertCommand::Failure);
}

#[test]
fn command_mobile_app_upload_apk_all_uploaded() {
TestManager::new()
Expand Down Expand Up @@ -160,3 +172,57 @@ fn command_mobile_app_upload_apk_chunked() {
.register_trycmd_test("mobile_app/mobile_app-upload-apk.trycmd")
.with_default_token();
}

#[test]
#[cfg(target_os = "macos")]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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")]

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I updated the ipa support to be macOS only

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

[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?

/// This test simulates a full chunk upload for an IPA file (with only one chunk).
/// It verifies that the Sentry CLI makes the expected API calls to the chunk upload endpoint
/// and that the data sent to the chunk upload endpoint is exactly as expected.
/// It also verifies that the correct calls are made to the assemble endpoint.
fn command_mobile_app_upload_ipa_chunked() {
let is_first_assemble_call = AtomicBool::new(true);

TestManager::new()
.mock_endpoint(
MockEndpointBuilder::new("GET", "/api/0/organizations/wat-org/chunk-upload/")
.with_response_file("mobile_app/get-chunk-upload.json"),
)
.mock_endpoint(
MockEndpointBuilder::new("POST", "/api/0/organizations/wat-org/chunk-upload/")
.with_response_fn(move |request| {
let content_type_headers = request.header("content-type");
assert_eq!(
content_type_headers.len(),
1,
"content-type header should be present exactly once, found {} times",
content_type_headers.len()
);
vec![] // Client does not expect a response body
}),
)
.mock_endpoint(
MockEndpointBuilder::new(
"POST",
"/api/0/projects/wat-org/wat-project/files/preprodartifacts/assemble/",
)
.with_header_matcher("content-type", "application/json")
.with_matcher(r#"{"checksum":"ed9da71e3688261875db21b266da84ffe004a8a4","chunks":["ed9da71e3688261875db21b266da84ffe004a8a4"],"git_sha":"test_sha"}"#)
.with_response_fn(move |_| {
if is_first_assemble_call.swap(false, Ordering::Relaxed) {
r#"{
"state": "created",
"missingChunks": ["ed9da71e3688261875db21b266da84ffe004a8a4"]
}"#
} else {
r#"{
"state": "ok",
"missingChunks": []
}"#
}
.into()
})
.expect(2),
)
.register_trycmd_test("mobile_app/mobile_app-upload-ipa.trycmd")
.with_default_token();
}