-
-
Notifications
You must be signed in to change notification settings - Fork 240
Feat/key rotation #499
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feat/key rotation #499
Conversation
…nality - Refactored the project structure to separate components into a dedicated directory. - Introduced a new App component with secure storage capabilities using react-native-sensitive-info. - Added ActionButton, ActionsPanel, Card, Header, ModeSelector, SecretForm, SecretsList components for improved UI and functionality. - Implemented secure storage logic including saving, revealing, removing, and clearing secrets. - Enhanced user experience with loading states and error handling. - Updated configuration files for Babel, Metro, and React Native. - Added utility functions for error formatting and constants for default values. - Improved ESLint and Prettier configurations for better code quality.
Replace the legacy sample with a simplified "Sensitive Info Playground" example that leverages useSecureStorage and useSecurityAvailability. Clean up imports, reduce access-control options to 'open' and 'biometric', wire up save/reveal/delete/clear/refresh handlers, update ActionButton behaviour, and refresh styles/layout to match the streamlined UI.
- Update dev deps: @eslint/compat -> 1.4.1, @eslint/js -> 9.39.0, eslint -> 9.39.0, globals -> 16.5.0, nitrogen -> 0.31.4 - Update runtime/example deps: react-native-nitro-modules -> 0.31.4, react-native-safe-area-context -> ^5.6.2 - Update yarn.lock to match dependency bumps - Docs: change README references from "5.6.0" to "5.6.x" for consistent versioning
…p native cancel codes - Add CODE_OF_CONDUCT - Bump LICENSE copyright range to 2016-2025 - Android: - Introduce SensitiveInfoException.AuthenticationCanceled and throw/resume with it when user cancels biometric/device-credential prompts - Simplify device credential flow to always return cipher after prompt - iOS: - Map relevant OSStatus values to an E_AUTH_CANCELED runtime error for friendly messaging - Internal errors: - Add AUTH_CANCELED marker and helper hasErrorMarker/isAuthenticationCanceledError - Centralize detection of auth-cancelled errors - Hooks & utilities: - Export and use isAuthenticationCanceledError in error-utils - Create user-friendly hook error message for canceled auths and export detector - Update hooks (useSecretItem, useHasSecret, useSecureOperation, useSecureStorage, useSecurityAvailability) to treat auth cancellations as non-fatal: preserve/clear state appropriately and avoid surfacing HookError when user dismisses prompts - Add applyError helper in useSecureStorage to centralize error handling - Update hook types and exports - Nitro/native layers & types: - Type and formatting fixes across sensitive-info.nitro.ts, internal/native, options, core/storage and index exports - Tests & tooling: - Apply consistent code style (semicolons, trailing commas) across tests and configs - Update many test files to match changes and ensure behavior for canceled auth flows - Misc: - Update package.json description - ESLint config formatting fixes This change makes authentication prompt cancellations explicit (E_AUTH_CANCELED) and prevents noisy error states in hooks when users dismiss biometric / device credential prompts.
…ample iOS Podfile.lock deps
- Apply consistent TypeScript formatting (semicolons, trailing commas, minor punctuation) across hook modules:
src/hooks/{error-utils,useAsyncLifecycle,useHasSecret,useSecret,useSecretItem,useSecureOperation,useSecureStorage}.ts
- Update example/ios/Podfile.lock dependency versions/checksums:
- SensitiveInfo -> 6.0.0-rc.8
- NitroModules -> 0.31.4
- react-native-safe-area-context -> 5.6.2
…el handling - Replace direct SecItemCopyMatching calls with performCopyMatching that ensures the Sec API is invoked on the main thread. - Stop treating errSecInteractionNotAllowed as a user cancellation; only errSecUserCanceled is considered a cancellation. chore(package): update/normalize keywords in package.json
…robe security on main thread - Invoke simulator biometric prompt (when needed) before calling SecItemCopyMatching so simulator flows mirror device auth and cancellation maps to a consistent RuntimeError. - Add performSimulatorBiometricPromptIfNeeded helper that evaluates LAContext on the main thread and surfaces user-cancel errors. - Ensure SecurityAvailabilityResolver probes capabilities on the main thread and correctly distinguishes simulator biometry/secure-enclave availability. - Update example Podfile.lock: SensitiveInfo -> 6.0.0-rc.10 and CocoaPods -> 1.16.2.
- Added `envelope.ts` for envelope encryption implementation, including functions for creating, parsing, and validating encrypted envelopes. - Introduced `migration.ts` to handle migration from legacy encrypted data to the new versioned envelope format, supporting batch processing and progress tracking. - Created `rotation-api.ts` to expose key rotation functionalities, including initialization, rotation, migration, and event handling. - Defined types and interfaces in `types.ts` for key versions, encrypted envelopes, rotation policies, and migration results.
…idation - Introduced a centralized error factory for consistent error handling across hooks. - Added option validation and normalization utilities to ensure consistent configurations. - Updated `useSecret`, `useSecretItem`, and `useSecureStorage` hooks to utilize new error handling and option extraction methods. - Improved documentation for hooks, including examples and detailed descriptions of parameters and return values. - Enhanced error messages to provide clearer guidance for common issues.
…yption and event handling
…ling and manual rotation support
- Added structured error classification with SensitiveInfoError and ErrorCode for better error management. - Introduced branded types for StorageKey, ServiceName, and StorageValue to improve type safety. - Implemented comprehensive validation functions for storage keys, values, services, and options. - Updated useSecureStorage hook to reset errors on successful operations. - Removed legacy error handling code and consolidated error handling logic. - Created a StorageValidator class for reusable validation methods across storage operations.
Introduces modular internal managers for authentication, access control, and metadata on Android and iOS. Adds new files for Android (AuthenticationManager, AccessControlManager, MetadataManager and their implementations), and restructures iOS logic to use dependency injection and single-responsibility managers. Updates HybridSensitiveInfo on both platforms to delegate to these managers, improving maintainability and testability. Removes obsolete docs/REFACTORING_SUMMARY.md. Also updates dependencies in example and root package files.
Unifies and updates access control and metadata management across Android and iOS. Android now uses StorageMetadata directly for metadata encoding/decoding, and access control policies are aligned with iOS, including new enum values and mappings. iOS refactors AccessControlFactory and iOSAccessControlManager to match updated access control policies and simplifies policy resolution. Adds NitroModules imports to iOS files for consistency. Updates .npmignore and package.json to refine published files.
… value decryption
…e info management
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request introduces a comprehensive key rotation feature to the SensitiveInfo module, along with significant improvements to type safety, validation, and response consistency. The changes span TypeScript business logic, iOS native implementations, testing, and configuration.
Key changes:
- Implements automatic key rotation with envelope encryption (DEK/KEK pattern) supporting time-based, biometric-triggered, and manual rotation
- Adds type-safe error handling with
SensitiveInfoErrorclass andErrorCodeenum replacing string-based error markers - Introduces input validation framework with
StorageValidatorand branded types for enhanced type safety - Updates metadata serialization to include
aliasfield for key rotation tracking
Reviewed Changes
Copilot reviewed 97 out of 101 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| src/rotation/* | New key rotation engine with envelope encryption, migration utilities, and TypeScript API |
| src/internal/error-classifier.ts | Type-safe error classification replacing string-based error detection |
| src/internal/validator.ts | Comprehensive input validation for storage operations |
| src/internal/branded-types.ts | Branded types for compile-time safety (StorageKey, ServiceName) |
| src/hooks/* | Updated hooks with new error handling and validation patterns |
| src/core/storage.ts | Enhanced with validation, error classification, and comprehensive JSDoc |
| ios/KeyRotation.swift | iOS key rotation implementation using Keychain and Secure Enclave |
| ios/Internal/* | Modular iOS architecture with managers for metadata, authentication, access control |
| src/tests/* | Test coverage for rotation engine and envelope encryption |
| package.json | Updated dependencies, added lint script, fixed prettier config |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| backend: .keychain, | ||
| accessControl: .none, | ||
| timestamp: Date().timeIntervalSince1970, | ||
| alias: nil |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The alias field is set to nil in the default metadata, but the StorageMetadata type in the Nitro schema defines alias as string, not optional. This will cause a type mismatch. Change to empty string \"\" to match the schema and other implementations.
| let value: String? = if includeValue, | ||
| let data = dict[kSecValueData as String] as? Data { | ||
| String(data: data, encoding: .utf8) | ||
| } else { | ||
| nil | ||
| } |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encrypted data from encryptData is being treated as UTF-8 string directly, but the data is encrypted and needs decryption first. The encrypted value should be decrypted using dependencies.cryptoService.decryptData before converting to String, otherwise the value will be garbled or nil.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| // Trigger rotation if policy allows | ||
| if (rotationManager.shouldRotate('biometric-change')) { | ||
| try { | ||
| await rotateKeys({ reason: 'security-audit' }); |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rotation reason 'security-audit' doesn't match the expected rotation reasons in the type system. The rotateKeys function expects reasons like 'manual', 'biometric-change', etc. but passes 'security-audit' which is not a valid reason in the RotationOptions type. Change to reason: 'manual' to match the expected values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| // Trigger rotation if policy allows | ||
| if (rotationManager.shouldRotate('credential-change')) { | ||
| try { | ||
| await rotateKeys({ reason: 'security-audit' }); |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as Comment 3: 'security-audit' is not a valid rotation reason. Should be 'manual' or another valid value from the rotation reason union type.
| } | ||
| } | ||
| }, [begin, mountedRef, stableOptions]); | ||
| }, [begin, mountedRef, stableOptions, applyError]); |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The applyError callback is included in the dependency array but is defined with useCallback without its own dependencies in scope. This can cause unnecessary re-renders. Consider memoizing applyError properly with useCallback and including its actual dependencies, or remove it from this dependency array if it's stable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| // First, check if current key matches | ||
| if (this.state.currentKeyVersion?.id === envelopeKEKVersion) { |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Duplicate comment 'First, check if current key matches' appears on consecutive lines (413-414). Remove the duplicate comment on line 413.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
| "prettier": { | ||
| "quoteProps": "consistent", | ||
| "singleQuote": true, | ||
| "semi": true, |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The semi prettier configuration is set to true, but the PR description mentions enforcing semicolons. However, looking at the actual code changes, semicolons are being added consistently. Verify this matches the team's style guide, as the previous config had \"semi\": false. This is a significant style change affecting the entire codebase.
| }); | ||
|
|
||
| const preview = items.map((item) => { | ||
| let currentFormat: 'versioned' | 'legacy' | 'unreadable' = 'unreadable'; |
Copilot
AI
Nov 10, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The initial value of currentFormat is unused, since it is always overwritten.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
This pull request introduces several significant improvements and new features to the Android core of the SensitiveInfo module, focusing on type safety, validation, and response consistency. It also includes some minor code style and configuration updates.
Android core improvements:
Result<T>class inResult.ktto standardize error handling and composition of operations, along with extension functions for easier usage.Extensions.ktwith utility functions for working withResult, validation helpers, and keystore alias generation.SensitiveInfoModule.ktfor dependency configuration and storage validation logic, with clear guidance for future Hilt DI integration.ResponseBuilderinterface and a standard implementation inResponseBuilder.ktto ensure consistent, type-safe responses from native operations.StorageValidatorinterface and Android-specific implementation for robust input validation, enforcing key, value, and service constraints.Metadata serialization and persistence:
PersistedMetadataandPersistedEntryto include analiasfield, ensuring alias information is serialized and deserialized correctly for storage metadata. [1] [2] [3] [4] [5] [6]Code style and configuration:
.prettierrc.js,babel.config.js, andapp.plugin.jsto enforce consistent code style (e.g., semicolons, tab width, quote style, trailing commas). [1] [2] [3]