Repository navigation
Add iPhone-only Journal release checks and guarded Fastlane upload - #103
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3524dc7ab
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| app=File.join(archive,'Products/Applications/JournalApp.app') | ||
| JournalRelease.validate_info!(JournalRelease.read_plist(File.join(app,'Info.plist')),version: inputs.fetch('version'),build: inputs.fetch('build'),experimental: true) |
There was a problem hiding this comment.
In the documented archive → export flow, a post-build failure—such as the dSYM UUID check on line 49 or the second input-integrity check on line 44—leaves the newly created archive in place, but this lane accepts that same archive after only checking its Info.plist. An operator who continues after the archive lane fails can therefore sign and upload an archive that explicitly failed its release checks; record and require successful archive validation, or repeat all archive checks before export.
Useful? React with 👍 / 👎.
| export_options: {method: 'app-store-connect',destination: 'export',signingStyle: 'manual', | ||
| teamID: JournalRelease::TEAM,signingCertificate: identity, | ||
| provisioningProfiles: {JournalRelease::BUNDLE=>profile.fetch('UUID')}, |
There was a problem hiding this comment.
When profile: points to an external .mobileprovision that is not already installed, this code only decodes the file and then gives xcodebuild its UUID; it never gives Xcode the file itself. Fastlane's pinned profile helper explicitly installs profiles by copying them into Xcode's provisioning-profile directory (source), so the runbook command can fail with “profile not found” despite validating the supplied file, or resolve a different installed profile. Install the exact supplied profile temporarily, or require and hash-check its installed copy before export.
Useful? React with 👍 / 👎.
| destination=File.expand_path(entry.name,directory) | ||
| raise Failure, 'Unsafe IPA entry' unless destination.start_with?(directory+'/') && !entry.symlink? | ||
| FileUtils.mkdir_p(File.dirname(destination)) | ||
| entry.extract(destination) |
There was a problem hiding this comment.
With the pinned rubyzip 3.7 API, the positional argument to Zip::Entry#extract is an entry path beneath the default destination_directory: '.'; passing this absolute-looking path therefore produces $PWD/tmp/..., not the intended /tmp/.... A normal IPA consequently either fails extraction or leaves files under the repository, while the later /tmp/.../Payload/*.app lookup sees an empty app and makes every export, verify, and upload fail. Extract entry.name with destination_directory: directory instead.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
| def self.read_plist(path) | ||
| Plist.parse_xml(run!('/usr/bin/plutil','-convert','xml1','-o','-',path)) |
There was a problem hiding this comment.
The offline verifier parses the extracted, attacker-controlled Info.plist before checking its code signature, while plist 3.7.2 defaults <data> nodes to Marshal.load unless marshal: false is supplied (parser behavior). Verifying a crafted IPA can therefore instantiate arbitrary Ruby objects and execute deserialization hooks before the package is rejected; parse all package-derived plists with marshal: false.
Useful? React with 👍 / 👎.
| rescue StandardError => error | ||
| result['error_class']=error.class.name | ||
| result['error']=redact(error.message,secrets: [secret]) | ||
| ensure | ||
| result['transport']=transport || {} |
There was a problem hiding this comment.
When the operator presses Ctrl-C during Altool, Interrupt bypasses this StandardError rescue. The ensure block still assembles the captured transport output, stage, and error codes in result, but the block never returns that result to Ledger#attempt!, so the durable record contains only the initial generic failed_or_uncertain state. This loses the evidence needed to decide whether Apple received the interrupted upload; merge the result into the ledger before re-raising non-standard exceptions.
Useful? React with 👍 / 👎.
1 participant
Journal's external release project defaulted to iPhone+iPad, causing Apple validation to reject its three iPhone orientations. Upload diagnostics also lost the useful failure when Altool returned exit 0 with an error. This brings the reusable release fixes into the repository.
The adapter remains experimental host-restamp with explicit opt-in,
cross_abi_validated=false, actual native Debug configuration and Swift Release archive. Apple acceptance or a Release archive does not establish full cross-compilation, ABI correctness or runtime acceptance. The encryption key carries forward the team's declaration and owner-confirmed implementation equivalence; it is not a general legal guarantee.Validation on final head
805c38175518c4e25d72182f3f178dae8dfe1301:1.0 (1.5877.58): Journal app/team/profile,UIDeviceFamily=[1],ITSAppUsesNonExemptEncryption=false, strict signature, arm64/iPhoneOS, experimental provenance and executable/dSYM UUID agreement passed. IPA SHA256:4b8a7cc977a75d5420daf15f737f0bc4bb3df5e798ad069f9311a91ce3e474e7.spec-dev-toolis unavailable locally; no decision documents were generated. Runtime acceptance/full cross ABI were not tested.This is a formal PR; do not merge automatically. The two P1 findings are fixed. Three P2 findings remain open and outside the requested fix scope: archive-validation completion, installed provisioning-profile identity, and interrupted-upload evidence. Latest comments/reviews/threads were checked; no discussion was replied to or resolved. No credentials, signing/recovery material, graphs or release artifacts are included in the PR.