Skip to content

feat(secrets-overview): re-vamp create secret form#5611

Merged
scott-ray-wilson merged 13 commits intomainfrom
SECRETS-34
Mar 6, 2026
Merged

feat(secrets-overview): re-vamp create secret form#5611
scott-ray-wilson merged 13 commits intomainfrom
SECRETS-34

Conversation

@scott-ray-wilson
Copy link
Contributor

Context

This PR updates the create secret form with v3 components, full secret property editing and the ability to create more secrets to keep the sheet open.

PR includes new v3 components

Screenshots

CleanShot 2026-03-05 at 18 47 26@2x

Steps to verify the change

  • test create secret form fields, validation, sheet behavior, create more behavior and error handling
  • test new components, with special emphasis on v3 secret input / infisical secret input (references, text wrapping, etc.)

Type

  • Fix
  • Feature
  • Improvement
  • Breaking
  • Docs
  • Chore

Checklist

  • Title follows the conventional commit format: type(scope): short description (scope is optional, e.g., fix: prevent crash on sync or fix(api): handle null response).
  • Tested locally
  • Updated docs (if needed)
  • Read the contributing guide

@linear
Copy link

linear bot commented Mar 6, 2026

@maidul98
Copy link
Collaborator

maidul98 commented Mar 6, 2026

Snyk checks have passed. No issues have been found so far.

Status Scanner Critical High Medium Low Total (0)
Open Source Security 0 0 0 0 0 issues

💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse.

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Mar 6, 2026

Greptile Summary

This PR re-vamps the create secret form in the Secrets Overview page with updated v3 components, adds full secret property editing (tags, metadata, skip multiline encoding), and introduces a "Create More" mode to keep the sheet open after creation. Several new v3 components are introduced: SecretInput, InfisicalSecretInput, PasswordGenerator, and supporting Sheet/Input updates.

Key issues found:

  • Cryptographically weak password generation: PasswordGenerator uses Math.random() to generate passwords, which is not a CSPRNG. An attacker who can observe or predict timing could reproduce generated passwords. crypto.getRandomValues() should be used instead.
  • Native regex without RE2: SecretInput.tsx uses a native JS regex against user-controlled secret values, violating the project's RE2 security requirement. The pattern should be validated at https://devina.io/redos-checker and migrated to the re2 package.
  • Inverted skipMultilineEncoding label: The toggle is labelled "Enable Multiline Encoding" but the underlying field (skipMultilineEncoding) means the opposite — when the switch is ON the value is true and encoding is skipped. This is a direct UX/logic mismatch that will confuse users.
  • Silent partial failures: When creating a secret across multiple environments, individual environment failures are swallowed unless all environments fail. The user receives no feedback about which environments failed.
  • Incomplete form reset in "Create More" mode: Only key, value, and comment are cleared between creations; metadata entries carry over silently to the next secret.

Confidence Score: 2/5

  • Not safe to merge as-is — the password generator uses a non-cryptographic RNG to produce values that will be stored as secrets, and the skipMultilineEncoding label is semantically inverted.
  • Two blocking issues: (1) Math.random() in a password generator for secret values is a meaningful security regression, and (2) the inverted skipMultilineEncoding label will actively mislead users about secret encoding behavior. The native-regex RE2 violation and partial-failure handling are lower severity but still need attention before this ships broadly.
  • frontend/src/components/v3/generic/PasswordGenerator/PasswordGenerator.tsx (insecure RNG) and frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx (inverted label, partial-failure handling, incomplete reset).

Important Files Changed

Filename Overview
frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx Re-vamped create secret form with v3 components, multi-environment creation, tags, metadata, and "Create More" mode. Contains an inverted skipMultilineEncoding label, silent partial-failure handling across environments, and incomplete field reset in "Create More" mode.
frontend/src/components/v3/generic/PasswordGenerator/PasswordGenerator.tsx New v3 PasswordGenerator component using Math.random() for password generation — not cryptographically secure. Should use crypto.getRandomValues() instead.
frontend/src/components/v3/generic/SecretInput/SecretInput.tsx New v3 SecretInput component with syntax highlighting for secret references. Uses a native JS regex against user-controlled secret values, violating the project's RE2 requirement.
frontend/src/components/v3/generic/SecretInput/InfisicalSecretInput.tsx New v3 InfisicalSecretInput with autocomplete for secret references. Permission checks are performed before navigation. No major issues found.
frontend/src/hooks/api/secrets/mutations.tsx Added secretMetadata to the useCreateSecretV3 mutation payload. Clean, consistent with the update mutation pattern.
frontend/src/hooks/api/secrets/types.ts Added secretMetadata field to TCreateSecretsV3DTO. Consistent with existing update DTO. No issues found.

Comments Outside Diff (2)

  1. frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx, line 209-222 (link)

    Silent partial failures swallowed when creating across multiple environments

    Promise.allSettled is used to fan out the create call across all selected environments. The notification logic only shows an error when all environments fail (!updatedEnvs.length && !forApprovalEnvs.length). If, say, 3 environments are selected and 1 returns a rejected promise, the user only sees the success notification for the 2 that succeeded — the failure on the third is silently discarded.

    Consider tracking and surfacing rejected results:

    const failedEnvs = results
      .map((result, i) => (result.status === "rejected" ? selectedEnv[i].name : undefined))
      .filter(Boolean) as string[];
    
    if (failedEnvs.length) {
      createNotification({
        type: "error",
        text: `Failed to create secret in: ${failedEnvs.join(", ")}`
      });
    }
    
  2. frontend/src/pages/secret-manager/OverviewPage/components/CreateSecretForm/CreateSecretForm.tsx, line 214-222 (link)

    Metadata and tags not cleared on "Create More"

    When createMore is true, only key, value, and comment are reset. metadata, tags, and skipMultilineEncoding carry over to the next secret. While preserving environments/tags across "Create More" entries may be intentional, carrying over metadata entries (especially encrypted ones set per-secret) is likely unintentional and could cause users to accidentally attach the same metadata to every subsequent secret.

    Consider at least resetting metadata:

    setValue("metadata", []);
    

    If carrying over tags is intentional, a comment explaining the design decision would help future maintainers.

Last reviewed commit: 17b3518

Copy link
Contributor

@victorvhs017 victorvhs017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The input placeholder is showing in the select list

Image

Copy link
Contributor

@victorvhs017 victorvhs017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's showing only the key as required, but env is also required, or I get an error.

Image

Copy link
Contributor

@victorvhs017 victorvhs017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to double-click on the field after closing the multiselect, more on the video.

Also happens with the tag multiselect.

Screenshare.-.2026-03-06.5_28_33.PM.mp4

Copy link
Contributor

@victorvhs017 victorvhs017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we trim this field, we should only show the warning message after the user adds something after the whitespace

Image

Copy link
Contributor

@victorvhs017 victorvhs017 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All tested, looking great!

@scott-ray-wilson scott-ray-wilson merged commit c8007f8 into main Mar 6, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants