App Shield: attestation API, network guard, and iOS App Attest fixes - #5486
Open
shai-almog wants to merge 97 commits into
Open
App Shield: attestation API, network guard, and iOS App Attest fixes#5486shai-almog wants to merge 97 commits into
shai-almog wants to merge 97 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: 3d4382ea0f
ℹ️ 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".
… resolution removeRequestHeader scanned the header map, but addRequestHeader routes Content-Type to a dedicated field -- so removing it removed nothing and initConnection went on emitting it. A removal that silently does not remove is worse than an unsupported one, because the caller has been told otherwise. It resets the field and the explicitly-set flag, which is the state a request that never mentioned it is in. attach() also has to clear the token header's other spellings on the request it is about to decorate. Header names are case-insensitive, so adding ours beside an app's existing x-cn1-attest left two fields on the wire for the backend or an intermediary to choose between, with attach() reporting success. Done at the attach site rather than in the general cleanup, which must not touch a request the shield is not decorating. And the Windows cross-compile retry loop matched every failure. An intermittent compiler or packaging regression could pass on a later attempt and turn a blocking gate green, and the retries reused the previous attempt's outputs. It now matches the output for a dependency-resolution error, retries from clean, and reads PIPESTATUS rather than the pipeline's status -- tee succeeds even when Maven does not, and this step sets no pipefail. 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: 645e09c6ae
ℹ️ 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 70 out of 71 changed files in this pull request and generated no new comments.
Suppressed comments (1)
CodenameOne/src/com/codename1/security/shield/ShieldConfig.java:83
ShieldConfig.tokenHeader()normalizes the header name viaShieldHosts.normalize(name), which is designed for hostnames and strips a trailing dot. HTTP header field-names may legally end with '.', so this can incorrectly treat distinct header names as the same (or reject a valid header because the trailing '.' is removed during reserved-name checks). Header normalization here should be ASCII case-folding only, without hostname-specific dot trimming.
public ShieldConfig tokenHeader(String name) {
if (name != null && name.length() > 0) {
String normalized = ShieldHosts.normalize(name);
if ("content-type".equals(normalized)) {
AsyncResource.complete() does not consult the cancelled flag -- it stores the value, marks the resource done and runs the success callback regardless -- so a caller that gave up while the attestation round trip was in flight still had its ready callback invoked when the answer arrived, and the error branches did the same. Cancelling means the caller has stopped listening, which the rest of the framework is built on and tests. fetchToken now hands back a resource where cancellation and delivery claim the same slot and the loser does nothing; both public entry points are overridden, not only the paths this class uses, because the object is handed to application code. AndroidSecureStorage answered true for a removal it had only queued. apply() persists on a background thread, so an app clearing a credential on logout and then being killed -- which on Android is how a process usually ends -- finds it back on the next launch. The removal commits and returns what the commit says, under the same lock as the write and the reset. The three other places in the file with the same shape went with it: the legacy API-22 write and both halves of the prompting tier, where an apply() could also persist a ciphertext whose IV had not landed. Two CI failures, both with a mechanism rather than a re-run. packageDebug was dying intermittently in OutOfMemoryError inside PackageAndroidArtifact -- the --stacktrace added last round is what showed it. This script runs --no-daemon, so merging, dexing and packaging share one 2048m JVM and the packaging step, which reads each entry into a byte[], is simply last in line; it now asks for 4096m, which the runner has. The Android BrowserComponent screenshot emitted a blank white frame because the readiness check looked only for bright pixels: it was written for iOS, where an uncomposited peer is BLACK, and on Android an unpainted WebView is WHITE and satisfies it on the first try. It now requires the fixture's dark background as well as its light text, which describes the fixture rather than one platform's failure colour, and neither blank state can satisfy it. Core suite 4739 green; SpotBugs 0 across core-unittests, android, ios and the maven plugin; PMD gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 71 out of 72 changed files in this pull request and generated no new comments.
Suppressed comments (2)
scripts/ci/retry.sh:64
- grep should use "--" before the pattern so a RETRY_ONLY_MATCHING value that begins with '-' isn't interpreted as a grep flag (and to avoid surprising failures if someone changes the regex).
CodenameOne/src/com/codename1/security/shield/ShieldConfig.java:96 - Possible NullPointerException: ConnectionRequest.setCookieHeader() allows setting the cookie header name to null, which would make ShieldHosts.normalize(ConnectionRequest.getCookieHeader()) return null and then .equals(normalized) will throw. This would break ShieldConfig.tokenHeader() for apps that cleared the cookie header.
if (ShieldHosts.normalize(ConnectionRequest.getCookieHeader())
.equals(normalized)) {
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f9bec7ebe6
ℹ️ 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".
Collaborator
Author
|
Compared 144 screenshots: 144 matched. |
Collaborator
Author
|
Compared 143 screenshots: 143 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
Collaborator
Author
|
Compared 217 screenshots: 217 matched. |
Collaborator
Author
|
Compared 149 screenshots: 149 matched. Benchmark Results
Build and Run Timing
Detailed Performance Metrics
|
AsyncResource.onResult is ready() followed by except(), and only the first of those was overridden here -- so a worker thread registering on a resource that had already failed ran the error half immediately, on that worker. onResult is the application-facing form, so what that produces is an app handling a health error by touching a form off the EDT: the exact thing this class exists to prevent, reached through the other half of the same method. except() on its own stays synchronous. Reading the error out of an already-failed resource by registering a callback and looking at what it captured is an established idiom here and depends on that call being synchronous, and introspecting a failure is not the same act as handling one. Test settles a read into a failure, waits for it, then registers from a worker and asserts the error arrives on the EDT. It fails on the parent commit. Core suite 4740 green, SpotBugs 0, PMD gate clean. 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: 753e2ac959
ℹ️ 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 71 out of 72 changed files in this pull request and generated no new comments.
Suppressed comments (1)
quality-report.md:6
quality-report.mdappears to be a CI-generated artifact (the PR workflow runs.github/scripts/generate-quality-report.pyand then publishesquality-report.md). Committing a static copy into the repo can cause misleading PR comments/artifacts if the generation step fails or is skipped: the workflow’shashFiles('quality-report.md')condition will still be true and it may publish this checked-in (stale) report instead of the run’s actual results. Recommend removing this file from the repo (and optionally adding it to.gitignore) so the workflow output always reflects the current run.
…s under one lock An attestation cannot be produced twice, so the one whose caller cancelled is retained for the retry that asks for the same challenge. But retaining and delivering are not simultaneous: the callback retains the copy and queues the handover onto the EDT, so for the length of that hop the token is retained AND still reachable by the caller it was made for. A request arriving in that window took it, and both callers then submitted the same replay-protected attestation -- the second submission is rejected, and an app that reads a rejection as a bad key resets one the backend had just registered. The retained copy now remembers which resource it was produced for and is only handed on once that resource has lost its claim; a request arriving while the delivery is still undecided is told to retry rather than given a second copy. Losing the claim is now distinguishable from spending it on a delivery, which is what makes the question answerable. Separately, AppShield stored a status and enqueued its notification as two steps, so two network threads could interleave as: A stores, B stores, B enqueues, A enqueues. Listeners then finished on A while getStatus() already answered B -- a UI left saying "service down" for a shield that is fine, with nothing to correct it until the next transition. Both happen under the one lock now. The status test is staged rather than stressed: racing two threads at a window this narrow did not reproduce the old behaviour once in three runs, and a test that cannot fail on the unfixed code is not a regression test. Holding the listener monitor stops the first transition exactly where the enqueue happens, which makes "has the second one stored its status yet" a question with an answer. Core suite 4741 green, SpotBugs 0 on core-unittests and the iOS port, PMD gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 71 out of 72 changed files in this pull request and generated no new comments.
Suppressed comments (2)
CodenameOne/src/com/codename1/security/shield/spi/UnprotectedEngine.java:43
- The class-level comment says
fetchToken“never throws synchronously”, but this implementation always throws aShieldExceptionsynchronously. Even thoughAppShield.fetchToken()catches this and completes itsAsyncResourceasynchronously, the statement here is still incorrect/misleading for anyone reading the SPI contract.
quality-report.md:5 quality-report.mdis generated by CI (.github/scripts/generate-quality-report.py) and then uploaded/commented on in.github/workflows/pr.yml. Committing it into the repo makes the workflow’shashFiles('quality-report.md')condition always true and risks publishing stale/incorrect reports when a run fails before regenerating the file. This looks like a build artifact that should be removed from the PR and ignored in git.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f7957fe80
ℹ️ 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".
…nd stop calling a Mac jailbroken Three things, one of which was breaking CI. isJailbrokenDevice() used to be the cydia probe, which never answers on a modern iOS, and this branch rewired it to the real detector. On a Mac -- Catalyst, and the mac-native screenshot suite -- that detector fires: /bin/bash and /usr/sbin/sshd ship with macOS and /private is writable, so the two probes that describe an escaped iOS sandbox both hit on a stock machine. The smoke app asks the question at startup and refuses to launch, which is why the suite emitted nothing at all and timed out waiting for a completion marker. Those two probes are now iOS-only; the instrumentation ones on either side stay, because an injected dylib means the same thing wherever it is loaded. Verified by compiling the file for Catalyst and running it natively: jailbreakFile before, nothing after. A restart over the same transport instance could leave two readers on one stream. Both production transports clear their closed flag in open(), so a reader still parked in readMessage() from the previous generation is looking at a live stream again the moment the replacement opens, and can take the new client's first frame. The replacement now waits for the previous reader, and every read re-checks whether its server is still current before handling anything. The wait is bounded at two seconds, which the first version was not, and that cost me a three-hour hang: MCPTransport is a public interface and an implementation may park readMessage() until something other than close() releases it -- the loopback test transport is exactly that shape -- so waiting forever deadlocks the restart against a thread only the caller can end. stop() closes first, so a real socket unwinds in microseconds and the bound is never observed. And a superseded signal observation is no longer announced. Storing and enqueueing cannot be merged here, because callSerially runs inline before the EDT is up and notifying under the signal monitor would run application listeners while holding it -- so the dispatch checks on arrival whether it still describes what the bus holds. Listeners can no longer end on an observation snapshot() has already replaced. Core suite 4743 green, SpotBugs 0 on core-unittests and the iOS port, PMD gate clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The Java 25 docs gate refuses classic Javadoc markers in CodenameOne and CLDC11, and I wrote the new helper the old way. Same text, /// form. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
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.
Contributor
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)) {
Collaborator
Author
|
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>
Collaborator
Author
|
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>
Contributor
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>
Contributor
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.
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