Make the database portable and encryptable (#3848) - #5526
Conversation
The database API was five unrelated implementations sharing an interface. Cursors counted from zero on some ports and one on others, iOS reported success on an empty result set and returned null for every blob, the simulator could not seek at all, and no port could encrypt anything. This lands the port-independent half: - package-info.java now carries the normative contract every port must satisfy: zero-based positions, first() lands on a row, execute() runs a whole script while the parameterized forms take exactly one statement, typed parameter binding, flat transactions, IOException with a chained cause, idempotent close. - AbstractDBCursor derives all navigation from two primitives, rewind() and stepForward(), so every port gets identical semantics rather than each reimplementing them. Seeks rewind and re-step, which is what Android's windowed cursor already does on a window miss; buffering rows instead would mean materializing every column of every row stepped past. - SQLStatementSplitter splits a script the way SQLite does, respecting string literals, quoted identifiers, comments and CREATE TRIGGER bodies. - DatabaseConfig, DatabaseEncryptionException and ManagedKeys add keyed opens. Managed keys are resolved in the core so every platform derives identical material from an alias, and a key that cannot be stored is fatal rather than a silent downgrade to plaintext. - db.legacy restores each platform's previous behaviour for the ten changes that alter a previously successful result. It is read lazily, because the generated stubs set it after Display.init. Blob parameters now raise IOException rather than RuntimeException, and the truncated javadoc samples in Database, Cursor and Row are replaced with complete ones. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The simulator was the weakest database implementation, which mattered more than it sounds: it is where people develop. Its cursor could not seek at all, because the JDBC driver only produces TYPE_FORWARD_ONLY result sets and first(), last(), prev() and position() each threw outright. execute() silently ran the first statement of a script and discarded the rest. rollbackTransaction() left the connection outside autocommit, so every following statement quietly joined a new implicit transaction. Every query leaked its PreparedStatement. - SECursor now extends AbstractDBCursor, rewinding by re-executing the statement. The simulator has working random access for the first time. - execute(String) splits the script and runs each statement, rather than trusting a driver to decide how much of it to run. - The parameterized forms reject a multi-statement script instead of dropping its tail. - Statements are closed on the success path, cursors are closed with the database, close() is idempotent and rollback restores autocommit. - getColumnName reports the result set label, matching getColumnIndex, so an aliased column can be found under the name it was found by. The shaded driver moves from org.xerial to io.github.willena, which is the same driver with SQLite3MC compiled in: same package, same config, verified identical on plaintext databases, plus the SQLCipher-compatible cipher the simulator needs to open a database written on a device. getV4Defaults() is required over getDefault() - the latter selects SQLite3MC's own variant, which real SQLCipher cannot read. That driver also stops being frozen. Freezing assumed the shaded content never changed; it now carries a crypto-bearing engine that has to track upstream security releases. SEDatabaseConformanceTest runs the portable contract against the real SEDatabase headlessly in about two seconds, including both the strict and legacy modes and the encrypt/decrypt round trip. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
iOS was the port the "radically different implementations" complaint is
really about, and it had real bugs behind the divergence:
- sqlDbClose called sqlite3_free on the connection handle. That never
closed it, leaked the file descriptor, skipped the WAL checkpoint and
handed the pointer to the wrong allocator. Now sqlite3_close_v2.
- sqlCursorValueAtColumnBlob was { return nil; }, so iOS could not read
a blob at all, in either direction.
- Opening a database called sqlite3_config(SQLITE_CONFIG_SERIALIZED) and,
on failure, sqlite3_shutdown(). That has to run before
sqlite3_initialize() to do anything, and calling shutdown with
connections open is undefined behaviour. Replaced with per-connection
SQLITE_OPEN_FULLMUTEX.
Behaviour now matches the portable contract:
- CursorImpl extends AbstractDBCursor, so last(), prev() and position()
work instead of throwing "Unsupported", and first() lands on a row and
reports false for an empty result set rather than reporting success and
leaving the statement unpositioned.
- Parameters bind by runtime type through new statement natives. They
used to be stringified, which stored an Integer as TEXT, and a comment
conceded it "will probably fail with blobs".
- Parameter count mismatches and multi-statement scripts in the
parameterized forms are rejected rather than silently mis-executed.
- Errors carry sqlite3_errmsg unconditionally; the dead XMLVM branches
that gated error reporting are gone.
- finalize() is removed from the database and cursor. Closing sqlite
handles from the GC thread is the "platform specific nuance" that
defeated ThreadSafeDatabase.
- Custom file:// database paths work, matching Android and the simulator.
Keying is a separate native that reports success rather than throwing, so
the Java side can tell a wrong key from a failure to open the file
without the native layer naming a core exception class.
isDatabaseEncryptionSupported() asks the linked engine via PRAGMA
cipher_version rather than assuming, so it reports honestly on a build
that does not bundle a cipher-capable SQLite.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Android was already the most capable port, so this is mostly tightening rather than rebuilding: - A null element in a String[] now binds SQL NULL. bindString rejects null, so passing one used to fail the whole statement. - execute(sql, (Object[]) null) no longer dereferences a null array. - execute(String) runs a whole script. execSQL refuses anything after the first statement, so the script is split and run statement by statement. - executeQuery forces the window fill before returning, so malformed SQL is reported there rather than from the first next(). rawQuery is lazy. - Transactions use the shared flat-transaction guards, so a nested begin is rejected here as it already was everywhere else. - Exceptions carry their cause and are no longer printStackTrace'd on the way out. - Cursors are invalidated when the database closes, close() is idempotent, getRow() off a row throws, getColumnIndex is case insensitive, and wasNull() is false before any value has been read. - Blob query parameters work, bound through a cursor factory, which is the only supported route: rawQuery can carry text arguments only. This is what androidx.sqlite does for the same reason. Encryption lives in a new com/codename1/impl/android/cipher package built on net.zetetic:sqlcipher-android. It compiles against classes that are only on the classpath of app builds that use encryption, so it is excluded from the port's own javac and reached purely by reflection, letting the builder delete it for every app that never touches DatabaseConfig. That gating is why the package is a near copy of AndroidDB rather than a shared supertype: any shared type naming net.zetetic would have to live in the part of the port that must stay deletable. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both ports inherited the base openOrCreateDB, which returns null, so Database.openOrCreate() handed back null and calling code failed with a NullPointerException. They now have a full implementation that satisfies the same contract as every other port, encryption included. Neither runs a JVM, so JDBC was never an option; they needed a C binding. That is cheap because both are ParparVM C targets whose CMake project already compiles every .c in the source root. - The engine is SQLite3 Multiple Ciphers, bundled once in the translator and emitted only for applications that use com.codename1.db. iOS shares the same copy, so those three targets run one engine at one version, and the simulator's JDBC driver is built from the same upstream project. - The amalgamation is named .h deliberately. The iOS project generator lists .h but excludes it from the compile phase; CMake globs *.c for sources; and the ParparVM native symbol scanner reads only .c and .m. Named .c it would be compiled twice without its build options, named .inc it would ship inside the .ipa as 13MB of dead weight. - cn1_sqlite3.c is the single translation unit that compiles it, with the build options set immediately before the include so they cannot leak into unrelated sources. It is gated internally, so an emitted but disabled build produces an empty object rather than a link error. - The binding itself is shared. Both ports need identical code but mangle their entry points from different Java classes, so the logic lives once in cn1_db_sqlite_impl.h and each port's .c expands CN1_DB_DEFINE_NATIVES for its own prefix. Verified that every declared native has both its plain and its _R_ symbol in both ports. - iOS stops linking the system libsqlite3 when the bundled engine is used, rather than carrying two SQLite implementations in one process. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The JavaScript port sat on WebSQL, which Chrome removed in 119 and Firefox never implemented, so its database was dead on every current browser. What it did support was thin: transactions were printlns, getBlob threw, position(n) always returned the first row, close() did nothing, and the bridge busy-waited a CN1 thread on a lock. It now runs the same SQLite build the other ports use, compiled to WebAssembly, inside the application's own worker. Every call after the first is an ordinary synchronous call; only the initial load suspends, through the runtime's existing yield-on-promise support, so the lock and its 200ms poll are gone. Storage uses the opfs-sahpool VFS rather than the default OPFS one. The default needs crossOriginIsolated, which needs COOP/COEP response headers, which we cannot require of the arbitrary static hosting these bundles are deployed to. Browsers without synchronous OPFS access fall back to memory with a console warning, because silently losing every write on reload is not a failure anyone should discover in production. Gating, so nobody pays for what they do not use: - iOS emits the bundled engine, and drops the system libsqlite3, only for applications that reference DatabaseConfig. Everyone else keeps the system SQLite exactly as before. - Windows and Linux emit it for anything referencing com.codename1.db, since they have no system SQLite at all, and its cipher only when encryption is configured. - Android's SQLCipher package is deleted unless DatabaseConfig is referenced, and the AAR arrives through a new PlatformFeatureCatalog entry keyed on that same class. - The JavaScript builder prunes the 1.5MB engine from bundles that never open a database. The catalog entry is keyed on DatabaseConfig rather than the db package on purpose, and two new tests hold that line: every database application references com.codename1.db, so keying it there would bundle SQLCipher for all of them and push the minimum Android SDK from 19 to 23 for people who never asked for encryption. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The contract and the encryption are only real if they are checked, and the portability claim in particular is the kind that fails silently: a cipher misconfiguration produces files each platform reads perfectly well on its own and nothing else can touch. - Seven device tests run the shared conformance suite on every port through the existing screenshot harness. They are assertion only, so they take no screenshots and sit before the ordering-sensitive graphics baselines. Ports without a database self-skip, so a port turns green on its own once it has one. - Two of the seven run in legacy mode, which is what makes the compatibility promise testable rather than aspirational: they fail the moment a refactor changes what db.legacy restores. - Two Port Status features expose the results publicly, split so a threading regression cannot blank the whole database row. - scripts/ci/db-cipher-interop.sh checks our encrypted files against the stock sqlcipher client in both directions, with a raw key to isolate the cipher configuration and a passphrase leg to cover the key derivation. Wired into the pull request workflow. The developer guide's SQL section said the iOS SQLite "isn't threadsafe" and warned that the garbage collector closing a connection would crash the app. That was true, and this branch is what fixes it, so the section is rewritten and extended with encryption, key management, threading, cursor cost and the legacy compatibility table. ThreadSafeDatabase is un-deprecated. Its note blamed platform nuances; the nuance was the iOS finalizers, now gone. Its close() was fire and forget, so it returned before the database was closed and a following delete() raced it, which is fixed here too. The cursor inner classes are static: with an explicit owner field the implicit outer reference was dead weight, which SpotBugs flagged on iOS and would eventually have flagged everywhere. 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: ce77b834d3
ℹ️ 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".
|
Companion PR with the build-side gating: codenameone/BuildDaemon#172 |
✅ Continuous Quality ReportTest & Coverage
Static Analysis
Generated automatically by the PR CI workflow. |
✅ ByteCodeTranslator Quality ReportTest & Coverage
Benchmark Results
Static Analysis
Generated automatically by the PR CI workflow. |
- The Ant build for the JavaSE port links whichever sqlite-jdbc is pinned in cn1-binaries, which has no org.sqlite.mc, so importing the driver's config builder broke that build for everyone. JavaSEPort now writes the SQLCipher connection properties out literally, which needs no extra class at compile time, and reports isDatabaseEncryptionSupported() by probing for the cipher-capable driver rather than assuming it. The simulator therefore answers honestly under either build. - The Windows cross-compile failed to link. The sample application now uses com.codename1.db, but that integration test drives the translator directly rather than through the builder, so the engine was never emitted and the natives had no definitions. Two fixes: the shared binding header is always emitted and defines every entry point either way, as real bindings or as stubs that raise a clear IOException, so an application always links however the translator was invoked; and the integration tests ask for the engine explicitly, so those ports actually exercise the database instead of only ever self-skipping. Verified that both branches of the header export an identical symbol set. - The developer guide requires snippets to live in docs/demos and be included by tag. Migrated with the repository's own migration script. The snippet harness had no com.codename1.db import, which is why all three failed to compile once moved; added, since it is a core package the guide documents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Cloudflare Preview
|
The Maven build already excluded it, but the Ant target compiles every source in the port, so it tried to build the package against net.zetetic and failed for anyone building that way -- including BuildDaemon CI, which clones this repo and runs the Ant target. Mirrors the exclusion into both places the ARCore and AI packages already use: the javac in Ports/Android/build.xml and the excludes property in nbproject/project.properties. 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: d595bd94da
ℹ️ 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".
|
Compared 12 screenshots: 12 matched. |
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
|
Compared 149 screenshots: 149 matched. Benchmark ResultsDetailed Performance Metrics
|
Review findings, all eight real: - Database.encrypt() could never work on Android. The system SQLite has no cipher, so a plaintext database opened through it can never be re-keyed. Added openOrCreateDBForRekey(), which Android routes through SQLCipher (an empty key opens an unencrypted file, which can then be re-keyed). - A managed key resolves its keystore alias from the database name, and every port passed null when re-keying, so changeKey(managed()) raised a NullPointerException instead of encrypting. Each Database now retains the name it was opened under. - Two threads first-opening the same managed database could each see nothing stored, generate different keys and overwrite each other, leaving one of them holding data nobody could ever read. The read-generate-store sequence is now serialized. - isKeyHardwareBacked() inferred hardware backing from the API level, but emulators and plenty of real devices back AndroidKeyStore keys in software. It now asks the key itself, via KeyInfo. Applications are told they may use this to refuse to store sensitive data, so it has to be true. - checkEndTransaction() cleared the flag before the engine had ended the transaction, so a failed commit left the transaction open while the API believed it was closed, and the recovering rollback was rejected. Splitting out markTransactionEnded() means the flag drops only on success. A conformance check covers the failed-commit path. - An encrypted Android database opened by file:// URL had no toNativePath() conversion, so java.io.File treated the URL as a literal relative name. - Calling next() past the end repeatedly re-derived the row count each time, inflating it, after which last() would seek to a row that does not exist. Verified the new check fails against the old code (5 became 8). - PRAGMA rekey interpolated the key directly, so a passphrase containing a quote produced a different statement. Both Android and the simulator now go through one helper that quotes text and passes a raw key literal through untouched. CI failures: - Six SpotBugs findings in core-unittests, a module the earlier local runs had not covered: boxed constructors, a default-encoding String, and a Boolean-returning method that could return null. - The arm64 Linux and Windows cross-builds failed compiling the engine's ARM AES intrinsics. Where the compiler defines __ARM_FEATURE_CRYPTO the engine uses them directly, which is what Apple's toolchain does, so iOS is unaffected; otherwise it tags individual functions with __attribute__((target)), which the cross-compiling clang does not honour for these intrinsics. Rather than require ARM crypto extensions of every chip, that path now uses the software implementation. - DatabaseStatementLegacyTest failed on Android because the legacy expectation was wrong, not the code: only iOS ran a whole script before this branch, through sqlite3_exec. Android's execSQL and the simulator's PreparedStatement both dropped everything after the first statement. Corrected in the suite and in both places it is documented. - The migrated guide snippet fixture needed a copyright header. 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: 7f2f2c70ff
ℹ️ 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".
|
Compared 217 screenshots: 217 matched. |
Removing the database first and then failing on a companion reported a failure over a database that was already gone. The caller is told to retry something that cannot be retried, and reasonably reads the error as its data being intact -- which is the worst of the three possible outcomes, because it is the one that looks recoverable. Every port that deletes the set now takes the companions first and the database last, so a failure before that leaves a database the caller really can delete again. It also keeps the invariant the next open depends on: a journal that outlives its database is read as a hot one against whatever is created under that name next, which is the whole reason these are deleted together. The rule lives on databaseSidecarPaths, beside the list itself. Android is unchanged: it deletes through the platform, which takes the set as one operation and is then verified by requireDatabaseGone. JavaSEPortDatabaseDeleteOrderTest holds it, with a companion that cannot be removed -- a non-empty directory under the journal's name, which is the portable way to make File.delete() fail -- and asserts the database is still there afterwards. It fails against the previous order. 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: 1b0f3b0a18
ℹ️ 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".
Two ways one application's managed database key stopped being its own. On iOS the keychain service was the AppName property, which IPhoneBuilder sets from the build's display name. Renaming an application moved its keychain service, its key became unreachable, and ManagedKeys -- seeing nothing under the new service -- generated a replacement over a database encrypted with the old one. The service is now the bundle identifier, which the store will not let change. Entries an earlier build wrote are still read from the old service and moved across as they are read, copy before delete, because for a managed key that entry is the only copy in existence; set and remove cover both names so the two cannot diverge. On Windows and Linux the account name carried no application component at all. Storage there is one directory under the user account -- the app home is keyed by package precisely because this platform has no sandbox -- so two applications asking for the same alias derived the same account, read each other's key, and forgetting it in either one removed the other's only copy. The storage name now carries the package, with the same read-and-adopt path for entries written before it did. The namespace is derived once, in SecureStorage, because both desktop ports need the same answer and a future one will too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The third job on this branch to die before compiling anything, this time with "authorization failed for https://repo.maven.apache.org/maven2" against the JUnit BOM and the publishing plugin. It runs tests, so it goes through scripts/ci/retry.sh with RETRY_ONLY_MATCHING narrowed to resolution failures: an intermittent product failure still has to fail on the first attempt. The pattern the three wrapped steps share gains that phrasing, spelled with the Central host in it rather than as a bare "authorization failed" -- a purchase test that reported those two words would otherwise be retried until it passed, which is the laundering this whole mechanism exists to prevent. Checked both ways against the real message and against a test failure that merely contains the phrase. 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: 81aee050d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The simulator runs every project on one machine under one OS user and kept every account in one fixed Preferences node, so two projects that both asked for a managed key under the same alias read each other's key -- and forgetting it in either one removed the only copy either had. The device ports are separated by an OS sandbox; here the separation has to be written down. Entries now live in a per-application node under the shared one. The salt stays where it was, deliberately: it is what the key is derived from, so a value written before this split still decrypts afterwards and can simply be moved down a level the first time it is read. An entry in the shared node still counts as present, so nothing generates a second key over a database the first one encrypted. The namespace is resolved per call rather than at construction, because this object is built while the port is still coming up and an answer cached from that moment would name every project the same thing. It takes the launcher's main class, which is where the simulator's own package_name comes from once Display is running -- so SecureStorage.applicationNamespace now accepts an identity a port already knows, and asks Display only when it does not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
javase-single-component-inspector failed on a commit that touches secure storage, which cannot reach the inspector. The images say what really happened: the stored reference has the Component Details panel empty, and this run captured it with the form laid out -- Class, Name, UIID and the rest, values blank. The panel is built after the window it sits in, and the capture waits a fixed eight seconds, so which side of that the screenshot lands on is a matter of how quick the runner was. The suite had won that race all day and lost it here. So the capture now waits for the form, through the same poll that already waits for the device to paint, and the reference is the state it waits for. Read from the pixels because this verifier drives the simulator from another process: the populated form draws about 740 dark pixels in that band and the empty panel draws none, so the threshold sits between them at 200. If the deadline passes the assertion says the form never appeared, rather than storing a half-built panel as if it were the answer. 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: 5d73fd15da
ℹ️ 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".
| System.out.println( | ||
| "CN1SS:INFO:test=GoogleWebMap status=FAILED reason=tiles-never-loaded"); | ||
| "CN1SS:INFO:test=GoogleWebMap status=SKIPPED reason=map-tiles-never-loaded"); |
There was a problem hiding this comment.
When a WebMap provider regression, invalid API key, broken JavaScript bridge, or WebView failure prevents isMapReady() from becoming true, this path now marks the test skipped and calls done(), making the same product failure indistinguishable from a transient network outage. The test already retries with a fresh warmed-up WebView, and its preceding contract explicitly says the second exhaustion should fail loudly, so keep that exhaustion as a failure rather than allowing map initialization regressions to pass CI.
Useful? React with 👍 / 👎.
| | `DatabaseConfig` | ||
| | The cipher as well. On Android that's `net.zetetic:sqlcipher-android` and `androidx.sqlite`; on | ||
| iOS a bundled SQLCipher build that replaces the system `libsqlite3`. |
There was a problem hiding this comment.
For an application that only calls DatabaseConfig.plain(), this table says the cipher is bundled and the following paragraph says Android's minimum SDK is raised to 23, but Executor.DatabaseUsageVisitor sets the cipher flag only for calls to passphrase, rawKey, or managed and deliberately excludes plain. Update the guide to describe encrypting factory references rather than any DatabaseConfig reference; otherwise users may avoid the plaintext config API or incorrectly conclude that it drops pre-23 Android support.
Useful? React with 👍 / 👎.
The copyright gate is diff scoped, so this file had never been in its scope until the previous commit edited 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: 0334a8dc1a
ℹ️ 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".
| // generated gradle links them through a fileTree, so encryption used only inside a | ||
| // library is invisible to a scan of the loose class tree -- and the build would then | ||
| // delete the cipher implementation out from under the library that calls it. | ||
| DatabaseUsage libraryUsage = scanForDatabaseUsage(libsDir); |
There was a problem hiding this comment.
When an AAR/JAR supplied through android.gradleDep or gradleDependencies calls DatabaseConfig.managed(), passphrase(), or rawKey() internally while the application only invokes a library API, this scan never sees that bytecode: it inspects only dummyClassesDir and libsDir, whereas those dependencies are added to build.gradle later (around lines 6423-6426). dbCipherSupport therefore remains false, and the builder deletes com/codename1/impl/android/cipher at lines 3583-3595, so the library's encrypted database open reports NOT_SUPPORTED at runtime. Either inspect resolved Gradle dependencies before pruning or provide a build hint that forces cipher inclusion.
Useful? React with 👍 / 👎.
…passes through My previous commit had this backwards, and the run said so: the assertion fired with textPixels=0 after the full thirty second poll, so the form I was waiting for was not a state that arrives -- it is one the panel passes through. The inspector is created and then moved into its own window by showInFrame(), and the docked Component Details panel it leaves behind settles empty. That is what the stored reference holds and what every run captures once it has settled; the failure was a capture taken mid-move, holding the form laid out with no values in it. So the capture now waits for the panel to be empty and the reference goes back to the settled state it always had. The fixed eight second timer is still what made this possible either way, which is why the wait stays: an assertion that names the unsettled panel is a better failure than a screenshot mismatch, whichever way the race goes next. 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: 3c41c32d06
ℹ️ 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".
| // process kill there strands it, the next open creates an empty database in its place, | ||
| // and the migration after that removes the stranded copy as stale leftovers. Renaming | ||
| // means there is a complete database under one of the two names at every instant. | ||
| if (!original.renameTo(backup)) { |
There was a problem hiding this comment.
When the Android source database uses journal_mode=PERSIST and has updated or deleted data, closing it intentionally leaves path-journal containing old plaintext pages. This swap renames only the main file to backup, installs the encrypted target at the original path, and later deletes only backup, so encrypt() can report success while recoverable plaintext remains beside the encrypted database. Remove and verify the source sidecars after the clean close and before installing the target.
Useful? React with 👍 / 👎.
| if (slot >= 1 && slot <= params.length && params[slot - 1] instanceof String) { | ||
| reserveAttachmentParameter((String) params[slot - 1]); | ||
| return; |
There was a problem hiding this comment.
When the object overload binds an integer, blob, or other non-string value as the ATTACH filename, SQLite coerces it to text and opens that file—for example, ATTACH ? AS aux with Integer.valueOf(123) opens a file named 123. This branch skips the actual target and the fallback loop ignores it too, so the pre-execution reservation and relative-path rejection are bypassed; a concurrent delete or rekey can therefore claim the attached file before reconciliation. Reject non-string filename values or convert and reserve them using the same representation SQLite uses.
Useful? React with 👍 / 👎.
| // Resolved and then normalized, so two spellings of one path are one registry entry: the | ||
| // claim a key change takes is worth nothing if the other connection is filed under | ||
| // "/a/./b" -- or under a link the engine sees through. | ||
| this.openKey = registryKeyFor(path); |
There was a problem hiding this comment.
When the same Linux database is opened through two hard-link names, getCanonicalPath() preserves the distinct directory entries, so the two handles receive different openKey values even though they share one inode. Each then appears to be the sole connection and may run changeKey(), rekeying the shared file underneath the other handle and leaving it using the old key. Use device/inode identity for the live connection and rekey registry, while retaining the path-derived identity for persistent managed-key aliases.
Useful? React with 👍 / 👎.
The wrapper did engage on protocol-e2e -- three attempts, thirty seconds apart, each rejected with "authorization failed for https://repo.maven.apache.org" for the JUnit BOM and the publishing plugin, before anything compiled. That is about a minute of coverage, and Central refused two different jobs on this branch today, so the window was simply shorter than the incident. Five attempts a minute apart on the three steps that carry the resolution-only pattern. It costs nothing on a real failure, which RETRY_ONLY_MATCHING still returns on the first attempt, and nothing on a genuinely unavailable dependency, which fails the same way five times. The refusal itself is not something this repository can fix: there is no settings.xml here, and the downloads are rejected within a third of a second, so whatever credentials reach Central come from the runner image. 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: ad096aa8f2
ℹ️ 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".
| // container-relative one -- the open-database guard stopped firing and a database was | ||
| // unlinked underneath a live connection. Two spellings of one database still meet here, | ||
| // because the path is resolved before the container is taken off it. | ||
| this.openKey = aliasKey; |
There was a problem hiding this comment.
When the same iOS database is opened through two hard-link paths, managedKeyAliasForPath() uses realpath(), which resolves symlinks but preserves distinct hard-link names, so the two handles receive different openKey values even though they share one inode. Each handle can consequently appear to be the sole connection and run changeKey(), re-keying the file underneath the other handle and causing its later reads or writes to fail. Keep the persistent managed-key alias path-based, but use device/inode identity for the live connection and re-key registry.
Useful? React with 👍 / 👎.
It already retried, with the default three attempts at thirty seconds, and was refused on all three with "authorization failed for https://repo.maven.apache.org" before anything was built -- the fourth job Central has turned away on this branch today. Bounded on the step rather than by raising the default in retry.sh: half the call sites in these workflows have no RETRY_ONLY_MATCHING, and some of those run tests, so a larger default would widen exactly the blanket retrying the pattern exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fifth job in a day to die before touching our code, this time on "Plugin maven-install-plugin:2.5.2 or one of its dependencies could not be resolved". Same wrapper and the same resolution-only pattern as the others, so a failure in what this actually builds still fails on the first attempt. 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: f33c50e9d8
ℹ️ 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".
…tinct Two ways one application's managed key could still be lost. synchronized covers threads in one VM and nothing between two, and an application can run in more than one process -- Android components declared with their own android:process, or two runs of a desktop build. Both could find nothing stored, generate different keys and each overwrite the other, leaving the database encrypted with a key that no longer exists. SecureStorage.setIfAbsent is the operation that was missing: it stores only when there is nothing there and answers with what the store ended up holding, so a caller that lost the race takes the winner's key instead of overwriting it, and both open the database with the same one. iOS implements it through SecItemAdd, which refuses a duplicate inside the keychain daemon and so is atomic between processes; the default is the best a store without that can do, and says so -- the check and the write are still two steps. The namespace sanitizer folded every character it could not carry onto "_", which is not reversible: com.acme.foo$bar and com.acme.foo_bar became one namespace, as did "My App" and "My_App", so two applications shared the store the namespace exists to keep apart. It now escapes those characters the way ManagedKeys.accountName escapes the account half of the same name, the escape character included. The keychain add is verified by building: the port jar rebuilt, the project regenerated from it, the symbol present in the staged IOSNative.m and xcodebuild reporting BUILD SUCCEEDED. Its second symbol also corrected -- I had spelled it _R_int_R_int, which resolves to nothing; the alias this file uses carries no return suffix, and the same mistake in secureStorageEntryStatePlain is fixed with 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: 6cf81644f8
ℹ️ 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".
…cryptable-database # Conflicts: # docs/developer-guide/languagetool-accept.txt
Reading the store, generating a key and writing it back is three operations, and the review is right that reading again afterwards closes nothing: two processes can each complete all four steps and each believe its own key is the one stored. Only iOS was safe, because SecItemAdd refuses a duplicate inside the keychain daemon. The other four now have a gate of their own. Windows and Linux gained fileCreateExclusive, which is CREATE_NEW and O_CREAT|O_EXCL -- decided by the filesystem, so it cannot be won twice. The caller that creates the gate stores the value; the caller that finds it waits briefly for that value to land and never writes. The simulator takes a file lock, which is the mutual exclusion the JVM offers between processes rather than between threads, and re-reads Preferences inside it: that store caches per process, so without the sync the check would consult a copy taken before the other process wrote. Android creates the gate with createNewFile(), which is atomic, but its store is SharedPreferences and there is no way to make one process see another's write -- the cache is per process and MODE_MULTI_PROCESS was withdrawn for being unreliable. So the caller that loses reports nothing rather than writing, which turns a permanent silent corruption into a transient failure: KEY_UNAVAILABLE now, and the next launch reads the file fresh and finds the key. Also merges master, whose only conflict was two additions to the LanguageTool accept list, both kept. 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: a3d927401a
ℹ️ 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".
| java.io.File dir = new java.io.File(AndroidNativeUtil.getActivity() | ||
| .getApplicationContext().getFilesDir(), "cn1securestorage"); |
There was a problem hiding this comment.
When a managed database is first opened concurrently by components in separate android:process services before any Activity exists, getActivity() returns null, causing gateFile() to return null and setIfAbsent() to fall back to the non-atomic superclass implementation. Both processes can then generate and persist different keys through their independently cached SharedPreferences, potentially leaving the database encrypted with the key that lost the race. Build the gate path from AndroidNativeUtil.getContext().getApplicationContext() instead, as the storage code below already does for background-service support.
Useful? React with 👍 / 👎.
| } | ||
| boolean created; | ||
| try { | ||
| created = gate.createNewFile(); |
There was a problem hiding this comment.
After the first successful setIfAbsent(), this gate file remains permanently. If Database.forgetManagedKey() later removes the corresponding preference, reopening a newly created managed database under that alias finds no value but cannot recreate the gate; the false createNewFile() result leads to a null return and KEY_UNAVAILABLE on every subsequent attempt until app data is manually cleared. Successful removal of an account must also retire its gate, with synchronization that preserves the cross-process exclusion.
Useful? React with 👍 / 👎.
Three jobs on this branch timed out overnight and none of them reached any of our code: vm-tests spent its ninety minutes in "Install native build tools", and the Windows cross-compile and the website build each ran to GitHub's six hour ceiling in their own apt steps. All three had gone through scripts/ci/apt-get-update.sh a moment earlier, at 03:01 to 03:05 UTC, with the azure mirror answering Ign: on every index. Two things were missing. apt had no timeout, so a mirror that accepts the connection and then stalls is waited on forever -- and Acquire::Retries never comes into play, because nothing ever fails. And the settings were passed as options to apt-get update, so the apt-get install that follows in every caller inherited none of them. Both are fixed in one place: the script now drops the timeouts, retries and IPv4 preference into /etc/apt/apt.conf.d, which every later apt call in the job picks up, and runs the update itself under a five minute ceiling with three attempts. The two jobs that ran for six hours also had no timeout-minutes of their own, which is why a hang cost that much; they now have one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
These three do not go through scripts/ci/apt-get-update.sh -- they run as root in the CI container, without sudo -- so the timeouts that script installs never reach them. A stalled mirror there is still a hang rather than a failure, which is what cost three jobs their whole run overnight. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
It arrived with the master merge without one, and the copyright gate is diff scoped: merging master pulled the file into this pull request's scope, where it failed. Nothing else about the file changes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…test that never ran I added the seven database tests to the manifest and wrote "not-run" beside them in all eleven port reports, which is a statement that they were published and never executed. They do run: every port on this branch reports all seven passing, so the reports are replaced with the real thing -- run 32242868198 and its siblings on this head, not-run 0 across android, both iOS renderers, both Linux architectures, JavaScript, mac-native, tvOS, watchOS and both Windows architectures. The reason a hand-written absence survived is that nothing objected to it. A registered test sitting at "not-run" renders on the page exactly like one that runs and passes, so the contract now rejects it: a port that genuinely cannot do something reports "skip" from the suite itself, which is evidence, while "not-run" is the absence of evidence and the answer to it is to run the suite and check the report in. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Resolves #3848.
The request was database encryption. Encryption is here, but the reason it took a
whole PR is that
com.codename1.dbwas not one API over SQLite -- it was fiveunrelated implementations that happened to share an interface, and there was no
sensible place to add a key to.
What was actually wrong
Verified in the source, not from memory:
openOrCreatenull, callers NPElast()/prev()/position()IOException("Unsupported")position(n)always gave row 0getPosition()basefirst()trueon an empty set, then reads unset memorygetBlob{ return nil; }execute(sql)multi-statementBEGINprintlnno-opsRuntimeExceptionon every portPlus three defects worth calling out on their own:
sqlDbClosecalledsqlite3_freeon asqlite3*, so no iOS connection was ever closed, the WAL wasnever checkpointed and the handle went to the wrong allocator;
SEDatabaseleakeda
PreparedStatementper query; andThreadSafeDatabase.close()was fire andforget, so a following
delete()raced it.And no device test touched
Databaseat all -- 142 test classes in the screenshotsuite, none of them about databases. That is why Windows and Linux were allowed to
ship with no implementation.
What this does
One contract.
com.codename1.db/package-info.javanow states what every portmust do, and
DatabaseConformanceSuitein the framework checks it. Seven devicetests run that suite on every port in CI; two of them run in legacy mode.
One cursor implementation.
AbstractDBCursorderives all navigation from twoprimitives,
rewind()andstepForward(), so ports stop re-deriving it. Seeksrewind and re-step rather than buffering:
sqlite3_column_*is only valid on thecurrent row, so buffering would mean copying every column of every row stepped
past, blobs included. This is what Android's windowed cursor already does on a
window miss.
Encryption, with a passphrase, a keystore-managed random key, or raw bytes.
Managed keys resolve in the core so every platform derives identical material from
an alias, and a key that cannot be stored is fatal rather than a silent downgrade
to plaintext.
Windows and Linux get a database at all.
JavaScript stops using WebSQL, which Chrome removed in 119 and Firefox never
implemented, in favour of the same SQLite compiled to WebAssembly.
Compatibility
Ten behaviours change in ways an application could depend on. All ten are restored
by the
db.legacybuild hint, per platform, and two device tests assert that itreally does restore them -- so the promise is testable rather than aspirational.
The table is in the developer guide.
The hint deliberately does not cover defects, or capabilities that used to throw
and now work. Nobody can depend on
getBlobreturning null.Cost, when unused
Nothing. iOS keeps the system SQLite unless the app references
DatabaseConfig;Android's SQLCipher package is deleted and its AAR never added; Windows and Linux
compile the engine to an empty object; the JavaScript builder prunes 1.5MB from
bundles that never open a database. Two catalog tests hold that line, because the
entry is keyed on
DatabaseConfigrather than the package -- keying it on thepackage would bundle SQLCipher for every database app and push Android's minimum
SDK from 19 to 23 for people who never asked for encryption.
Verification
SEDatabaseConformanceTestcases, all green.android,ios,codenameone-maven-pluginandByteCodeTranslator.scripts/ci/db-cipher-interop.sh, wired into PR CI, writes an encrypted databasewith our engine and reads it with the stock
sqlcipherclient, and vice versa,with both a raw key and a passphrase. This is the check that matters: a cipher
misconfiguration produces files each platform reads happily and nothing else can
touch, which no single-platform test would catch.
sqlcipher4.17.0 client and the realnet.zetetic:sqlcipher-androidAAR, not against assumed APIs.Three things the spikes caught
Worth recording, because each would have shipped broken:
sqlcipher_export()does not exist in SQLite3MC, so the ATTACH-basedmigration everyone writes would have failed.
PRAGMA rekeyworks, and alsopreserves
user_version, whichsqlcipher_exportdrops.getConnection()on the simulator but on first read onthe device ports, so both paths need handling.
SQLiteMCSqlCipherConfig.getDefault()really does produce files real SQLCiphercannot open;
getV4Defaults()is required. One line, and nothing but across-engine test would have found it.
Review rounds
Nineteen findings from the automated reviewers, all real, all fixed. The ones worth knowing about:
Database.encrypt()could never have worked on Android. The system SQLite has no cipher, so aplaintext database opened through it can never be re-keyed; there is now a platform hook that
routes the migration through SQLCipher.
nullwhen re-keying, so
changeKey(managed())raised aNullPointerExceptionrather than encrypting./,\,:and space all to_, socustomer/dbandcustomer_dbshared one key and forgetting either destroyed the other.
and
sqlite3_close_v2then leaves a zombie connection alive forever.isEncrypted()reported every plaintext JavaScript database as encrypted, because that port hasno readable path and a failed header read is indistinguishable from ciphertext.
PRAGMA rekeyinterpolated the key directly, so a passphrase containing a quote changed thestatement.
Two of the fixes are covered by new conformance checks, including one verified by reinstating the
old code and watching it fail: the exhausted-cursor count went 5 to 8 before the fix.
Two decisions worth a second opinion
maven/sqlite-jdbcis no longer frozen. It was pinned and excluded frompublication because a shade of a fixed driver never changed. It now carries the
engine used to read encrypted databases, so it has to track upstream security
releases. Costs ~13.5MB per release, which is what the freeze was avoiding.
compile. It ships a prebuilt amalgamation where SQLCipher would need its
configure script run per build, and it is what the simulator's JDBC driver is
already built from -- so iOS, Windows, Linux, JavaScript and the simulator all
run one engine at one version. Android still uses the SQLCipher AAR because it
cannot compile C in our build; both write the same format, which is the part
that matters.
Companion PR
The build-side gating is mirrored in codenameone/BuildDaemon#172, which is green.
🤖 Generated with Claude Code