Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(platform): Ability to reveal value of secret #825

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Souvik9205
Copy link
Contributor

@Souvik9205 Souvik9205 commented Mar 13, 2025

User description

Description

Previously decryptValue=true automatically decrypted the values if the private key is present, but now to reveal a secret must use reveal icon to see the secret value

Fixes #773

Screenshots of relevant screens

Add screenshots of relevant screens

Screenshot from 2025-03-13 10-34-42
Screenshot from 2025-03-13 10-34-35
Screenshot from 2025-03-13 10-34-17

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

PR Type

Enhancement


Description

  • Added functionality to reveal or hide secret values conditionally.

  • Integrated new icons (EyeOpen, EyeSlash, InfoSVG) for UI enhancements.

  • Updated tooltip component to include an arrow for better UX.

  • Introduced a new atom state (revealSecretOpenAtom) for managing secret reveal state.


Changes walkthrough 📝

Relevant files
Enhancement
index.ts
Added new SVG icons for secret reveal functionality           

apps/platform/public/svg/shared/index.ts

  • Added new SVG icons: EyeOpen, EyeSlash, and InfoSVG.
  • Updated export list to include the new icons.
  • +7/-1     
    index.tsx
    Enhanced secret card component with reveal functionality 

    apps/platform/src/components/dashboard/secret/secretCard/index.tsx

  • Implemented conditional secret value reveal logic.
  • Added buttons for revealing or hiding secrets with appropriate icons.
  • Integrated tooltips for better user guidance.
  • Updated state management to handle secret reveal functionality.
  • +61/-15 
    tooltip.tsx
    Enhanced tooltip component with arrow support                       

    apps/platform/src/components/ui/tooltip.tsx

  • Added TooltipArrow component for improved tooltip design.
  • Updated tooltip exports to include the new arrow component.
  • +9/-1     
    index.ts
    Added new atom for secret reveal state management               

    apps/platform/src/store/index.ts

  • Introduced revealSecretOpenAtom to manage the state of secret reveal
    functionality.
  • +1/-0     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    773 - Partially compliant

    Compliant requirements:

    • Secrets are shown in hidden format by default
    • Auto decryption while fetching secrets has been removed
    • UI changes from Figma have been implemented (eye icons, tooltips)
    • Secrets can be revealed conditionally based on private key availability

    Non-compliant requirements:

    • No evidence of adding the decryption utility function under lib in platform

    Requires further human verification:

    • Verify that the decryption is actually happening client-side as required
    • Confirm that the implementation correctly checks both conditions for revealing secrets (project storing private key or key set locally in browser context)
    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Decryption Logic

    The PR implements UI for revealing secrets but doesn't show where the actual decryption happens. The ticket mentions adding a decryption utility function under lib in platform, but this doesn't appear in the PR.

    {isDecrypted &&
    IsSecretRevealed &&
    value.environment.slug === selectedSecretEnvironment
      ? value.value
      : value.value.replace(/./g, '*').substring(0, 20)}
    Conditional Checks

    The implementation only checks if isDecrypted is true to show the reveal button, but doesn't implement the specific conditions mentioned in the ticket (project storing private key or key set locally).

    {isDecrypted ? (
      <button
        className="duration-300 hover:scale-105"
        onClick={() =>
          handleRevealEnvironmentValueOfSecretClick(
            value.environment.slug
          )
        }
        type="button"
      >
        {!IsSecretRevealed ? <EyeOpen /> : <EyeSlash />}
      </button>

    Copy link
    Contributor

    codiumai-pr-agent-free bot commented Mar 13, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Security
    Fix secret reveal logic

    The current implementation toggles the reveal state globally for all secrets
    when clicking on any eye icon. This could lead to unintentionally revealing
    multiple secrets at once. Modify the logic to only toggle the specific
    secret-environment combination.

    apps/platform/src/components/dashboard/secret/secretCard/index.tsx [74-78]

     const handleRevealEnvironmentValueOfSecretClick = (environment: string) => {
       setSelectedSecret(secretData)
    -  setSelectedSecretEnvironment(environment)
    -  setIsSecretRevealed(!IsSecretRevealed)
    +  if (selectedSecretEnvironment === environment && IsSecretRevealed) {
    +    setIsSecretRevealed(false)
    +  } else {
    +    setSelectedSecretEnvironment(environment)
    +    setIsSecretRevealed(true)
    +  }
     }
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    __

    Why: This suggestion addresses a significant security issue where the current implementation could reveal multiple secrets simultaneously. The improved code ensures only the specific clicked secret-environment combination is revealed, preventing unintended exposure of sensitive information.

    High
    General
    Show full masked value

    The current code only shows the secret value when the specific environment is
    selected, but the substring(0, 20) on the masked value could truncate longer
    secrets. This creates inconsistency when toggling between revealed and masked
    states. Remove the substring limitation to show the full masked value.

    apps/platform/src/components/dashboard/secret/secretCard/index.tsx [169-173]

     {isDecrypted &&
     IsSecretRevealed &&
     value.environment.slug === selectedSecretEnvironment
       ? value.value
    -  : value.value.replace(/./g, '*').substring(0, 20)}
    +  : value.value.replace(/./g, '*')}
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: This suggestion improves consistency by showing the full masked value instead of truncating it to 20 characters. This ensures users see the same length of content when toggling between revealed and masked states, providing a better user experience.

    Medium
    • Update

    Copy link
    Member

    Choose a reason for hiding this comment

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

    i think we can manage the revealed state in a useState instead of useAtom. You can make the changes

    Copy link
    Member

    Choose a reason for hiding this comment

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

    also, i belive that you would need somthing like a Map<Secret['slug'], boolean> type to store the state of revealing.

    @@ -155,24 +166,59 @@ export default function SecretCard({
    {value.environment.name}
    </TableCell>
    <TableCell className="h-full text-base">
    {isDecrypted
    {isDecrypted &&
    Copy link
    Member

    Choose a reason for hiding this comment

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

    image there should be a slider kind of a thing to the left of add secret that says "Retrieve as decrypted". If this option is set to true, we will pass `isDecrypted=true` in the API calls. In this case, we won;t need to show the eye icons because by default everything will be decrypted.

    If this slider is not turned on, then everything will come as encrypted and isDecrypted=false would be sent to the backend. We then would like to show the eye icons. Also, when we "reveal" a secret using this eye icon, we would like to decrypt it on the frontend. There are 3 cases for this:

    • the project stores the private key in db: you can check if storePrivateKey of selectedProject is set to true. If yes, you can use selectedProject.privateKey to decrypt the secret
    • the project stores the key in browser: if selectedProject.storePrivateKey is false, and you can find the key in the store (localstore.get('project_name_pk')), then you can decrypt using this private key
    • the key isnt stored: if none of the above cases are satisfied, you would need to show the "can not decrypt" tooltip

    Copy the decrypt.ts file from apps/cli into apps/platform/src/lib to perform the decryption operations

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    PLATFORM: Ability to reveal value of secret
    2 participants