App Shield: attestation API, network guard, and iOS App Attest fixes - #5486
Open
shai-almog wants to merge 37 commits into
Open
App Shield: attestation API, network guard, and iOS App Attest fixes#5486shai-almog wants to merge 37 commits into
shai-almog wants to merge 37 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".
Contributor
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.
Collaborator
Author
|
Compared 12 screenshots: 12 matched. |
Collaborator
Author
|
Compared 146 screenshots: 146 matched. Benchmark ResultsDetailed Performance Metrics
|
Collaborator
Author
|
Compared 151 screenshots: 151 matched. Native Android coverage
✅ Native Android screenshot tests passed. Native Android coverage
Benchmark ResultsDetailed Performance Metrics
|
Contributor
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>
Contributor
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>
Contributor
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".
Contributor
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>
Contributor
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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a9dd40dd8
ℹ️ 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".
ShieldConfig snapshotted the default failure mode when a host was registered without an explicit policy, so .protect(h).defaultFailureMode( CLOSED) left that host fail-open while the finished config reported a closed default. A builder gets chained in whatever order reads well, so implicit registrations are now recorded as implicit and resolved against the default at read time. The recovery-spent marker's write result was ignored, so a refused keychain write let every later request discard the replacement key and generate another. It is mirrored in memory, which bounds the one-shot recovery for the life of the process even when the keychain will not persist it. 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: 08cbb049b4
ℹ️ 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".
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (3)
Ports/iOSPort/nativeSources/IOSNative.m:14604
- CN1_APP_ATTEST_ON_MAIN dispatches completion handlers onto the main queue but does not wrap the callback in POOL_BEGIN/POOL_END. Other dispatch_async callbacks in this file allocate a pool inside the block; without it, autoreleased objects (and conversions like fromNSString) can leak and VM re-entry may run without the expected pool context.
CodenameOne/src/com/codename1/security/shield/spi/EngineContext.java:70 - The comment says getAppSignerDigests() is "Not exposed as public framework API", but this PR adds Display.getAppSignerDigests() (and routes it through impl.getAppSignerDigests()). This is now inconsistent/misleading for engine implementers.
/// Not exposed as public framework API: nothing in an app needs this, and publishing it would
/// only tell an attacker exactly which value to fake.
String[] getAppSignerDigests();
CodenameOne/src/com/codename1/impl/WebSocketImpl.java:122
- appendRequestHeaders() assumes requestHeaders.get(key) always returns a non-null String[2]. If a header is removed (or otherwise mutated) while iterating, pair can be null and this will throw an NPE during the handshake.
String key = (String) keys.nextElement();
String[] pair = (String[]) requestHeaders.get(key);
String name = pair[0];
String value = pair[1];
MCPServer's reader thread identified its own run by transport identity, so a caller that stopped and restarted with the SAME transport instance left the old thread looking current -- it could run on beside the replacement and eventually stop the server and close the transport underneath it. Each start now carries a generation and the thread checks both. The spent-recovery marker deletion result is honoured: if the keychain refuses it, the in-memory copy stays set so the process keeps treating the recovery as spent. Forgetting it while the persisted marker survived would refuse the one-shot replacement the next time iOS legitimately invalidated the key, leaving every assertion failing until the app reset by hand. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
CodenameOne/src/com/codename1/impl/WebSocketImpl.java:131
- appendRequestHeaders() iterates requestHeaders without synchronizing and assumes each enumerated key still maps to a non-null String[2]. If another thread calls setRequestHeader() concurrently (or a port subclasses/mutates the map), requestHeaders.get(key) can return null and pair[0] will throw, potentially crashing the connect/handshake thread. Synchronize on requestHeaders while enumerating and guard against null/short pairs.
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];
CodenameOne/src/com/codename1/security/shield/ShieldConfig.java:115
- ShieldConfig.protect(hostPattern, null) records implicit hosts by unconditionally adding to implicitHosts, which can accumulate duplicates if protect() is called multiple times with the same host. This wastes memory and makes later operations (contains/removeElement) do extra work. Treat implicitHosts as a set by only adding when not already present.
hostPolicies.remove(key);
implicitHosts.addElement(key);
} else {
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c22f167308
ℹ️ 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".
…transport NetworkManager holds one guard and seals the slot, so an app with its own guard had no way to keep the shield's. The advice it printed -- call attach() from your own guard -- restores the token header and silently drops the certificate callbacks, so HostPolicy.isEnforcePins() stopped being enforced and nothing about the app's behaviour said so. AppShield.getNetworkGuard() now returns the shield's guard so an app can delegate to it, documented with the whole delegation including the interestingResponseHeaders/afterResponse positional pairing, and the recovery message points at it. MCPServer's stale-run teardown closed the transport unconditionally. A stop()/start() pair over the same transport instance therefore had the old reader close the transport the restarted server was serving over: the generation check correctly kept it from stopping the server, then it killed the server anyway one line later. Ownership and the close are now decided together under the lock -- close when still current, close when superseded by a replacement holding a different transport (nobody else can, and a leaked transport stays registered process-wide), leave it alone when the replacement reused this one. The test parks each reader on its own ticket so a superseded reader can be released while the current one stays blocked, which is the only way to observe what the stale thread does on its way out; it fails on the previous code. The two anonymous Runnables in IOSDeviceIntegrity's instance methods captured the enclosing instance for as long as the EDT queue held them, for no reason. Extracted as a static failResource(), which is also what SpotBugs was asking for. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
CodenameOne/src/com/codename1/impl/WebSocketImpl.java:131
- appendRequestHeaders() iterates over requestHeaders without external synchronization and assumes requestHeaders.get(key) always returns a non-null String[]; if a header is removed concurrently (e.g. user calls WebSocket.header(name, null) while the connect thread is building the handshake), pair can be null and this will throw a NullPointerException, potentially crashing the connection attempt. Wrap the enumeration in a synchronized(requestHeaders) block (so it shares the Hashtable monitor with put/remove), and defensively skip null pairs.
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];
Ports/Android/src/com/codename1/impl/android/AndroidSecureStorage.java:465
- legacyPlainGet() only catches IOException, but Base64.decode() can throw IllegalArgumentException on malformed input (e.g. a corrupted preferences entry). That would escape and crash callers on API <23, defeating the intent of this tier to degrade safely. Catch Throwable (or at least IllegalArgumentException) the same way decodeLegacyPlain() does.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4bba0eeca6
ℹ️ 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".
…tation serverUnavailable recorded the backoff and left the started marker in place, so after the deadline the interrupted-attestation branch treated an explicit retryable failure as an unknown outcome, discarded a perfectly good rate-limited hardware key and generated another. Repeated often enough -- once per backoff through an Apple outage -- the recovery throttles the device more thoroughly than the outage did. The marker means "submitted, outcome unknown", and discarding the key is the right answer to that, because a spent one-time attestation cannot be resubmitted. serverUnavailable is not that: Apple has told us it did not process the attestation, so the key is untouched and the marker has nothing left to protect. Cleared for that code alone, on the attest operation alone. A failed removal leaves it in place, which is the conservative state the branch reached before. 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: b4efd02fdf
ℹ️ 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".
nativeKeyGenerated with no key identifier, and nativeAttestationReady with no attestation, failed only the request that started the bootstrap. bootstrapInFlight stayed set, so every caller already queued behind it -- and every caller that arrived afterwards and was queued precisely because a bootstrap looked live -- waited on a bootstrap that had already stopped, with nothing left to release them. The mid-flow storage failures in both callbacks got this right; the two give-up-early branches did not. Both now go through one helper that clears the flag and fails the waiters under the same lock a reset takes. A stale request still only fails itself: a reset has already cleared the flag and failed the waiters, and clearing it again would clear the flag belonging to the replacement bootstrap that reset started. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
CodenameOne/src/com/codename1/security/shield/PinSet.java:169
- PinSet.matches() currently calls isEnforcedFor(host) (which internally calls pinsFor(host)) and then calls pinsFor(host) again, causing two Vector copies and two wildcard lookups per match check. Since this runs on every pinned connection, it’s worth avoiding the duplicate work by doing the enforcement check inline with a single pinsFor() call.
public boolean matches(String host, String[] chainSpkiDigests) {
if (!isEnforcedFor(host)) {
return true;
}
if (chainSpkiDigests == null || chainSpkiDigests.length == 0) {
return false;
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0d605094af
ℹ️ 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".
…t it clears nativeAssertionReady checked staleness under the lock, released it, and then reacquired it to reset the throttle state. A reset landing in between let the old callback clear currentBackoff, retryAfterFallback and the stored retry deadline on behalf of the replacement generation -- and if that replacement had already recorded a serverUnavailable, its deadline went with them, while succeed() went on to reject this token as stale anyway. So the next request walked straight back into a service Apple had just told the device to stay away from, which is how an app gets its attestation budget suspended. One acquisition now covers both, which is the rule nativeAttestError already states and follows. The empty-payload check moved above it: it is a test of an argument, not of shared state, and doing it first keeps the lock holding only what the generation guards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 51 out of 52 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
Ports/Android/src/com/codename1/impl/android/AndroidImplementation.java:7213
- Avoid writing stack traces directly to stderr in the port implementation. This method currently calls
printStackTrace(), which makes noisy/unstyled logs and bypasses the framework’s logging facilities (and any log routing the app/framework might provide). Usecom.codename1.io.Log.e(ex)(or another existing logging mechanism in this file) instead.
Ports/JavaSE/src/com/codename1/impl/javase/JavaSEPort.java:14455 - Avoid using
printStackTrace()here. It bypasses Codename One’s logging and produces noisy stderr output in production/simulator logs. Prefercom.codename1.io.Log.e(ex)(or the existing logging approach used elsewhere in this port) so failures are captured consistently.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a88c9a8c33
ℹ️ 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".
Comment on lines
+268
to
+269
| if (releaseIfCurrent(t, generation)) { | ||
| t.close(); |
There was a problem hiding this comment.
Fresh evidence after the prior fix is that releaseIfCurrent() only returns the ownership decision; its synchronized monitor has already been released before t.close() runs here. If a current reader reaches EOF, marks running false, and another thread immediately restarts the server with the same transport in that gap, the old reader closes the transport now owned by the replacement generation. Perform the ownership check, state transition, and close atomically under the same monitor.
Useful? React with 👍 / 👎.
| SecureStorage.getInstance().set(KEY_RETRY_AFTER, | ||
| Long.toString(deadline)); | ||
| instance.currentBackoff = Math.min(backoff * 2, MAX_BACKOFF_MILLIS); | ||
| if (pending.op == PendingRequest.OP_ATTEST) { |
There was a problem hiding this comment.
When attestKey returns DCError.invalidInput (or featureUnsupported), this cleanup is skipped because only serverUnavailable clears KEY_ATTEST_STARTED. The callback has definitively reported a failed attestation, but the next requestToken() interprets the retained marker as an unknown crash outcome, deletes the still-unconsumed key, and generates another rate-limited hardware key; a persistent invalid-input condition therefore burns one key per request. Clear the start marker for definitive non-invalidKey failures or persist a terminal failure without replacing the key.
Useful? React with 👍 / 👎.
…ed failure releaseIfCurrent decided ownership under the lock and returned; the caller closed afterwards. A reader reaching EOF clears running, a restart over the same transport lands in that gap, and the close then lands on the replacement's transport -- the same outcome the ownership check was added to prevent, moved a few instructions later. The decision and the close now happen in one synchronized method, which is what stop() already does. Attestation kept its start marker for every error except serverUnavailable, so a persistent invalidInput or featureUnsupported discarded a still-unconsumed hardware key and generated another on every request, with nothing bounding it. Reaching that callback at all means Apple answered, so the outcome is not unknown -- and for anything but invalidKey, which resets separately, the key was not consumed. The marker is cleared for all of them, including unknownSystemFailure: if that one did land after Apple consumed the attestation, resubmitting is answered with invalidKey and self-corrects through the one-shot recovery, which is cheaper than a fresh rate-limited key per request forever. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
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