-
Notifications
You must be signed in to change notification settings - Fork 23
feat: add icon and description fields to workspace preset #422
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
Changes from all commits
c9a6321
6a4d070
fac00c7
d2efd30
6c22150
2a17ccc
23765b0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -18,9 +18,11 @@ import ( | |||||||||||
var PrebuildsCRONParser = rbcron.NewParser(rbcron.Minute | rbcron.Hour | rbcron.Dom | rbcron.Month | rbcron.Dow) | ||||||||||||
|
||||||||||||
type WorkspacePreset struct { | ||||||||||||
Name string `mapstructure:"name"` | ||||||||||||
Default bool `mapstructure:"default"` | ||||||||||||
Parameters map[string]string `mapstructure:"parameters"` | ||||||||||||
Name string `mapstructure:"name"` | ||||||||||||
Description string `mapstructure:"description"` | ||||||||||||
Icon string `mapstructure:"icon"` | ||||||||||||
Default bool `mapstructure:"default"` | ||||||||||||
Parameters map[string]string `mapstructure:"parameters"` | ||||||||||||
// There should always be only one prebuild block, but Terraform's type system | ||||||||||||
// still parses them as a slice, so we need to handle it as such. We could use | ||||||||||||
// an anonymous type and rd.Get to avoid a slice here, but that would not be possible | ||||||||||||
|
@@ -93,6 +95,24 @@ func workspacePresetDataSource() *schema.Resource { | |||||||||||
Required: true, | ||||||||||||
ValidateFunc: validation.StringIsNotEmpty, | ||||||||||||
}, | ||||||||||||
"description": { | ||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Copied the schema definition for description and icon from the parameter schema (see https://github.com/coder/terraform-provider-coder/blob/main/provider/parameter.go#L184). Neither field currently has a size limit, but it might be good to add one to ensure the UI isn’t broken by overly large descriptions. Wdyt? There is also a security consideration: we don’t seem to perform proper escaping or sanitization, which means it could be possible to inject HTML or JavaScript via the description. For the icon, since it’s used to fetch local files, we should be careful to prevent directory traversal attacks or other malicious paths. Given that Coder typically runs on-premises, these risks might be lower, but it could still be a good practice to address them. Do we currently have any mechanisms in place to handle these concerns? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you mean in the provider, or in the database?
What do you mean by 'local' here? The icon is used by the UI, and the icons are generally hosted on the control plane, although I've seen people link to icons on other domains.
Where specifically have you checked?
Template admins have a good bit of power in a Coder deployment, so there is some element of trust related to allowing users to create templates. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
In both:
I think it would make sense to add reasonable size limits to both description and icon, similar to what we do for
Wdyt?
By "local" I meant relative paths like
I tested the integration between the Terraform provider and Coder. I inserted a description with inline JavaScript and confirmed it was passed through and stored in the database. However, from my quick test, the UI seems to display the content as plain text and doesn’t interpret or render it as raw HTML, so that is good. I didn't test the icon path.
Totally agree. Given Coder is self-hosted and template creation is an admin-level task, I don't think this is a critical issue, more of an observation from working on this PR. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think that's reasonable!
Right, but I don't think we would expose that in our static file server though. Might be no harm to sanitize the path though, good callout! |
||||||||||||
Type: schema.TypeString, | ||||||||||||
Optional: true, | ||||||||||||
Description: "Describe what this preset does.", | ||||||||||||
ValidateFunc: validation.StringLenBetween(0, 128), | ||||||||||||
}, | ||||||||||||
"icon": { | ||||||||||||
Type: schema.TypeString, | ||||||||||||
Description: "A URL to an icon that will display in the dashboard. View built-in " + | ||||||||||||
"icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " + | ||||||||||||
"built-in icon with `\"${data.coder_workspace.me.access_url}/icon/<path>\"`.", | ||||||||||||
ForceNew: true, | ||||||||||||
Optional: true, | ||||||||||||
ValidateFunc: validation.All( | ||||||||||||
helpers.ValidateURL, | ||||||||||||
validation.StringLenBetween(0, 256), | ||||||||||||
), | ||||||||||||
}, | ||||||||||||
"default": { | ||||||||||||
Type: schema.TypeBool, | ||||||||||||
Description: "Whether this preset should be selected by default when creating a workspace. Only one preset per template can be marked as default.", | ||||||||||||
|
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.
nit: should we use rather realistic values?
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 followed the naming convention used by the other parameters, as they follow a similar logic. Since this is an integration test, I don’t think adding realistic values here is necessary.