ci: Add automated client apk build workflow

[pugal66]

• Add manual trigger to generate white-labeled client apps from GitHub Actions.
• Injects secure API key from secrets and passes configuration on the fly.
• Generated APKs are automatically uploaded as artifacts for easy download.

Note: This PR is intentionally exempt from the OpenSpec process as it is strictly a CI/tooling addition and does not modify any application features or architecture.

[claude]

Claude finished @pugal66's task in 1m 47s —— View job

---

Todo List

• Read AGENTS.md
• Read packages/core/docs/ai_context.md and architecture.md
• Fetch and review the PR diff (.github/workflows/build_client_apk.yaml)
• Post structured review

---

PR Review Summary

This PR adds a single CI workflow file (.github/workflows/build_client_apk.yaml) with a workflow_dispatch trigger to build white-labeled client APKs. No Dart/Flutter application code is touched, so the SDK-boundary, design-system, accessibility, motion, navigation, Riverpod, data-layer, and localization rules from AGENTS.md/ai_context.md don't apply here — this review focuses on CI/workflow correctness and security.

✅ What's good

• Flutter version (3.44.4) matches what's already pinned in test.yaml — good consistency.
• The API key is correctly injected via secrets.TESTPRESS_API_KEY → env: CLIENT_API_KEY, matching what app/scripts/client_utils.dart expects (Platform.environment['CLIENT_API_KEY']) — it's never printed or passed as a CLI arg, so it won't leak into logs.
• The "Fetch Dependencies" step correctly loops over app and all packages/* before invoking the script, since generate_client_app.dart lives under app/ and needs pub get to have run.
• --config/--api-base-url flags line up with parseArgs in client_utils.dart, and the artifact output path matches Flutter's default APK output location.
• Sensible use of actions/upload-artifact@v4 with a bounded retention-days: 7.

🔴 CRITICAL — Must fix before merge

1. Script injection via unquoted workflow_dispatch inputs (lines 45-48): ${{ inputs.config }} and ${{ inputs.api_base_url }} are interpolated directly into the run: shell block. GitHub Actions expands ${{ }} textually before the shell runs, so a value containing shell metacharacters ($(...), backticks, ;, &&) executes arbitrary commands on the runner — with access to secrets.TESTPRESS_API_KEY and the checked-out repo. This is the well-documented GitHub Actions script-injection pattern (CWE-78). Fix: pass both inputs through env: and reference them as quoted shell variables ("$CONFIG_PATH", "$API_BASE_URL") instead of inlining the template expressions — see inline suggestion.

🟡 WARNING — Should fix

1. No permissions: block (workflow/job level): the GITHUB_TOKEN inherits the repo/org default, which can be broader than needed (e.g. contents: write). This job only needs to check out code and upload an artifact — scope it down with permissions: contents: read at the workflow or job level (see inline comment).
2. OpenSpec — this is a CI/tooling addition rather than an application feature or architectural change, and there's no equivalent precedent requiring an OpenSpec entry for build tooling in openspec/changes/archive/. Treating this as exempt, but flagging as a WARNING rather than silently passing since it does add new automated build capability — worth a sentence in the PR description confirming this is intentionally out of OpenSpec scope.

🔵 SUGGESTION — Nice to have

1. Trailing whitespace after flutter-version: '3.44.4' on line 32 (harmless but inconsistent with test.yaml's formatting).
2. Consider adding actions/cache (or relying on subosito/flutter-action's built-in cache option) for pub get / Flutter SDK to speed up manual APK builds.
3. Consider light validation that inputs.config ends in .json and inputs.api_base_url looks like a URL before invoking the Dart script, to fail fast with a clearer error than whatever client_utils.dart produces.

Verdict

CHANGES REQUESTED

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 40s —— View job

---

Todo List

• Fetch latest PR diff and current workflow file
• Verify the two previously flagged issues (script injection, missing permissions) were fixed
• Read AGENTS.md / ai_context.md / architecture.md for any applicable rules (CI-only change, no app code — same conclusion as prior review)
• Post review summary

---

PR Review Summary

Re-reviewed .github/workflows/build_client_apk.yaml after the c235914 fixup commit. Both issues from the previous review are resolved, plus two of the three suggestions were also addressed.

✅ What's good

• Script injection (previously CRITICAL) — fixed. inputs.config / inputs.api_base_url are now routed through env: (CONFIG_PATH, API_BASE_URL) in both the new validation step and the "Generate Client APK" step, and referenced as quoted shell variables ("$CONFIG_PATH", "$API_BASE_URL") rather than inlined ${{ }} expressions. This closes the CWE-78 template-expansion vector.
• Missing permissions: block (previously WARNING) — fixed. permissions: contents: read is now declared at the workflow level (lines 15-16), scoping GITHUB_TOKEN down to least privilege.
• New "Validate Inputs (Fail Fast)" step (lines 23-35) — nice addition beyond what was asked; it checks CONFIG_PATH ends in .json and API_BASE_URL starts with http/https before any dependency fetching or build work runs. This also addresses suggestion chore(openspec): archive completed lms-navigation-shell change #3 from the original review.
• Flutter caching enabled (cache: true on subosito/flutter-action@v2, line 50) — addresses suggestion feat(navigation-shell): implement unified routing architecture #2 from the original review.
• Trailing whitespace after the Flutter version string is gone.
• CLIENT_API_KEY continues to be injected via secrets.TESTPRESS_API_KEY → env: only, never interpolated into the run: block directly — still safe.

🔴 CRITICAL — Must fix before merge

None.

🟡 WARNING — Should fix

None.

🔵 SUGGESTION — Nice to have

1. The validation step re-declares CONFIG_PATH/API_BASE_URL as env vars separately from the "Generate Client APK" step. Minor duplication — could hoist both to job-level env: since they're used identically in two steps. Not worth blocking on.

Verdict

APPROVED

[pugal66]

• File Rename: Renamed the workflow file from build_client_apk.yaml to generate_client_app.yaml to match the script name
• Testing Trigger: The on: push trigger is included temporarily to test the CI workflow on this branch, and will be removed before merging to prevent spamming releases.

@claude review

[claude]

Claude finished @pugal66's task in 2m 8s —— View job

---

Todo List

• Load context (AGENTS.md, ai_context.md, architecture.md — no CI/script-specific rules apply)
• Fetch latest PR diff and current workflow file (renamed to generate_client_app.yaml)
• Verify script-injection / permissions fixes still hold after rename
• Review the new temporary on: push trigger and new release step
• Post review summary

---

PR Review Summary

Reviewed the latest commit (67171c8), which renames build_client_apk.yaml → generate_client_app.yaml, adds a temporary on: push trigger, switches from subdomain/api_base_url inputs to a derived API_BASE_URL, and replaces artifact upload with a GitHub Release step. This surfaces one new CRITICAL issue in the Dart script change.

✅ What's good

• Previously-fixed issues remain fixed: CONFIG_PATH/API_BASE_URL are still referenced as quoted shell variables ("$CONFIG_PATH", "$API_BASE_URL") in the run: blocks, not inlined ${{ }} expressions — no regression on the script-injection fix.
• permissions: contents: write is a legitimate re-widening (not a regression) — it's now required by softprops/action-gh-release@v2 to create releases/upload assets.
• Deriving API_BASE_URL from subdomain via env: (rather than the shell) is safe: GitHub Actions expands ${{ }} at the runner layer before the value ever reaches a shell, so it can't be used for command injection here — it's just string concatenation into an env var.
• The on: push trigger + "will remove before merging" is explicitly called out in the PR comment, so it's a known, temporary state rather than an oversight.

🔴 CRITICAL — Must fix before merge

1. GITHUB_ENV injection via unsanitized appName (app/scripts/generate_client_app.dart:99-105): appName comes from remoteConfig['app_name'], which is fetched over the network from https://{subdomain}.testpress.in (fetchRemoteConfig, line 11) — i.e. it's externally-controlled data from an arbitrary subdomain the workflow_dispatch caller supplies. The code does appName.replaceAll(' ', '_') and appends it directly to the GITHUB_ENV file with 'APP_NAME=$safeAppName\n', without stripping newlines or = characters. If the remote config response contains a value like "Foo\nMALICIOUS_VAR=evil\nOTHER=1", it injects arbitrary extra environment variables into every subsequent step in the job (classic GITHUB_ENV poisoning, CWE-77). Combined with the new "Create Release and Upload APK" step that consumes env.APP_NAME in the release tag_name, this is now a real, reachable sink, not just theoretical. Fix: strip control characters (\r, \n) — and ideally restrict to a safe charset — before writing, e.g. appName.replaceAll(RegExp(r'[\r\n]'), '').replaceAll(' ', '_'), or use the multi-line-safe <<EOF delimiter form when writing to GITHUB_ENV.
   Fix this →

🟡 WARNING — Should fix

1. Release step runs even when the build silently failed (app/scripts/generate_client_app.dart:47-48 + workflow's new "Create Release and Upload APK" step): main()'s catch (e) { print('❌ Error: $e'); } never calls exit(1) or rethrows, so the Dart process — and therefore the "Generate Client APK" workflow step — always exits 0, even if fetchRemoteConfig/_buildApk throws. Previously this just meant a misleading "green" CI run with an artifact upload step that would simply find no APK. Now it means the new release step still executes on failure, creating a GitHub Release with a possibly-empty/garbage tag_name (since APP_NAME is only set in the success path of _buildApk) and no APK asset. Worth adding an explicit failure exit code in the catch block, or gating the release step with a check that the APK glob actually matched.
2. on: push with no branch/path filter + contents: write + auto-release is live on this branch right now. Understood this is temporary per the PR description and will be removed before merge — flagging only as a reminder to confirm removal, since forgetting it would mean every future push (once merged) triggers a full Flutter build and creates a GitHub Release.

🔵 SUGGESTION — Nice to have

1. subdomain has no format validation (e.g., alphanumeric/hyphen only) before being interpolated into API_BASE_URL. Low risk today since it only becomes an env var value (not shell-executed), but worth constraining to avoid malformed URLs from stray input.
2. Consider fail_if_no_files_found: error (or equivalent) on the softprops/action-gh-release@v2 step so a missing APK causes a visible failure rather than a silent empty release.

Verdict

CHANGES REQUESTED
· branch

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 1m 26s —— View job

---

Todo List

• Load context (AGENTS.md, ai_context.md, architecture.md — no CI/script-specific rules apply, same conclusion as prior rounds)
• Fetch latest PR diff (commit b9b2686) — workflow renamed to generate_client_app.yaml, temporary on: push trigger + release step removed, back to workflow_dispatch + artifact upload
• Verify previously-flagged CRITICAL (GITHUB_ENV injection via unsanitized appName) — still unresolved
• Verify previously-flagged WARNINGs (silent build failure, temporary on: push) — push trigger removed, silent-failure issue still present
• Post review summary

---

PR Review Summary

Reviewed the latest commit (b9b2686, "Clean up client apk workflow and improve artifact naming"). The temporary on: push trigger and the GitHub Release step from the prior round have been removed — the workflow is back to a clean workflow_dispatch → artifact-upload shape, which resolves that round's WARNINGs.

✅ What's good

• Temporary on: push trigger + release step — removed. The workflow is back to workflow_dispatch-only, so the "release created on every push" and "release created even when build silently fails" risks from the previous round are gone.
• Script injection — still fixed. CONFIG_PATH / API_BASE_URL are set via job-level env: (derived from inputs.config / inputs.subdomain) and only ever referenced as quoted shell variables ("$CONFIG_PATH", "$API_BASE_URL") inside run: blocks — no ${{ }} is inlined into shell text. No regression.
• Permissions — still scoped. permissions: contents: read at the workflow level, correctly re-tightened now that the release step (which needed contents: write) is gone.
• "Validate Inputs (Fail Fast)" step still checks CONFIG_PATH ends in .json before any build work runs.
• Removed the earlier duplication of CONFIG_PATH/API_BASE_URL between steps by hoisting both to job-level env: — addresses a suggestion from the previous review.

🔴 CRITICAL — Must fix before merge

1. GITHUB_ENV injection via unsanitized appName — still present (app/scripts/generate_client_app.dart:99-105): appName comes from remoteConfig['app_name'], fetched over the network from https://{subdomain}.testpress.in — i.e. it's data controlled by whatever server the workflow_dispatch caller points subdomain at. safeAppName = appName.replaceAll(' ', '_') still only strips spaces, then gets appended straight into the GITHUB_ENV file ('APP_NAME=$safeAppName\n') with no newline/=-character stripping. A response like {"app_name": "Foo\nMALICIOUS_VAR=evil"} injects arbitrary env vars into every later step in the job (GITHUB_ENV poisoning, CWE-77) — and env.APP_NAME is consumed directly in the "Upload APK Artifact" step's name: field, so it's a real, reachable sink, not just theoretical. This was flagged in the previous review round and the fix (stripping \r/\n, or restricting to a safe charset) was not applied in this commit. Suggested fix: appName.replaceAll(RegExp(r'[\r\n]'), '').replaceAll(' ', '_'), or use the <<EOF heredoc form for multi-line-safe GITHUB_ENV writes.
   Fix this →

🟡 WARNING — Should fix

1. Build failures don't fail the CI step (app/scripts/generate_client_app.dart:47-48): main()'s catch (e) { print('❌ Error: $e'); } still never calls exit(1), and _buildApk's false return value on a failed flutter build apk is never checked in main(). So the Dart process — and the "Generate Android APK" workflow step — always exits 0, even when the build genuinely fails. In that case APP_NAME never gets written to GITHUB_ENV, and the "Upload APK Artifact" step runs with name: -APK and no matching files; since actions/upload-artifact@v4 defaults to if-no-files-found: warn, the whole run still shows green with no clear signal that the build failed. Consider an explicit exit(1) in the catch block (and/or checking _buildApk's return value), plus if-no-files-found: error on the upload step.
2. subdomain has no format validation before being interpolated into API_BASE_URL (https://${{ inputs.subdomain }}.testpress.in). Not a shell-injection risk (it's env-level string concatenation, not run: interpolation), but an unconstrained value could produce a malformed URL that only fails deep inside fetchRemoteConfig. Consider validating against a safe charset (alphanumeric/hyphen) in the "Validate Inputs" step.

🔵 SUGGESTION — Nice to have

None beyond the items above.

Verdict

CHANGES REQUESTED
· branch

[pugal66]

@claude review

[claude]

Claude finished @pugal66's task in 55s —— View job

---

Todo List

• Load context (AGENTS.md, ai_context.md, architecture.md — CI-only change, no app-code rules apply, consistent with prior rounds)
• Fetch latest PR diff (commit a69dd53, "Add input validation for subdomains and improve error handling...") and re-verify previously flagged issues
• Post review summary

---

PR Review Summary

Reviewed the latest commit (a69dd53). All previously outstanding issues from earlier rounds are now resolved.

✅ What's good

• GITHUB_ENV injection via unsanitized appName (previously CRITICAL) — fixed. app/scripts/generate_client_app.dart:103 now does appName.replaceAll(RegExp(r'[^a-zA-Z0-9_\-]'), '_'), an allowlist charset regex, instead of the old replaceAll(' ', '_'). This strips newlines, =, and any other control/metacharacters before the value is written to GITHUB_ENV, closing the CWE-77 poisoning vector even if a malicious/misbehaving {subdomain}.testpress.in server returns a crafted app_name.
• Silent build failures (previously WARNING) — fixed. main() now captures _buildApk's return value (final success = await _buildApk(...)) and sets exitCode = 1 if it's false, and the catch block also sets exitCode = 1 (generate_client_app.dart:39-52). Setting the exitCode global rather than calling exit() directly is the correct approach here since it still lets the finally cleanup (cleanupTempFiles, restoreGitChanges) run before the process exits non-zero.
• subdomain format validation (previously SUGGESTION) — added. The "Validate Inputs (Fail Fast)" step now checks SUBDOMAIN against ^[a-zA-Z0-9-]+$ before it's interpolated into API_BASE_URL, preventing malformed URLs from reaching fetchRemoteConfig.
• if-no-files-found: error (previously SUGGESTION) — added on the Upload APK Artifact step, so a missing APK now fails the run visibly instead of silently warning.
• Script-injection and permissions: contents: read fixes from earlier rounds remain intact — no regressions.

🔴 CRITICAL — Must fix before merge

None.

🟡 WARNING — Should fix

None.

🔵 SUGGESTION — Nice to have

None beyond what's already been addressed.

Verdict

APPROVED
· branch