feat: make sensitive fields secret-picker-only#20
Conversation
Greptile SummaryThis PR removes the plaintext password Key changes:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[SecretPickerInput renders] --> B{value is SECRET ref?}
B -- Yes --> C[State 1: Badge + Clear button]
B -- No --> D{value is non-empty plaintext?}
D -- Yes --> E[State 2: Destructive warning badge + Clear button]
D -- No --> F[State 3: Empty picker button]
E --> G{environmentId set?}
F --> G
G -- No --> H[Fallback text message]
G -- Yes --> I[Popover trigger rendered]
I --> J{Popover opened?}
J -- Yes --> K[Fetch secret.list for environment]
K --> L{showCreate?}
L -- No --> M[List existing secrets + Create new secret button]
L -- Yes --> N[Inline create form: Name + Value inputs]
M -- Select existing --> O[onChange: SECRET ref • close popover]
M -- Click Create new secret --> L
N -- Click Create & Use --> P[secret.create mutation]
P -- onSuccess --> Q[Invalidate secret.list cache]
Q --> R[onChange: new SECRET ref • close popover • toast]
N -- Cancel --> M
Last reviewed commit: 67b725a |
| <Label className="text-xs">Value</Label> | ||
| <Input | ||
| type="password" | ||
| autoComplete="off" |
There was a problem hiding this comment.
autoComplete="off" is ignored by modern browsers on password fields.
The PR description explicitly states this was added to prevent password manager capture, but Chrome, Firefox, and Safari intentionally ignore autoComplete="off" on type="password" inputs per the HTML spec, as a user-protection measure against being locked out of their own accounts.
autoComplete="new-password" is the widely-accepted signal that tells browsers this is a new secret being entered (not a login form), which suppresses autofill from saved credentials:
| autoComplete="off" | |
| type="password" | |
| autoComplete="new-password" | |
| value={newValue} |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/config-forms/secret-picker-input.tsx
Line: 171
Comment:
`autoComplete="off"` is ignored by modern browsers on password fields.
The PR description explicitly states this was added to prevent password manager capture, but Chrome, Firefox, and Safari intentionally ignore `autoComplete="off"` on `type="password"` inputs per the HTML spec, as a user-protection measure against being locked out of their own accounts.
`autoComplete="new-password"` is the widely-accepted signal that tells browsers this is a *new* secret being entered (not a login form), which suppresses autofill from saved credentials:
```suggestion
type="password"
autoComplete="new-password"
value={newValue}
```
How can I resolve this? If you propose a fix, please make it concise.Browsers commonly ignore autoComplete="off" on password fields. "new-password" is more reliably respected to prevent autofill.
| createMutation.mutate({ | ||
| environmentId, | ||
| name: newName.trim(), | ||
| value: newValue.trim(), |
There was a problem hiding this comment.
Silent whitespace stripping on secret value
newValue.trim() is called before sending the secret value to the server. While accidental leading/trailing whitespace is common in pasted API keys, there are secret formats (e.g. some base64-padded tokens or generated passwords) where the stored value must be byte-exact. Silently trimming means a user who pastes such a value will get a secret that doesn't work, with no indication of why.
The name field trimming (same line) is unambiguously correct — names should never have surrounding whitespace. The value trim is the debatable one.
Consider trimming name only, or at minimum alerting the user if trimming actually changed their input:
| value: newValue.trim(), | |
| name: newName.trim(), | |
| value: newValue, |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/config-forms/secret-picker-input.tsx
Line: 77
Comment:
**Silent whitespace stripping on secret value**
`newValue.trim()` is called before sending the secret value to the server. While accidental leading/trailing whitespace is common in pasted API keys, there are secret formats (e.g. some base64-padded tokens or generated passwords) where the stored value must be byte-exact. Silently trimming means a user who pastes such a value will get a secret that doesn't work, with no indication of why.
The `name` field trimming (same line) is unambiguously correct — names should never have surrounding whitespace. The value trim is the debatable one.
Consider trimming `name` only, or at minimum alerting the user if trimming actually changed their input:
```suggestion
name: newName.trim(),
value: newValue,
```
How can I resolve this? If you propose a fix, please make it concise.| {secrets.length === 0 && !secretsQuery.isLoading ? ( | ||
| <p className="p-3 text-xs text-muted-foreground text-center"> | ||
| No secrets yet | ||
| </p> | ||
| ) : secretsQuery.isLoading ? ( | ||
| <p className="p-3 text-xs text-muted-foreground text-center"> | ||
| Loading... | ||
| </p> |
There was a problem hiding this comment.
Brief "No secrets yet" flash on first open
The condition order can show "No secrets yet" for a frame before "Loading..." when the popover first opens. When popoverOpen flips to true, the query's enabled flag transitions from false → true. In the same render where PopoverContent appears, secretsQuery.isLoading may still be false (the fetch hasn't started yet), so secrets.length === 0 && !secretsQuery.isLoading evaluates to true and "No secrets yet" briefly flashes before the loading state kicks in.
A more robust guard is to also check whether the query is enabled but hasn't yet returned data (secretsQuery.isPending):
| {secrets.length === 0 && !secretsQuery.isLoading ? ( | |
| <p className="p-3 text-xs text-muted-foreground text-center"> | |
| No secrets yet | |
| </p> | |
| ) : secretsQuery.isLoading ? ( | |
| <p className="p-3 text-xs text-muted-foreground text-center"> | |
| Loading... | |
| </p> | |
| {secretsQuery.isPending ? ( | |
| <p className="p-3 text-xs text-muted-foreground text-center"> | |
| Loading... | |
| </p> | |
| ) : secrets.length === 0 ? ( | |
| <p className="p-3 text-xs text-muted-foreground text-center"> | |
| No secrets yet | |
| </p> | |
| ) : ( |
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/components/config-forms/secret-picker-input.tsx
Line: 130-137
Comment:
**Brief "No secrets yet" flash on first open**
The condition order can show "No secrets yet" for a frame before "Loading..." when the popover first opens. When `popoverOpen` flips to `true`, the query's `enabled` flag transitions from `false` → `true`. In the same render where `PopoverContent` appears, `secretsQuery.isLoading` may still be `false` (the fetch hasn't started yet), so `secrets.length === 0 && !secretsQuery.isLoading` evaluates to `true` and "No secrets yet" briefly flashes before the loading state kicks in.
A more robust guard is to also check whether the query is enabled but hasn't yet returned data (`secretsQuery.isPending`):
```suggestion
{secretsQuery.isPending ? (
<p className="p-3 text-xs text-muted-foreground text-center">
Loading...
</p>
) : secrets.length === 0 ? (
<p className="p-3 text-xs text-muted-foreground text-center">
No secrets yet
</p>
) : (
```
How can I resolve this? If you propose a fix, please make it concise.
Summary
SecretPickerInput— sensitive fields can only be set via SECRET[] referencesautoComplete="off"on secret value input to prevent browser password manager captureTest Plan