diff --git a/README.md b/README.md index 78b7b01..f5a3585 100644 --- a/README.md +++ b/README.md @@ -138,7 +138,7 @@ Checks an Android app against Google Play's Developer Program Policies and its p Deadlines: -- Target API level. New apps and updates must target API 36 from August 31, 2026. Apps below API 35 already lose distribution to new users on newer devices. CRITICAL / HIGH +- Target API level. New apps and updates must target API 36 from August 31, 2026. Apps below API 35 already lose distribution to new users on newer devices. CRITICAL / HIGH. Play runs a separate schedule for Wear OS, Android TV, Automotive, and XR, so an app declaring one of those form factors gets a WARN naming its track instead of a blocking finding on the phone schedule. - Play Billing Library. v7 and below lose support on August 31, 2026, and there is no direct v7 to v9 upgrade path. Versions reached through a variable or a version catalog `version.ref` are resolved. HIGH Restricted permissions, each of which needs an approved use case or a declaration form: @@ -162,9 +162,20 @@ Manifest and build: - An ads SDK shipped without `com.google.android.gms.permission.AD_ID`, which silently returns a zeroed advertising ID - Account creation without the required in-app *and* web deletion paths -`--apk` and `--aab` read the *merged* manifest, so they see permissions contributed by library manifests that a source scan structurally cannot. Every check above runs against a built artifact, plus native code: +`--apk` and `--aab` read the *merged* manifest, so they see permissions contributed by library manifests that a source scan structurally cannot. Every manifest and permission check above runs against a built artifact, plus the native code checks below. + +The three checks that read the Gradle model (Play Billing version, ads-SDK detection, auth-SDK detection) cannot run on an archive, because a built artifact does not carry one. The scan reports that gap as a finding rather than staying silent, so a clean artifact scan is never mistaken for a clean scan of everything. Source and artifact are complementary; `preflight` accepts both at once: + +```bash +greenlight preflight . --aab app-release.aab +``` + +Native code checks: - 16 KB page size. Google Play requires apps targeting Android 15+ to support 16 KB memory pages. Greenlight checks ELF `LOAD` segment alignment on `arm64-v8a` libraries, 16 KB zip alignment of uncompressed libraries, and `GNU_RELRO` presence. CRITICAL / HIGH +- 64-bit requirement. Every 32-bit ABI must ship with its 64-bit counterpart, so `armeabi-v7a` without `arm64-v8a`, or `x86` without `x86_64`, is flagged. CRITICAL + +In an AAB, native code is read from every module rather than `base/` alone, so a library that ships in a feature module is checked like any other. Both formats are decoded in pure Go, an APK's compiled binary XML and an AAB's protobuf manifest alike, so no Android SDK, `aapt2`, or `bundletool` is required. diff --git a/internal/cli/preflight.go b/internal/cli/preflight.go index 5f7ce6b..ae22935 100644 --- a/internal/cli/preflight.go +++ b/internal/cli/preflight.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "os" + "path/filepath" "sort" "strings" "time" @@ -17,6 +18,8 @@ import ( var ( preflightIPA string + preflightAPK string + preflightAAB string preflightFormat string preflightOutput string preflightExitCode bool @@ -63,6 +66,8 @@ Usage: func init() { preflightCmd.Flags().StringVar(&preflightIPA, "ipa", "", "path to .ipa file for binary inspection") + preflightCmd.Flags().StringVar(&preflightAPK, "apk", "", "path to a built .apk — adds the merged-manifest and native code checks") + preflightCmd.Flags().StringVar(&preflightAAB, "aab", "", "path to a built .aab — adds the merged-manifest and native code checks") preflightCmd.Flags().StringVar(&preflightFormat, "format", "terminal", "output format: terminal, json, sarif") preflightCmd.Flags().StringVar(&preflightOutput, "output", "", "write report to file (stdout if omitted)") preflightCmd.Flags().BoolVar(&preflightExitCode, "exit-code", false, "exit non-zero on any CRITICAL or HIGH finding (or, with --verify, a failed flow) — for CI gating") @@ -97,6 +102,20 @@ func runPreflight(cmd *cobra.Command, args []string) error { } } + // --apk and --aab both feed the same archive scanner, so only one can be set. + if preflightAPK != "" && preflightAAB != "" { + return fmt.Errorf("pass --apk or --aab, not both") + } + androidArtifact := preflightAPK + if androidArtifact == "" { + androidArtifact = preflightAAB + } + if androidArtifact != "" { + if _, err := os.Stat(androidArtifact); os.IsNotExist(err) { + return fmt.Errorf("Android artifact not found: %s", androidArtifact) + } + } + // Banner (suppressed for --format json so stdout stays valid JSON). if f := strings.ToLower(preflightFormat); f != "json" && f != "sarif" { purple.Println("\n greenlight preflight — every check, one command, zero uploads.") @@ -104,6 +123,9 @@ func runPreflight(cmd *cobra.Command, args []string) error { if preflightIPA != "" { fmt.Printf(" IPA: %s\n", preflightIPA) } + if androidArtifact != "" { + fmt.Printf(" Android: %s\n", androidArtifact) + } // Mirrors the gating in preflight.Run so the banner never advertises a // scanner that will not run. isIOS, isAndroid := preflight.DetectPlatforms(path) @@ -111,7 +133,7 @@ func runPreflight(cmd *cobra.Command, args []string) error { if isIOS || !isAndroid { scanners = append(scanners, "metadata", "codescan", "privacy") } - if isAndroid { + if isAndroid || androidArtifact != "" { scanners = append(scanners, "playscan") } if preflightIPA != "" { @@ -122,7 +144,7 @@ func runPreflight(cmd *cobra.Command, args []string) error { // Run all checks start := time.Now() - result, err := preflight.Run(path, preflightIPA, verbose) + result, err := preflight.Run(path, preflightIPA, androidArtifact, verbose) if err != nil { return fmt.Errorf("preflight failed: %w", err) } @@ -175,13 +197,16 @@ func runPreflight(cmd *cobra.Command, args []string) error { } // --verify: the static cycle runs, THEN Revyl runs the flows on a device. + // + // The platform follows the project rather than being fixed to iOS. An + // Android-only project verified as iOS would have its artifact rejected and + // its flows run against the wrong store's expectations. + verifyPlatform := verifyPlatformFor(path, preflightArtifact) if preflightArtifact != "" { if preflightBuildName == "" { return fmt.Errorf("--artifact requires --build-name (to name or match the Revyl app for the uploaded build)") } - // preflight's runtime tier is iOS-only (App Store), matching the - // hardcoded Platform: "ios" passed to verify.Run below. - if err := verify.ValidateArtifact(preflightArtifact, "ios"); err != nil { + if err := verify.ValidateArtifact(preflightArtifact, verifyPlatform); err != nil { return err } } @@ -189,7 +214,7 @@ func runPreflight(cmd *cobra.Command, args []string) error { vres, verr := verify.Run(verify.Config{ ProjectPath: path, BuildName: preflightBuildName, - Platform: "ios", + Platform: verifyPlatform, Vars: parseVars(preflightVarsRaw), DeviceModel: preflightDeviceModel, OSVersion: preflightOSVersion, @@ -538,3 +563,22 @@ func writeCombinedJSON(w *os.File, result *preflight.Result, vres *verify.Result enc.SetIndent("", " ") return enc.Encode(combined) } + +// verifyPlatformFor picks the platform the runtime tier should target. An +// explicit artifact extension is authoritative, since the file itself says what +// it is; otherwise the project layout decides, defaulting to iOS for a +// cross-platform or unrecognised project because the App Store flows are the +// ones greenlight models most completely. +func verifyPlatformFor(projectPath, artifact string) string { + switch strings.ToLower(filepath.Ext(artifact)) { + case ".apk": + return "android" + case ".app": + return "ios" + } + isIOS, isAndroid := preflight.DetectPlatforms(projectPath) + if isAndroid && !isIOS { + return "android" + } + return "ios" +} diff --git a/internal/cli/preflight_test.go b/internal/cli/preflight_test.go index ebbeb86..0cefa7b 100644 --- a/internal/cli/preflight_test.go +++ b/internal/cli/preflight_test.go @@ -2,6 +2,8 @@ package cli import ( "errors" + "os" + "path/filepath" "testing" "github.com/RevylAI/greenlight/internal/preflight" @@ -50,3 +52,40 @@ func TestPreflightExit(t *testing.T) { t.Errorf("clean static + passed runtime should not trip, got %v", err) } } + +// --- review regression: runtime tier platform --------------------------- + +// preflight --verify used to hardcode Platform: "ios", so an Android-only +// project had its flows run against the wrong store and its .apk rejected. +func TestVerifyPlatformFollowsProject(t *testing.T) { + androidOnly := t.TempDir() + if err := os.MkdirAll(filepath.Join(androidOnly, "app", "src", "main"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(androidOnly, "app", "src", "main", "AndroidManifest.xml"), + []byte(``), 0o644); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(androidOnly, "app", "build.gradle"), + []byte("android { defaultConfig { targetSdk 36 } }"), 0o644); err != nil { + t.Fatal(err) + } + + if got := verifyPlatformFor(androidOnly, ""); got != "android" { + t.Errorf("android-only project: platform = %q, want android", got) + } + + // An explicit artifact extension wins over project detection: the file + // itself is unambiguous about what it is. + if got := verifyPlatformFor(androidOnly, "build/MyApp.app"); got != "ios" { + t.Errorf("explicit .app: platform = %q, want ios", got) + } + if got := verifyPlatformFor(t.TempDir(), "build/app-release.apk"); got != "android" { + t.Errorf("explicit .apk: platform = %q, want android", got) + } + + // An empty or iOS project keeps the previous default. + if got := verifyPlatformFor(t.TempDir(), ""); got != "ios" { + t.Errorf("unrecognised project: platform = %q, want ios", got) + } +} diff --git a/internal/playscan/archive.go b/internal/playscan/archive.go index b04a02a..9692790 100644 --- a/internal/playscan/archive.go +++ b/internal/playscan/archive.go @@ -96,8 +96,9 @@ func ScanArchive(archivePath string) (*ScanResult, error) { ctx := &ruleContext{ manifest: manifest, - // A built archive carries no Gradle state; rules that read it are - // source-only and correctly stay silent. + // A built archive carries no Gradle state, so rules that read it + // cannot fire here. That is a coverage gap rather than a pass, and + // gradleOnlyCoverageFinding below says so instead of staying silent. gradle: &GradleInfo{}, targetSDK: result.TargetSDK, manifestFile: manifestEntry, @@ -107,13 +108,85 @@ func ScanArchive(archivePath string) (*ScanResult, error) { } } + // The Gradle-only gap exists whether or not the manifest decoded, so this + // sits outside the block above. + result.Findings = append(result.Findings, gradleOnlyCoverageFinding(archivePath)) + libs := collectNativeLibs(&zr.Reader, kind) result.NativeLibCount = len(libs) result.Findings = append(result.Findings, checkPageAlignment(libs, kind)...) + // ABI parity is a property of the whole app. A config split legitimately + // carries one ABI, so running the rule against it would report a violation + // that does not exist in the release it belongs to. + isSplit := manifest != nil && strings.TrimSpace(manifest.Split) != "" + if !isSplit { + result.Findings = append(result.Findings, check64BitParity(libs)...) + } + return result, nil } +// gradleOnlyCoverageFinding names the checks an artifact scan cannot perform. +// Play Billing version, ads-SDK detection, and auth-SDK detection all read the +// Gradle model, which a built archive does not carry. Reporting the gap keeps a +// clean artifact scan from reading as a clean scan of everything. +func gradleOnlyCoverageFinding(archivePath string) Finding { + return Finding{ + Severity: sevInfo, + Policy: "Scan coverage", + Title: "Dependency-based checks were skipped for this artifact", + Detail: "A built archive carries no Gradle state, so these source-only checks did not run: " + + "Play Billing Library version, ads-SDK detection (the AD_ID permission check), and auth-SDK detection " + + "(the account-deletion requirement). Manifest, permission, and native code checks all ran normally.", + Fix: "Run `greenlight playscan .` against the project source as well. The two scans are complementary: source sees dependencies, the artifact sees the merged manifest.", + File: archivePath, + } +} + +// abi64For maps each 32-bit ABI Play recognises to the 64-bit ABI that must +// ship alongside it. +var abi64For = map[string]string{ + "armeabi-v7a": "arm64-v8a", + "x86": "x86_64", +} + +// check64BitParity enforces Google Play's 64-bit requirement: an app that ships +// a 32-bit native ABI must ship the matching 64-bit ABI too. An app with no +// native code is unaffected, as is one that is already 64-bit only. +func check64BitParity(libs []nativeLib) []Finding { + if len(libs) == 0 { + return nil + } + + present := make(map[string]bool) + for _, lib := range libs { + present[lib.ABI] = true + } + + var missing []string + for abi32, abi64 := range abi64For { + if present[abi32] && !present[abi64] { + missing = append(missing, fmt.Sprintf("%s is present with no %s", abi32, abi64)) + } + } + if len(missing) == 0 { + return nil + } + sort.Strings(missing) + + return []Finding{{ + Severity: sevCritical, + Policy: "64-bit requirement", + Title: "32-bit native code ships without its 64-bit counterpart", + Detail: "Google Play requires every app with native code to provide a 64-bit library for each 32-bit ABI it supports. " + + strings.Join(missing, "; ") + ". Play rejects a bundle that carries only 32-bit native code for an ABI.", + Fix: "Add the 64-bit ABIs to your NDK build (abiFilters or ndk.abiFilters), rebuild, and confirm every dependency ships 64-bit libraries too. " + + "Dropping the 32-bit ABI entirely also satisfies the requirement.", + Doc: doc64Bit, + }} +} + // classifyArchive determines the format and locates the manifest entry. func classifyArchive(zr *zip.Reader) (ArchiveKind, string) { var hasBundleLayout bool @@ -148,15 +221,18 @@ func readZipEntry(zr *zip.Reader, name string) ([]byte, error) { // collectNativeLibs finds every packaged .so and reads what the alignment // checks need. Both archive layouts are handled. +// +// An APK holds native code at lib//. An AAB holds it at /lib//, +// and a bundle has one module directory per feature module on top of "base", so +// scanning only base/ misses native code that ships in a feature module. func collectNativeLibs(zr *zip.Reader, kind ArchiveKind) []nativeLib { - prefix := "lib/" - if kind == KindAAB { - prefix = "base/lib/" - } - var libs []nativeLib for _, f := range zr.File { - if !strings.HasPrefix(f.Name, prefix) || !strings.HasSuffix(f.Name, ".so") { + if !strings.HasSuffix(f.Name, ".so") { + continue + } + prefix, ok := nativeLibPrefix(f.Name, kind) + if !ok { continue } lib := nativeLib{ @@ -189,6 +265,28 @@ func collectNativeLibs(zr *zip.Reader, kind ArchiveKind) []nativeLib { return libs } +// nativeLibPrefix reports the "<...>lib/" prefix a packaged .so sits under, and +// whether the entry is native code at all. For an AAB any top-level module +// qualifies, so feature-module native code is scanned alongside base/. +func nativeLibPrefix(name string, kind ArchiveKind) (string, bool) { + if kind != KindAAB { + if strings.HasPrefix(name, "lib/") { + return "lib/", true + } + return "", false + } + // /lib//.so + slash := strings.Index(name, "/") + if slash <= 0 { + return "", false + } + prefix := name[:slash+1] + "lib/" + if !strings.HasPrefix(name, prefix) { + return "", false + } + return prefix, true +} + func abiFromPath(name, prefix string) string { rest := strings.TrimPrefix(name, prefix) if i := strings.Index(rest, "/"); i > 0 { diff --git a/internal/playscan/archive_test.go b/internal/playscan/archive_test.go index 90ae1cd..f3fe759 100644 --- a/internal/playscan/archive_test.go +++ b/internal/playscan/archive_test.go @@ -591,8 +591,17 @@ func TestScanArchiveAABEndToEnd(t *testing.T) { if findByPolicy(res.Findings, "Package visibility") == nil { t.Error("QUERY_ALL_PACKAGES from the bundle manifest was not flagged") } - if findByPolicy(res.Findings, "Scan coverage") != nil { - t.Error("a valid protobuf manifest should decode cleanly") + // A valid protobuf manifest should decode cleanly, so the decode-failure + // coverage finding must be absent. The dependency-coverage finding shares the + // "Scan coverage" policy and is expected on every artifact scan, so assert on + // the specific title rather than the category. + for _, f := range res.Findings { + if strings.Contains(f.Title, "Could not decode") { + t.Errorf("a valid protobuf manifest should decode cleanly, got: %s", f.Title) + } + } + if findByTitle(res.Findings, "Dependency-based checks were skipped for this artifact") == nil { + t.Error("an artifact scan should report the Gradle-only checks it could not run") } if f := findByPolicy(res.Findings, "16 KB page size"); f != nil { t.Errorf("aligned bundle library reported: %s", f.Title) @@ -736,6 +745,7 @@ func TestFrameworkAttrIDsMatchAapt2(t *testing.T) { 0x0101000f: "debuggable", 0x01010010: "exported", 0x0101020c: "minSdkVersion", + 0x0101028e: "required", 0x01010270: "targetSdkVersion", 0x01010280: "allowBackup", 0x010104ec: "usesCleartextTraffic", @@ -771,3 +781,310 @@ func TestAttrNameFallsBackToResourceMap(t *testing.T) { t.Errorf("string pool name should take precedence, got %q", got) } } + +// --- review regressions: native code coverage --------------------------- + +// buildTestAAB writes a bundle with the given zip entries plus a decodable +// protobuf manifest at base/manifest/AndroidManifest.xml. +func buildTestAAB(t *testing.T, entries map[string][]byte) string { + t.Helper() + path := filepath.Join(t.TempDir(), "app.aab") + f, err := os.Create(path) + if err != nil { + t.Fatalf("create: %v", err) + } + defer f.Close() + + zw := zip.NewWriter(f) + mw, err := zw.Create("base/manifest/AndroidManifest.xml") + if err != nil { + t.Fatalf("zip create: %v", err) + } + manifest := pbElement("manifest", + [][]byte{pbAttr("package", "com.example.bundle")}, + [][]byte{ + pbElement("uses-sdk", [][]byte{pbAttr("targetSdkVersion", "36")}, nil), + pbElement("application", nil, nil), + }, + ) + if _, err := mw.Write(manifest); err != nil { + t.Fatalf("write manifest: %v", err) + } + for name, data := range entries { + w, err := zw.Create(name) + if err != nil { + t.Fatalf("zip create %s: %v", name, err) + } + if _, err := w.Write(data); err != nil { + t.Fatalf("write %s: %v", name, err) + } + } + if err := zw.Close(); err != nil { + t.Fatalf("zip close: %v", err) + } + return path +} + +// An AAB can carry native code in any module, not just base. Scanning only +// base/lib/ let a misaligned feature-module library pass. +func TestScanArchiveScansFeatureModuleLibs(t *testing.T) { + aab := buildTestAAB(t, map[string][]byte{ + "base/lib/arm64-v8a/libbase.so": buildTestELF(16384, true), + "payments/lib/arm64-v8a/libpay.so": buildTestELF(4096, true), + "onboarding/lib/arm64-v8a/libonb.so": buildTestELF(16384, true), + }) + + res, err := ScanArchive(aab) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + if res.NativeLibCount != 3 { + t.Errorf("NativeLibCount = %d, want 3 (base + two feature modules)", res.NativeLibCount) + } + f := findByPolicy(res.Findings, "16 KB page size") + if f == nil { + t.Fatal("a misaligned feature-module library was not flagged") + } + if !strings.Contains(f.Detail, "libpay.so") { + t.Errorf("finding does not name the offending library: %s", f.Detail) + } +} + +// Play requires a 64-bit library for every 32-bit ABI shipped. +func TestScanArchive64BitParity(t *testing.T) { + t.Run("32-bit only is flagged", func(t *testing.T) { + apk := buildTestAPK(t, map[string][]byte{ + "lib/armeabi-v7a/libnative.so": buildTestELF(16384, true), + }) + res, err := ScanArchive(apk) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + f := findByPolicy(res.Findings, "64-bit requirement") + if f == nil { + t.Fatal("armeabi-v7a without arm64-v8a was not flagged") + } + if f.Severity != sevCritical { + t.Errorf("severity = %v, want CRITICAL", f.Severity) + } + }) + + t.Run("both ABIs present is clean", func(t *testing.T) { + apk := buildTestAPK(t, map[string][]byte{ + "lib/armeabi-v7a/libnative.so": buildTestELF(16384, true), + "lib/arm64-v8a/libnative.so": buildTestELF(16384, true), + }) + res, err := ScanArchive(apk) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + if f := findByPolicy(res.Findings, "64-bit requirement"); f != nil { + t.Errorf("a 32/64 pair should not be flagged: %s", f.Title) + } + }) + + t.Run("64-bit only is clean", func(t *testing.T) { + apk := buildTestAPK(t, map[string][]byte{ + "lib/arm64-v8a/libnative.so": buildTestELF(16384, true), + }) + res, err := ScanArchive(apk) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + if f := findByPolicy(res.Findings, "64-bit requirement"); f != nil { + t.Errorf("a 64-bit-only app should not be flagged: %s", f.Title) + } + }) + + t.Run("x86 without x86_64 is flagged", func(t *testing.T) { + apk := buildTestAPK(t, map[string][]byte{ + "lib/x86/libnative.so": buildTestELF(16384, true), + }) + res, err := ScanArchive(apk) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + if findByPolicy(res.Findings, "64-bit requirement") == nil { + t.Error("x86 without x86_64 was not flagged") + } + }) +} + +// An artifact scan cannot read the Gradle model, so it must say which checks it +// skipped rather than presenting a partial scan as a complete one. +func TestScanArchiveReportsGradleOnlyCoverageGap(t *testing.T) { + apk := buildTestAPK(t, map[string][]byte{ + "lib/arm64-v8a/libnative.so": buildTestELF(16384, true), + }) + res, err := ScanArchive(apk) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + f := findByTitle(res.Findings, "Dependency-based checks were skipped for this artifact") + if f == nil { + t.Fatal("artifact scan did not report its coverage gap") + } + for _, want := range []string{"Play Billing", "ads-SDK", "auth-SDK"} { + if !strings.Contains(f.Detail, want) { + t.Errorf("coverage finding does not mention %q: %s", want, f.Detail) + } + } +} + +// --- code review regressions -------------------------------------------- + +// buildTestAABWithFeature writes a bundle whose protobuf manifest declares a +// uses-feature and a targetSdk, which is what a real Wear or TV bundle looks +// like after the merge. +func buildTestAABWithFeature(t *testing.T, feature, required string, targetSDK string) string { + t.Helper() + path := filepath.Join(t.TempDir(), "app.aab") + f, err := os.Create(path) + if err != nil { + t.Fatalf("create: %v", err) + } + defer f.Close() + + featureAttrs := [][]byte{pbAttr("name", feature)} + if required != "" { + featureAttrs = append(featureAttrs, pbAttr("required", required)) + } + + zw := zip.NewWriter(f) + mw, err := zw.Create("base/manifest/AndroidManifest.xml") + if err != nil { + t.Fatalf("zip create: %v", err) + } + manifest := pbElement("manifest", + [][]byte{pbAttr("package", "com.example.bundle")}, + [][]byte{ + pbElement("uses-sdk", [][]byte{pbAttr("targetSdkVersion", targetSDK)}, nil), + pbElement("uses-feature", featureAttrs, nil), + pbElement("application", nil, nil), + }, + ) + if _, err := mw.Write(manifest); err != nil { + t.Fatalf("write manifest: %v", err) + } + if err := zw.Close(); err != nil { + t.Fatalf("zip close: %v", err) + } + return path +} + +// The form-factor rule was inert on the archive path: applyElement had no +// uses-feature case, so a compiled manifest never populated UsesFeatures and a +// Wear bundle still got the phone schedule's CRITICAL. +func TestScanArchiveDecodesUsesFeatureForFormFactor(t *testing.T) { + aab := buildTestAABWithFeature(t, "android.hardware.type.watch", "", "33") + res, err := ScanArchive(aab) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + f := findByPolicy(res.Findings, "Target API level") + if f == nil { + t.Fatal("no target API finding") + } + if f.Severity != sevWarn { + t.Errorf("severity = %v, want WARN — a Wear bundle must not be gated on the phone schedule (got %q)", f.Severity, f.Title) + } + if !strings.Contains(f.Title, string(FormFactorWear)) { + t.Errorf("finding does not name the form factor: %s", f.Title) + } +} + +// A phone app that also ships to TV declares leanback with required="false". +// Treating that as a TV app would downgrade a genuine phone CRITICAL. +func TestScanArchiveIgnoresUnrequiredFormFactorFeature(t *testing.T) { + aab := buildTestAABWithFeature(t, "android.software.leanback", "false", "33") + res, err := ScanArchive(aab) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + f := findByPolicy(res.Findings, "Target API level") + if f == nil { + t.Fatal("no target API finding") + } + if f.Severity != sevCritical { + t.Errorf("severity = %v, want CRITICAL — leanback required=false is a phone app, not a TV app", f.Severity) + } +} + +// The Gradle coverage gap exists whether or not the manifest decoded. +func TestScanArchiveReportsCoverageGapWhenManifestUndecodable(t *testing.T) { + path := filepath.Join(t.TempDir(), "app.aab") + f, _ := os.Create(path) + zw := zip.NewWriter(f) + w, _ := zw.Create("base/manifest/AndroidManifest.xml") + w.Write([]byte{0x00}) + zw.Close() + f.Close() + + res, err := ScanArchive(path) + if err != nil { + t.Fatalf("ScanArchive: %v", err) + } + if findByTitle(res.Findings, "Dependency-based checks were skipped for this artifact") == nil { + t.Error("the Gradle-only gap must be reported even when the manifest cannot be decoded") + } +} + +// The APK path decodes uses-feature from binary XML, where android:required is +// a typed boolean rather than the literal string "false". Both compiled forms +// must reach the form-factor logic identically. +func TestBinaryXMLDecodesUsesFeatureRequired(t *testing.T) { + build := func(requiredData uint32, withRequired bool) *Manifest { + t.Helper() + b := &axmlBuilder{} + nameAttr := b.attrRef(0x01010003) + requiredAttr := b.attrRef(0x0101028e) // android:required + pkgIdx := b.str("com.example.tv") + featIdx := b.str("android.software.leanback") + packageAttr := b.str("package") + + b.startTag("manifest", []buildAttr{ + {nameIdx: packageAttr, dataType: typeString, data: pkgIdx, rawIdx: pkgIdx}, + }) + attrs := []buildAttr{ + {nameIdx: nameAttr, dataType: typeString, data: featIdx, rawIdx: featIdx}, + } + if withRequired { + attrs = append(attrs, buildAttr{ + nameIdx: requiredAttr, dataType: typeIntBoolean, data: requiredData, rawIdx: 0xFFFFFFFF, + }) + } + b.startTag("uses-feature", attrs) + b.endTag("uses-feature") + b.startTag("application", nil) + b.endTag("application") + b.endTag("manifest") + + m, err := DecodeBinaryXML(b.build()) + if err != nil { + t.Fatalf("DecodeBinaryXML: %v", err) + } + return m + } + + t.Run("required=false is not a TV app", func(t *testing.T) { + m := build(0, true) + if len(m.UsesFeatures) != 1 { + t.Fatalf("UsesFeatures = %d, want 1 (binary XML must decode uses-feature)", len(m.UsesFeatures)) + } + if got := m.FormFactor(); got != FormFactorPhone { + t.Errorf("FormFactor = %q, want phone — leanback required=false is a phone app", got) + } + }) + + t.Run("required=true is a TV app", func(t *testing.T) { + if got := build(0xFFFFFFFF, true).FormFactor(); got != FormFactorTV { + t.Errorf("FormFactor = %q, want %q", got, FormFactorTV) + } + }) + + t.Run("absent required defaults to true", func(t *testing.T) { + if got := build(0, false).FormFactor(); got != FormFactorTV { + t.Errorf("FormFactor = %q, want %q — required defaults to true when absent", got, FormFactorTV) + } + }) +} diff --git a/internal/playscan/axml.go b/internal/playscan/axml.go index a89b7cf..acbfe4b 100644 --- a/internal/playscan/axml.go +++ b/internal/playscan/axml.go @@ -122,12 +122,22 @@ func applyElement(m *Manifest, name string, attrs []axmlAttr, path []string, cur switch name { case "manifest": m.Package = get("package") + m.Split = get("split") case "uses-permission": m.Permissions = append(m.Permissions, UsesPermission{Name: get("name")}) case "uses-permission-sdk-23": m.PermissionsSDK23 = append(m.PermissionsSDK23, UsesPermission{Name: get("name")}) + case "uses-feature": + // The merged manifest is where a form factor declaration is + // authoritative, so this has to be decoded on the archive path too, not + // only by the source scan's encoding/xml pass. + m.UsesFeatures = append(m.UsesFeatures, UsesFeature{ + Name: get("name"), + Required: get("required"), + }) + case "uses-sdk": m.UsesSDK = &UsesSDK{ MinSDKVersion: get("minSdkVersion"), @@ -262,6 +272,7 @@ var frameworkAttrIDs = map[uint32]string{ 0x0101000f: "debuggable", 0x01010010: "exported", 0x0101020c: "minSdkVersion", + 0x0101028e: "required", 0x01010270: "targetSdkVersion", 0x01010280: "allowBackup", 0x010104ec: "usesCleartextTraffic", diff --git a/internal/playscan/manifest.go b/internal/playscan/manifest.go index 094b96d..2b4ec09 100644 --- a/internal/playscan/manifest.go +++ b/internal/playscan/manifest.go @@ -14,13 +14,18 @@ import ( // working on manifests that bind the android prefix unusually or omit the // xmlns declaration entirely (both appear in real generated manifests). type Manifest struct { - XMLName xml.Name `xml:"manifest"` - Package string `xml:"package,attr"` + XMLName xml.Name `xml:"manifest"` + Package string `xml:"package,attr"` + // Split names the split APK this manifest belongs to ("config.armeabi_v7a", + // a feature module name). A split carries only part of the app, so + // whole-app rules that depend on seeing everything must not run on one. + Split string `xml:"split,attr"` Permissions []UsesPermission `xml:"uses-permission"` // PermissionsSDK23 covers , which grants the same // policy obligations as a plain . PermissionsSDK23 []UsesPermission `xml:"uses-permission-sdk-23"` UsesSDK *UsesSDK `xml:"uses-sdk"` + UsesFeatures []UsesFeature `xml:"uses-feature"` Application *Application `xml:"application"` } @@ -28,6 +33,63 @@ type UsesPermission struct { Name string `xml:"name,attr"` } +type UsesFeature struct { + Name string `xml:"name,attr"` + // Required is the raw android:required attribute. It defaults to true when + // absent, and a phone app that also ships to TV declares leanback with + // required="false" — so an unrequired feature must not move the app off the + // phone track. + Required string `xml:"required,attr"` +} + +// isRequired reports whether the feature is required, which is the default when +// the attribute is absent or unparseable. +func (f UsesFeature) isRequired() bool { + return !strings.EqualFold(strings.TrimSpace(f.Required), "false") +} + +// FormFactor is the Play distribution channel an app targets. Play sets target +// API level requirements per form factor, so a Wear or TV app is not held to +// the phone schedule. +type FormFactor string + +const ( + FormFactorPhone FormFactor = "phone" + FormFactorWear FormFactor = "Wear OS" + FormFactorTV FormFactor = "Android TV" + FormFactorAutomotive FormFactor = "Android Automotive" + FormFactorXR FormFactor = "Android XR" +) + +// featureFormFactors maps the declaration that puts an app on a +// non-phone Play track to that form factor. +var featureFormFactors = map[string]FormFactor{ + "android.hardware.type.watch": FormFactorWear, + "android.hardware.type.television": FormFactorTV, + "android.software.leanback": FormFactorTV, + "android.hardware.type.automotive": FormFactorAutomotive, + "android.software.xr.immersive": FormFactorXR, + "android.hardware.xr.head_tracking": FormFactorXR, +} + +// FormFactor reports the non-phone form factor this manifest declares, or +// FormFactorPhone when it declares none. Phone and tablet share one schedule, +// so they are not distinguished. +func (m *Manifest) FormFactor() FormFactor { + if m == nil { + return FormFactorPhone + } + for _, f := range m.UsesFeatures { + if !f.isRequired() { + continue + } + if ff, ok := featureFormFactors[f.Name]; ok { + return ff + } + } + return FormFactorPhone +} + type UsesSDK struct { MinSDKVersion string `xml:"minSdkVersion,attr"` TargetSDKVersion string `xml:"targetSdkVersion,attr"` diff --git a/internal/playscan/rules.go b/internal/playscan/rules.go index 0d5a60e..7578f49 100644 --- a/internal/playscan/rules.go +++ b/internal/playscan/rules.go @@ -20,6 +20,7 @@ const ( docProgramPolicy = "https://support.google.com/googleplay/android-developer/answer/16810878" docJul2026Policy = "https://support.google.com/googleplay/android-developer/answer/17134731" docPageSizes = "https://developer.android.com/guide/practices/page-sizes" + doc64Bit = "https://developer.android.com/google/play/requirements/64-bit" ) // Google Play's published requirements, as of the 2026 cycle. @@ -59,6 +60,15 @@ func (c *ruleContext) hasPermission(name string) bool { return c.manifest != nil && c.manifest.HasPermission(name) } +// formFactor reports the Play track this app ships on, defaulting to phone when +// there is no manifest to read. +func (c *ruleContext) formFactor() FormFactor { + if c.manifest == nil { + return FormFactorPhone + } + return c.manifest.FormFactor() +} + type rule func(*ruleContext) []Finding func allRules() []rule { @@ -100,6 +110,32 @@ func ruleTargetAPILevel(c *ruleContext) []Finding { line = 0 } + // Play publishes a separate target API schedule per form factor. Holding a + // Wear, TV, Automotive, or XR app to the phone schedule produces a blocking + // finding Play would not produce, so report it without gating the build. + // + // An unresolved targetSdk falls through to the shared "could not determine" + // finding below, because not knowing the value is worth reporting on every + // track. + if ff := c.formFactor(); ff != FormFactorPhone && c.targetSDK != 0 { + if c.targetSDK >= requiredTargetSDKNew { + return nil + } + return []Finding{{ + Severity: sevWarn, + Policy: "Target API level", + Title: fmt.Sprintf("targetSdk %d is below the phone requirement, and this is a %s app", c.targetSDK, ff), + Detail: fmt.Sprintf( + "This manifest declares a %s app, which Play holds to its own target API schedule rather than the phone and tablet one. "+ + "targetSdk %d is below the API %d the phone track requires from August 31, 2026, but the deadline that applies here is the %s schedule.", + ff, c.targetSDK, requiredTargetSDKNew, ff), + Fix: fmt.Sprintf("Check the target API level Play requires for %s and confirm this app meets it. If the app also ships to phones, raise targetSdk to %d.", ff, requiredTargetSDKNew), + Doc: docTargetAPI, + File: file, + Line: line, + }} + } + if c.targetSDK == 0 { // Nothing to check against, but silence would be misleading: the value // exists somewhere the scan could not resolve (an ext property, a diff --git a/internal/playscan/rules_test.go b/internal/playscan/rules_test.go new file mode 100644 index 0000000..07c60c8 --- /dev/null +++ b/internal/playscan/rules_test.go @@ -0,0 +1,73 @@ +package playscan + +import "testing" + +// --- review regression: form factor ------------------------------------- + +// Play publishes a separate target API schedule per form factor, so a Wear, TV, +// Automotive, or XR app must not get the phone track's blocking finding. +func TestTargetAPILevelRespectsFormFactor(t *testing.T) { + cases := []struct { + name string + feature string + want FormFactor + }{ + {"wear", "android.hardware.type.watch", FormFactorWear}, + {"tv", "android.software.leanback", FormFactorTV}, + {"automotive", "android.hardware.type.automotive", FormFactorAutomotive}, + {"xr", "android.software.xr.immersive", FormFactorXR}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := &ruleContext{ + manifest: &Manifest{ + UsesFeatures: []UsesFeature{{Name: tc.feature}}, + }, + gradle: &GradleInfo{}, + targetSDK: 33, + manifestFile: "AndroidManifest.xml", + } + if got := c.formFactor(); got != tc.want { + t.Fatalf("formFactor = %q, want %q", got, tc.want) + } + findings := ruleTargetAPILevel(c) + if len(findings) != 1 { + t.Fatalf("want 1 finding, got %d", len(findings)) + } + if findings[0].Severity != sevWarn { + t.Errorf("severity = %v, want WARN (a non-phone app must not be gated on the phone schedule)", findings[0].Severity) + } + }) + } +} + +// A phone app keeps the blocking behaviour. +func TestTargetAPILevelPhoneStillBlocks(t *testing.T) { + c := &ruleContext{ + manifest: &Manifest{}, + gradle: &GradleInfo{}, + targetSDK: 33, + manifestFile: "AndroidManifest.xml", + } + findings := ruleTargetAPILevel(c) + if len(findings) != 1 { + t.Fatalf("want 1 finding, got %d", len(findings)) + } + if findings[0].Severity != sevCritical { + t.Errorf("severity = %v, want CRITICAL for a phone app below the floor", findings[0].Severity) + } +} + +// A non-phone app already at or above the phone requirement needs no finding. +func TestTargetAPILevelFormFactorCleanWhenCurrent(t *testing.T) { + c := &ruleContext{ + manifest: &Manifest{ + UsesFeatures: []UsesFeature{{Name: "android.hardware.type.watch"}}, + }, + gradle: &GradleInfo{}, + targetSDK: requiredTargetSDKNew, + } + if findings := ruleTargetAPILevel(c); len(findings) != 0 { + t.Errorf("want no findings, got %d: %s", len(findings), findings[0].Title) + } +} diff --git a/internal/playscan/scanner_test.go b/internal/playscan/scanner_test.go index 53a5a5e..c633b9b 100644 --- a/internal/playscan/scanner_test.go +++ b/internal/playscan/scanner_test.go @@ -34,6 +34,15 @@ func findByPolicy(findings []Finding, policy string) *Finding { return nil } +func findByTitle(findings []Finding, title string) *Finding { + for i := range findings { + if findings[i].Title == title { + return &findings[i] + } + } + return nil +} + func countByPolicy(findings []Finding, policy string) int { n := 0 for _, f := range findings { diff --git a/internal/preflight/runner.go b/internal/preflight/runner.go index 713ed17..a044d13 100644 --- a/internal/preflight/runner.go +++ b/internal/preflight/runner.go @@ -33,11 +33,12 @@ type Finding struct { // Result holds the combined output from all scanners. type Result struct { - ProjectPath string `json:"project_path"` - IPAPath string `json:"ipa_path,omitempty"` - Findings []Finding `json:"findings"` - Summary Summary `json:"summary"` - Elapsed time.Duration `json:"elapsed"` + ProjectPath string `json:"project_path"` + IPAPath string `json:"ipa_path,omitempty"` + AndroidArtifact string `json:"android_artifact,omitempty"` + Findings []Finding `json:"findings"` + Summary Summary `json:"summary"` + Elapsed time.Duration `json:"elapsed"` // Incomplete is true when a requested sub-scanner failed to run, so the // results may be partial. CI gating (--exit-code) treats this as a failure. @@ -66,11 +67,15 @@ type Summary struct { Passed bool `json:"passed"` // true if zero CRITICALs } -// Run executes all scanners and returns a unified result. -func Run(projectPath string, ipaPath string, verbose bool) (*Result, error) { +// Run executes every applicable scanner. androidArtifact is an optional .apk or +// .aab: when set, the archive is scanned in addition to the source tree, because +// the two see different things (source resolves the Gradle model, the archive +// carries the merged manifest and the native libraries). +func Run(projectPath string, ipaPath string, androidArtifact string, verbose bool) (*Result, error) { result := &Result{ - ProjectPath: projectPath, - IPAPath: ipaPath, + ProjectPath: projectPath, + IPAPath: ipaPath, + AndroidArtifact: androidArtifact, } // Optional .greenlight.yml (rule overrides / ignores) for the code scan. @@ -181,30 +186,57 @@ func Run(projectPath string, ipaPath string, verbose bool) (*Result, error) { // // A cross-platform repo satisfies both this and runApple, so it is checked // against both stores in a single pass. - if isAndroid { + if isAndroid || androidArtifact != "" { wg.Add(1) go func() { defer wg.Done() - playResult, err := playscan.Scan(projectPath) - if err != nil { - errs <- err - return + + // Source and archive are complementary, so when both are available + // both run and their findings are merged. + // + // A failure in one does not discard the other: the error is recorded + // (which marks the run incomplete) and whatever did scan is still + // merged, so a broken --apk cannot silently erase the source + // findings that already succeeded. + var results []*playscan.ScanResult + if isAndroid { + playResult, err := playscan.Scan(projectPath) + if err != nil { + errs <- err + } else { + results = append(results, playResult) + } } + if androidArtifact != "" { + archiveResult, err := playscan.ScanArchive(androidArtifact) + if err != nil { + errs <- err + } else { + results = append(results, archiveResult) + } + } + mu.Lock() - result.PackageName = playResult.PackageName - result.TargetSDK = playResult.TargetSDK - for _, f := range playResult.Findings { - result.Findings = append(result.Findings, Finding{ - Source: "playscan", - Severity: f.Severity, - Guideline: f.Policy, - Title: f.Title, - Detail: f.Detail, - Fix: f.Fix, - Doc: f.Doc, - File: f.File, - Line: f.Line, - }) + for _, playResult := range results { + if playResult.PackageName != "" { + result.PackageName = playResult.PackageName + } + if playResult.TargetSDK != 0 { + result.TargetSDK = playResult.TargetSDK + } + for _, f := range playResult.Findings { + result.Findings = append(result.Findings, Finding{ + Source: "playscan", + Severity: f.Severity, + Guideline: f.Policy, + Title: f.Title, + Detail: f.Detail, + Fix: f.Fix, + Doc: f.Doc, + File: f.File, + Line: f.Line, + }) + } } mu.Unlock() }() diff --git a/internal/preflight/runner_test.go b/internal/preflight/runner_test.go index f923fc9..1c4ae9d 100644 --- a/internal/preflight/runner_test.go +++ b/internal/preflight/runner_test.go @@ -56,7 +56,7 @@ func TestRunDetectsAndroidProject(t *testing.T) { `) - result, err := Run(root, "", false) + result, err := Run(root, "", "", false) if err != nil { t.Fatalf("Run: %v", err) } @@ -88,7 +88,7 @@ func TestRunLeavesIOSProjectsUnchanged(t *testing.T) { mustWrite(t, root, "app.json", `{"expo":{"name":"Demo","description":"d","version":"1.0.0","icon":"./icon.png","ios":{"bundleIdentifier":"com.example.demo"}}}`) mustWrite(t, root, "App.swift", "import SwiftUI\n") - result, err := Run(root, "", false) + result, err := Run(root, "", "", false) if err != nil { t.Fatalf("Run: %v", err) } @@ -125,7 +125,7 @@ func TestAndroidOnlyProjectGetsNoAppleFindings(t *testing.T) { `) mustWrite(t, root, "app/src/main/java/com/example/app/MainActivity.kt", "package com.example.app\n") - result, err := Run(root, "", false) + result, err := Run(root, "", "", false) if err != nil { t.Fatalf("Run: %v", err) } @@ -153,7 +153,7 @@ func TestCrossPlatformProjectRunsBothScanners(t *testing.T) { t.Fatalf("DetectPlatforms = (ios=%v, android=%v), want both", ios, android) } - result, err := Run(root, "", false) + result, err := Run(root, "", "", false) if err != nil { t.Fatalf("Run: %v", err) } @@ -225,3 +225,46 @@ func TestDetectIOSSkipsVendoredDirectories(t *testing.T) { t.Error("vendored iOS sources should not classify the repo as iOS") } } + +// A failing archive scan must not discard the source-scan findings that already +// succeeded: the run is marked incomplete, but what did scan is still reported. +func TestArchiveFailureKeepsSourceFindings(t *testing.T) { + root := t.TempDir() + mustWrite := func(rel, body string) { + t.Helper() + full := filepath.Join(root, rel) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + // An Android project with a debuggable manifest, which playscan flags. + mustWrite("app/src/main/AndroidManifest.xml", + ``) + mustWrite("app/build.gradle", "android { defaultConfig { targetSdk 36 } }") + + // A file that is not a valid archive, so ScanArchive errors. + broken := filepath.Join(root, "broken.apk") + if err := os.WriteFile(broken, []byte("not a zip"), 0o644); err != nil { + t.Fatal(err) + } + + result, err := Run(root, "", broken, false) + if err != nil { + t.Fatalf("Run: %v", err) + } + if !result.Incomplete { + t.Error("a failed archive scan must mark the run incomplete") + } + var playFindings int + for _, f := range result.Findings { + if f.Source == "playscan" { + playFindings++ + } + } + if playFindings == 0 { + t.Error("source playscan findings were discarded when the archive scan failed") + } +}