App Shield: attestation API, network guard, and iOS App Attest fixes - #5486
App Shield: attestation API, network guard, and iOS App Attest fixes#5486shai-almog wants to merge 104 commits into
Conversation
…kage
Introduces com.codename1.security.shield, the client half of an enterprise
attestation and API-shielding service, and fixes several pre-existing defects
found while building it.
Fixes that stand on their own:
- iOS App Attest never linked. IOSNative.java declares `native boolean
isAppAttestSupported()` but IOSNative.m defined it without ParparVM's
`_R_boolean` mangling in both #ifdef arms, so `ios.appAttest=true` failed at
link time. Renamed; the App Attest path now builds and links.
- App Attest also re-generated a hardware key on every request and never
asserted. Apple's model is attest once, then assert many times against the
recorded key; the old flow burned Apple's per-device attestation budget and
made device continuity impossible. Rewritten as a keychain-backed state
machine with DCError-driven recovery and throttle backoff.
- The non-prompting SecureStorage tier was missing on Android and JavaSE, so
Secrets.get() re-hit the network on every call on Android. Implemented with a
dedicated AndroidKeyStore key (separate from the biometric key, which is
invalidated on re-enrolment) and PBKDF2/AES-GCM in the simulator.
- CN1JailbreakDetector's fork() probe exited the child rather than the app, so
it never did what its comment claimed. Removed, and the detector split into a
signal source plus a thin exit wrapper -- iOS getCompromiseReasons() was
previously only a canExecute("cydia://") probe that returns false on modern
iOS unless the app also declares the scheme.
New API:
- AppShield plus an SPI seam (ShieldEngine/EngineContext/ShieldEngineRegistry).
Discovery is a registered singleton rather than Class.forName, matching the
convention documented in SystemBrowser: class names are obfuscated, so
name-based lookup is unreliable by construction. The in-tree UnprotectedEngine
is the open-source default and fails open in every direction, so an app
written against this API compiles and runs unchanged without an engine.
- NetworkGuard, a narrow interception seam in com.codename1.io, wired into
ConnectionRequest for header attachment and certificate vetting. The
per-request checkSSLCertificates hook still runs first and unchanged.
- SSLCertificate gains public-key digests. The existing fingerprints are
whole-certificate digests, which change on every renewal even on the same key
-- pinning those is how pinning earns its reputation for causing outages. The
richer form is opt-in, so existing callers see byte-identical data.
- WebSocket.header(), emitted by the ports that build the handshake themselves
and documented as unavailable on iOS and in the browser rather than silently
dropped.
Simulator: JavaSEPort had no DeviceIntegrity overrides at all, so the branches
an app takes on a compromised device were unreachable off real hardware. Adds
Simulate > App Shield covering attestation outcomes, device signals, token
expiry and forced pin mismatch.
Verified: core suite (4309 tests) green; Android, JavaSE, iOS, Windows and Linux
ports compile; the generated iOS project builds and links both with and without
ios.appAttest. The iOS DER walk for public-key digests was checked against
openssl output for EC and RSA chains before being written in Objective-C.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b7fc9ec7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR introduces Codename One’s “App Shield” client-side API surface (attestation token plumbing, certificate pinning seam, and runtime self-protection signal reporting), adds simulator support for exercising those branches, and fixes multiple platform defects uncovered while integrating the new security layer (notably iOS App Attest linking/flow and richer SSL certificate reporting).
Changes:
- Added
com.codename1.security.shieldAPI +spiseam (registered singleton engine; inert default engine when absent). - Added
NetworkGuardinterception points and extended TLS certificate reporting to support per-certificate grouping and SPKI (public-key) digests. - Fixed/rewired iOS App Attest + jailbreak signal reporting, added JavaSE “Simulate > App Shield”, and added unit tests for the new value/parsing behaviors.
Reviewed changes
Copilot reviewed 44 out of 45 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Ports/WindowsPort/src/com/codename1/impl/windows/WindowsWebSocketImpl.java | Includes extra caller-provided WebSocket handshake headers. |
| Ports/LinuxPort/src/com/codename1/impl/linux/LinuxWebSocketImpl.java | Includes extra caller-provided WebSocket handshake headers. |
| Ports/JavaSE/src/com/codename1/impl/javase/JavaSEWebSocketImpl.java | Includes extra caller-provided WebSocket handshake headers. |
| Ports/Android/src/com/codename1/impl/android/AndroidWebSocketImpl.java | Includes extra caller-provided WebSocket handshake headers. |
| CodenameOne/src/com/codename1/impl/WebSocketImpl.java | Adds request-header storage + safe header appending (reserved header blocking/CRLF guard). |
| CodenameOne/src/com/codename1/io/WebSocket.java | Public API to set WebSocket handshake headers (documenting unsupported platforms). |
| CodenameOne/src/com/codename1/io/NetworkGuard.java | New app-wide request decoration + certificate-vetting interface. |
| CodenameOne/src/com/codename1/io/NetworkManager.java | Adds sealed global NetworkGuard installation + afterResponse() hook. |
| CodenameOne/src/com/codename1/io/ConnectionRequest.java | Integrates NetworkGuard (beforeRequest/cert checks), adds grouped cert parsing + SPKI digest support, improves pin-mismatch error surfacing on iOS. |
| CodenameOne/src/com/codename1/impl/CodenameOneImplementation.java | Adds canGetPublicKeyDigests() and getSSLCertificatesEx() extension points; adds platform attestation reset + app signer digests hooks. |
| Ports/JavaSE/src/com/codename1/impl/javase/JavaSEShield.java | New simulator state backing “Simulate > App Shield”. |
| Ports/JavaSE/src/com/codename1/impl/javase/JavaSEPort.java | Adds “Simulate > App Shield” menu + simulator DeviceIntegrity overrides; adds public-key digest certificate export implementation. |
| Ports/Android/src/com/codename1/impl/android/AndroidSecureStorage.java | Implements non-prompting secure storage tier with separate keystore key + AES/GCM; legacy fallback for API<23. |
| Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java | Adds getSSLCertificatesEx() with SPKI digests, adds app signer digests, and minor formatting adjustments. |
| Ports/iOSPort/src/com/codename1/impl/ios/IOSNative.java | Splits App Attest native API into generate/attest/assert steps; adds jailbreak signal native call. |
| Ports/iOSPort/src/com/codename1/impl/ios/IOSImplementation.java | Routes integrity token/reset calls through IOSDeviceIntegrity; adds extended SSL cert handling and compromise reason mapping. |
| Ports/iOSPort/src/com/codename1/impl/ios/IOSDeviceIntegrity.java | Reworks App Attest into persisted state machine (attest once/assert many), adds throttling backoff and reset flow, exposes jailbreak signals. |
| Ports/iOSPort/nativeSources/IOSNative.m | Fixes App Attest symbol mangling and implements new generate/attest/assert native entrypoints; adds jailbreak signal bridge. |
| Ports/iOSPort/nativeSources/CN1JailbreakDetector.h | Refactors into signal-returning API plus legacy exit wrapper behind CN1_DETECT_JAILBREAK. |
| Ports/iOSPort/nativeSources/CN1JailbreakDetector.m | Removes ineffective fork() probe, returns stable signal codes, preserves optional exit gate. |
| Ports/iOSPort/nativeSources/NetworkConnectionImpl.m | Adds per-cert chain grouping markers and SPKI digests during iOS TLS callback. |
| CodenameOne/src/com/codename1/ui/Display.java | Exposes resetAttestation() and getAppSignerDigests() on Display. |
| CodenameOne/src/com/codename1/security/DeviceIntegrity.java | Adds public resetAttestation() API and documentation. |
| CodenameOne/src/com/codename1/security/SecureStorage.java | Updates documentation describing the non-prompting storage tier on Android. |
| CodenameOne/src/com/codename1/security/shield/AppShield.java | New public App Shield facade (init/token fetch/attach/pins/signals/listeners/host policy). |
| CodenameOne/src/com/codename1/security/shield/ShieldConfig.java | New configuration object (endpoint/header/host policies/failure mode/signal collection). |
| CodenameOne/src/com/codename1/security/shield/HostPolicy.java | New per-host policy type (token attach/pin enforcement/failure mode). |
| CodenameOne/src/com/codename1/security/shield/FailureMode.java | New enum for open vs closed behavior when token fetch fails. |
| CodenameOne/src/com/codename1/security/shield/ShieldStatus.java | New wire-visible status vocabulary (non-enum constants + unknown-id round-trip). |
| CodenameOne/src/com/codename1/security/shield/ShieldToken.java | New token value type (local TTL validity, binding support, safe toString). |
| CodenameOne/src/com/codename1/security/shield/ShieldException.java | New IOException subtype carrying ShieldStatus. |
| CodenameOne/src/com/codename1/security/shield/PinSet.java | New immutable published pin set type with soft/hard expiry and wildcard support. |
| CodenameOne/src/com/codename1/security/shield/ShieldSignal.java | New signal value type (bounded severity + timestamp). |
| CodenameOne/src/com/codename1/security/shield/ShieldSignals.java | New bounded signal bus with listener dispatch on EDT. |
| CodenameOne/src/com/codename1/security/shield/ShieldListener.java | New listener for token status changes and signal events. |
| CodenameOne/src/com/codename1/security/shield/package-info.java | Package-level documentation for App Shield API. |
| CodenameOne/src/com/codename1/security/shield/spi/ShieldEngine.java | SPI contract for pluggable enterprise engine. |
| CodenameOne/src/com/codename1/security/shield/spi/EngineContext.java | Narrow framework services surface handed to engines. |
| CodenameOne/src/com/codename1/security/shield/spi/DefaultEngineContext.java | Framework-backed EngineContext implementation (defensive wrappers). |
| CodenameOne/src/com/codename1/security/shield/spi/ShieldEngineRegistry.java | Sealed engine registration mechanism (first registration wins). |
| CodenameOne/src/com/codename1/security/shield/spi/UnprotectedEngine.java | Inert default engine behavior when no enterprise engine is registered. |
| CodenameOne/src/com/codename1/security/shield/spi/package-info.java | SPI package documentation. |
| maven/core-unittests/src/test/java/com/codename1/security/shield/ShieldApiTest.java | Unit tests for shield value-types (statuses, tokens, host policies, pins, signals). |
| maven/core-unittests/src/test/java/com/codename1/io/SSLCertificateChainParsingTest.java | Unit tests for grouped certificate parsing and SPKI extraction behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Compared 12 screenshots: 12 matched. |
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Cloudflare Preview
|
SpotBugs SE_BAD_FIELD: IOException is serializable, so a non-serializable ShieldStatus field on ShieldException is both a static-analysis error and a latent null after a round trip -- which would break getStatus()'s never-null contract at exactly the moment someone is trying to work out why a request failed. Stores the id instead. ShieldStatus.forId resolves it back to the canonical constant, so identity comparisons and isTransient() still hold; tests pin that. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
CodenameOne/src/com/codename1/security/shield/ShieldToken.java:52
- ShieldToken currently defaults a null status to ShieldStatus.OK, which can incorrectly treat an unknown/buggy engine result as success and make isValid() return true. Safer default is a non-success status (consistent with ShieldException’s null handling).
public ShieldToken(String value, ShieldStatus status, long fetchedAt,
long ttlMillis, String binding) {
this.value = value;
this.status = status == null ? ShieldStatus.OK : status;
this.fetchedAt = fetchedAt;
this.ttlMillis = ttlMillis;
CodenameOne/src/com/codename1/impl/WebSocketImpl.java:118
- appendRequestHeaders() calls containsCrLf(value) without null/type checks. Because requestHeaders() exposes the backing Hashtable to subclasses/ports, a null or non-String value can be inserted and trigger an NPE during handshake construction.
protected final void appendRequestHeaders(StringBuilder req) {
java.util.Enumeration keys = requestHeaders.keys();
while (keys.hasMoreElements()) {
String name = (String) keys.nextElement();
String value = (String) requestHeaders.get(name);
CodenameOne/src/com/codename1/io/ConnectionRequest.java:956
- collectPublicKeyDigests is only ever set to true (when a NetworkGuard requires certificate checks) and is never reset. If a ConnectionRequest instance is reused for another URL/host later, this can leak into subsequent requests and change getSSLCertificates() behavior (breaking the “byte-identical unless pinned” guarantee).
boolean performOperationComplete() throws IOException {
if (shouldStop()) {
return true;
}
pinFailure = null;
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 19abeb5e33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
DCError.invalidKey is 3, not 2 -- 2 is invalidInput. With the wrong constant an invalidated key never entered the reset-and-reattest branch, so a device whose key the OS had discarded would fail forever, while malformed input would pointlessly burn a fresh hardware key. Verified against the DeviceCheck header. Key generation is asynchronous, so holding the flow lock only until the native call was issued did not serialize anything: a second caller still saw no key and generated its own, spending a second key against Apple's per-device budget. Callers arriving mid-bootstrap now queue and assert against the key the first bootstrap establishes, which is unlimited. They are also released on failure -- otherwise they would wait forever. The retry marker was dropped when key generation handed off to attestation, so a recovery whose replacement key also reported invalidKey would recover again rather than surface the failure. The network guard ran before the offline-cache check, so a fail-closed guard could fail a request that needed no network at all by being unable to fetch a token while offline. Moved after the cache hit. Also: null-guard parseGroupedCertificates, since a port returning null would surface as an NPE on the network path rather than as an empty chain; reset secureScreen in the simulator's reset; and release the CFDataRef in getFingerprint/getFingerprint256, which leaked one certificate's worth of data per digest on every connection. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (2)
Ports/Android/src/com/codename1/impl/android/AndroidSecureStorage.java:277
- In the non-prompting tier get(account), decryption/parse failures fall into the generic catch(Throwable) and return null but leave the corrupted ciphertext in SharedPreferences. This will cause repeated exceptions (and log spam) on every read of that account. Consider deleting the entry when decryption fails so subsequent reads fail fast and cleanly.
} catch (Throwable t) {
Log.e(t);
return null;
}
Ports/iOSPort/nativeSources/IOSNative.m:14593
- The comment mapping DCError values is off by one: DCError.invalidKey is ordinal 3 (unknownSystemFailure=0, featureUnsupported=1, invalidInput=2, invalidKey=3, serverUnavailable=4). Keeping the wrong number here is risky because it can lead to future fixes mis-handling the error code mapping.
installShieldSimulationMenu added the menu to simulateMenu and also returned it, while installNfcSimulationMenu and installFoldableSimulationMenu only return. The menu was not actually added twice -- simulateMenu.removeAll() runs between the two calls, in the block that rebuilds the final menu order -- but the inconsistency made that non-obvious enough to read as a bug. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2e6472ccf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (3)
CodenameOne/src/com/codename1/security/shield/ShieldToken.java:52
- ShieldToken defaults a null status to ShieldStatus.OK. That makes a token constructed with a missing/unknown status look successful, which can cause isValid()/shouldRefresh() decisions to silently treat a failure as success. A defensive default should be a non-success status (consistent with ShieldException’s null handling).
public ShieldToken(String value, ShieldStatus status, long fetchedAt,
long ttlMillis, String binding) {
this.value = value;
this.status = status == null ? ShieldStatus.OK : status;
this.fetchedAt = fetchedAt;
this.ttlMillis = ttlMillis;
Ports/iOSPort/nativeSources/IOSNative.m:14592
- The DCError ordinal in this comment looks incorrect: DCError.invalidKey is 3 (unknownSystemFailure=0, featureUnsupported=1, invalidInput=2, invalidKey=3, serverUnavailable=4). The Java side branches on invalidKey=3, so leaving “(2)” here is misleading when debugging failures.
CodenameOne/src/com/codename1/io/ConnectionRequest.java:940 - shouldInspectCertificates() sets collectPublicKeyDigests=true but never resets it. Because ConnectionRequest instances can be retried/redirected and reused, this can cause later certificate parsing to unexpectedly switch to the grouped/extended form (and drop SHA1 entries), changing what checkSSLCertificates() receives and adding extra work for hosts that aren’t pinned. Also, when checkSSLCertificates==true the method returns early and never consults NetworkGuard, so a pinned host won’t get SPKI digests if a request opted into the legacy certificate hook.
private boolean shouldInspectCertificates() {
if (checkSSLCertificates) {
return true;
}
NetworkGuard guard = NetworkManager.getNetworkGuard();
…me monotonically Four more from review. defaultFailureMode did nothing for hosts registered the short way. protect(host) stored the HostPolicy.PROTECTED constant, whose mode is always OPEN, so an app that asked to fail closed still sent requests without a token. The implicit policy is now built from the configured default. An explicitly supplied policy still wins. The certificate chain is cached on the request and survives retries and redirects, so the guard was vetting the previous connection's certificates: a request that first connected to a pinned host could then accept an unpinned certificate on a retry, and a redirect between differently pinned hosts could fail for no reason. Cleared at the start of each attempt. Token lifetime used System.currentTimeMillis(), which is the wall clock and therefore adjustable -- on a rooted device, by the attacker. That contradicted the class's own documented guarantee. Elapsed time now comes from System.nanoTime(); fetchedAt is kept for log correlation and is exposed and rendered rather than sitting unread. A null status defaulted to OK, so an engine bug could produce a token that reported itself valid and got attached to requests. It now defaults to a non-success status. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 44 out of 45 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
CodenameOne/src/com/codename1/io/ConnectionRequest.java:962
- collectPublicKeyDigests is set lazily in shouldInspectCertificates() but is never reset between attempts. Since ConnectionRequest instances can be retried/redirected, this can cause a later attempt (or a request that only opted into legacy checkSSLCertificates) to incorrectly keep using the extended certificate format even when the guard no longer requires it, breaking the "byte-identical legacy data" guarantee.
pinFailure = null;
// Each attempt gets its own chain. This field is populated lazily and
// survives retries and redirects, so without clearing it the guard would
// vet the previous connection's certificates -- accepting an unpinned
// certificate on a retried request, or rejecting a redirect to a
// differently pinned host.
sslCertificates = null;
CodenameOne/src/com/codename1/security/shield/spi/UnprotectedEngine.java:43
- The class-level contract comment says fetchToken "never throws synchronously", but UnprotectedEngine.fetchToken() always throws ShieldException. This is an implementation/doc mismatch that can mislead engine implementers and reviewers.
/// - `fetchToken` **completes**, with a [ShieldStatus#UNPROTECTED] failure. It never hangs and
/// never throws synchronously, so callers written for the real engine follow their normal error
/// path instead of deadlocking.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 41c16d6f5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three more from review. A redirect reuses the same request object with its headers intact, and attach() only ever added. A protected endpoint with an open redirect would therefore hand a replayable attestation token to whatever host it pointed at. The header is now cleared before the policy for the current host is evaluated, and re-added only if that host is protected. Adds ConnectionRequest.removeRequestHeader, since there was no way to remove a header at all. The earlier bootstrap fix left the STATE_NEW window open: between key generation persisting the identifier and its attestation completing, the key exists but is not attested, so a caller arriving then bypassed the bootstrapInFlight check and attested the same key again. Attestation is rate limited, so that costs real budget and races its own result. The check now happens before branching on key state, and a key that exists but was never attested is attested rather than replaced. iOS getCompromiseReasons dropped the Cydia probe, which for apps that declare the scheme was the only signal on a device where the file, dyld, write and tracing probes come back clean -- a regression against the previous behaviour. It is consulted again, through a helper rather than isJailbrokenDevice(), which now delegates the other way. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 73 out of 74 changed files in this pull request and generated no new comments.
Suppressed comments (1)
scripts/hellocodenameone/common/src/main/java/com/codenameone/examples/hellocodenameone/tests/BrowserComponentScreenshotTest.java:198
requiredBrightPixels/requiredDarkPixelscan exceed the total number of pixels in the sampled band (e.g., on very small layouts or extreme scaling), makingcontainsRenderedBrowserContent()impossible to satisfy and causing the test to time out/fail deterministically. Cap both thresholds tobandWidth * bandHeightso the predicate remains achievable for small bands while preserving the existing behavior for normal sizes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 73 out of 74 changed files in this pull request and generated no new comments.
Suppressed comments (1)
CodenameOne/src/com/codename1/security/shield/ShieldConfig.java:96
ConnectionRequest.setCookieHeader()allows setting the cookie header name to null, but this code calls.equals()on the normalized cookie header without a null check. IfcookieHeaderis null,ShieldHosts.normalize(...)returns null and this throws aNullPointerExceptionwhen configuring the token header.
if (ShieldHosts.normalize(ConnectionRequest.getCookieHeader())
.equals(normalized)) {
|
Compared 148 screenshots: 148 matched. Benchmark Results
Detailed Performance Metrics
|
My check for outstanding feedback queried review THREADS, and these arrived as review bodies -- so I reported these PRs as having nothing open while four findings sat in them. The mechanism is now part of the sweep; the findings are below. One cipher per biometric operation. The prompt is raised from a UI runnable, so an operation is in flight from the moment it initialises its cipher until that runnable executes -- and with one instance field a second set() or get() starting in that window re-initialised the same object, handing the first prompt the second operation's cipher. Wrong mode, or the wrong account's IV, and then the failure handler read that as an invalidated key. Which is the second finding: that handler deleted the single keystore key behind every biometric account for ANY failure -- a malformed stored value, an Activity that went away mid-prompt -- making every other entry permanently unreadable while telling the caller its key had been revoked. Only the two shapes that mean the key itself is finished delete it now: the exception Android raises, and the Samsung 8.0.0 quirk of a cipher that initialises and then fails inside doFinal with a keystore error underneath, which is what the original workaround was for. A loopback connection belongs to the listener that accepted it. The callback resolved the process-wide active transport when it RAN, so a transport that accepted a client just before closing handed those streams to whatever opened next -- a client of the stopped listener taking over the new server's session. The binding is captured when the socket API constructs the callback, which is at accept time, and a connection whose listener has since closed is released rather than adopted. And two in the simulator's secure storage: the key derivation is locked on the class rather than the instance, because JavaSEPort.getSecureStorage() builds its singleton without synchronization and two instances would each generate a salt into the same shared node -- whichever lost leaving permanently undecryptable ciphertext; and the literal zero byte in the source is now an escape, which is what makes git treat the file as text again. It had been classified as binary, so diffs reported "- -", grep matched nothing, and the two greps I ran while investigating this file silently returned empty. Core suite 4744 green, SpotBugs 0 on core-unittests, android and ios, PMD gate clean, markdown-docs gate clean. The MCP regression test runs the callback on its own thread with a deadline: under the old behaviour it parks forever, and a test that hangs the suite is worse than one that fails. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Four findings from this PR's review bodies are fixed in fa7c944, and the reason they sat there is worth stating first: my check for outstanding feedback queried review THREADS only. These arrived as review bodies, so I reported this PR as having nothing open while four findings were in it. The sweep now covers both. Isolate the cipher for each biometric operation -- the prompt is raised from a UI runnable, so an operation is in flight from the moment it initialises its cipher until that runnable executes. A second Preserve unrelated secrets on per-entry failures / Delete the biometric key only for invalidation failures -- the same catch, reported twice, and both were right. One keystore key backs every biometric account, so a malformed stored value or an Activity that disappeared mid-prompt destroyed every other entry and told the caller its key had been revoked. Deletion is now restricted to the two shapes that mean the key is finished: Bind callbacks to the listener that accepted them -- the binding is captured in the Make the simulator key lock process-wide -- Replace the embedded NUL with a source escape -- done, and it had already cost something: while investigating this very file I ran two greps that silently returned nothing, because git and grep were treating it as binary. Escaped as an octal Verification: core suite 4744 green; SpotBugs 0 on core-unittests, android and ios; PMD and markdown-docs gates clean. The MCP regression test runs the callback on its own thread with a deadline -- under the old behaviour it parks forever, and a test that hangs the suite is worse than one that fails; I confirmed it fails (not hangs) against the unfixed resolution. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fa7c94412a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ot tokens Holding the class monitor across callSerially was the wrong fix and would have been a worse bug than the one it closed: callSerially runs the task INLINE before the EDT is up, so it would have run application listeners under that monitor -- and a listener that touches the shield, or waits on a thread that does, deadlocks against attach(), which waits on the same monitor for initialization. The transition is written under the lock and the dispatch is queued outside it again; staleness is settled where ShieldSignals settles it, by a dispatch that drops itself when it no longer describes the current status. The test moved with the invariant. It no longer asserts that a second transition cannot be stored -- that is exactly what is now allowed -- but that a superseded status is never announced and that what a listener is left holding is what getStatus reports. It fails on the unfixed code with the listener ending on serviceUnavailable. Header names are validated as HTTP field tokens before anything else is decided about them. Screening only for CR and LF let "Sec-WebSocket-Extensions " -- one trailing space -- past the reserved-name comparison, and a lenient server trims that and negotiates permessage-deflate. No reader here looks at RSV1 or inflates anything, so every frame after that arrives as garbage. Rejected rather than trimmed: a caller who wrote a trailing space meant one header and the server would read another, and repairing that quietly is how the two ends stop agreeing about what was sent. And the simulator's secure storage flushes before reporting success, on both the write and the removal. Preferences writes back on its own schedule, and the simulator is killed abruptly all the time -- by the run button, by the IDE -- so "stored" meant "in memory" and a cleared credential could come back. Core suite 4746 green, SpotBugs 0, PMD and markdown-docs gates clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 75 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
quality-report.md:4
- This looks like a CI-generated artifact that the workflows already produce/upload (see
.github/scripts/generate-quality-report.pywriting toquality-report.md, and.github/workflows/pr.ymluploading it). Committing it to the repo will likely create noisy diffs on unrelated PRs; consider removing it from version control and (optionally) adding it to.gitignoreinstead.
The flush() I added references it and the import never landed, so every job that compiles the JavaSE port failed. Verified by building the port rather than by reading: the class compiles now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 75 changed files in this pull request and generated no new comments.
Suppressed comments (3)
CodenameOne/src/com/codename1/impl/WebSocketImpl.java:122
appendRequestHeaders()assumes the hashtable entry is always present for a key it just enumerated. If a header is removed concurrently (or the table is modified while iterating),paircan be null and this will throw a NullPointerException during handshake header emission.
String key = (String) keys.nextElement();
String[] pair = (String[]) requestHeaders.get(key);
String name = pair[0];
String value = pair[1];
scripts/build-android-app.sh:174
- This script runs with
set -euo pipefail, and thegrepinside the command substitution will exit with status 1 if the property line isn't present (e.g., ifsedfails or the file format changes), which would terminate the script while only trying to log diagnostics. Make the log line non-fatal.
quality-report.md:5 quality-report.mdappears to be a CI-generated artifact (see.github/scripts/generate-quality-report.pywriting to this path, andpublish-quality-comment.jsposting it). Committing a static report risks publishing stale/incorrect results (e.g., if report generation fails, the workflow can still find this checked-in file and comment/upload it). This file should be generated during CI, not versioned in the repo.
ios.facebook.version is appended to the pod list and interpolated into the generated Podfile unescaped, and a Podfile is Ruby that pod install executes in a workspace holding signing material. The daemon copy is constrained; this is the same builder, so it carries the same constraint. Refused rather than escaped, and the list construction is its own method so the check has a caller a test can reach. Core suite 4746 green, PMD gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 76 out of 76 changed files in this pull request and generated no new comments.
Suppressed comments (1)
CodenameOne/src/com/codename1/security/shield/PinSet.java:169
- PinSet.matches() calls isEnforcedFor(host) and then calls pinsFor(host) again, which means the wildcard lookup and Vector copy work happens twice per check. This is on the request path, so it’s worth avoiding the duplicate allocation/work by doing the enforcement check and match in one pass.
public boolean matches(String host, String[] chainSpkiDigests) {
if (!isEnforcedFor(host)) {
return true;
}
if (chainSpkiDigests == null || chainSpkiDigests.length == 0) {
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ba32bf7bc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The cleanup was scoped to the request the shield decorated, but the request is not untouched in between: performOperationComplete() calls onRedirect() between the attachment and the next attach(), and that hook is where an app installs the headers the redirect target needs. An app whose target expects its own key under the name the shield uses lost it, because the record held the request and the name and not the value that says whose header it is. The record now carries the token, and ConnectionRequest gains the primitive that compares it -- removeRequestHeaderIfUnchanged -- since a decorator cannot read back what it set. A value that no longer matches means the header is the app's, so it stays and the bookkeeping entry goes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Flush authenticated preferences before reporting success
Fresh evidence beyond the fixed non-prompting overload is that this authenticated simulator path still completes with true immediately after Preferences.put(), without flushing. If the simulator is stopped or crashes before the preferences implementation writes back, the supposedly stored credential is missing after restart; the authenticated remove() path at line 118 has the symmetric problem and can resurrect a deleted value. Flush both operations and surface persistence failure through the returned resource.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 75 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
scripts/ci/retry.sh:64
- In RETRY_ONLY_MATCHING mode,
grepis invoked without--. If the regex begins with-(e.g. a pattern copied from a tool that prefixes flags),grepwill treat it as an option instead of a pattern and the wrapper will misclassify failures (or even error out). Adding--makes this robust for all valid regex values.
An identical repeat replaces the stored entry and deliberately queues nothing, so a detector polling on a timer could replace the entry while the first sighting's notification was still in flight. That notification then asked whether it was still the current OBJECT, found it was not, and dropped itself -- and nothing else was ever going to announce it. The signal sat in snapshot() with no listener told, and the sighting lost is the first one, which is the reason a listener is attached at all. The delivery test now asks whether the entry still says the same thing. A superseded report -- different severity or detail -- is still dropped, which is what the test exists for. The cost is that two reports of one observation can both be announced, and being told twice about a signal that is really there is not a defect. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… guard The challenge delegate is entered once per challenge and a connection can face several -- a redirect to another host presents its own chain -- so overwriting the field leaked the string from the previous one. The retain was also unconditional while the rest of the file keeps its retains behind CN1_USE_ARC, and clang refuses retain outright under ARC. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f95273778b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
addProtectedHost takes the same patterns as ShieldConfig.protect, but the runtime table was looked up by exact name, so a registered "*.example.com" covered nothing: api.example.com missed it, fell through to a configuration that had never heard of the host, and went out with no token and no certificate check -- while protectedHosts() published the wildcard to the engine, so a pin set was built for a host the request path had already called unprotected. One key generator now serves both, so there is no second resolution to drift, and the two tables are consulted key by key rather than one whole before the other: specificity decides first, and at the same key the runtime registration wins as the later statement of intent. A configured exact host is therefore not relaxed by a wildcard registered afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 75 changed files in this pull request and generated no new comments.
Suppressed comments (1)
CodenameOne/src/com/codename1/impl/WebSocketImpl.java:122
- appendRequestHeaders() assumes requestHeaders.get(key) always returns a non-null String[2], but keys() enumeration isn’t synchronized with modifications. If a caller mutates headers after connect() starts (easy to do given connect is async), requestHeaders.get(key) can return null and this will throw a NullPointerException during handshake construction.
protected final void appendRequestHeaders(StringBuilder req) {
java.util.Enumeration keys = requestHeaders.keys();
while (keys.hasMoreElements()) {
String key = (String) keys.nextElement();
String[] pair = (String[]) requestHeaders.get(key);
String name = pair[0];
String value = pair[1];
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd99a9cdea
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Folding case does not make a malformed name comparable: "Cookie " normalizes to "cookie ", matches nothing on any reserved list, and walks straight past the checks written for exactly that name. It then goes on the wire -- the JavaSE path hands it to HttpURLConnection, which accepts a trailing space -- where an intermediary is entitled to drop the malformed field or read it as transport metadata. A fail-closed host is then satisfied by attach() reporting success while the backend received no usable token. Validated as an RFC 9110 token before any reserved-name check, refusing rather than trimming for the reason the websocket handshake gives: a caller who wrote a trailing space meant one header and a lenient server would read another. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 448e267e39
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Taking the claim and recording what it was spent on were two publications, so between them the resource read as claimed-but-not-delivered -- which is exactly how lostItsClaim() spells "the delivery lost". A same-nonce retry landing in that interval took the retained attestation while the delivery it had just misread went on to complete, leaving two callers holding one attestation. It is replay-protected, so the second submission is rejected, and an app that reads a rejection as a bad key resets one the backend had just registered. The claim now moves straight to DELIVERED or LOST in the compare-and-set that takes it, so there is no interval to observe. Isolated in a harness, the old shape had the retry observe a loss during a winning delivery in 1998 of 2000 rounds; the new one never does. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Close clients accepted during shutdown
When stop() closes the transport after ss.accept() returns but before this assignment, close() cannot see the newly accepted socket and therefore never closes it. acceptNextClient() then publishes the socket and readMessage() can block indefinitely on a client belonging to a stopped generation, leaking both the socket and reader thread; a same-instance restart can consequently overlap this stale reader after the two-second handover timeout. Recheck closed while publishing the accepted socket and close it instead when shutdown has already won.
Cap JavaSE MCP request frames
When a malformed or hostile local MCP client sends a very large request—or simply never sends the terminating newline—BufferedReader.readLine() grows its buffer without any limit and can exhaust the simulator JVM heap. The portable production transport explicitly caps incoming frames at 8 MiB in MCPLoopbackSocketTransport.MAX_FRAME_BYTES; enforce an equivalent bound while reading the JavaSE transport rather than materializing an arbitrarily large line.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 75 out of 75 changed files in this pull request and generated no new comments.
Suppressed comments (2)
CodenameOne/src/com/codename1/impl/WebSocketImpl.java:122
appendRequestHeaders()dereferencespair[0]/pair[1]without guarding againstrequestHeaders.get(key)returning null (possible if a header is removed concurrently while enumerating). That can lead to an NPE during the handshake; adding a null/length guard is cheap and makes this robust even if callers mutate headers unexpectedly.
java.util.Enumeration keys = requestHeaders.keys();
while (keys.hasMoreElements()) {
String key = (String) keys.nextElement();
String[] pair = (String[]) requestHeaders.get(key);
String name = pair[0];
String value = pair[1];
CodenameOne/src/com/codename1/security/shield/spi/EngineContext.java:69
- The Javadoc here says app-signer digests are "Not exposed as public framework API", but
Display#getAppSignerDigests()is a public method in this PR. This is an internal-consistency/doc issue: either the API is intentionally public (then the doc should be softened), or it should be kept off public surfaces.
Client half of an enterprise attestation / API-shielding service, plus several pre-existing defects it uncovered.
Paired with codenameone/BuildCloud (verification service) and codenameone/BuildDaemon (build provenance). This PR is safe to merge on its own — with no engine present everything degrades to a no-op.
Fixes worth reviewing on their own merits
iOS App Attest never linked.
IOSNative.javadeclaresnative boolean isAppAttestSupported(), butIOSNative.mdefined it without ParparVM's_R_booleanmangling in both#ifdefarms.ios.appAttest=truetherefore failed at link time, which means the feature has never shipped working. CompareisDarkMode___R_boolean.App Attest was also wrong once it linked. It called
generateKey+attestKeyon every request and never calledgenerateAssertion. Apple's model is attest once, then assert many times against the key the server recorded — the old flow burned the per-device attestation budget and made device continuity impossible. Rewritten as a keychain-backed state machine withDCError-driven recovery and throttle backoff.The non-prompting
SecureStoragetier was missing on Android and JavaSE, soSecrets.get()re-hit the network on every call on Android. Implemented with a dedicated AndroidKeyStore key — deliberately separate from the biometric key, which is invalidated whenever the user re-enrols.CN1JailbreakDetector'sfork()probe exited the child, not the app, so it never did what its comment claimed. Removed rather than repaired: it is a restricted syscall that trips App Review static analysis, for a signal the dyld and path probes already carry. The detector is now a signal source plus a thin exit wrapper, so iOSgetCompromiseReasons()reports something real — previously it was only acanExecute("cydia://")probe, which returns false on modern iOS unless the app separately declares the scheme.New API
com.codename1.security.shield.AppShieldplus an SPI seam. Discovery is a registered singleton, notClass.forName— the convention documented inSystemBrowser, because class names are obfuscated and name-based lookup is unreliable by construction.The in-tree
UnprotectedEngineis the open-source default and fails open in every direction:fetchToken()completes rather than hangs,attach()is a no-op, nothing is ever blocked. An app written against this API compiles and runs unchanged without an engine.NetworkGuardis a narrow seam incom.codename1.io, wired intoConnectionRequest. The existing per-requestcheckSSLCertificateshook still runs first and unchanged, so an app that already pins keeps working.SSLCertificategains public-key digests. The existing fingerprints are whole-certificate digests, which change on every renewal even on the same key — pinning those is how pinning earns its reputation for outages. Opt-in, so existing callers see byte-identical data.Simulator
JavaSEPorthad noDeviceIntegrityoverrides, so the branches an app takes on a compromised device were unreachable off real hardware. AddsSimulate > App Shield. The forced pin-mismatch toggle is the most valuable item: a fail-closed pinning branch is otherwise only testable by deliberately mis-pinning a live host.Verification
ios.appAttestopenssl x509 -pubkey | openssl pkey -pubin -outform der | openssl dgst -sha256for EC and RSA chains before being written in Objective-CReviewer notes
HostPolicy.UNPROTECTEDmeans an unregistered host is never touched — no header, no pin check, no possibility of a blocked request.WebSocket.header()is documented as silently dropped on iOS and in the browser, because those hand the handshake to a platform WebSocket that exposes no way to add headers. Stated plainly rather than implied to work.cn1aa1:token prefix is a wire-format break, taken deliberately: since App Attest never linked, there are no deployed consumers.🤖 Generated with Claude Code