Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds SecureKeystoreV2 interface and SigningAlgorithm enum; SecureKeystoreImpl now implements V2 with new storeKeyPair/retrieveKeyPair APIs and deprecated wrappers for legacy storeGenericKey/retrieveGenericKey. README rewritten to Kotlin/Android-centric API docs. No runtime behavior changes beyond API surface and delegation. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Client/App
participant Keystore as SecureKeystoreV2
participant Storage as KeyStore/Storage
participant Biometric as BiometricAuth
rect rgba(200,200,255,0.5)
App->>Keystore: storeKeyPair(public, private, alias)
Keystore->>Storage: persist(publicBlob, privateBlob, metadata)
Storage-->>Keystore: ack
Keystore-->>App: success
end
rect rgba(200,255,200,0.5)
App->>Keystore: retrieveKeyPair(alias, context, isBiometricRequired)
alt isBiometricRequired
Keystore->>Biometric: authenticate(context)
Biometric-->>Keystore: success/failure
Keystore->>Storage: load(privateBlob) // unlocked
else no biometric
Keystore->>Storage: load(publicBlob, privateBlob)
end
Storage-->>Keystore: keyData
Keystore-->>App: [public, private]
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
README.md (1)
503-507: Clarify the structure of the returned array.The return type documentation states "Array of keys associated with the account" but doesn't specify:
- Does the array contain both the public and private keys?
- If so, what is the order (e.g.,
[publicKey, privateKey])?- Can multiple key pairs be stored per account?
Please add details about the array structure to help users properly handle the returned data.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 503 - 507, Update the "Returns" section that currently lists "Array<String>" to explicitly describe the array structure: state whether each entry is a single key or a pair, the element order (e.g., [publicKey, privateKey] or [{"public":"...", "private":"..."}]), and whether multiple key pairs may be returned (e.g., one pair vs. an array of pairs), and include examples of the expected shape so consumers know how to access public vs. private keys.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 534-538: Update the Returns section to explicitly describe which
key material is returned: state that for asymmetric key pairs (RSA, EC) the
function returns the public key only (the private key is non-exportable as noted
by "private keys remain non-exportable"); for symmetric keys (AES) and HMAC keys
indicate that key material is not retrievable (not applicable) and the function
will either return null/empty or throw an error as per implementation; mention
the return type remains String (e.g., PEM/Base64 public key) and add a short
note referencing the "private keys remain non-exportable" behavior for clarity.
- Line 181: Decide on a single expected input format (preferably plain UTF-8
text) and make the README consistent: update the summary sentence for the
encrypt function to "Encrypts the given data (plain text) using the key assigned
to the alias.", update the parameter description for the data parameter to state
"Plain text data to encrypt (UTF-8 string). The library will Base64-encode as
needed internally.", and update the example at the usage section to pass data =
"Sensitive information" (plain text) so all three occurrences referring to the
data parameter (the function summary, the parameter description, and the
example) are aligned; reference the parameter name data and the encrypt function
in your edits.
- Line 336: The README's sign function docs are inconsistent about the data
parameter format: update the description of the `data` parameter in the
parameters table and the example usage of the `sign` function so they match and
clearly state the accepted input format (either "Base64-encoded string" or
"plain UTF-8 string"); pick one behavior, document it (e.g., "data: String
(Base64-encoded) — Data to sign (Base64 encoded)"), and update the example where
`data = "data to sign"` to use the correct form (e.g., base64-encode that
example or change the description to plain text), and if the library accepts
both, state that explicitly and show both examples and note which is preferred;
adjust the `sign` function's README entry and its example consistently.
- Line 23: The README table incorrectly lists "Public Key" under Retrieval for
"AES (Symmetric)"; update the AES (Symmetric) row (the cell containing "AES
(Symmetric)") so the Retrieval column reflects that symmetric keys do not have
public keys — replace "Public Key" with a correct value such as "❌" or a
clarifying term like "Key Handle" / "Alias Only" to accurately describe what can
be retrieved.
- Around line 28-29: The Markdown table ending with the row containing "EC
secp256k1" needs a trailing blank line to satisfy Markdown formatting rules;
edit the README.md where the table row "| **EC secp256k1** | ❌ | ❌ | ❌
| ❌ | ✅ | Public & Private Key |" appears and insert one empty line immediately
after the table's closing line so the table is separated from subsequent
content.
---
Nitpick comments:
In `@README.md`:
- Around line 503-507: Update the "Returns" section that currently lists
"Array<String>" to explicitly describe the array structure: state whether each
entry is a single key or a pair, the element order (e.g., [publicKey,
privateKey] or [{"public":"...", "private":"..."}]), and whether multiple key
pairs may be returned (e.g., one pair vs. an array of pairs), and include
examples of the expected shape so consumers know how to access public vs.
private keys.
|
|
||
| | Key Type | Signing | Encryption / Decryption | HMAC | Hardware-Backed | Storage | Retrieval | | ||
| |---------------------------|----------|--------------------------|------|---------------|----------|----------------------| | ||
| | **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | Public Key | |
There was a problem hiding this comment.
Correct the Retrieval column for AES keys.
AES is a symmetric encryption algorithm and does not have public/private key pairs. The "Retrieval" column showing "Public Key" is technically incorrect. Consider changing this to either "❌" (if the symmetric key cannot be retrieved) or clarify what is actually retrievable (e.g., "Key Handle" or "Alias Only").
📝 Suggested fix
-| **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | Public Key |
+| **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | ❌ |📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | Public Key | | |
| | **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | ❌ | |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 23, The README table incorrectly lists "Public Key" under
Retrieval for "AES (Symmetric)"; update the AES (Symmetric) row (the cell
containing "AES (Symmetric)") so the Retrieval column reflects that symmetric
keys do not have public keys — replace "Public Key" with a correct value such as
"❌" or a clarifying term like "Key Handle" / "Alias Only" to accurately describe
what can be retrieved.
| |------|------|----------|-------------| | ||
| | signAlgorithm | String | Yes | Signing algorithm: `"SHA256withRSA"` or `"SHA256withECDSA"` | | ||
| | alias | String | Yes | Key pair identifier for signing | | ||
| | data | String | Yes | Data to sign (Base64 encoded) | |
There was a problem hiding this comment.
Clarify the expected input format for data parameter in sign function.
The documentation is inconsistent:
- Line 336 states the
dataparameter should be "Base64 encoded" - Line 346 example shows:
data = "data to sign"(plain text)
Please clarify whether the library expects Base64-encoded input or accepts plain text strings, and ensure the description and example are consistent.
Also applies to: 346-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 336, The README's sign function docs are inconsistent
about the data parameter format: update the description of the `data` parameter
in the parameters table and the example usage of the `sign` function so they
match and clearly state the accepted input format (either "Base64-encoded
string" or "plain UTF-8 string"); pick one behavior, document it (e.g., "data:
String (Base64-encoded) — Data to sign (Base64 encoded)"), and update the
example where `data = "data to sign"` to use the correct form (e.g.,
base64-encode that example or change the description to plain text), and if the
library accepts both, state that explicitly and show both examples and note
which is preferred; adjust the `sign` function's README entry and its example
consistently.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
README.md (5)
28-29:⚠️ Potential issue | 🟡 MinorAdd a blank line after the table for markdown compliance.
The table is immediately followed by
---without a separating empty line, which triggers MD058.Suggested fix
| **EC secp256k1** | ❌ | ❌ | ❌ | ❌ | ✅ | Public & Private Key | + ---🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 28 - 29, The Markdown table row containing "EC secp256k1" is immediately followed by a horizontal rule (`---`); insert a single blank line after the table (i.e., after the row with "EC secp256k1") so there's an empty line between the table and the `---` to satisfy MD058 and ensure proper markdown rendering.
23-23:⚠️ Potential issue | 🟡 MinorFix AES retrieval semantics in capability matrix.
Line 23 lists
Public Keyfor AES, which is technically incorrect for symmetric keys and can mislead API consumers.Suggested fix
-| **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | Public Key | +| **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | ❌ |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 23, The capability matrix row for AES (Symmetric) incorrectly lists "Public Key"; locate the row with "**AES (Symmetric)**" in the README capability table and change the retrieval semantics label from "Public Key" to an accurate term such as "Secret Key" or "Symmetric Key" (and update any tooltip/alt text if present) so it correctly reflects symmetric-key usage instead of public-key usage.
181-181:⚠️ Potential issue | 🟡 MinorUnify
encryptDatainput format documentation.Line 181 and Line 199 describe Base64-encoded input, but Line 208 example passes plain text. Please document one expected format consistently.
Also applies to: 199-199, 208-208
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 181, The README is inconsistent about encryptData input (some lines say Base64 while the example shows plain text); choose and document one canonical input format for encryptData (either "Base64-encoded input" or "raw/plain text input") and update all occurrences to match: adjust the descriptive sentences (previously at the two spots claiming Base64) and the usage/example block (the example at Line ~208) so the example data format matches the documented format for encryptData; ensure the README clearly states the required encoding and, if you keep Base64, show the example using Base64-encoded payload (or if you keep plain text, change the other descriptions to say plain text) and update any mentions of encryptData accordingly.
336-336:⚠️ Potential issue | 🟡 MinorUnify
signinput format documentation.Line 336 says Base64-encoded input, while Line 346 example uses plain text. This inconsistency can cause integration errors.
Also applies to: 346-346
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 336, The README has inconsistent documentation for the sign operation's input: the parameters table lists the "data" field as "Base64 encoded" while the example uses plain text; choose and standardize one format (preferably Base64) and update all references accordingly by changing the table entry for "data" to "Base64 string" and converting the example payload and any sample responses in the "sign" example to use Base64-encoded data (update the example request body and any variable names referencing data), ensuring the parameter name "data" and the operation "sign" are consistent throughout.
534-538:⚠️ Potential issue | 🟠 MajorClarify what
retrieveKeyreturns by key type.
"The key associated with the alias"is ambiguous and security-sensitive. Please explicitly state behavior for asymmetric (public key only) vs symmetric/HMAC (non-retrievable key material).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 534 - 538, The README's description for retrieveKey is ambiguous about whether raw key material is returned; update the docs for retrieveKey to explicitly state by key type what is returned: for asymmetric key pairs (RSA/EC) return the public key (PEM/JWK) or associated public-key metadata only, never the private key material; for symmetric keys/HMAC return that raw keying material is not retrievable and instead retrieveKey returns only the key identifier/metadata or throws/returns an error indicating non-extractable key; also mention any permissions/flags (e.g., exportable) that allow private/symmetric material to be returned and the exact return type (string/bytes/object) and error behavior when material is not retrievable so callers know how to handle each case.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 490-501: The doc for retrieveKeyPair uses inconsistent terminology
("account" vs "alias"); pick one term (preferably the parameter name alias or
change the parameter to account) and update the function description, the
parameter table, and the example so they all use the same word consistently;
specifically edit the retrieveKeyPair signature/description and any examples
referencing account/alias to match the chosen term (e.g., change "keys
associated with the specified account" to "keys associated with the specified
alias" or rename the parameter to account everywhere).
- Around line 462-466: The README shows a parameter name mismatch for the
function storeKeyPair: the signature uses account but the parameter table and
examples use alias; standardize on account by updating the parameter table, all
example calls, and any prose that references alias to use account instead (also
update the other occurrences noted around the subsequent blocks). Locate
references to storeKeyPair, its parameter table and examples (the examples at
the later blocks mentioned) and replace alias with account so the signature and
docs are consistent.
---
Duplicate comments:
In `@README.md`:
- Around line 28-29: The Markdown table row containing "EC secp256k1" is
immediately followed by a horizontal rule (`---`); insert a single blank line
after the table (i.e., after the row with "EC secp256k1") so there's an empty
line between the table and the `---` to satisfy MD058 and ensure proper markdown
rendering.
- Line 23: The capability matrix row for AES (Symmetric) incorrectly lists
"Public Key"; locate the row with "**AES (Symmetric)**" in the README capability
table and change the retrieval semantics label from "Public Key" to an accurate
term such as "Secret Key" or "Symmetric Key" (and update any tooltip/alt text if
present) so it correctly reflects symmetric-key usage instead of public-key
usage.
- Line 181: The README is inconsistent about encryptData input (some lines say
Base64 while the example shows plain text); choose and document one canonical
input format for encryptData (either "Base64-encoded input" or "raw/plain text
input") and update all occurrences to match: adjust the descriptive sentences
(previously at the two spots claiming Base64) and the usage/example block (the
example at Line ~208) so the example data format matches the documented format
for encryptData; ensure the README clearly states the required encoding and, if
you keep Base64, show the example using Base64-encoded payload (or if you keep
plain text, change the other descriptions to say plain text) and update any
mentions of encryptData accordingly.
- Line 336: The README has inconsistent documentation for the sign operation's
input: the parameters table lists the "data" field as "Base64 encoded" while the
example uses plain text; choose and standardize one format (preferably Base64)
and update all references accordingly by changing the table entry for "data" to
"Base64 string" and converting the example payload and any sample responses in
the "sign" example to use Base64-encoded data (update the example request body
and any variable names referencing data), ensuring the parameter name "data" and
the operation "sign" are consistent throughout.
- Around line 534-538: The README's description for retrieveKey is ambiguous
about whether raw key material is returned; update the docs for retrieveKey to
explicitly state by key type what is returned: for asymmetric key pairs (RSA/EC)
return the public key (PEM/JWK) or associated public-key metadata only, never
the private key material; for symmetric keys/HMAC return that raw keying
material is not retrievable and instead retrieveKey returns only the key
identifier/metadata or throws/returns an error indicating non-extractable key;
also mention any permissions/flags (e.g., exportable) that allow
private/symmetric material to be returned and the exact return type
(string/bytes/object) and error behavior when material is not retrievable so
callers know how to handle each case.
Comment on lines
+462
to
+466
| fun storeKeyPair( | ||
| publicKey: String, | ||
| privateKey: String, | ||
| account: String | ||
| ) |
There was a problem hiding this comment.
Resolve storeKeyPair parameter name mismatch (account vs alias).
The signature uses account, but the parameter table and example use alias. Copy-pasting this doc currently leads to incorrect usage.
Suggested fix (align on account)
fun storeKeyPair(
publicKey: String,
privateKey: String,
- account: String
+ account: String
)
@@
-| alias | String | Yes | Key pair identifier |
+| account | String | Yes | Account identifier |
@@
SecureKeystore.storeKeyPair(
publicKey = "-----BEGIN PUBLIC KEY-----...",
privateKey = "-----BEGIN PRIVATE KEY-----...",
- alias = "user@example.com"
+ account = "user@example.com"
)Also applies to: 471-476, 479-483
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` around lines 462 - 466, The README shows a parameter name mismatch
for the function storeKeyPair: the signature uses account but the parameter
table and examples use alias; standardize on account by updating the parameter
table, all example calls, and any prose that references alias to use account
instead (also update the other occurrences noted around the subsequent blocks).
Locate references to storeKeyPair, its parameter table and examples (the
examples at the later blocks mentioned) and replace alias with account so the
signature and docs are consistent.
Signed-off-by: abhip2565 <paul.apaul.abhishek.AP@gmail.com>
Signed-off-by: abhip2565 <paul.apaul.abhishek.AP@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
README.md (5)
336-336:⚠️ Potential issue | 🟡 Minor
signinput format is still inconsistent with its example.Docs say Base64 input, but the example passes plain text. Pick one contract and keep both sections aligned.
Also applies to: 343-347
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 336, The README's sign input description and example disagree (table row "sign" says Base64 but the example shows plain text); choose one canonical contract and make both sections consistent: update the table entry for "sign" and all examples that construct the request payload (the example code/JSON blocks shown later) so they both state and use the same format (either Base64-encoded data everywhere or plain text everywhere), and add a brief note about encoding expectations if necessary; search for occurrences of "sign" in the README and align the table row and the example blocks accordingly.
23-23:⚠️ Potential issue | 🟡 MinorFix AES retrieval semantics in the capability matrix.
AES keys are symmetric; “Public Key” in Retrieval is technically incorrect.
Suggested doc fix
-| **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | Public Key | +| **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | ❌ |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 23, The capability matrix row labeled "AES (Symmetric)" has an incorrect "Public Key" retrieval label; update that cell to reflect symmetric key semantics (e.g., "Symmetric Key" or "Secret Key") so the row text "AES (Symmetric)" aligns with the retrieval column instead of "Public Key".
28-29:⚠️ Potential issue | 🟡 MinorAdd a blank line after the table to satisfy Markdown linting.
The table is not separated from the following horizontal rule.
Suggested doc fix
| **EC secp256k1** | ❌ | ❌ | ❌ | ❌ | ✅ | Public & Private Key | + ---🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 28 - 29, Add a blank line after the Markdown table row containing "EC secp256k1" so the table is separated from the following horizontal rule; locate the table row with the cell text "EC secp256k1" and insert one empty line immediately after that row (before the '---' horizontal rule) to satisfy Markdown linting.
462-466:⚠️ Potential issue | 🟠 Major
storeKeyPair/retrieveKeyPairREADME signatures don’t match the Kotlin API.Current docs mix
account/alias, omitcontextforretrieveKeyPair, and showArray<String>instead ofList<String>. Copy-pasted usage will not compile against the current interfaces.Suggested doc fix
fun storeKeyPair( publicKey: String, privateKey: String, - account: String + alias: String ) -| alias | String | Yes | Key pair identifier | +| alias | String | Yes | Key pair identifier | SecureKeystore.storeKeyPair( publicKey = "-----BEGIN PUBLIC KEY-----...", privateKey = "-----BEGIN PRIVATE KEY-----...", alias = "user@example.com" ) -fun retrieveKeyPair(alias: String): Array<String> +fun retrieveKeyPair(alias: String, context: Any): List<String> -| Array<String> | Array of keys associated with the account | +| List<String> | Keys associated with the alias | -val keys = SecureKeystore.retrieveKeyPair("user@example.com") +val keys = SecureKeystore.retrieveKeyPair("user@example.com", this)Also applies to: 471-476, 479-483, 490-495, 505-508
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 462 - 466, Update the README Kotlin signatures so they exactly match the Kotlin API: replace the ambiguous "account" parameter name with "alias" in storeKeyPair and related examples, add the missing context parameter to retrieveKeyPair, and change any Array<String> examples to Kotlin List<String>; ensure all occurrences of storeKeyPair and retrieveKeyPair (and the other mentioned ranges) use the correct parameter names and types so the copy-pasted snippets compile against the real Kotlin functions.
181-181:⚠️ Potential issue | 🟡 Minor
encryptDatainput format is still inconsistent (Base64 vs plain text).The summary/params and example currently describe different expectations for
data.Suggested doc fix (plain text contract)
-Encrypts the given data (encoded in Base64) using the key assigned to the alias. +Encrypts the given data (plain UTF-8 text) using the key assigned to the alias. -| data | String | Yes | Plain text data to encrypt (Base64 encoded) | +| data | String | Yes | Plain text data to encrypt (UTF-8 string). Internal encoding is handled by the library. |Also applies to: 199-199, 206-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 181, The README describes encryptData inconsistently (saying Base64 in one place and plain text elsewhere); update the documentation to adopt a single input contract (use plain text for data) and ensure the summary, params, and examples for encryptData (and similarly for decryptData if present) consistently state that the function accepts plain text and returns Base64 (or vice-versa if you prefer the opposite contract), adjusting example payloads and parameter descriptions to match the chosen contract and reflecting the same wording in all occurrences (including the examples and the parameter list).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt`:
- Around line 319-327: The current branch that handles biometric auth
success/failure (the block that checks the boolean success and reads
privateKeyAlias/publicKeyAlias from preferences into keyPair) returns an empty
list on biometric failure; change this so biometric failures are signaled
explicitly instead of returning an empty key list—e.g., throw or propagate an
AuthenticationException (or return a Result/nullable with an explicit error)
when success is false, do not add empty strings to keyPair, and ensure callers
of the method that uses keyPair handle the authentication error; update the
branch that logs "Biometric authentication failed" (the same block referencing
privateKeyAlias, publicKeyAlias, preferences, and keyPair) to return/throw the
explicit error and not an empty list.
- Around line 313-317: The code incorrectly gates biometric authentication on
the variable alias by comparing it to SigningAlgorithm.ES256K/EDDSA and casts
context to FragmentActivity before that check, which can throw for non-biometric
flows; change the logic so biometric flow is selected based on the actual key
identifier/metadata (e.g., privateKeyAlias or a stored biometric protection
flag) rather than the algorithm constants, and move the FragmentActivity cast
into the biometric branch to only execute when calling
authenticateBiometricallyBlocking(fragmentActivity, privateKeyAlias); update the
conditional that currently references
SigningAlgorithm.ES256K/SigningAlgorithm.EDDSA to instead check the correct
biometric-protection predicate for privateKeyAlias (or call a helper like
isBiometricProtected(privateKeyAlias)) and then perform the FragmentActivity
cast and call authenticateBiometricallyBlocking only inside that branch.
- Around line 334-340: The catch block in SecureKeystoreImpl.kt currently
rethrows a new Exception with only e.message which drops the original
stack/cause; update the catch in the biometric authentication/key retrieval code
to rethrow the original throwable or wrap it while preserving the cause (e.g.,
throw e or throw Exception(e.message, e)) so that the original exception and
stacktrace from the biometric/key methods are retained for diagnosis; modify the
catch for Exception in the relevant method inside class SecureKeystoreImpl
accordingly.
---
Duplicate comments:
In `@README.md`:
- Line 336: The README's sign input description and example disagree (table row
"sign" says Base64 but the example shows plain text); choose one canonical
contract and make both sections consistent: update the table entry for "sign"
and all examples that construct the request payload (the example code/JSON
blocks shown later) so they both state and use the same format (either
Base64-encoded data everywhere or plain text everywhere), and add a brief note
about encoding expectations if necessary; search for occurrences of "sign" in
the README and align the table row and the example blocks accordingly.
- Line 23: The capability matrix row labeled "AES (Symmetric)" has an incorrect
"Public Key" retrieval label; update that cell to reflect symmetric key
semantics (e.g., "Symmetric Key" or "Secret Key") so the row text "AES
(Symmetric)" aligns with the retrieval column instead of "Public Key".
- Around line 28-29: Add a blank line after the Markdown table row containing
"EC secp256k1" so the table is separated from the following horizontal rule;
locate the table row with the cell text "EC secp256k1" and insert one empty line
immediately after that row (before the '---' horizontal rule) to satisfy
Markdown linting.
- Around line 462-466: Update the README Kotlin signatures so they exactly match
the Kotlin API: replace the ambiguous "account" parameter name with "alias" in
storeKeyPair and related examples, add the missing context parameter to
retrieveKeyPair, and change any Array<String> examples to Kotlin List<String>;
ensure all occurrences of storeKeyPair and retrieveKeyPair (and the other
mentioned ranges) use the correct parameter names and types so the copy-pasted
snippets compile against the real Kotlin functions.
- Line 181: The README describes encryptData inconsistently (saying Base64 in
one place and plain text elsewhere); update the documentation to adopt a single
input contract (use plain text for data) and ensure the summary, params, and
examples for encryptData (and similarly for decryptData if present) consistently
state that the function accepts plain text and returns Base64 (or vice-versa if
you prefer the opposite contract), adjusting example payloads and parameter
descriptions to match the chosen contract and reflecting the same wording in all
occurrences (including the examples and the parameter list).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8644744f-8291-4514-b600-69ce1920bfc2
📒 Files selected for processing (5)
README.mdkotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystore.ktkotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.ktkotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreV2.ktkotlin/android/src/main/java/com/reactnativesecurekeystore/common/Constants.kt
Comment on lines
+313
to
+317
| val fragmentActivity = context as? FragmentActivity | ||
| ?: throw IllegalArgumentException("Context must be a FragmentActivity for biometric authentication") | ||
| if (alias == SigningAlgorithm.ES256K.value || alias == SigningAlgorithm.EDDSA.value) { | ||
|
|
||
| val success = authenticateBiometricallyBlocking(fragmentActivity, privateKeyAlias) |
There was a problem hiding this comment.
Biometric gating is tied to alias value, which can bypass protection.
alias is an identifier, but the check compares it to algorithm constants (ES256K/EdDSA). For normal aliases, the biometric branch is skipped and keys are returned directly. Also, FragmentActivity is cast before that check, which can throw even in non-biometric paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt`
around lines 313 - 317, The code incorrectly gates biometric authentication on
the variable alias by comparing it to SigningAlgorithm.ES256K/EDDSA and casts
context to FragmentActivity before that check, which can throw for non-biometric
flows; change the logic so biometric flow is selected based on the actual key
identifier/metadata (e.g., privateKeyAlias or a stored biometric protection
flag) rather than the algorithm constants, and move the FragmentActivity cast
into the biometric branch to only execute when calling
authenticateBiometricallyBlocking(fragmentActivity, privateKeyAlias); update the
conditional that currently references
SigningAlgorithm.ES256K/SigningAlgorithm.EDDSA to instead check the correct
biometric-protection predicate for privateKeyAlias (or call a helper like
isBiometricProtected(privateKeyAlias)) and then perform the FragmentActivity
cast and call authenticateBiometricallyBlocking only inside that branch.
Comment on lines
+319
to
+327
| if (success) { | ||
| val privateKey = preferences.getPreference(privateKeyAlias, "") | ||
| val publicKey = preferences.getPreference(publicKeyAlias, "") | ||
| keyPair.add(privateKey) | ||
| keyPair.add(publicKey) | ||
| } else { | ||
| Log.e("SecureKeystore", "Biometric authentication failed") | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Do not return an empty key list on biometric failure.
Returning an empty list conflates “auth denied” with “no key data” and hides a security-relevant failure.
Suggested fix
- } else {
- Log.e("SecureKeystore", "Biometric authentication failed")
- }
+ } else {
+ throw SecurityException("Biometric authentication failed for alias: $alias")
+ }Also applies to: 333-333
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt`
around lines 319 - 327, The current branch that handles biometric auth
success/failure (the block that checks the boolean success and reads
privateKeyAlias/publicKeyAlias from preferences into keyPair) returns an empty
list on biometric failure; change this so biometric failures are signaled
explicitly instead of returning an empty key list—e.g., throw or propagate an
AuthenticationException (or return a Result/nullable with an explicit error)
when success is false, do not add empty strings to keyPair, and ensure callers
of the method that uses keyPair handle the authentication error; update the
branch that logs "Biometric authentication failed" (the same block referencing
privateKeyAlias, publicKeyAlias, preferences, and keyPair) to return/throw the
explicit error and not an empty list.
Comment on lines
+334
to
+340
| } catch (e: Exception) { | ||
| Log.e( | ||
| "SecureKeystore", | ||
| "Error during biometric authentication or retrieving key-data: ${e.message}" | ||
| ) | ||
| throw Exception(e.message) | ||
| } |
There was a problem hiding this comment.
Preserve the original exception cause when rethrowing.
throw Exception(e.message) drops stack/cause context, making diagnosis and handling harder.
Suggested fix
- } catch (e: Exception) {
- Log.e(
- "SecureKeystore",
- "Error during biometric authentication or retrieving key-data: ${e.message}"
- )
- throw Exception(e.message)
- }
+ } catch (e: Exception) {
+ Log.e(
+ "SecureKeystore",
+ "Error during biometric authentication or retrieving key-data: ${e.message}",
+ e
+ )
+ throw IllegalStateException("Failed to retrieve key pair for alias: $alias", e)
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| } catch (e: Exception) { | |
| Log.e( | |
| "SecureKeystore", | |
| "Error during biometric authentication or retrieving key-data: ${e.message}" | |
| ) | |
| throw Exception(e.message) | |
| } | |
| } catch (e: Exception) { | |
| Log.e( | |
| "SecureKeystore", | |
| "Error during biometric authentication or retrieving key-data: ${e.message}", | |
| e | |
| ) | |
| throw IllegalStateException("Failed to retrieve key pair for alias: $alias", e) | |
| } |
🧰 Tools
🪛 detekt (1.23.8)
[warning] 334-334: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt`
around lines 334 - 340, The catch block in SecureKeystoreImpl.kt currently
rethrows a new Exception with only e.message which drops the original
stack/cause; update the catch in the biometric authentication/key retrieval code
to rethrow the original throwable or wrap it while preserving the cause (e.g.,
throw e or throw Exception(e.message, e)) so that the original exception and
stacktrace from the biometric/key methods are retained for diagnosis; modify the
catch for Exception in the relevant method inside class SecureKeystoreImpl
accordingly.
abhip2565
force-pushed
the
injimob-3779
branch
2 times, most recently
from
c9e2e89 to
fdd1e8a
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (7)
README.md (4)
36-36:⚠️ Potential issue | 🟡 MinorCorrect the Retrieval column for AES keys.
AES is a symmetric encryption algorithm and does not have public/private key pairs. The "Retrieval" column showing "Public Key" is technically incorrect.
📝 Suggested fix
-| **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | Public Key | +| **AES (Symmetric)** | ❌ | ✅ | ❌ | ✅ | ✅ | ❌ |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 36, The AES row in the README table incorrectly lists "Public Key" under the Retrieval column; update that row (the table line containing "**AES (Symmetric)**") to reflect that AES is symmetric by replacing "Public Key" with an accurate value such as "Secret Key" or "N/A (symmetric key)" so the Retrieval column correctly indicates symmetric key retrieval rather than a public key.
378-378:⚠️ Potential issue | 🟡 MinorClarify the expected input format for the
dataparameter in the sign function.The documentation is inconsistent:
- Line 378 states the
dataparameter should be "Base64 encoded"- Line 388 example shows:
data = "data to sign"(plain text)Specify whether the library expects Base64-encoded input or plain text strings, and ensure the description and example match.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 378, The README currently contradicts itself about the sign function's data parameter; update the documentation so the `data` parameter in the sign function is clearly specified as either Base64-encoded or plain text and make the example consistent: choose the expected format (e.g., Base64) and change the table entry for `data` to "Base64-encoded string" and modify the example usage that sets `data = "data to sign"` to provide the corresponding Base64-encoded value (or conversely change the table to "String (plain text)" and keep the example), ensuring the `data` parameter description and the example match exactly.
223-223:⚠️ Potential issue | 🟡 MinorClarify the expected input format for the
dataparameter.The documentation contains contradictory statements about the
dataparameter format:
- Line 223 states: "data (encoded in Base64)"
- Line 241 states: "Plain text data to encrypt (Base64 encoded)"
- Line 250 example shows:
data = "Sensitive information"(plain text)Specify whether the library expects Base64-encoded input or plain text, and ensure the description, parameter table, and example are consistent.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` at line 223, The README contradicts the expected format for the data parameter; decide whether the API expects plain text or Base64 input and make all references consistent: update the short description "Encrypts the given data (encoded in Base64)" and the parameter table entry for "data" to state the chosen format (e.g., "Plain text data to encrypt" or "Base64-encoded data to encrypt"), and update the example value for data to match (change data = "Sensitive information" to a Base64 string if Base64 is required, or change the descriptive lines that mention Base64 to say plain text if plain text is required); ensure the term "data" is used consistently throughout the README and add a note about whether the library performs Base64 encoding/decoding internally or expects the caller to do so.
41-42:⚠️ Potential issue | 🟡 MinorAdd blank line after the table.
Tables should be surrounded by blank lines for proper Markdown formatting.
📝 Suggested fix
| **EC secp256k1** | ❌ | ❌ | ❌ | ❌ | ✅ | Public & Private Key | + ---🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 41 - 42, Add a blank line after the Markdown table row containing "**EC secp256k1**" (the table ending with "| **EC secp256k1** | ❌ | ❌ | ❌ | ❌ | ✅ | Public & Private Key |") so the table is surrounded by a blank line for proper Markdown rendering; locate that row in README.md and insert one empty line immediately after the table delimiter/row block.kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt (3)
338-344:⚠️ Potential issue | 🟠 MajorException rethrow drops the original cause and stacktrace.
Line 343 recreates a generic
Exceptionwith only message text. Preserve the original throwable for diagnosability.Suggested fix
} catch (e: Exception) { Log.e( "SecureKeystore", - "Error during biometric authentication or retrieving key-data: ${e.message}" + "Error during biometric authentication or retrieving key-data: ${e.message}", + e ) - throw Exception(e.message) + throw IllegalStateException("Failed to retrieve key pair for alias: $alias", e) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt` around lines 338 - 344, The catch block in SecureKeystoreImpl.kt currently logs the error then recreates a new Exception via throw Exception(e.message), which drops the original cause and stacktrace; update the catch to rethrow the original throwable or wrap it preserving the cause (e.g., replace throw Exception(e.message) with throw e or throw Exception("Biometric auth/key retrieval failed", e)) so the original stacktrace and cause from the caught exception are preserved; locate the catch using Log.e and the existing throw Exception(e.message) to make the change.
328-337:⚠️ Potential issue | 🟠 MajorBiometric auth failure should be explicit, not a silent empty result.
Line 329 logs failure but still returns
keyPair(often empty), which makes auth-denied indistinguishable from missing data.Suggested fix
if (success) { val privateKey = preferences.getPreference(privateKeyAlias, "") val publicKey = preferences.getPreference(publicKeyAlias, "") keyPair.add(privateKey) keyPair.add(publicKey) } else { - Log.e("SecureKeystore", "Biometric authentication failed") + throw SecurityException("Biometric authentication failed for alias: $alias") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt` around lines 328 - 337, The code currently logs biometric auth failure with Log.e("SecureKeystore", "Biometric authentication failed") but then falls through to return keyPair (often empty), making denial indistinguishable from missing keys; in the else branch where Log.e is called inside SecureKeystoreImpl.kt, replace the silent return with an explicit failure signal — either throw a specific exception (e.g., SecurityException("Biometric authentication failed")) or return a well-documented null/Result type instead of the empty keyPair — so callers can clearly detect authentication denial; ensure you reference the same symbols (Log.e, keyPair, privateKeyAlias/publicKeyAlias, preferences.getPreference) when implementing the change and update any callers to handle the new error/return contract.
317-321:⚠️ Potential issue | 🔴 CriticalBiometric gating logic is tied to
alias, andFragmentActivityis required even for non-biometric fetches.Line 317 casts
contextbefore deciding biometric flow, and Line 319 usesSigningAlgorithm.contains(alias)as part of the biometric predicate. This can both bypass intended protection and break non-biometric retrieval paths.Suggested fix
- val fragmentActivity = context as? FragmentActivity - ?: throw IllegalArgumentException("Context must be a FragmentActivity for biometric authentication") - if (SigningAlgorithm.contains(alias) || isBiometricRequiredToFetch) { + if (isBiometricRequiredToFetch) { + val fragmentActivity = context as? FragmentActivity + ?: throw IllegalArgumentException("Context must be a FragmentActivity for biometric authentication") val success = authenticateBiometricallyBlocking(fragmentActivity, privateKeyAlias)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt` around lines 317 - 321, The code currently casts context to FragmentActivity before deciding whether biometric auth is needed and uses SigningAlgorithm.contains(alias) which ties biometric gating to the wrong variable; change the logic so you first determine the biometric predicate (use SigningAlgorithm.contains(privateKeyAlias) || isBiometricRequiredToFetch), and only if that predicate is true cast context to FragmentActivity and call authenticateBiometricallyBlocking(fragmentActivity, privateKeyAlias); ensure non-biometric fetch paths never perform the FragmentActivity cast and continue using the existing fetch flow (refer to SigningAlgorithm.contains, isBiometricRequiredToFetch, and authenticateBiometricallyBlocking).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt`:
- Around line 324-335: The code is masking missing keys by using "" as the
default in preferences.getPreference(privateKeyAlias,
"")/getPreference(publicKeyAlias, ""), so update SecureKeystoreImpl to treat
missing keys as errors: call getPreference without a blank default or with null
if supported, then check the returned privateKey/publicKey for null or empty and
fail-fast (log error and return an error/throw or resolve the promise as
failure) instead of adding empty strings to keyPair; reference the
privateKeyAlias/publicKeyAlias lookups and the keyPair population in the
biometric and non-biometric branches to implement this validation.
In `@README.md`:
- Line 534: The README incorrectly states biometric auth is "triggered
automatically" for sensitive key types; update the documentation for
retrieveKeyPair to state the current implementation only auto-triggers biometric
authentication when the alias string itself matches a SigningAlgorithm name
(e.g., "ES256K" or "EdDSA"); otherwise biometric enforcement happens only if
isBiometricRequiredToFetch is true. Reference the retrieveKeyPair method and the
use of SigningAlgorithm.contains(alias) in the docs, and add a short note
suggesting the alternative implementation fix (store key type separately from
alias and check stored key type instead of alias) if consumers need seamless
automatic biometric enforcement.
---
Duplicate comments:
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt`:
- Around line 338-344: The catch block in SecureKeystoreImpl.kt currently logs
the error then recreates a new Exception via throw Exception(e.message), which
drops the original cause and stacktrace; update the catch to rethrow the
original throwable or wrap it preserving the cause (e.g., replace throw
Exception(e.message) with throw e or throw Exception("Biometric auth/key
retrieval failed", e)) so the original stacktrace and cause from the caught
exception are preserved; locate the catch using Log.e and the existing throw
Exception(e.message) to make the change.
- Around line 328-337: The code currently logs biometric auth failure with
Log.e("SecureKeystore", "Biometric authentication failed") but then falls
through to return keyPair (often empty), making denial indistinguishable from
missing keys; in the else branch where Log.e is called inside
SecureKeystoreImpl.kt, replace the silent return with an explicit failure signal
— either throw a specific exception (e.g., SecurityException("Biometric
authentication failed")) or return a well-documented null/Result type instead of
the empty keyPair — so callers can clearly detect authentication denial; ensure
you reference the same symbols (Log.e, keyPair, privateKeyAlias/publicKeyAlias,
preferences.getPreference) when implementing the change and update any callers
to handle the new error/return contract.
- Around line 317-321: The code currently casts context to FragmentActivity
before deciding whether biometric auth is needed and uses
SigningAlgorithm.contains(alias) which ties biometric gating to the wrong
variable; change the logic so you first determine the biometric predicate (use
SigningAlgorithm.contains(privateKeyAlias) || isBiometricRequiredToFetch), and
only if that predicate is true cast context to FragmentActivity and call
authenticateBiometricallyBlocking(fragmentActivity, privateKeyAlias); ensure
non-biometric fetch paths never perform the FragmentActivity cast and continue
using the existing fetch flow (refer to SigningAlgorithm.contains,
isBiometricRequiredToFetch, and authenticateBiometricallyBlocking).
In `@README.md`:
- Line 36: The AES row in the README table incorrectly lists "Public Key" under
the Retrieval column; update that row (the table line containing "**AES
(Symmetric)**") to reflect that AES is symmetric by replacing "Public Key" with
an accurate value such as "Secret Key" or "N/A (symmetric key)" so the Retrieval
column correctly indicates symmetric key retrieval rather than a public key.
- Line 378: The README currently contradicts itself about the sign function's
data parameter; update the documentation so the `data` parameter in the sign
function is clearly specified as either Base64-encoded or plain text and make
the example consistent: choose the expected format (e.g., Base64) and change the
table entry for `data` to "Base64-encoded string" and modify the example usage
that sets `data = "data to sign"` to provide the corresponding Base64-encoded
value (or conversely change the table to "String (plain text)" and keep the
example), ensuring the `data` parameter description and the example match
exactly.
- Line 223: The README contradicts the expected format for the data parameter;
decide whether the API expects plain text or Base64 input and make all
references consistent: update the short description "Encrypts the given data
(encoded in Base64)" and the parameter table entry for "data" to state the
chosen format (e.g., "Plain text data to encrypt" or "Base64-encoded data to
encrypt"), and update the example value for data to match (change data =
"Sensitive information" to a Base64 string if Base64 is required, or change the
descriptive lines that mention Base64 to say plain text if plain text is
required); ensure the term "data" is used consistently throughout the README and
add a note about whether the library performs Base64 encoding/decoding
internally or expects the caller to do so.
- Around line 41-42: Add a blank line after the Markdown table row containing
"**EC secp256k1**" (the table ending with "| **EC secp256k1** | ❌ | ❌ |
❌ | ❌ | ✅ | Public & Private Key |") so the table is surrounded by a blank
line for proper Markdown rendering; locate that row in README.md and insert one
empty line immediately after the table delimiter/row block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45f950e1-29a8-4467-bf5b-4831fbd1acda
📒 Files selected for processing (4)
README.mdkotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.ktkotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreV2.ktkotlin/android/src/main/java/com/reactnativesecurekeystore/common/Constants.kt
🚧 Files skipped from review as they are similar to previous changes (2)
- kotlin/android/src/main/java/com/reactnativesecurekeystore/common/Constants.kt
- kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreV2.kt
Comment on lines
+324
to
+335
| val privateKey = preferences.getPreference(privateKeyAlias, "") | ||
| val publicKey = preferences.getPreference(publicKeyAlias, "") | ||
| keyPair.add(privateKey) | ||
| keyPair.add(publicKey) | ||
| } else { | ||
| Log.e("SecureKeystore", "Biometric authentication failed") | ||
| } | ||
| } else { | ||
| val privateKey = preferences.getPreference(privateKeyAlias, "") | ||
| val publicKey = preferences.getPreference(publicKeyAlias, "") | ||
| keyPair.add(privateKey) | ||
| keyPair.add(publicKey) |
There was a problem hiding this comment.
Missing-key cases are masked by default empty strings.
Using "" defaults can return invalid key material as if retrieval succeeded. Fail fast when either key is absent.
Suggested fix
- val privateKey = preferences.getPreference(privateKeyAlias, "")
- val publicKey = preferences.getPreference(publicKeyAlias, "")
+ val privateKey = preferences.getPreference(privateKeyAlias, "")
+ val publicKey = preferences.getPreference(publicKeyAlias, "")
+ if (privateKey.isBlank() || publicKey.isBlank()) {
+ throw KeyNotFound("Key pair not found for alias: $alias")
+ }
keyPair.add(privateKey)
keyPair.add(publicKey)
...
- val privateKey = preferences.getPreference(privateKeyAlias, "")
- val publicKey = preferences.getPreference(publicKeyAlias, "")
+ val privateKey = preferences.getPreference(privateKeyAlias, "")
+ val publicKey = preferences.getPreference(publicKeyAlias, "")
+ if (privateKey.isBlank() || publicKey.isBlank()) {
+ throw KeyNotFound("Key pair not found for alias: $alias")
+ }
keyPair.add(privateKey)
keyPair.add(publicKey)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt`
around lines 324 - 335, The code is masking missing keys by using "" as the
default in preferences.getPreference(privateKeyAlias,
"")/getPreference(publicKeyAlias, ""), so update SecureKeystoreImpl to treat
missing keys as errors: call getPreference without a blank default or with null
if supported, then check the returned privateKey/publicKey for null or empty and
fail-fast (log error and return an error/throw or resolve the promise as
failure) instead of adding empty strings to keyPair; reference the
privateKeyAlias/publicKeyAlias lookups and the keyPair population in the
biometric and non-biometric branches to implement this validation.
| #### retrieveKeyPair | ||
|
|
||
| `removeAllKeys() => void` | ||
| Retrieves the public and private keys associated with the specified alias. For sensitive key types (`ES256K`, `EdDSA`), biometric authentication is triggered automatically. This method replaces the deprecated `retrieveGenericKey`. |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's read the README.md file around the mentioned lines
head -n 560 README.md | tail -n 50Repository: inji/secure-keystore
Length of output: 2166
🏁 Script executed:
# Search for retrieveKeyPair implementation in the codebase
find . -type f \( -name "*.kt" -o -name "*.java" -o -name "*.ts" -o -name "*.js" \) | head -20Repository: inji/secure-keystore
Length of output: 1040
🏁 Script executed:
# Look for the function definition and biometric handling
rg -n "retrieveKeyPair" --type-list | head -5
rg -n "retrieveKeyPair" -A 20 -B 2Repository: inji/secure-keystore
Length of output: 18029
🏁 Script executed:
# Search for SigningAlgorithm class definition and contains method
rg -n "SigningAlgorithm" -A 5 -B 2Repository: inji/secure-keystore
Length of output: 3447
🏁 Script executed:
# Find the file containing SigningAlgorithm
find . -type f -name "*.kt" -o -name "*.java" | xargs grep -l "object SigningAlgorithm\|class SigningAlgorithm"Repository: inji/secure-keystore
Length of output: 144
🏁 Script executed:
# Read the Constants.kt file to see the full SigningAlgorithm definition
cat kotlin/android/src/main/java/com/reactnativesecurekeystore/common/Constants.ktRepository: inji/secure-keystore
Length of output: 807
Clarify when biometric authentication is triggered for retrieveKeyPair.
The description states biometric authentication is "triggered automatically" for sensitive key types (ES256K, EdDSA), but examining the implementation reveals a disconnect. The code checks SigningAlgorithm.contains(alias) to determine whether to trigger biometric authentication. This checks if the provided alias string matches one of the algorithm names (e.g., "ES256K", "EdDSA"). However, the documented examples show that alias is a user-defined identifier like "user@example.com"—not an algorithm name. With typical usage, SigningAlgorithm.contains(alias) would return false, and biometric authentication would only be enforced when isBiometricRequiredToFetch is explicitly set to true.
Update the documentation to clarify whether:
- Automatic biometric triggering only occurs if the
aliasparameter matches an algorithm name, or - The implementation should be modified to detect and store key types separately from aliases
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@README.md` at line 534, The README incorrectly states biometric auth is
"triggered automatically" for sensitive key types; update the documentation for
retrieveKeyPair to state the current implementation only auto-triggers biometric
authentication when the alias string itself matches a SigningAlgorithm name
(e.g., "ES256K" or "EdDSA"); otherwise biometric enforcement happens only if
isBiometricRequiredToFetch is true. Reference the retrieveKeyPair method and the
use of SigningAlgorithm.contains(alias) in the docs, and add a short note
suggesting the alternative implementation fix (store key type separately from
alias and check stored key type instead of alias) if consumers need seamless
automatic biometric enforcement.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt (1)
240-243: UnusedkeyAliasparameter.The
keyAliasparameter is never used withinauthenticateBiometricallyBlocking. Either remove it or use it (e.g., for logging, key-specific prompt text, or crypto object initialization).♻️ Suggested fix: Remove unused parameter
- private fun authenticateBiometricallyBlocking( - activity: FragmentActivity, - keyAlias: String - ): Boolean { + private fun authenticateBiometricallyBlocking( + activity: FragmentActivity + ): Boolean {And update the call site at line 321:
- val success = authenticateBiometricallyBlocking(fragmentActivity, privateKeyAlias) + val success = authenticateBiometricallyBlocking(fragmentActivity)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt` around lines 240 - 243, The keyAlias parameter on authenticateBiometricallyBlocking is unused; remove it from the function signature (private fun authenticateBiometricallyBlocking(activity: FragmentActivity): Boolean) and update all call sites (including the call at line 321) to stop passing keyAlias, and adjust any references to the old signature. If you prefer to keep keyAlias, instead incorporate it into the method (e.g., include it in prompt text or use it when building a CryptoObject) and ensure call sites continue to pass the value; choose one approach and make the signature and all callers consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/common/Constants.kt`:
- Around line 16-27: SigningAlgorithm currently lists EDDSA and ES256K but
SecureKeystoreImpl.generateKeyPair only handles RSA and ES256, which can cause
KeyNotFound when those enums are used; either remove EDDSA and ES256K from
SigningAlgorithm or add a clear TODO noting planned support and that
SecureKeystoreImpl.generateKeyPair must be extended to handle EDDSA and ES256K
(implement key generation branches and platform-specific logic) before they are
exposed, referencing the SigningAlgorithm enum and
SecureKeystoreImpl.generateKeyPair so reviewers can find and reconcile the
mismatch.
---
Nitpick comments:
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.kt`:
- Around line 240-243: The keyAlias parameter on
authenticateBiometricallyBlocking is unused; remove it from the function
signature (private fun authenticateBiometricallyBlocking(activity:
FragmentActivity): Boolean) and update all call sites (including the call at
line 321) to stop passing keyAlias, and adjust any references to the old
signature. If you prefer to keep keyAlias, instead incorporate it into the
method (e.g., include it in prompt text or use it when building a CryptoObject)
and ensure call sites continue to pass the value; choose one approach and make
the signature and all callers consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2133c237-27a7-40f2-ba44-c67238e871f4
📒 Files selected for processing (4)
README.mdkotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreImpl.ktkotlin/android/src/main/java/com/reactnativesecurekeystore/SecureKeystoreV2.ktkotlin/android/src/main/java/com/reactnativesecurekeystore/common/Constants.kt
Comment on lines
+16
to
+27
| enum class SigningAlgorithm(val value: String) { | ||
| RSA("RS256"), | ||
| ES256("ES256"), | ||
| EDDSA("EdDSA"), | ||
| ES256K("ES256K"); | ||
|
|
||
| companion object { | ||
| fun contains(value: String): Boolean { | ||
| return values().any { it.value.equals(value, ignoreCase = true) } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the generateKeyPair implementation
find . -type f -name "*.kt" | head -20
rg -n "generateKeyPair" --type kotlin -A 10Repository: inji/secure-keystore
Length of output: 15214
🏁 Script executed:
# Check Kotlin version in gradle files
find . -type f \( -name "build.gradle*" -o -name "gradle.properties" \) | xargs cat 2>/dev/null | grep -i kotlinRepository: inji/secure-keystore
Length of output: 1595
🏁 Script executed:
# Verify SecureKeystoreImpl and its generateKeyPair method
rg -n "class SecureKeystoreImpl" --type kotlin -A 50Repository: inji/secure-keystore
Length of output: 5916
🏁 Script executed:
# Check the specific gradle config for the kotlin/android module
cat kotlin/android/build.gradle* 2>/dev/null | head -80Repository: inji/secure-keystore
Length of output: 2496
🌐 Web query:
Kotlin 1.9 values() deprecation enum entries
💡 Result:
In Kotlin 1.9.0, enum classes got a stable entries property as the modern, more performant replacement for the synthetic values() function. Kotlin still supports values(), but IDEs flag it as “replace with entries” (a soft deprecation/inspection), because values() returns a new array copy each call, while entries returns an efficient immutable list-like view (EnumEntries). [1][2][3]
What to use instead of values()
- Before:
MyEnum.values()(returnsArray<MyEnum>) - Now:
MyEnum.entries(returnsEnumEntries<MyEnum>which is aList<MyEnum>) [3][4]
If you truly need an array (API requires Array<T>), convert:
MyEnum.entries.toTypedArray()[1]
Generic form
- Kotlin has
EnumEntries/enumEntriesavailable in stdlib (since 1.9 forEnumEntriesitself) [4] - The docs also describe
enumEntries<T>()as the replacement direction forenumValues<T>()(notingenumEntries<T>()is introduced in Kotlin 2.0.0). [3]
Sources: Kotlin 1.9 “What’s new” (enum entries) [2], Kotlin enum classes docs (values vs entries, and generic helpers) [3], Kotlin stdlib API (EnumEntries) [4], JetBrains Inspectopedia inspection about replacing values() with entries [1].
Enum declares algorithms not yet supported by key generation.
SigningAlgorithm defines EDDSA and ES256K, but generateKeyPair in SecureKeystoreImpl only branches on RSA and ES256. Attempting to generate keys with these types will throw KeyNotFound. Consider either:
- Adding a
TODOcomment documenting planned support, or - Removing unsupported values until implementation is ready
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@kotlin/android/src/main/java/com/reactnativesecurekeystore/common/Constants.kt`
around lines 16 - 27, SigningAlgorithm currently lists EDDSA and ES256K but
SecureKeystoreImpl.generateKeyPair only handles RSA and ES256, which can cause
KeyNotFound when those enums are used; either remove EDDSA and ES256K from
SigningAlgorithm or add a clear TODO noting planned support and that
SecureKeystoreImpl.generateKeyPair must be extended to handle EDDSA and ES256K
(implement key generation branches and platform-specific logic) before they are
exposed, referencing the SigningAlgorithm enum and
SecureKeystoreImpl.generateKeyPair so reviewers can find and reconcile the
mismatch.
Signed-off-by: abhip2565 <paul.apaul.abhishek.AP@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit