-
Notifications
You must be signed in to change notification settings - Fork 67
fix: trim whitespace for secrete name and file paths in UI #628
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
base: main
Are you sure you want to change the base?
fix: trim whitespace for secrete name and file paths in UI #628
Conversation
@srikanth-s2003 can you please sign your commit? Thanks |
…stic duration formatting
9e20ef7
to
5bffc4f
Compare
Hey @srikanth-s2003 , I like your change for the time formatting, can you split this into two PRs please. The implemented trimming does not work for file paths, you can do (partial) manual integration test like this:
also in the original ticket the proposed ideal solution is to do this in the Why it is not working: I will propose changes to the PR that would fix it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll add inline suggestions for the improvements we discussed
front/assets/js/manage_secret.js
Outdated
|
||
attachFilePathTrimmer: function(fileInputId) { | ||
// fileInputId is the id of the visible text input for the file path (e.g. files_0_path) | ||
$("body").on("change blur input", "#" + fileInputId, function() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Remove blur
event - it's redundant since we're already handling change
and input
:
$("body").on("change blur input", "#" + fileInputId, function() { | |
$("body").on("change input", "#" + fileInputId, function() { |
front/assets/js/manage_secret.js
Outdated
valid_name_regex = /^[a-zA-Z_][a-zA-Z0-9_]*$/; | ||
let name = $(this).val(); | ||
// Validate on input and change, but always validate the trimmed value. | ||
$("body").on("change textInput input blur", "#" + selector, function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Remove blur
event here too for consistency:
$("body").on("change textInput input blur", "#" + selector, function () { | |
$("body").on("change textInput input", "#" + selector, function () { |
secretEditor.browseFilesOnClick(fileUploadLink, fileUpload); | ||
secretEditor.formatUploadedFile(fileUpload, fileContent, fileDiv, fileInput, fileDeleteLink, fileMd5PresentId, fileImageId); | ||
secretEditor.activateFilesDeleteLink(fileDeleteLink, fileDiv, fileInput, fileContent, md5Id); | ||
// Attach trimming handler for the file path input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Pass filePathInput
(the actual input field ID) instead of fileInput
(the container):
// Attach trimming handler for the file path input | |
secretEditor.attachFilePathTrimmer(filePathInput); |
secretEditor.browseFilesOnClick(fileUploadLink, fileUpload); | ||
secretEditor.formatUploadedFile(fileUpload, fileContent, fileDiv, fileInput, fileDeleteLink, fileMd5PresentId, fileImageId); | ||
secretEditor.activateFilesDeleteLink(fileDeleteLink, fileDiv, fileInput, fileContent, md5Id); | ||
// Attach trimming handler for newly cloned file path input |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Pass filePathInput
instead of fileInput
:
// Attach trimming handler for newly cloned file path input | |
secretEditor.attachFilePathTrimmer(filePathInput); |
Additional suggestions for variable namingThe inline suggestions above reference 1. Update the return statement in return [element, secretUploadLinkId, secretUploadId, fileContentId, fileDivId, secretInputId, fileDeleteLink, fileMd5Id, fileMd5PresentId, fileImageId, filePathId]; 2. Update destructuring in let [element, fileUploadLink, fileUpload, fileContent, fileDiv, fileInputContainer, fileDeleteLink, md5Id, fileMd5PresentId, fileImageId, filePathInput] =
secretEditor.fileElement(file, index); 3. Update destructuring in let [element, fileUploadLink, fileUpload, fileContent, fileDiv, fileInputContainer, fileDeleteLink, md5Id, fileMd5PresentId, fileImageId, filePathInput] =
secretEditor.fileElement(file, fileIndex); Then use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LMK if you have trouble testing this or if you need further some further help.
…redundant blur events - Pass correct filePathInput ID to attachFilePathTrimmer instead of container ID - Remove redundant blur events from both env var and file path handlers - Update variable destructuring to match fileElement return values - File path trimming now works correctly for both existing and newly added files
Moved duration formatter fix to separate PR as requested by reviewer. This PR now focuses solely on secret name and file path trimming functionality.
Summary
Trim leading and trailing whitespace for secret names and secret file paths in the front-end editor so accidental spaces no longer end up in secret names or paths. Also made the Formatter.duration implementation deterministic (arithmetic-based) to avoid timezone-dependent formatting issues that caused tests to fail.
This is a UI-side improvement that reduces the chance of invisible whitespace causing CI/CD failures. The canonical fix should also trim on the server (secrethub) to cover API/CLI — see "Next steps" below.
What I changed
front/assets/js/manage_secret.js
front/assets/js/manage_secret.js
front/assets/js/manage_secret.js
valid_name_regex
withconst
.front/assets/js/manage_secret.js
moment
formatting inFormatter.duration
with arithmetic-based formatting so tests are deterministic.front/assets/js/insights/util/formatter.ts
Motivation / context
Issue: Automatic Whitespace Trimming for Secret Names (#502)
Users can accidentally add leading/trailing whitespace to secret names; because that whitespace is invisible in many UIs, it can silently break CI/CD usage. Trimming in the UI avoids many accidental cases; however, server-side trimming is necessary for a complete fix across UI/CLI/API.
How I tested
front/assets
(ESLint warnings only; no errors).npm ci && npm test
infront/assets
.Formatter.duration
test had previously failed due to timezone-dependent logic; the new arithmetic-based implementation fixes that.How to review
front/assets/js/manage_secret.js
for trimming/validation logic.front/assets/js/insights/util/formatter.ts
for the deterministic duration implementation.✅ Checklist