Add SSH TOTP authentication support #350
Reference in New Issue
Block a user
Delete Branch "feature-ssh-totp"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Test Plan
Summary of Changes
Hello @ZacharyZcR, I'm Gemini Code Assist1 ! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request significantly enhances the security of SSH connections by integrating Time-based One-Time Password (TOTP) authentication. It provides a seamless user experience for entering TOTP codes during SSH login for both the Terminal and File Manager, leveraging keyboard-interactive authentication. The changes also ensure that the Server Stats feature correctly identifies and handles servers configured with TOTP, preventing connection issues and informing users about the incompatibility with automatic data polling.
Highlights
TOTPDialog) to handle user input for TOTP verification codes.Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point by creating a comment using either
/gemini <command>or@gemini-code-assist <command>. Below is a summary of the supported commands on the current page./gemini review/gemini summary/gemini helpCustomization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a
.gemini/folder in the base of the repository. Detailed instructions can be found here.Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩︎
Code Review
This pull request introduces support for SSH TOTP (Time-based One-Time Password) authentication for both the Terminal and File Manager features. It adds a reusable
TOTPDialogcomponent for entering verification codes and modifies the backend SSH connection logic to handle the keyboard-interactive authentication flow required for TOTP. The changes are well-structured, but there are opportunities for improvement in error handling, type safety, and code duplication. I've identified a potential memory leak in how pending TOTP sessions are managed and a bug in the server-stats module's handling of keyboard-interactive prompts. Additionally, some refactoring could make the code more maintainable.@@ -94,7 +94,16 @@ interface SSHSession {timeout?: NodeJS.Timeout;For better type safety and maintainability, the
configproperty should be typed more specifically thanany. TheConnectConfigtype from thessh2library would be appropriate here.@@ -97,1 +103,4 @@}const sshSessions: Record<string, SSHSession> = {};const pendingTOTPSessions: Record<string, PendingTOTPSession> = {};The
pendingTOTPSessionsobject stores sessions awaiting TOTP verification. If a client initiates a connection but never completes the TOTP step, the corresponding entry in this object is never removed, leading to a memory leak. Consider implementing a periodic cleanup mechanism to remove stale entries frompendingTOTPSessions.@@ -370,0 +482,4 @@fileLogger.success("TOTP verification successful", {operation: "file_totp_verify",sessionId,userId,The timeout value
120000is a magic number. It's better to define it as a named constant at the top of the file for better readability and easier maintenance (e.g.,const PENDING_TOTP_SESSION_TIMEOUT_MS = 120 * 1000;).@@ -370,0 +485,4 @@userId,});res.json({ status: "success", message: "TOTP verified, SSH connection established" });Swallowing errors in an empty
catchblock can hide potential issues and make debugging difficult. It's better to at least log the error, even if it's just a warning.The timeout value
60000is a magic number. It's better to define it as a named constant for better readability and easier maintenance (e.g.,const TOTP_VERIFICATION_TIMEOUT_MS = 60 * 1000;).@@ -95,6 +95,40 @@ class SSHConnectionPool {reject(err);Calling
finish([])sends an empty array of responses. If the server expects a response for each prompt (even an empty string), this will likely cause authentication to fail. To ensure compatibility, you should provide one empty string response for each prompt.This
ifcondition is redundant becausethrow error;is executed regardless of whether the condition is met. Thecatchblock can be simplified by removing the conditional check.This regular expression for detecting TOTP prompts is duplicated across multiple files (
file-manager.ts,server-stats.ts). To improve maintainability and ensure consistency, consider extracting it into a shared constant in a common utility file.There is code duplication in this
catchblock. The calls tosetMetrics(null)andsetShowStatsUI(false)are repeated in every conditional branch. You can refactor this to set these states once after the conditional logic, making the code cleaner and easier to maintain.@@ -0,0 +42,4 @@if (input && input.value.trim()) {onSubmit(input.value.trim());}}}The form submission logic directly accesses the form element from the event to get the input value. A more idiomatic React approach is to use a controlled component by managing the input's value with
useState. This makes the component's state more predictable and easier to manage.Example:
Attaching the
responseobject usingas anybypasses TypeScript's type checking. For better type safety, consider adding an optionalresponseproperty to theApiErrorclass definition. This would make the error structure more explicit and easier to work with.