feat: Add template display name (backend) #4966
Conversation
mtojek
changed the title
mtojek
requested review from
Kira-Pilot and
a team
and removed request for
a team and
Kira-Pilot
| @@ -57,6 +59,7 @@ func templateEdit() *cobra.Command { | |||
| } | |||
|
|
|||
| cmd.Flags().StringVarP(&name, "name", "", "", "Edit the template name") | |||
There was a problem hiding this comment.
What do you think about changing name to slug instead? We could make this naming consistent as well across all objects. This can happen in another PR, just thinking about the API moving forward.
There was a problem hiding this comment.
That's an interesting idea, let me think loud...
We could define a policy that every database object has a mandatory unique ID, unique slug, and optional display name. Potentially slugs can be used as source data for a global search feature and we won't need to index descriptions, like in OSX cmd+space.
This can happen in another PR, just thinking about the API moving forward.
Yup, I can raise an issue and we can continue the discussion there. Otherwise, it will be scattered across different comments :)
coderd/httpapi/name_test.go
Outdated
| {"-a1b2c3d4e5f6g7h8i9j0k", false}, | ||
| {"a1b2c3d4e5f6g7h8i9j0k-", false}, | ||
| {"BANANAS_wow", false}, | ||
| {"test--now", false}, |
There was a problem hiding this comment.
Why are these not valid for a human readable name? I mean, they're not pretty but a user wants what a user wants.. 😂
There was a problem hiding this comment.
Oh that's basically because of character set _-. I think that you're right, let's extend the regexp.
There was a problem hiding this comment.
I replaced the regexp with a more free form: ^[a-zA-Z0-9](.*[^ ])?$. We can translate it to:
- must start with a-z, A-Z, 0-9
- can't end with a space
- everything in the middle is allowed.
Although, I'm not sure if it's safe enough...
There was a problem hiding this comment.
I don't think we need to sanitize the input here, tbh. For instance, what if someone wants for the display name to be a bunch of emojis? Sanitation shouldn't really be a problem in the frontend unless someone is doing something weird with React.
Or what say you @coder/frontend? Is it bad if we don't limit display names to "safe" characters?
There was a problem hiding this comment.
For instance, what if someone wants for the display name to be a bunch of emojis?
That is correct, but should preserve the basic sanitation level. For instance: <space><space> or M<space><space><space> doesn't make any logical sense.
unless someone is doing something weird with React.
.. or customers are writing unsafe characters to logs (e.g. audit or access logs). Using emojis and allowing for a wide set of characters may increase the "shellshock" attack risk. This may not be applicable in our case, but still a security concern.
I guess that we perform proper sanitization, so it won't be easy to inject the XSS snippet.
There was a problem hiding this comment.
What do you think about just blocking whitespace at the beginning and ends? I think people will want to use emojis in names (we probably will too).
There was a problem hiding this comment.
It's an option to introduce this pattern, but then we need to verify if it doesn't introduce any extra risk, something like "shellshock" against audit logs. I believe that we can track this effort in a separate issue.
EDIT:
It looks like the majority prefers a more permissive pattern, so I adjusted the validation.
There was a problem hiding this comment.
Some thoughts from the FE:
- To my knowledge, we don't sanitize other fields. It would be nice to have some consistency however we decide to move forward. Some documentation would be nice, too.
- React escapes any values embedded in JSX before rendering them, unless you use
dangerouslySetInnerHTML. - Current approach looks fine to me.
|
I updated the validation to be more permissive: |
Issue: #3321
This PR adds the
display_nametemplate property to the CLI/backend.I will modify the
sitecode (frontend) in the follow-up PR:display_namewherever possible.If it's unsafe to split the change into 2 PRs, please let me know and I will iterate on this one.