🛡️ Sentinel: [MEDIUM] Fix UI credential exposure#84
Conversation
Replaced `type="text"` with `type="password"` and added `autoComplete="off"` to inputs handling sensitive credentials (C411 API Key, C411 Passkey, and TMDB API Key) across `UploadTrackersManagerPanel.tsx`, `UploadTrackerPanel.tsx`, and `TmdbConfig.tsx`. This prevents shoulder-surfing and stops browsers from auto-saving or leaking these secrets via autofill. Co-authored-by: bobdivx <6737167+bobdivx@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances security by converting sensitive credential inputs (API keys and passkeys) to password fields and adding autocomplete prevention across several settings components. It also updates the security log to document these changes. Review feedback identifies a bug in a focus handler where the incorrect state variable was being reset and recommends using autoComplete="new-password" instead of "off" for better browser compatibility. Additionally, a suggestion was made to maintain reverse-chronological order in the security log file.
| <input | ||
| type="text" | ||
| type="password" | ||
| autoComplete="off" |
There was a problem hiding this comment.
In addition to using new-password, please note that there is a bug in the onFocus handler for this specific input at line 292 (just outside this diff hunk). It incorrectly calls setApiKey('') instead of setPasskey(''), which will clear the wrong state when the passkey field is focused.
| autoComplete="off" | |
| autoComplete="new-password" |
| **Vulnerability:** Weak PRNG `Math.random()` was used for generating IDs and UUIDs across multiple files (`src/lib/client/server-api/indexers.ts`, `src/api-routes-backup/v1/setup/indexers.ts`, `src/lib/utils/device-id.ts`, `src/components/torrents/MediaDetailPage/hooks/useNotifications.ts`). | ||
| **Learning:** `Math.random()` is not cryptographically secure and shouldn't be used for IDs, especially not for indexer setups and device IDs. Moreover, calling `globalThis.crypto.randomUUID()` directly fails on HTTP non-localhost sites since it requires a Secure Context. | ||
| **Prevention:** Always use the Web Crypto API (`crypto.getRandomValues()` or `crypto.randomUUID()`) through a centralized utility like `src/lib/utils/uuid.ts` that provides safe fallbacks for unsupported environments and non-secure contexts. | ||
| ## 2024-05-24 - Secure Credential Inputs in Settings Panels |
There was a problem hiding this comment.
The log entries in this file appear to follow a reverse-chronological order (newest at the top). The new entry for 2024-05-24 should be placed before the 2024-05-18 entry to maintain consistency.
| ## 2024-05-24 - Secure Credential Inputs in Settings Panels | ||
| **Vulnerability:** Several inputs handling sensitive credentials (C411 API Key, C411 Passkey, and TMDB API Key) were using standard `type="text"` input fields across multiple React components (`UploadTrackersManagerPanel.tsx`, `UploadTrackerPanel.tsx`, and `TmdbConfig.tsx`). | ||
| **Learning:** Using `type="text"` for credentials exposes the raw secret on the user's screen (shoulder-surfing risk) and allows browsers to store and auto-suggest the values in the future via their built-in autofill mechanisms, creating a client-side leakage risk. | ||
| **Prevention:** Always use `type="password"` combined with `autoComplete="off"` when rendering inputs that collect or display sensitive user credentials, API keys, or passkeys, even when they are part of a general configuration panel and not a traditional login form. |
There was a problem hiding this comment.
While autoComplete="off" is a good start, autoComplete="new-password" is often more effective for sensitive configuration fields. Modern browsers sometimes ignore off for password fields to ensure password managers can function, whereas new-password explicitly signals that the browser should not attempt to autofill the field with existing site credentials.
| **Prevention:** Always use `type="password"` combined with `autoComplete="off"` when rendering inputs that collect or display sensitive user credentials, API keys, or passkeys, even when they are part of a general configuration panel and not a traditional login form. | |
| **Prevention:** Always use `type="password"` combined with `autoComplete="new-password"` when rendering inputs that collect or display sensitive user credentials, API keys, or passkeys, even when they are part of a general configuration panel and not a traditional login form. |
| <input | ||
| type="text" | ||
| type="password" | ||
| autoComplete="off" |
There was a problem hiding this comment.
For sensitive API keys in configuration panels, autoComplete="new-password" is generally preferred over autoComplete="off". This prevents browsers from incorrectly associating this field with the user's account password for the application and attempting to autofill it.
| autoComplete="off" | |
| autoComplete="new-password" |
| <input | ||
| type="text" | ||
| type="password" | ||
| autoComplete="off" |
There was a problem hiding this comment.
Using autoComplete="new-password" is more reliable than off for preventing unwanted browser autofill on sensitive configuration inputs.
| autoComplete="off" | |
| autoComplete="new-password" |
| <input | ||
| type="text" | ||
| type="password" | ||
| autoComplete="off" |
There was a problem hiding this comment.
Suggesting new-password to better prevent browser autofill interference.
| autoComplete="off" | |
| autoComplete="new-password" |
1 participant
🚨 Severity: MEDIUM
💡 Vulnerability: Several configuration inputs handling sensitive secrets (C411 API Key, C411 Passkey, and TMDB API Key) were using standard
type="text"fields.🎯 Impact: This exposed the raw secrets on the user's screen (shoulder-surfing risk) and allowed the browser to store and auto-suggest the values in the future via built-in autofill mechanisms, creating a client-side leakage risk.
🔧 Fix: Modified the affected inputs across
UploadTrackersManagerPanel.tsx,UploadTrackerPanel.tsx, andTmdbConfig.tsxto usetype="password"and addedautoComplete="off".✅ Verification: Verified the codebase still builds and tests pass. Visually, the inputs will now correctly mask the characters.
PR created automatically by Jules for task 16817019003414971376 started by @bobdivx