some key stuff

2026-01-11 06:12:51 +00:00 · 2025-09-12 01:37:17 -04:00
parent 92882e276e
commit d9879d7a87
8 changed files with 509 additions and 431 deletions
--- a/docs/key-management-plan.md
+++ b/docs/key-management-plan.md
@@ -0,0 +1,141 @@
+# Key Management UX & Tech Plan
+
+Goal: Make SSH private key management clear and consistent: selecting a key,
+generating/importing keys, renaming, deleting, and setting a default — all with
+a unified, styled UI and predictable data semantics.
+
+## Current State (Summary)
+
+- Security type uses a `Picker` in the main form; defaults to `password`.
+- Switching to key auth shows a `Picker` for keys (awkward empty state) and a
+  “Manage Keys” button.
+- Keys can be generated, renamed, deleted, set as default inside a modal.
+- Storage uses a chunked manifest in `expo-secure-store` (works for >2KB values)
+  with metadata (`priority`, `createdAtMs`, `label?`, `isDefault?`).
+- All key mutations go through one `upsertPrivateKey` that invalidates the React
+  Query `keys` query.
+
+## UX Objectives
+
+- Remove the inline key `Picker`; select a key within the Key Manager modal.
+- Replace the security type `Picker` with a simple toggle/switch.
+- Handle the “no keys yet” case gracefully by auto-opening the modal when user
+  chooses key auth.
+- Keep visual styling consistent with text inputs (button heights, paddings,
+  colors).
+- Prepare the modal for importing private keys (paste text or select file) in a
+  future phase.
+
+## Phase 1 — Selection UX + Styling
+
+1. Replace security type `Picker` with toggle
+   - Use a `SwitchField` (styled like inputs) labeled "Use Private Key".
+   - Value mapping: off → password; on → key.
+   - When toggled ON and there is no selected key and no keys exist, auto-open
+     Key Manager.
+
+2. Remove inline key `Picker`
+   - Replace with a read-only field styled like inputs: label: "Private Key",
+     value: current key label or "None".
+   - The field is a `Pressable` that opens the Key Manager for selection.
+   - Disabled state (no keys): show "None" with hint "Open Key Manager to add a
+     key".
+
+3. Key Manager: add selection mode
+   - Add a radio-style control on each row to select the key for this session.
+   - Modal accepts an optional `selectedKeyId` and `onSelect(id)` callback.
+   - When user taps a row (or radio), call `onSelect(id)` and close the modal.
+   - Continue to support generate/rename/delete/set-default; selection should
+     update live.
+
+4. Styling parity
+   - Ensure the read-only "Private Key" field height/padding matches
+     `TextField`.
+   - Use consistent typography/colors for labels, values, and hints.
+
+5. Empty states
+   - If no keys: show a friendly empty state in modal with primary action
+     "Generate New Key" and secondary "Import Key" (Import lands in Phase 2).
+
+Deliverables (Phase 1)
+
+- Update `apps/mobile/src/app/index.tsx` form:
+  - Toggle for auth type.
+  - Read-only key field that opens modal.
+- Update `KeyManagerModal`:
+  - Support selection behavior with visual radio and `onSelect` callback.
+  - Keep existing generate/rename/delete/set-default.
+
+## Phase 2 — Import Keys
+
+1. Import entry points in modal
+   - Add "Import" button/menu in the modal header or as secondary action in
+     empty state.
+   - Options:
+     - Paste PEM text (multiline input)
+     - Pick a file (use `expo-document-picker`)
+
+2. Validation + Storage
+   - Validate PEM or OpenSSH formats; detect supported types
+     (rsa/ecdsa/ed25519/ed448/dsa).
+   - Optional passphrase field (store encrypted if supported by library;
+     otherwise prompt each use — confirm feasibility with SSH lib).
+   - On success, call the single `upsertPrivateKey({ keyId, value, metadata })`
+     and close import flow.
+
+3. UX details
+   - Show parse/validation errors inline.
+   - Set initial `label` from filename (file import) or "Imported Key" (paste);
+     allow editing label on success or selection.
+
+Deliverables (Phase 2)
+
+- Modal import screen(s) with paste/file flows.
+- PEM validation utility and error handling.
+
+## Phase 3 — Data Model + Semantics
+
+1. Default key semantics
+   - Guarantee exactly one default key at most by flipping `isDefault` on upsert
+     when `isDefault === true`.
+   - Consider: separate lightweight persistent "defaultId" in manifest root to
+     avoid iterating all keys on default change.
+
+2. Robustness
+   - Add safety around manifest chunk growth: if
+     `manifestChunkSize + newEntrySize > sizeLimit`, create a new chunk.
+   - Ensure `createdAtMs` is preserved across upserts (done) and add
+     `modifiedAtMs` if useful.
+
+3. Logging + Dev ergonomics
+   - Gate verbose logs behind a debug flag to avoid log spam in production
+     builds.
+
+## Phase 4 — Edge Cases & Polish
+
+- Deleting the currently-selected key: clear selection and show hint to
+  pick/create a new key.
+- Auto-select the default key when switching to key auth and no key is selected.
+- Handle failures from `SecureStore` (permissions/storage full) with
+  user-friendly messages.
+- Handle very large keys with chunking (already supported), surface an error if
+  size exceeds safe thresholds.
+- Accessibility: ensure labels/roles for controls in modal and the selection
+  field.
+
+## Phase 5 — Testing & QA
+
+- Unit test PEM parsing/validation (Phase 2).
+- E2E flows:
+  - First run → toggle to key → auto-open modal → generate key → selected →
+    connect.
+  - Import key by paste/file; rename; set default; delete.
+  - Delete selected key; confirm the form state updates.
+
+## Implementation Notes
+
+- Keep a single write path for keys: `upsertPrivateKey` (invalidates the `keys`
+  query).
+- Prefer modal-based selection with a simple “field-as-button” in the form.
+- For future passphrase support, confirm the SSH library’s API shape and storage
+  expectations.
--- a/docs/ssh-native-module-fix.md
+++ b/docs/ssh-native-module-fix.md
@@ -0,0 +1,245 @@
+# Android SSH Native Module: Crash Root Cause and Fix Plan
+
+This document explains the dev-only crash observed when navigating back
+(unmounting the Shell screen) and disconnecting an SSH session, and details the
+native changes required in `@dylankenneally/react-native-ssh-sftp` to resolve it
+properly.
+
+## Summary
+
+- Symptom: App crashes in development when leaving the Shell screen while an SSH
+  shell is active and `disconnect()` runs. Release builds do not crash.
+- Root cause: The Android native module sometimes invokes the `startShell`
+  callback more than once and lets the shell read loop terminate via an
+  exception. In React Native dev mode, invoking a native callback multiple times
+  can crash the bridge.
+- JS-side mitigations we applied in the app:
+  - Removed the artificial 3s timeout before disconnect; disconnect immediately
+    on unmount.
+  - Replaced the Shell event handler with a no-op on unmount to avoid setState
+    after unmount.
+- Proper native fix (recommended upstream):
+  1. Ensure `startShell` callback is invoked exactly once (on initial start
+     only).
+  2. Make `closeShell` shut down the loop cleanly and null out resources.
+  3. Guard `sendEvent` against emitting when the bridge is inactive.
+  4. Optionally remove clients from the `clientPool` after disconnect.
+
+## Where the issue happens
+
+File:
+`node_modules/@dylankenneally/react-native-ssh-sftp/android/src/main/java/me/keeex/rnssh/RNSshClientModule.java`
+
+Relevant methods:
+
+- `startShell(String key, String ptyType, Callback callback)`
+  - Starts the shell on a background thread, calls `callback.invoke()` when
+    connected, then loops reading lines and emitting `Shell` events.
+  - On stream termination, the code currently exits via exceptions
+    (`IOException`, etc.) and in the catch blocks also calls
+    `callback.invoke(error.getMessage())` a second time.
+
+- `closeShell(String key)`
+  - Closes output stream, input reader, and the channel, but it does not set the
+    fields to `null`. The `startShell` loop uses
+    `while (client._bufferedReader != null && ...)`, which relies on `null` to
+    terminate cleanly.
+
+- `sendEvent(ReactContext reactContext, String eventName, @Nullable WritableMap params)`
+  - Emits DeviceEventManager events without checking whether the React bridge is
+    alive.
+
+- `disconnect(String key)`
+  - Calls `closeShell(key)` and `disconnectSFTP(key)` and then disconnects the
+    session but does not remove the entry from `clientPool`.
+
+## Why dev-only
+
+React Native’s dev bridge is stricter: invoking a native callback multiple times
+can trigger an error that surfaces as an immediate crash during development. In
+release builds, this usually doesn’t bring the app down the same way, which
+matches the behavior observed.
+
+Specifically:
+
+1. `startShell` calls the callback on success (good) but may call the same
+   callback again inside a catch block when the shell loop ends with an
+   exception (bad). That is a “callback invoked twice” scenario.
+
+2. Because `closeShell` does not null
+   `_bufferedReader`/`_dataOutputStream`/`_channel`, the read loop
+   (`while (client._bufferedReader != null && (line = ...))`) doesn’t exit by
+   the `null` check; it exits by throwing `IOException` when the stream is
+   closed, sending control flow to the catch block that re-invokes the callback.
+
+3. Emitting events after the bridge is torn down (e.g., during fast
+   unmount-navigate) can also cause noise in dev logs and compound timing
+   problems.
+
+## Native changes (proposed)
+
+1. Send the `startShell` callback once only
+
+- Current pattern:
+  - On success: `callback.invoke();`
+  - On exceptions: `callback.invoke(error.getMessage());` (second invocation)
+
+- Proposed change:
+  - Keep the success callback invocation.
+  - Remove all subsequent `callback.invoke(...)` inside the catch blocks of
+    `startShell`.
+  - Log the exception with `Log.e(...)` but do not call the callback again. The
+    callback is for “start shell” completion, not for “shell ended later”.
+
+2. Make `closeShell` cleanly terminate the read loop
+
+- After closing resources, set fields to `null`:
+  - `client._channel = null;`
+  - `client._dataOutputStream = null;`
+  - `client._bufferedReader = null;`
+
+- With this, the read loop conditional `client._bufferedReader != null` becomes
+  false and exits without throwing.
+
+3. Harden `sendEvent`
+
+- Before emitting:
+  - `if (reactContext == null || !reactContext.hasActiveCatalystInstance()) { return; }`
+  - Wrap `emit` in a try/catch to prevent rare bridge-state races:
+    ```java
+    try {
+      reactContext
+        .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
+        .emit(eventName, params);
+    } catch (Throwable t) {
+      Log.w(LOGTAG, "Failed to emit event " + eventName, t);
+    }
+    ```
+
+4. Optionally remove clients from the pool after disconnect
+
+- In `disconnect(String key)`, after `client._session.disconnect();`, call
+  `clientPool.remove(key);` so future lookups cannot access stale references.
+
+## Example diffs (illustrative)
+
+Note: Line numbers may vary; these are conceptual patches to apply in the
+indicated methods.
+
+### In `startShell`
+
+```java
+// On success (keep):
+callback.invoke();
+
+// In catch blocks (change):
+} catch (JSchException error) {
+  Log.e(LOGTAG, "Error starting shell: " + error.getMessage());
+  // DO NOT invoke callback again here
+} catch (IOException error) {
+  Log.e(LOGTAG, "Error starting shell: " + error.getMessage());
+  // DO NOT invoke callback again here
+} catch (Exception error) {
+  Log.e(LOGTAG, "Error starting shell: " + error.getMessage());
+  // DO NOT invoke callback again here
+}
+```
+
+### In `closeShell`
+
+```java
+if (client._channel != null) {
+  client._channel.disconnect();
+}
+if (client._dataOutputStream != null) {
+  client._dataOutputStream.flush();
+  client._dataOutputStream.close();
+}
+if (client._bufferedReader != null) {
+  client._bufferedReader.close();
+}
+
+// Ensure the shell loop terminates cleanly:
+client._channel = null;
+client._dataOutputStream = null;
+client._bufferedReader = null;
+```
+
+### In `sendEvent`
+
+```java
+private void sendEvent(ReactContext reactContext, String eventName, @Nullable WritableMap params) {
+  if (reactContext == null || !reactContext.hasActiveCatalystInstance()) {
+    return;
+  }
+  try {
+    reactContext
+      .getJSModule(DeviceEventManagerModule.RCTDeviceEventEmitter.class)
+      .emit(eventName, params);
+  } catch (Throwable t) {
+    Log.w(LOGTAG, "Failed to emit event " + eventName, t);
+  }
+}
+```
+
+### In `disconnect`
+
+```java
+SSHClient client = clientPool.get(key);
+if (client != null) {
+  client._session.disconnect();
+  clientPool.remove(key); // optional but recommended
+}
+```
+
+## Why these changes fix the issue
+
+- Single-callback guarantee: React Native mandates native callbacks are invoked
+  once per request. Making `startShell` callback only for the initial start
+  satisfies this and prevents dev crashes.
+- Clean loop termination: Nulling the stream references ensures the shell loop
+  exits by condition rather than by exception, avoiding the catch path that
+  previously re-invoked the callback.
+- Safe event emission: Avoids emitting into a destroyed bridge during fast
+  navigation/unmount cycles, reducing flakiness in dev.
+- Resource hygiene: Removing client entries and nulling references prevents
+  accidental reuse of stale state and helps GC.
+
+## What we changed in app code (JS)
+
+- In `apps/mobile/src/app/shell.tsx`:
+  - Removed `setTimeout` and disconnect immediately in the unmount cleanup.
+  - Replaced the `Shell` event handler with a no-op in the effect cleanup to
+    avoid setState on an unmounted component while native drains.
+  - We deliberately did not call `closeShell()` ourselves, since the library’s
+    `disconnect()` already handles it.
+
+These app-level changes reduce the timing window and stop React state updates
+after unmount. They help, but the true fix is in the native module as outlined
+above.
+
+## Testing
+
+1. Build a Dev Client (or run a development build) so the native module is
+   loaded.
+2. Connect, navigate to the shell, then press back to unmount while the shell is
+   active.
+3. Capture logs:
+   - `adb logcat --pid=$(adb shell pidof -s dev.fressh.app) RNSSHClient:V ReactNative:V ReactNativeJS:V AndroidRuntime:E *:S`
+4. With the native changes applied, verify:
+   - No “callback invoked twice” or RN bridge callback violations.
+   - No crash in dev.
+   - Events stop cleanly and disconnect completes.
+
+## Upstreaming
+
+These fixes are small, safe, and self-contained. Consider opening a PR to
+`@dylankenneally/react-native-ssh-sftp` with:
+
+- Callback discipline in `startShell`.
+- Clean resource nulling in `closeShell`.
+- Safe `sendEvent`.
+- Optional `clientPool.remove(key)` on disconnect.
+
+Until it’s merged, you can maintain a `patch-package` to keep the project stable
+in development.
--- a/docs/todos.md
+++ b/docs/todos.md
@@ -1,9 +1,30 @@
+# Big TODOS
+
 - real terminal emulation
 - llm command suggestions
 - llm output synthesis
+- better keyboard management
+- import private key

-https://github.com/software-mansion/react-native-executorch
-https://docs.expo.dev/guides/keyboard-handling/
-https://kirillzyusko.github.io/react-native-keyboard-controller/
+### Future reading

-https://reactnativereusables.com/
+#### AI Libraries
+
+- https://github.com/software-mansion/react-native-executorch
+- https://github.com/callstackincubator/ai
+
+#### Keyboard handling
+
+- https://docs.expo.dev/guides/keyboard-handling/
+- https://kirillzyusko.github.io/react-native-keyboard-controller/
+
+#### Styling and components
+
+- https://reactnativereusables.com/ https://www.unistyl.es/
+- https://github.com/Shopify/restyle
+
+#### SSH Conn
+
+- https://github.com/dylankenneally/react-native-ssh-sftp
+- https://xtermjs.org/
+- https://docs.expo.dev/versions/latest/sdk/webview/