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

fix: Fix name validation consistency #349

Open
wants to merge 17 commits into
base: main
Choose a base branch
from

Conversation

Its-treason
Copy link
Member

@Its-treason Its-treason commented Oct 4, 2023

Edit: Updated the PR, please read next comment

With this PR I want to fix validation problems when creating folder, requests or environments. All names now use a validation regex:

/^(?!CON|PRN|AUX|NUL|COM\d|LPT\d|^ |^\-)[\w\-\. \(\)\[\]]+[^\. ]$/

This regex ensures various rules when naming files on the file system, so users don't run into weird problems:

  • Only allowing certain characters: a-z, A-Z, 0-9, _, -, ., (, ), [, ] and (Space) to ensure file name compatibility between operating and file systems
  • Disallowing special cases, e.g., Windows file names must not end with a .
  • Disallowing Windows Device names e.g., CON, NUL

You can test this regex on regexr: https://regexr.com/7l552

For reference for all limitations, I used:

I also removed the is-valid-path/isValidPathname check from the collection creation as this check was very strict and would not pass with names containing common special characters and a failed checked would only fail with an “An error occurred” toast. This should resolve #174


For the future we should make it, so all characters are allowed in the Request/Folder name, but the special characters are only saved in the .bru file meta and the file name is automatically stripped from special characters. This should then resolve: #189 & #388. But will take some time to fix as it is more complicated.

@lared
Copy link
Contributor

lared commented Oct 4, 2023

Somewhat related to #365

@Its-treason Its-treason changed the title fix: Fix name validation consistency Draft: fix: Fix name validation consistency Oct 6, 2023
@Its-treason
Copy link
Member Author

Its-treason commented Oct 6, 2023

I updated my PR to allow all special chars in request names, this should resolve #388, #189 & #174. I also updated the name length validation to 250 characters, 256 characters is the windows length limit, so 250 leaves some space for the file extension, this should resolve: #163, #378 & #501.

Example:

2023-10-06_21-49-51.mp4

Example with old project, created with the current bruno version:

2023-10-06_21-57-30.mp4

There might still some things to fix, I look into that tomorrow

@Its-treason Its-treason changed the title Draft: fix: Fix name validation consistency fix: Fix name validation consistency Oct 8, 2023
@Its-treason
Copy link
Member Author

Its-treason commented Oct 8, 2023

Hey @helloanoop, this PR should be ready to be reviewed and merged now. After this, I can start working on updating the Postman/Insomnia importer to close #365

@Its-treason
Copy link
Member Author

I update the sanitization Regex to match the directory regex. I noticed that it would be very hard for a language like Japanese to use Bruno with the other regex, because こんにちは & ありがとう would both be converted to _____.bru so only one would be allowed.

@nomorsug
Copy link

I think { and } should also be added to the regex as they are frequently used in postman collection and are allowed on unix/windows

/^(?!CON|PRN|AUX|NUL|COM\d|LPT\d|^ |^\-)[\w\-\. {}\(\)\[\]\!]+[^\. ]$/

@helloanoop helloanoop added this to the v1 milestone Oct 18, 2023
@helloanoop helloanoop mentioned this pull request Oct 18, 2023
@Its-treason
Copy link
Member Author

I think { and } should also be added to the regex as they are frequently used in postman collection and are allowed on unix/windows

/^(?!CON|PRN|AUX|NUL|COM\d|LPT\d|^ |^\-)[\w\-\. {}\(\)\[\]\!]+[^\. ]$/

Hey @nomorsug, sorry for the confusion, but the Regex in my initial comment is not up-to-date anymore.

With the newest changes, you can use any character in the request name. To make sure filenames are valid, characters like ? or / will be replaced with _, but this only affects the filenames.

@nomorsug
Copy link

@Its-treason was talking about the folder names, this regex to be specific packages/bruno-app/src/utils/common/regex.js to allow for curly braces {}

@Its-treason
Copy link
Member Author

Its-treason commented Oct 18, 2023

@nomorsug Sorry, I forget about the directory regex. I updated it to match the request sanitization regex:

/^(?!CON|PRN|AUX|NUL|COM\d|LPT\d|^ |^\-)[^<>:"/\\|?*\x00-\x1F]+[^\. ]$/

This now disallows only the problematic special characters like ?, < & some other windows related stuff, while allowing {, } and much more like Japanese characters.

Thank you for pointing that out!

@cardonator
Copy link
Contributor

@Its-treason scanning through the changes in this PR, it seems that "sanitize" and "filename" are misspelled in many places and in many different ways. Can you do a cleanup pass to fix these typos?

@@ -17,8 +17,8 @@ const CreateEnvironment = ({ collection, onClose }) => {
},
validationSchema: Yup.object({
name: Yup.string()
.min(1, 'must be at least 1 character')
.max(50, 'must be 50 characters or less')
.min(1, 'must be atleast 1 characters')
Copy link
Contributor

Choose a reason for hiding this comment

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

what was the intention of this change? The previous wording was grammatically correct.

@cardonator
Copy link
Contributor

@helloanoop can we get this PR reviewed/merged? It will dramatically improve file handling for environments.

@nomorsug
Copy link

@helloanoop any chance of this getting merged soon? Postman imports are still impacted by the incorrect validation and v1.0 has been released

@fzberlin23
Copy link

@helloanoop any chance of this getting merged soon? Postman imports are still impacted by the incorrect validation and v1.0 has been released

That would be very nice. I'm also struggeling with the fact that i cannot use "/" in my request names.

@fractalf
Copy link

fractalf commented Feb 2, 2024

Also looking forward to this being merged! :)
..this as been a show stopper for me since the Insomnia drama
image

@wouter-leistra
Copy link

Really needing this to get into Bruno to make it more usable (also using / in request names)

@Its-treason
Copy link
Member Author

Really needing this to get into Bruno to make it more usable (also using / in request names)

Not sure when, or if this will ever be merged into Bruno. For now you can use my fork: https://github.com/Its-treason/bruno/releases/tag/nightly it's compatible with mainline Bruno and currently used by our Team, because it includes some performance improvements.

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.

Support special characters in request name Can't create a collection in a folder that contains a #
9 participants