Skip to content

fix: enhance TextField component to handle sensitive configuration #4418

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

Merged
merged 13 commits into from
Apr 12, 2025

Conversation

tuantran0910
Copy link
Contributor

@tuantran0910 tuantran0910 commented Apr 5, 2025

Closes #4417

📑 Description

In the provider configuration UI, sensitive fields such as bot_token (e.g., in the Telegram provider) are correctly rendered with type="password" to hide their values. However, the actual token value is still visible in the DOM (e.g., through browser dev tools), defeating the purpose of marking it as sensitive.

Before:

image

After:

image

NOTE: One drawback is that you can no longer view the previously hidden secret configuration in the installed provider.

image

✅ Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

Copy link

vercel bot commented Apr 5, 2025

@tuantran0910 is attempting to deploy a commit to the KeepHQ Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot bot added size:S This PR changes 10-29 lines, ignoring generated files. Enhancement New feature or request UI User interface related issues labels Apr 5, 2025
@shahargl
Copy link
Member

shahargl commented Apr 7, 2025

@tuantran0910 let's make it optional with some environment variable? I don't want to change the default behaviour

@shahargl shahargl self-requested a review April 7, 2025 07:12
Copy link
Member

@shahargl shahargl left a comment

Choose a reason for hiding this comment

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

see comment

@tuantran0910
Copy link
Contributor Author

Hi @shahargl, I have added an environment variable to hide sensitive field. You have a check at my commits: 65dc642

@tuantran0910 tuantran0910 requested a review from shahargl April 7, 2025 07:59
@shahargl
Copy link
Member

shahargl commented Apr 7, 2025

@tuantran0910 yea so in Keep we do not use NEXT_PUBLIC_ env vars since they are baked in build time and not in run time. What you'll need to do is to add it as KEEP_HIDE_SENSITIVE_FIELDS and add it in useConfig and getConfig

@dosubot dosubot bot added size:M This PR changes 30-99 lines, ignoring generated files. and removed size:S This PR changes 10-29 lines, ignoring generated files. labels Apr 7, 2025
@tuantran0910
Copy link
Contributor Author

Hi @shahargl, can you review this again ? I have modified the environment variable name, also added it in useConfig and getConfig.

@tuantran0910
Copy link
Contributor Author

tuantran0910 commented Apr 7, 2025

Sorry, seems that it is still not working, which made the test failed, will resolve asap.

@tuantran0910 tuantran0910 marked this pull request as draft April 7, 2025 10:35
@tuantran0910 tuantran0910 marked this pull request as ready for review April 7, 2025 10:39
@tuantran0910
Copy link
Contributor Author

It's fine now :D

Copy link
Member

@shahargl shahargl left a comment

Choose a reason for hiding this comment

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

lgtm

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Apr 12, 2025
Copy link

vercel bot commented Apr 12, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
keep ✅ Ready (Inspect) Visit Preview 💬 Add feedback Apr 12, 2025 10:16am

@shahargl shahargl merged commit 6b64ab2 into keephq:main Apr 12, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request lgtm This PR has been approved by a maintainer size:M This PR changes 30-99 lines, ignoring generated files. UI User interface related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[🐛 Bug]: Sensitive Input Field Displays Real Value in DOM
2 participants