Skip to content

fix: Android code push config refreshes on every launch#18

Merged
fonkamloic merged 8 commits into
mainfrom
fix/android-config-refresh
Jun 12, 2026
Merged

fix: Android code push config refreshes on every launch#18
fonkamloic merged 8 commits into
mainfrom
fix/android-config-refresh

Conversation

@fonkamloic

@fonkamloic fonkamloic commented Jun 12, 2026

Copy link
Copy Markdown
Contributor

What

Two changes:

1. Android config refresh fix

The Android setup generated by fcp codepush init copied codepush.yaml from the APK assets into the app files directory only when no copy existed yet. Configuration shipped in an app update (e.g. a new release version) was therefore never picked up — the app kept running with the configuration from its first install.

  • The generated CodePushApp.kt now performs the copy on every launch, overwriting the previous copy. The file is tiny, so the cost is negligible.
  • Re-running fcp codepush init in an existing project upgrades a previously generated CodePushApp.kt in place (exact-match replacement, so user-modified files are left untouched).
  • The manual-integration instructions printed for apps with a custom Application class are updated to match.

2. CHANGELOG cleanup

Replaces the verbose engineering-notes entries for 0.19.11–0.19.14 on main with the concise user-facing versions already shipped with the 0.19.15 rc series on pub.dev, and records the rc history so main's changelog matches the published package.

Notes

  • dart analyze clean on the touched file.
  • Version bumped to 0.19.15-rc.9 with a CHANGELOG entry.

The generated CodePushApp copied codepush.yaml from assets to the
files dir only when no copy existed, so configuration shipped in an
app update was never picked up. The copy now runs on every launch,
and re-running 'fcp codepush init' upgrades a previously generated
CodePushApp.kt in place.
Replaces the engineering post-mortem entries on main with the
user-facing versions already published with the 0.19.15 rc series.
Aligns main's changelog with what pub.dev ships.
@fonkamloic fonkamloic changed the title Android: code push config refreshes on every launch fix: Android code push config refreshes on every launch Jun 12, 2026
fcp codepush init now writes the local signing public key into
assets/codepush.yaml as a block scalar, and fcp codepush keys register
injects it into an existing project's config — parity with the
FLTCodePushPublicKey Info.plist entry on iOS. With a key in the config,
devices require a valid patch signature before loading a patch.
@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review

Bug: double file read in the upgrade path

_codepush_init.dart reads CodePushApp.kt twice in the upgrade branch: once for the .contains() guard and once inside writeAsStringSync(codePushAppFile.readAsStringSync()...). TOCTOU and unnecessary I/O. Fix by reading into a local variable first.

Bug: HOME env var is not set on Windows

_publicKeyYamlBlock() does Platform.environment['HOME'] ?? '/tmp'. HOME is a Unix convention; Windows uses USERPROFILE. The /tmp fallback is a Unix path that does not exist on Windows, so publicKeyFile.existsSync() silently returns false and the public key is never embedded in the Android asset on Windows. The codebase already has F.homeDir() for exactly this cross-platform lookup.

Silent I/O errors on every launch

The generated CodePushApp.kt now copies the config on every launch but still has catch (_: Exception) {}. With the old guarded code a failure was a one-time miss at first install. Now a persistent problem (storage full, permission denied) silently leaves the app running with a stale config on every launch. Consider at minimum android.util.Log.e("CodePushApp", "Failed to copy codepush.yaml", e) so there is something to find in logcat.

Manual integration migration not documented

The else if upgrade logic detects the old auto-generated CodePushApp.kt by exact-string match. Developers who followed the manual integration instructions (printed for flavored apps) and added the if (!dest.exists()) guard by hand will not be upgraded — they must edit their Application class manually. The release note says "Re-run fcp codepush init", which works for auto-generated files but is misleading for manual integrations.

Comment thread lib/src/commands/codepush_commands/_codepush_init.dart Outdated
Comment thread lib/src/commands/codepush_commands/_codepush_init.dart Outdated
… surfaced copy failures

- Read CodePushApp.kt once in the upgrade branch (was read twice).
- _publicKeyYamlBlock uses F.homeDir() so the key is found on Windows.
- The generated copy block logs failures to logcat instead of
  swallowing them silently (fully qualified android.util.Log, so the
  in-place upgrade needs no new import).
- CHANGELOG notes the manual-integration migration step.
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Addressed in ab2387a:

  • Double read / TOCTOU: the upgrade branch now reads CodePushApp.kt once into a local and reuses it.
  • Windows home dir: _publicKeyYamlBlock now uses the existing F.homeDir() helper.
  • Silent copy failures: the generated (and in-place-upgraded) copy block now logs failures via fully-qualified android.util.Log.e, so the upgrade path needs no new import in user files. The printed manual-integration snippets match.
  • Manual-integration migration: the CHANGELOG entry now tells manual integrators to drop the exists-check from their Application class.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review

Overall: The config-refresh fix is correct and well-motivated — overwriting on every launch is the right call for a tiny YAML file that changes with each app update. The in-place upgrade path for existing CodePushApp.kt files is clever. CHANGELOG cleanup is a net positive.

A few issues worth addressing before merge:

Bugs / correctness

1. Silent upgrade no-op gives no user feedback
In _codepush_init.dart, when CodePushApp.kt already exists but does not contain oldCopyBlock (user-modified file, different indentation, or custom Application class), the upgrade silently does nothing. The user ends up with the stale config even after running fcp codepush init, with no indication of what to do. At minimum, log a warning pointing to the manual steps.

2. Unguarded file I/O in _publicKeyYamlBlock() and _maybeAddPublicKeyToAndroidConfig()
Both helpers call readAsStringSync() / writeAsStringSync() without a try/catch. A permission error or concurrent write surfaces as an unhandled FileSystemException and crashes the command. Wrap in try/catch and emit a _logger.warn() so the command degrades gracefully instead of aborting.

3. No empty-PEM guard before writing in _maybeAddPublicKeyToAndroidConfig()
The method receives publicKeyPem but does not validate it is non-empty before embedding it in the YAML. An empty string writes an invalid key block, silently breaking signature verification on the next release build. Add a guard if (publicKeyPem.trim().isEmpty) return; at the top of the method.

Security

4. Mutable action tags in CI workflow
anthropics/claude-code-action@v1 and actions/checkout@v4 are floating tags. Combined with pull-requests: write and id-token: write permissions, a tag-hijack would let an attacker post PR comments and request OIDC tokens. Pin both to a specific commit SHA.

5. id-token: write may be unnecessary
If the action authenticates via the CLAUDE_CODE_OAUTH_TOKEN secret, OIDC is not needed. Remove id-token: write if the action does not explicitly require it — least-privilege principle.

Minor

The on-every-launch overwrite of codepush.yaml via outputStream() truncates the file immediately before writing. A crash mid-copy leaves a corrupt config until the next clean launch. Writing to a temp file and renaming atomically would be more robust, though this is low-priority given the file size.

Comment thread .github/workflows/claude-review.yml
Comment thread .github/workflows/claude-review.yml
Comment thread lib/src/commands/codepush_commands/_codepush_init.dart
Comment thread lib/src/commands/codepush_commands/_codepush_init.dart
Comment thread lib/src/commands/codepush_commands/_codepush_keys.dart Outdated
@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review — fix: Android code push config refreshes on every launch

Overview

The core fix (copy codepush.yaml on every launch instead of only when absent) is correct and necessary. The in-place upgrade of existing CodePushApp.kt is a nice touch. The CHANGELOG cleanup is a pure editorial improvement. A few correctness and backwards-compatibility issues are worth addressing before merge.


Issues

🔴 Key rotation silently broken in _maybeAddPublicKeyToAndroidConfig

_codepush_keys.dart:269if (content.contains("public_key:")) return; means re-running fcp codepush keys register after a key rotation never updates codepush.yaml. Devices keep verifying against the stale public key while the server signs with the new private key — every patch fails verification. The method should replace an existing public_key: block rather than bail out.

🔴 fcp codepush init drops an injected public key on re-run

_codepush_init.dart:243-245configFile.writeAsStringSync unconditionally replaces codepush.yaml. If the user previously injected a public_key: via fcp codepush keys register but re-runs fcp codepush init from a machine where ~/.flutter_codepush/codepush_public.pem is absent (CI, a teammate's laptop, post-rotation), _publicKeyYamlBlock() returns "" and the public key is silently dropped. Subsequent release builds ship codepush.yaml without the key, breaking device-side signature verification with no diagnostic.

🟡 Upgrade path for CodePushApp.kt fails silently

_codepush_init.dart:341 — If existing.contains(oldCopyBlock) is false (user edited the file, or an intermediate CLI version produced different whitespace), the upgrade silently does nothing. The user re-runs init, gets no feedback, and retains the old broken behavior. At minimum log a warning directing the user to update manually.

🟡 id-token: write is broader than necessary

.github/workflows/claude-review.yml:21 — The job only reads PR content and posts comments. id-token: write enables OIDC token minting unnecessarily and widens the blast radius if the action is compromised. Remove it unless the action explicitly documents requiring it.

🟡 claude-code-action@v1 is a mutable tag

.github/workflows/claude-review.yml:27 — Pin to a full commit SHA to prevent silent tag-move attacks on the CI pipeline.


What looks good

  • Copy-on-every-launch is correct; the file is tiny so the cost is negligible.
  • catch (_: Exception) {}catch (e: Exception) { Log.e(...) } is a real fix — the underscore wildcard requires Kotlin 2.0+, so the old generated code would not compile on older Android projects.
  • The _publicKeyYamlBlock() indentation handles CRLF correctly via per-line .trim().
  • The fork check (head.repo.full_name == github.repository) is the right guard for secret-gated CI.

Comment thread lib/src/commands/codepush_commands/_codepush_keys.dart Outdated
Comment thread lib/src/commands/codepush_commands/_codepush_init.dart
Comment thread lib/src/commands/codepush_commands/_codepush_init.dart
Comment thread .github/workflows/claude-review.yml
- keys register now replaces an existing public_key block instead of
  bailing out, so key rotation reaches devices; guards empty PEM and
  file I/O failures with a non-fatal warning.
- init preserves a previously injected public_key when no local key
  exists (CI, different machine), instead of silently dropping it and
  disabling device-side verification.
- init warns when CodePushApp.kt cannot be upgraded automatically.
- _publicKeyYamlBlock tolerates an unreadable pem file.
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round-2 findings addressed in 64274a6:

  • Key rotation (🔴): keys register now replaces an existing public_key: block (shared kPublicKeyYamlBlockPattern) instead of bailing out, with an updated vs added message; no-ops when the embedded key is already current.
  • Key dropped on init re-run (🔴): init now preserves a previously injected public_key: block when no local pem exists, instead of silently disabling verification.
  • Silent upgrade no-op (🟡): warns when CodePushApp.kt can't be upgraded automatically (and stays quiet when the file is already current).
  • Unguarded pem read / empty PEM / helper I/O: all guarded with non-fatal warnings.

Two deliberate non-changes:

  • id-token: write: required by claude-code-action's documented setup — the action uses OIDC to exchange for its scoped GitHub token regardless of which Anthropic auth secret is used. Removing it breaks comment posting.
  • @v1 tag: keeping the floating major tag for the first-party Anthropic action, consistent with this repo's dependabot-managed action versioning. SHA-pinning everything is an org-level policy choice we can adopt separately.

@claude

claude Bot commented Jun 12, 2026

Copy link
Copy Markdown

Code Review summary — see inline comments for specifics.

What this PR does: fixes Android code-push config refresh (CodePushApp.kt now copies codepush.yaml on every launch, not only when missing), adds public-key embedding into the Android YAML on init and keys register, and rewrites the CHANGELOG.

Correctness issues:

  1. Hardcoded PEM path in _publicKeyYamlBlock() — the function reads ~/.flutter_codepush/codepush_public.pem directly rather than the stored key-path from ~/.flutter_compilerc. A user who ran fcp codepush keys generate --path /custom/dir will silently get no public-key block in codepush.yaml. Should use the same stored-path helper the rest of the codepush commands use. (inline comment on the file)

  2. Preserve-existing-key is wrong after key rotation — _setupAndroid preserves whatever public_key: block is in codepush.yaml when there is no local key file. The comment names post-rotation as a valid scenario, but in that case the preserved key is the old rotated-out key; devices would continue verifying against it. This path should be restricted to different-machine/same-key; the rotation case needs a warning to run fcp codepush keys register first. (inline comment on the file)

  3. Idempotency check in _maybeAddPublicKeyToAndroidConfig is fragile — content.contains(block) reconstructs block from the in-memory PEM. CRLF/LF mismatch causes the search to miss an up-to-date block; the replaceAll+append that follows is still idempotent so the file ends up correct, but the early-return short-circuit is unreliable. Normalise line endings before the comparison or drop the contains check and rely solely on the regex. (inline comment on the file)

Security: id-token: write in claude-review.yml is not needed for CLAUDE_CODE_OAUTH_TOKEN auth — drop it for least privilege. (inline comment on the file)

What is correct: the copy-on-every-launch fix is the right approach; in-place upgrade strategy is sensible; manual-integration snippet matches the generated code.

Comment thread .github/workflows/claude-review.yml
Comment thread lib/src/commands/codepush_commands/_codepush_init.dart
Comment thread lib/src/commands/codepush_commands/_codepush_init.dart
Comment thread lib/src/commands/codepush_commands/_codepush_keys.dart
Comment thread lib/src/shared/functions.dart
@fonkamloic

Copy link
Copy Markdown
Contributor Author

Round-3 triage under the new review policy (only Critical blocks merge):

  • Hardcoded PEM path (Medium): valid for --output-dir users; tabled as a follow-up to derive the public-key path from the stored signing-key path.
  • Preserve-key after rotation (Medium): the preserve branch only runs when no local key exists; a rotation-aware warning is tabled with the same follow-up.
  • CRLF idempotency check (Low): the reviewer notes the file still ends up correct; tabled.
  • id-token: write : answered in round 2 — required by the action's documented token exchange.

Merging.

@fonkamloic fonkamloic merged commit 698c7f6 into main Jun 12, 2026
6 checks passed
@fonkamloic fonkamloic deleted the fix/android-config-refresh branch June 12, 2026 05:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant