From 7f860f0282c4091c459aff8d668b0e3741ed2376 Mon Sep 17 00:00:00 2001 From: Chris Busillo Date: Tue, 2 Jun 2026 16:05:56 -0400 Subject: [PATCH] Harden App Store review submission workflow --- .github/workflows/submit-app-store-review.yml | 38 ++- .../test_submit_app_store_review.py | 229 ++++++++++++++++++ scripts/submit-app-store-review.py | 204 ++++++++++++---- 3 files changed, 421 insertions(+), 50 deletions(-) create mode 100644 Tests/ScriptsTests/test_submit_app_store_review.py diff --git a/.github/workflows/submit-app-store-review.yml b/.github/workflows/submit-app-store-review.yml index 1d40f74..db49f84 100644 --- a/.github/workflows/submit-app-store-review.yml +++ b/.github/workflows/submit-app-store-review.yml @@ -20,8 +20,16 @@ name: Submit App Store Review description: Existing App Store version to copy metadata from. required: false type: string + remove_active_review_version: + description: >- + Existing App Store version to remove from active review first. + required: false + type: string dry_run: - description: Prepare the version and review item without submitting. + description: >- + Prepare the version and review item without submitting. With + remove_active_review_version, only validates metadata/build and the + removal target because the active review is not removed. required: true default: false type: boolean @@ -30,7 +38,7 @@ permissions: contents: read concurrency: - group: submit-app-store-review-${{ github.event.inputs.version }} + group: submit-app-store-review-${{ inputs.version }} cancel-in-progress: false jobs: @@ -56,17 +64,31 @@ jobs: - name: Submit Review shell: bash + env: + INPUT_VERSION: ${{ inputs.version }} + INPUT_BUILD_NUMBER: ${{ inputs.build_number }} + INPUT_WHATS_NEW: ${{ inputs.whats_new }} + INPUT_COPY_FROM_VERSION: ${{ inputs.copy_from_version }} + INPUT_REMOVE_ACTIVE_REVIEW_VERSION: >- + ${{ inputs.remove_active_review_version }} + INPUT_DRY_RUN: ${{ inputs.dry_run }} run: | set -euo pipefail args=( - --version "${{ inputs.version }}" - --build-number "${{ inputs.build_number }}" - --whats-new "${{ inputs.whats_new }}" + --version "${INPUT_VERSION}" + --build-number "${INPUT_BUILD_NUMBER}" + --whats-new "${INPUT_WHATS_NEW}" ) - if [[ -n "${{ inputs.copy_from_version }}" ]]; then - args+=(--copy-from-version "${{ inputs.copy_from_version }}") + if [[ -n "${INPUT_COPY_FROM_VERSION}" ]]; then + args+=(--copy-from-version "${INPUT_COPY_FROM_VERSION}") + fi + if [[ -n "${INPUT_REMOVE_ACTIVE_REVIEW_VERSION}" ]]; then + args+=( + --remove-active-review-version + "${INPUT_REMOVE_ACTIVE_REVIEW_VERSION}" + ) fi - if [[ "${{ inputs.dry_run }}" == "true" ]]; then + if [[ "${INPUT_DRY_RUN}" == "true" ]]; then args+=(--dry-run) fi submit=( diff --git a/Tests/ScriptsTests/test_submit_app_store_review.py b/Tests/ScriptsTests/test_submit_app_store_review.py new file mode 100644 index 0000000..a879aed --- /dev/null +++ b/Tests/ScriptsTests/test_submit_app_store_review.py @@ -0,0 +1,229 @@ +import importlib.util +import unittest +from pathlib import Path +from types import SimpleNamespace +from typing import Any + + +MODULE_PATH = Path(__file__).resolve().parents[2] / "scripts" / "submit-app-store-review.py" +SPEC = importlib.util.spec_from_file_location("submit_app_store_review", MODULE_PATH) +if SPEC is None or SPEC.loader is None: + raise RuntimeError(f"Unable to load {MODULE_PATH}") +submit_app_store_review = importlib.util.module_from_spec(SPEC) +SPEC.loader.exec_module(submit_app_store_review) + + +class FakeASCClient: + def __init__( + self, + include_submission_version=True, + include_item_version=True, + app_store_state: str | None = "WAITING_FOR_REVIEW", + app_version_state: str | None = None, + ): + self.deleted_paths: list[str] = [] + self.requests: list[tuple[Any, ...]] = [] + self.include_submission_version = include_submission_version + self.include_item_version = include_item_version + self.app_store_state = app_store_state + self.app_version_state = app_version_state + + def request(self, method, path, params=None, body=None, allowed=(200,)): + self.requests.append((method, path, params, body, allowed)) + if method == "GET" and path == "/apps/app-id/appStoreVersions": + attributes = {"versionString": "1.0.13"} + if self.app_store_state is not None: + attributes["appStoreState"] = self.app_store_state + if self.app_version_state is not None: + attributes["appVersionState"] = self.app_version_state + return { + "data": [ + { + "id": "version-1-0-13", + "attributes": attributes, + } + ] + } + if method == "GET" and path == "/reviewSubmissions": + submission_relationships: dict[str, Any] = { + "items": {"data": [{"type": "reviewSubmissionItems", "id": "item-1"}]}, + } + if self.include_submission_version: + submission_relationships["appStoreVersionForReview"] = { + "data": {"type": "appStoreVersions", "id": "version-1-0-13"} + } + item_relationships: dict[str, Any] = {} + if self.include_item_version: + item_relationships["appStoreVersion"] = { + "data": {"type": "appStoreVersions", "id": "version-1-0-13"} + } + return { + "data": [ + { + "id": "submission-1", + "attributes": {"state": "WAITING_FOR_REVIEW"}, + "relationships": submission_relationships, + } + ], + "included": [ + { + "id": "item-1", + "type": "reviewSubmissionItems", + "relationships": item_relationships, + } + ], + } + if method == "DELETE" and path == "/reviewSubmissionItems/item-1": + self.deleted_paths.append(path) + return {} + raise AssertionError(f"unexpected request: {method} {path}") + + +class RemoveActiveReviewVersionTests(unittest.TestCase): + def test_deletes_matching_item(self): + client = FakeASCClient() + + submit_app_store_review.remove_active_review_version(client, "app-id", "1.0.13") + + self.assertEqual(client.deleted_paths, ["/reviewSubmissionItems/item-1"]) + + def test_dry_run_does_not_delete(self): + client = FakeASCClient() + + submit_app_store_review.remove_active_review_version(client, "app-id", "1.0.13", dry_run=True) + + self.assertEqual(client.deleted_paths, []) + + def test_deletes_when_submission_version_relationship_is_missing(self): + client = FakeASCClient(include_submission_version=False) + + submit_app_store_review.remove_active_review_version(client, "app-id", "1.0.13") + + self.assertEqual(client.deleted_paths, ["/reviewSubmissionItems/item-1"]) + + def test_raises_when_blocking_version_has_no_matching_submission_item(self): + client = FakeASCClient(include_submission_version=False, include_item_version=False) + + with self.assertRaises(submit_app_store_review.AppStoreConnectError): + submit_app_store_review.remove_active_review_version(client, "app-id", "1.0.13") + + def test_raises_when_only_app_version_state_is_blocking(self): + client = FakeASCClient( + include_submission_version=False, + include_item_version=False, + app_store_state=None, + app_version_state="WAITING_FOR_REVIEW", + ) + + with self.assertRaises(submit_app_store_review.AppStoreConnectError): + submit_app_store_review.remove_active_review_version(client, "app-id", "1.0.13") + + def test_dry_run_build_check_does_not_patch_encryption(self): + class BuildClient: + def __init__(self): + self.requests: list[tuple[Any, ...]] = [] + + def request(self, method, path, params=None, body=None, allowed=(200,)): + self.requests.append((method, path, params, body, allowed)) + if method == "GET" and path == "/builds": + return { + "data": [ + { + "id": "build-1", + "attributes": { + "processingState": "VALID", + "usesNonExemptEncryption": None, + }, + } + ] + } + raise AssertionError(f"unexpected request: {method} {path}") + + client = BuildClient() + args = SimpleNamespace(build_number="202606021931", non_exempt_encryption=False) + + submit_app_store_review.ensure_build(client, "app-id", args, allow_updates=False) + + self.assertEqual([request[0] for request in client.requests], ["GET"]) + + def test_dry_run_main_returns_before_version_or_submission_mutations(self): + class MainClient: + def __init__(self): + self.requests: list[tuple[Any, ...]] = [] + + def request(self, method, path, params=None, body=None, allowed=(200,)): + self.requests.append((method, path, params, body, allowed)) + if method == "GET" and path == "/apps": + return {"data": [{"id": "app-id", "attributes": {"name": "Context Panel"}}]} + if method == "GET" and path == "/apps/app-id/appStoreVersions": + if params and params.get("filter[versionString]") == "1.0.14": + raise AssertionError("dry run should not create or query target version") + return { + "data": [ + { + "id": "version-1-0-13", + "attributes": {"versionString": "1.0.13", "appStoreState": "READY_FOR_SALE"}, + "relationships": { + "appStoreVersionLocalizations": { + "data": [{"type": "appStoreVersionLocalizations", "id": "loc-1"}] + }, + "appStoreReviewDetail": { + "data": {"type": "appStoreReviewDetails", "id": "detail-1"} + }, + }, + } + ], + "included": [ + { + "id": "loc-1", + "type": "appStoreVersionLocalizations", + "attributes": {"locale": "en-US", "description": "desc", "supportUrl": "https://example.com"}, + }, + { + "id": "detail-1", + "type": "appStoreReviewDetails", + "attributes": {"contactEmail": "review@example.com"}, + }, + ], + } + if method == "GET" and path == "/builds": + return { + "data": [ + { + "id": "build-1", + "attributes": {"processingState": "VALID", "usesNonExemptEncryption": False}, + } + ] + } + raise AssertionError(f"unexpected request: {method} {path}") + + client = MainClient() + args = SimpleNamespace( + api_key_id="key-id", + api_issuer_id="issuer-id", + bundle_id="com.shinycomputers.contextpanel", + copy_from_version="1.0.13", + build_number="202606021931", + non_exempt_encryption=False, + remove_active_review_version=None, + dry_run=True, + version="1.0.14", + whats_new="Fixes", + release_type="AFTER_APPROVAL", + copyright="2026 Shiny Computers Leasing LLC", + uses_idfa=False, + ) + + source_localization, source_review_detail = submit_app_store_review.latest_source_metadata( + client, "app-id", args.copy_from_version + ) + submit_app_store_review.ensure_build(client, "app-id", args, allow_updates=not args.dry_run) + self.assertTrue(source_localization) + self.assertTrue(source_review_detail) + + mutation_methods = [request[0] for request in client.requests if request[0] != "GET"] + self.assertEqual(mutation_methods, []) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/submit-app-store-review.py b/scripts/submit-app-store-review.py index ae3c5fc..42ccd74 100755 --- a/scripts/submit-app-store-review.py +++ b/scripts/submit-app-store-review.py @@ -31,6 +31,10 @@ "PENDING_DEVELOPER_RELEASE", "READY_FOR_SALE", } +ACTIVE_REVIEW_SUBMISSION_STATES = {"READY_FOR_REVIEW", "WAITING_FOR_REVIEW", "IN_REVIEW"} +BLOCKING_REVIEW_VERSION_STATES = {"WAITING_FOR_REVIEW", "IN_REVIEW", "PENDING_DEVELOPER_RELEASE"} +CREATE_VERSION_MAX_ATTEMPTS = 6 +CREATE_VERSION_RETRY_SECONDS = 10 class AppStoreConnectError(RuntimeError): @@ -125,6 +129,11 @@ def relationship_id(resource: dict[str, Any], name: str) -> str | None: return None +def version_state(resource: dict[str, Any]) -> str | None: + attributes = resource.get("attributes", {}) + return attributes.get("appStoreState") or attributes.get("appVersionState") + + def expanded_key_path(args: argparse.Namespace) -> tuple[Path, Path | None]: if args.api_key: return Path(args.api_key).expanduser(), None @@ -164,7 +173,7 @@ def latest_source_metadata(client: ASCClient, app_id: str, prefer_version: str | ( version for version in versions - if version["attributes"].get("appStoreState") == "READY_FOR_SALE" + if version_state(version) == "READY_FOR_SALE" ), versions[0], ) @@ -174,6 +183,114 @@ def latest_source_metadata(client: ASCClient, app_id: str, prefer_version: str | return localization, review_detail +def app_store_version(client: ASCClient, app_id: str, version_string: str) -> dict[str, Any] | None: + payload = client.request( + "GET", + f"/apps/{app_id}/appStoreVersions", + { + "filter[platform]": "MAC_OS", + "filter[versionString]": version_string, + "fields[appStoreVersions]": "versionString,appStoreState,appVersionState", + "limit": 1, + }, + ) + versions = payload.get("data") or [] + return versions[0] if versions else None + + +def app_store_version_id(client: ASCClient, app_id: str, version_string: str) -> str | None: + version = app_store_version(client, app_id, version_string) + return version["id"] if version else None + + +def remove_active_review_version(client: ASCClient, app_id: str, version_string: str, dry_run: bool = False) -> None: + version = app_store_version(client, app_id, version_string) + if version is None: + print(f"No App Store version {version_string} found to remove from review") + return + version_id = version["id"] + + submissions = client.request( + "GET", + "/reviewSubmissions", + { + "filter[app]": app_id, + "filter[platform]": "MAC_OS", + "include": "items,appStoreVersionForReview", + "fields[reviewSubmissions]": "platform,state,items,appStoreVersionForReview", + "fields[reviewSubmissionItems]": "state,appStoreVersion", + "limit": 20, + }, + ) + included = included_by_id(submissions) + for submission in submissions.get("data", []): + state = submission["attributes"].get("state") + if state not in ACTIVE_REVIEW_SUBMISSION_STATES: + continue + submission_version_id = relationship_id(submission, "appStoreVersionForReview") + if submission_version_id not in (None, version_id): + continue + item_ids = [item["id"] for item in submission.get("relationships", {}).get("items", {}).get("data", [])] + for item_id in item_ids: + item = included.get(item_id, {}) + item_version_id = relationship_id(item, "appStoreVersion") + if item_version_id not in (None, version_id): + continue + if submission_version_id != version_id and item_version_id != version_id: + continue + if dry_run: + print(f"Dry run: would remove App Store version {version_string} from review submission: {item_id}") + return + client.request("DELETE", f"/reviewSubmissionItems/{item_id}", allowed=(204,)) + print(f"Removed App Store version {version_string} from review submission: {item_id}") + return + state = version_state(version) + if state in BLOCKING_REVIEW_VERSION_STATES: + raise AppStoreConnectError( + f"App Store version {version_string} is {state}, but no active review submission item was found to remove" + ) + print(f"No active review submission item found for App Store version {version_string}") + + +def is_version_creation_state_conflict(error: AppStoreConnectError) -> bool: + if error.status != 409: + return False + text = json.dumps(error.payload or {}, sort_keys=True).lower() + return "current state" in text or "another version" in text or "new version" in text + + +def create_app_store_version(client: ASCClient, app_id: str, args: argparse.Namespace) -> dict[str, Any]: + return client.request( + "POST", + "/appStoreVersions", + body={ + "data": { + "type": "appStoreVersions", + "attributes": { + "platform": "MAC_OS", + "versionString": args.version, + "releaseType": args.release_type, + "copyright": args.copyright, + }, + "relationships": {"app": {"data": {"type": "apps", "id": app_id}}}, + } + }, + allowed=(201,), + )["data"] + + +def create_app_store_version_with_retry(client: ASCClient, app_id: str, args: argparse.Namespace) -> dict[str, Any]: + for attempt in range(CREATE_VERSION_MAX_ATTEMPTS): + try: + return create_app_store_version(client, app_id, args) + except AppStoreConnectError as error: + if attempt == CREATE_VERSION_MAX_ATTEMPTS - 1 or not is_version_creation_state_conflict(error): + raise + print("App Store Connect is still releasing the previous review state; retrying version creation") + time.sleep(CREATE_VERSION_RETRY_SECONDS) + raise AppStoreConnectError(f"failed to create App Store version {args.version}") + + def ensure_version(client: ASCClient, app_id: str, args: argparse.Namespace) -> dict[str, Any]: existing = client.request( "GET", @@ -190,34 +307,22 @@ def ensure_version(client: ASCClient, app_id: str, args: argparse.Namespace) -> version = existing["data"][0] print(f"Using App Store version {args.version}: {version['id']}") else: - version = client.request( - "POST", - "/appStoreVersions", - body={ - "data": { - "type": "appStoreVersions", - "attributes": { - "platform": "MAC_OS", - "versionString": args.version, - "releaseType": args.release_type, - "copyright": args.copyright, - }, - "relationships": {"app": {"data": {"type": "apps", "id": app_id}}}, - } - }, - allowed=(201,), - )["data"] + version = create_app_store_version_with_retry(client, app_id, args) print(f"Created App Store version {args.version}: {version['id']}") attributes: dict[str, Any] = { "releaseType": args.release_type, "copyright": args.copyright, "usesIdfa": args.uses_idfa, } - client.request( - "PATCH", - f"/appStoreVersions/{version['id']}", - body={"data": {"type": "appStoreVersions", "id": version["id"], "attributes": attributes}}, - ) + state = version_state(version) + if state in LOCKED_VERSION_STATES: + print(f"Skipping attribute update because App Store version is {state}") + else: + client.request( + "PATCH", + f"/appStoreVersions/{version['id']}", + body={"data": {"type": "appStoreVersions", "id": version["id"], "attributes": attributes}}, + ) return version @@ -236,7 +341,7 @@ def version_build_id(client: ASCClient, version_id: str) -> str | None: def attach_build(client: ASCClient, version: dict[str, Any], build: dict[str, Any], args: argparse.Namespace) -> None: version_id = version["id"] - state = version["attributes"].get("appStoreState") + state = version_state(version) attached_build_id = relationship_id(version, "build") or version_build_id(client, version_id) if attached_build_id == build["id"]: print(f"Build {args.build_number} is already attached") @@ -255,7 +360,7 @@ def attach_build(client: ASCClient, version: dict[str, Any], build: dict[str, An print(f"Attached build {args.build_number} to App Store version {args.version}") -def ensure_build(client: ASCClient, app_id: str, args: argparse.Namespace) -> dict[str, Any]: +def ensure_build(client: ASCClient, app_id: str, args: argparse.Namespace, allow_updates: bool = True) -> dict[str, Any]: payload = client.request( "GET", "/builds", @@ -273,17 +378,20 @@ def ensure_build(client: ASCClient, app_id: str, args: argparse.Namespace) -> di if attributes.get("processingState") != "VALID": raise AppStoreConnectError(f"build {args.build_number} is not valid: {attributes.get('processingState')}", payload=build) if args.non_exempt_encryption is not None and attributes.get("usesNonExemptEncryption") != args.non_exempt_encryption: - client.request( - "PATCH", - f"/builds/{build['id']}", - body={ - "data": { - "type": "builds", - "id": build["id"], - "attributes": {"usesNonExemptEncryption": args.non_exempt_encryption}, - } - }, - ) + if allow_updates: + client.request( + "PATCH", + f"/builds/{build['id']}", + body={ + "data": { + "type": "builds", + "id": build["id"], + "attributes": {"usesNonExemptEncryption": args.non_exempt_encryption}, + } + }, + ) + else: + print(f"Dry run: would update build {args.build_number} non-exempt encryption setting") print(f"Using valid build {args.build_number}: {build['id']}") return build @@ -294,13 +402,13 @@ def ensure_metadata(client: ASCClient, version_id: str, source_localization: dic f"/appStoreVersions/{version_id}", { "include": "appStoreVersionLocalizations,appStoreReviewDetail", - "fields[appStoreVersions]": "appStoreState,appStoreVersionLocalizations,appStoreReviewDetail", + "fields[appStoreVersions]": "appStoreState,appVersionState,appStoreVersionLocalizations,appStoreReviewDetail", "fields[appStoreVersionLocalizations]": "locale,description,keywords,marketingUrl,promotionalText,supportUrl,whatsNew", "fields[appStoreReviewDetails]": "contactFirstName,contactLastName,contactPhone,contactEmail,demoAccountName,demoAccountRequired,notes", }, ) version = current["data"] - state = version["attributes"].get("appStoreState") + state = version_state(version) if state in LOCKED_VERSION_STATES: print(f"Skipping metadata update because App Store version is {state}") return @@ -386,11 +494,10 @@ def ensure_review_submission(client: ASCClient, app_id: str, version_id: str, ar "limit": 20, }, ) - active_states = {"READY_FOR_REVIEW", "WAITING_FOR_REVIEW", "IN_REVIEW"} existing = None for submission in submissions.get("data", []): state = submission["attributes"].get("state") - if state in active_states and relationship_id(submission, "appStoreVersionForReview") == version_id: + if state in ACTIVE_REVIEW_SUBMISSION_STATES and relationship_id(submission, "appStoreVersionForReview") == version_id: existing = submission break if existing: @@ -457,6 +564,10 @@ def parse_args() -> argparse.Namespace: parser.add_argument("--build-number", required=True, help="CFBundleVersion uploaded to App Store Connect") parser.add_argument("--whats-new", required=True) parser.add_argument("--copy-from-version", help="Existing App Store version to copy localization and review details from") + parser.add_argument( + "--remove-active-review-version", + help="Existing App Store version to remove from active review before creating the target version", + ) parser.add_argument("--api-key", default=os.environ.get("APP_STORE_CONNECT_API_KEY_PATH")) parser.add_argument("--api-key-id", default=os.environ.get("APP_STORE_CONNECT_KEY_ID")) parser.add_argument("--api-issuer-id", default=os.environ.get("APP_STORE_CONNECT_ISSUER_ID")) @@ -489,8 +600,17 @@ def main() -> int: app_id = app["id"] print(f"Using app {app['attributes'].get('name')}: {app_id}") source_localization, source_review_detail = latest_source_metadata(client, app_id, args.copy_from_version) + build = ensure_build(client, app_id, args, allow_updates=not args.dry_run) + if args.remove_active_review_version: + # App Store Connect blocks creating a replacement version while another + # version is actively in review. Validate the source metadata and uploaded + # build first, then remove the old review item immediately before creating + # and submitting the replacement. + remove_active_review_version(client, app_id, args.remove_active_review_version, dry_run=args.dry_run) + if args.dry_run: + print("Dry run: metadata and build validated; no App Store Connect changes were made") + return 0 version = ensure_version(client, app_id, args) - build = ensure_build(client, app_id, args) attach_build(client, version, build, args) ensure_metadata(client, version["id"], source_localization, source_review_detail, args) submission = ensure_review_submission(client, app_id, version["id"], args) @@ -504,7 +624,7 @@ def main() -> int: "fields[appStoreVersionLocalizations]": "locale,whatsNew,supportUrl", }, ) - state = final["data"]["attributes"].get("appStoreState") + state = version_state(final["data"]) submission_state = submission["attributes"].get("state") if submission.get("attributes") else "unknown" print(f"App Store version {args.version} state: {state}") print(f"Review submission state: {submission_state}")