Hardening & refactor: 1-month roadmap from ownership audit - #3
Merged
Conversation
The key had no writer anywhere in code, only a reader. Deleting it removes a latent injection surface (via ADB backup on a debug-unlocked device) without changing any exercised behavior. Forks change the compile-time DEFAULT_REMOTE_CONFIG_URL constant instead.
Silent catch(Throwable) hid JCA misconfiguration (no SHA256withECDSA provider on a device) as an ordinary bad-signature result. Narrow the catch to the two exception families that are actually expected (GeneralSecurityException for crypto failures, IllegalArgumentException for base64 encoding), log both, and let anything else propagate.
Without a content hash, the wrapper trusts services.gradle.org's hostname only. A CDN or DNS compromise on a CI runner could substitute a malicious Gradle distribution during the release build — the same build that materialises the release keystore from secrets. Pinning the SHA eliminates that class of substitution.
Without an explicit permissions block, the workflow inherits the repository default, which can be write-all. Since ci.yml runs contributor-supplied code on pull_request, a malicious PR could exfiltrate a write-scoped token. This closes that vector for CI; release.yml keeps its explicit contents:write (needed for the release).
customButtons already applied 256/50 caps; matcher viewIds/labels/prefixes and the top-level apps array were unbounded. The 512 KB body cap in RemoteConfigSync bounds the absolute total, but the parser should not trust the remote to be honest about individual sizes on the hot-path matching loop.
Naive "$url.sig" produced "config.json?v=1.sig", which is a 404 and silently suppresses signature verification. deriveSigUrl inserts .sig before the query/fragment so a URL containing a version pin still gets a valid signature URL.
Left over from when StatusCard was inlined here — now they live only in SkipperKitStatusCard.kt. The Kotlin compiler warns on each. Cleanup only.
Every toggle on the settings screen was read by TalkBack as a generic 'Switch, on/off' with no indication of what it controlled. Every switch now carries a contextual label derived from the app's display name.
The regexes were recompiled on every dialog open. Behavior unchanged; matches the RISKY_BUTTON_REGEX pattern (top-level val in config).
The service formatted a CUSTOM label with a verbatim-duplicated branch in two adjacent when arms — the sealed result type owns the target and now owns the label. Callers just read result.displayLabel.
sign-config.sh now defaults to ~/.config/skipperkit/signing/... and requires SKIPPERKIT_SIGNING_PASSPHRASE for encrypted keys. SECURITY.md documents the maintainer runbook: relocate, aes256-encrypt in place, store passphrase in Keychain. The unencrypted key currently on disk must be relocated + encrypted per the runbook before the next release.
Keystore currently lives inside the repo working tree with an unencrypted password file next to it. Runbook covers: generating fresh keystore into ~/.config/skipperkit/release/, independent 24-hex-byte store/key passwords, Keychain storage, and CI secret updates.
Preparation for enabling isMinifyEnabled. Keeps the three manifest-referenced classes, the DataStore key holder (so key names survive obfuscation), and the JCA reflective lookups.
Full class/method/string symbols were shipping in the production APK, including PUBLIC_KEY_B64 and every branch in ConfigSignature. R8 is on with the keeps added in the previous commit. CI now assembles the release variant (unsigned) so a broken minification config surfaces on every PR, not only at tag time.
Encapsulates the isAlive check + handler.post so callers can distinguish 'posted' from 'silently dropped because Looper is dead'. Pure and testable; the service wires it in the next commit.
Every workerHandler?.post now goes via safeWorker?.tryPost, which logs 'worker looper died' at Log.e when a post is silently refused. Makes worker-thread death visible instead of turning the skip loop into a silent no-op.
Without a handler an exception in the taughtApps collect body dropped to Thread.uncaughtExceptionHandler and the scope silently stopped observing. Now Log.e surfaces the death; the next commit wraps applyScope in try/catch so a single failure doesn't kill the collector.
A single throwing applyScope no longer terminates the taughtApps collector. Cancellation still propagates. Combined with the previous commit's exception handler, any failure is logged and taught-app scope updates continue to work for future emissions.
kotlin.runCatching catches CancellationException, which breaks cooperative cancellation. catchNonCancelling re-throws cancellation and routes other failures through onFailure. Two callers will migrate next.
runCatching was swallowing CancellationException across the 7-step startup, so process teardown could not cooperatively cancel the launched coroutine. Behavior on non-cancellation failures is unchanged (Log.w + continue).
Documents the two-endpoint trust model as code. Cleartext denied globally; only raw.githubusercontent.com and *.supabase.co are enumerated. A future third endpoint would show up here as a visible addition rather than as an inline HttpsURLConnection URL string.
The wrapper JAR is the bootstrap for every build. Without a checked-in integrity assertion, a modified JAR (contributor machine compromise, malicious PR) would silently execute during CI and every developer's build. This step fails the build if the JAR drifts.
Gates the perf counters added in the next commit. On in debug, off in release — the ownership guide's carmack-perf rule is 'measure first' and this is the seam that makes that measurement possible without shipping the counters to end users.
Compile-time-gated by BuildConfig.PERF_INSTRUMENTATION (true in debug, false in release). Reads: 'adb logcat -s SkipperKitPerf' during playback gives events/sec; systrace shows SkipEngine/pass sections. This is the measurement precondition to any hot-path optimization. R8 strips the counters object entirely from release (0 refs in mapping.txt).
Documents the measurement protocol: 60s adb logcat -s SkipperKitPerf during Netflix playback. Any optimization landing after this must cite before/after numbers from this workflow.
The four-input merge that produces effective configs was locked inside ConfigRepository's mutable object with @synchronized entry points and a clearRemoteForTest() teardown hook. Extracting the merge as a pure function makes it testable without global-state reset and clarifies that ConfigRepository is just a snapshot holder.
Two redundant empty-list guards collapse into one exhaustive when. The type now expresses the distinction between 'no fetch attempted' and 'fetch returned no apps' that the comments were previously trying to explain.
Three containers (Pending list, Approved LinkedHashMap, Dismissed HashSet) collapse into one LinkedHashMap<key, DiscoveryLifecycle>. propose's three-container duplicate check collapses to one containsKey. Public API unchanged; all existing tests pass.
First of three service extractions. Isolates the tap-fallback path so the service class shrinks toward its lifecycle-only responsibility.
Second service extraction. The 1000ms sweep-throttle timestamp moves out of the service; the sweep itself is now unit-tested (previously zero coverage). Service drops ~20 lines.
Third service extraction. 5000ms throttle timestamp and discovery dispatch move out of the service; service drops another ~30 lines and gains unit-tested coverage for the throttle.
SettingsStore is now ~120 lines of DataStore I/O; the JSON codecs are in their own testable file. New codec tests lift previously implicit coverage into explicit assertions.
Every persisted blob (taught apps, custom buttons, discovery entries, dismissed keys) now includes an integer 'version' field on write. Readers tolerate its absence for backward compatibility with installs that saved data before this commit. Prepares for future model changes without a migration.
The invariant that built-ins cannot become taught apps was enforced by a lambda-embedded check inside SettingsRoute. Now the repository refuses the add and returns false; any future caller (import flow, share flow, tests) gets the same guarantee without re-implementing the check.
Removes ~30 lines of imperative teach-pick domain logic from a Composable lambda. The coordinator handles the repositories (custom button add, or discovery approve + contribution offer, then disarm); the composable keeps only the UI-state reset.
Removes the ~9-line Intent-assembly block from SettingsRoute's onExport callback. The helper is a plain top-level fn; the composable now only does the state-lookup + delegation.
remove() cascades to four singletons. A throw in one previously left persistent state inconsistent with in-memory. Each sub-step is now try/log/continue; ordering-dependent partial failures degrade to a missing cleanup rather than a stale primary state. Returns false for an unknown package.
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements the full 1-month roadmap from
docs/ownership/SKIPPERKIT_OWNERSHIP_GUIDE.md— security hardening, resilience, perf instrumentation, and structural refactors. 37 commits, one small task per commit. Full clean build green (testDebugUnitTest,lintDebug,lintRelease,assembleDebug,assembleRelease).What changed, by phase
Security hardening
remote_config_urlDataStore key — latent injection surface with no writer (H-1)ConfigSignature.verify()logs JCA errors distinctly from bad signatures; narrowed catch (H-2)gradle-8.14-all.zipSHA-256 in the wrapper (H-4)ci.ymltoken tocontents: read; verifygradle-wrapper.jarSHA in CI (H-7, L-1)RemoteConfigParsermatcher string/array sizes (M-1, M-2).sigURL derivation under query strings (M-3)network_security_config.xml— explicit two-host trust model (L-2)Resilience
SafeHandlermakes worker-thread death detectable instead of silently dropping events (H-6)scopeWatchergets aCoroutineExceptionHandler+ isolated collect body so taught-app scope updates survive a failure (H-5)catchNonCancellinghelper replacesrunCatchingat 16SkipperAppsites so coroutine cancellation propagates (M-4)Performance (measurement only — no optimizations)
BuildConfig.PERF_INSTRUMENTATION-gated counters (event rate, worker queue depth, per-passTrace), fully stripped from release by R8tools/measure-perf.shdocuments the measurement protocolStructural refactors
ConfigMerge.merge()extracted from the statefulConfigRepositoryRemoteStatesealed type replaces a redundant empty-guard pairDiscoveryRepositoryunified into oneLinkedHashMap<key, DiscoveryLifecycle>GestureDispatcher,TeachScanner,DiscoveryCoordinator(the latter two now unit-tested)SettingsStoreCodecssplit out ofSettingsStore; JSON blobs gain aversionenvelope (backward-compatible with existing unversioned data)TaughtAppsRepository.add/removegain the built-in guard + cascade-failure isolationTeachPickCoordinatorandshareTaughtApplifted out of the settings composableHost-hygiene runbooks (C-1, C-2) — documented in
SECURITY.md. The actual key remediation is a manual maintainer action (see checklist below).Test plan
./gradlew clean testDebugUnitTest lintDebug lintRelease assembleDebug assembleRelease— BUILD SUCCESSFUL/code-reviewor equivalent)config-signing-key.pemout of the repo tree (runbook:SECURITY.md→ "Signing key hygiene")SECURITY.md→ "Release keystore rotation")These two Critical findings remain open until the runbooks are run — this PR only adds the tooling/runbooks, it does not touch the keys.
Notes