Skip to content

Commit d5496af

Browse files
ssncferreirassncferreira
authored
fix: correct URL validation and centralize logic (#421)
* fix: correct URL validation and centralize logic * fix: go imports order * tests: add validation URL tests * revert: parameter_test ValidDefaultWithOptions test update * test: update validation_test to use require * test: update validation_test tests
1 parent f239e51 commit d5496af

File tree

8 files changed

+279
-48
lines changed

8 files changed

+279
-48
lines changed

‎provider/app.go

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,13 +2,14 @@ package provider
22

33
import (
44
"context"
5-
"net/url"
65
"regexp"
76

87
"github.com/google/uuid"
98
"github.com/hashicorp/go-cty/cty"
109
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
1110
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
11+
12+
"github.com/coder/terraform-provider-coder/v2/provider/helpers"
1213
)
1314

1415
var (
@@ -93,15 +94,9 @@ func appResource() *schema.Resource {
9394
Description: "A URL to an icon that will display in the dashboard. View built-in " +
9495
"icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " +
9596
"built-in icon with `\"${data.coder_workspace.me.access_url}/icon/<path>\"`.",
96-
ForceNew: true,
97-
Optional: true,
98-
ValidateFunc: func(i any, s string) ([]string, []error) {
99-
_, err := url.Parse(s)
100-
if err != nil {
101-
return nil, []error{err}
102-
}
103-
return nil, nil
104-
},
97+
ForceNew: true,
98+
Optional: true,
99+
ValidateFunc: helpers.ValidateURL,
105100
},
106101
"slug": {
107102
Type: schema.TypeString,

‎provider/app_test.go

Lines changed: 57 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -478,4 +478,61 @@ func TestApp(t *testing.T) {
478478
})
479479
}
480480
})
481+
482+
t.Run("Icon", func(t *testing.T) {
483+
t.Parallel()
484+
485+
cases := []struct {
486+
name string
487+
icon string
488+
expectError *regexp.Regexp
489+
}{
490+
{
491+
name: "Empty",
492+
icon: "",
493+
},
494+
{
495+
name: "ValidURL",
496+
icon: "/icon/region.svg",
497+
},
498+
{
499+
name: "InvalidURL",
500+
icon: "/icon%.svg",
501+
expectError: regexp.MustCompile("invalid URL escape"),
502+
},
503+
}
504+
505+
for _, c := range cases {
506+
c := c
507+
508+
t.Run(c.name, func(t *testing.T) {
509+
t.Parallel()
510+
511+
config := fmt.Sprintf(`
512+
provider "coder" {}
513+
resource "coder_agent" "dev" {
514+
os = "linux"
515+
arch = "amd64"
516+
}
517+
resource "coder_app" "code-server" {
518+
agent_id = coder_agent.dev.id
519+
slug = "code-server"
520+
display_name = "Testing"
521+
url = "http://localhost:13337"
522+
open_in = "slim-window"
523+
icon = "%s"
524+
}
525+
`, c.icon)
526+
527+
resource.Test(t, resource.TestCase{
528+
ProviderFactories: coderFactory(),
529+
IsUnitTest: true,
530+
Steps: []resource.TestStep{{
531+
Config: config,
532+
ExpectError: c.expectError,
533+
}},
534+
})
535+
})
536+
}
537+
})
481538
}

‎provider/helpers/validation.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
package helpers
2+
3+
import (
4+
"fmt"
5+
"net/url"
6+
)
7+
8+
// ValidateURL validates that value is a valid URL string.
9+
// Accepts empty strings, local file paths, file:// URLs, and http/https URLs.
10+
// Example: for `icon = "/icon/region.svg"`, value is `/icon/region.svg` and label is `icon`.
11+
func ValidateURL(value any, label string) ([]string, []error) {
12+
val, ok := value.(string)
13+
if !ok {
14+
return nil, []error{fmt.Errorf("expected %q to be a string", label)}
15+
}
16+
17+
if _, err := url.Parse(val); err != nil {
18+
return nil, []error{err}
19+
}
20+
21+
return nil, nil
22+
}

‎provider/helpers/validation_test.go

Lines changed: 151 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,151 @@
1+
package helpers
2+
3+
import (
4+
"testing"
5+
6+
"github.com/stretchr/testify/require"
7+
)
8+
9+
func TestValidateURL(t *testing.T) {
10+
tests := []struct {
11+
name string
12+
value any
13+
label string
14+
expectError bool
15+
errorContains string
16+
}{
17+
// Valid cases
18+
{
19+
name: "empty string",
20+
value: "",
21+
label: "url",
22+
expectError: false,
23+
},
24+
{
25+
name: "valid http URL",
26+
value: "http://example.com",
27+
label: "url",
28+
expectError: false,
29+
},
30+
{
31+
name: "valid https URL",
32+
value: "https://example.com/path",
33+
label: "url",
34+
expectError: false,
35+
},
36+
{
37+
name: "absolute file path",
38+
value: "/path/to/file",
39+
label: "url",
40+
expectError: false,
41+
},
42+
{
43+
name: "relative file path",
44+
value: "./file.txt",
45+
label: "url",
46+
expectError: false,
47+
},
48+
{
49+
name: "relative path up directory",
50+
value: "../config.json",
51+
label: "url",
52+
expectError: false,
53+
},
54+
{
55+
name: "simple filename",
56+
value: "file.txt",
57+
label: "url",
58+
expectError: false,
59+
},
60+
{
61+
name: "URL with query params",
62+
value: "https://example.com/search?q=test",
63+
label: "url",
64+
expectError: false,
65+
},
66+
{
67+
name: "URL with fragment",
68+
value: "https://example.com/page#section",
69+
label: "url",
70+
expectError: false,
71+
},
72+
73+
// Various URL schemes that url.Parse accepts
74+
{
75+
name: "file URL scheme",
76+
value: "file:///path/to/file",
77+
label: "url",
78+
expectError: false,
79+
},
80+
{
81+
name: "ftp scheme",
82+
value: "ftp://files.example.com/file.txt",
83+
label: "url",
84+
expectError: false,
85+
},
86+
{
87+
name: "mailto scheme",
88+
value: "mailto:user@example.com",
89+
label: "url",
90+
expectError: false,
91+
},
92+
{
93+
name: "tel scheme",
94+
value: "tel:+1234567890",
95+
label: "url",
96+
expectError: false,
97+
},
98+
{
99+
name: "data scheme",
100+
value: "data:text/plain;base64,SGVsbG8=",
101+
label: "url",
102+
expectError: false,
103+
},
104+
105+
// Invalid cases
106+
{
107+
name: "non-string type - int",
108+
value: 123,
109+
label: "url",
110+
expectError: true,
111+
errorContains: "expected \"url\" to be a string",
112+
},
113+
{
114+
name: "non-string type - nil",
115+
value: nil,
116+
label: "config_url",
117+
expectError: true,
118+
errorContains: "expected \"config_url\" to be a string",
119+
},
120+
{
121+
name: "invalid URL with spaces",
122+
value: "http://example .com",
123+
label: "url",
124+
expectError: true,
125+
errorContains: "invalid character",
126+
},
127+
{
128+
name: "malformed URL",
129+
value: "http://[::1:80",
130+
label: "endpoint",
131+
expectError: true,
132+
errorContains: "missing ']'",
133+
},
134+
}
135+
136+
for _, tt := range tests {
137+
t.Run(tt.name, func(t *testing.T) {
138+
warnings, errors := ValidateURL(tt.value, tt.label)
139+
140+
if tt.expectError {
141+
require.Len(t, errors, 1, "expected an error but got none")
142+
require.Contains(t, errors[0].Error(), tt.errorContains)
143+
} else {
144+
require.Empty(t, errors, "expected no errors but got: %v", errors)
145+
}
146+
147+
// Should always return nil for warnings
148+
require.Nil(t, warnings, "expected warnings to be nil but got: %v", warnings)
149+
})
150+
}
151+
}

‎provider/metadata.go

Lines changed: 5 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,12 +2,13 @@ package provider
22

33
import (
44
"context"
5-
"net/url"
65

76
"github.com/google/uuid"
87
"github.com/hashicorp/terraform-plugin-sdk/v2/diag"
98
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
109
"golang.org/x/xerrors"
10+
11+
"github.com/coder/terraform-provider-coder/v2/provider/helpers"
1112
)
1213

1314
func metadataResource() *schema.Resource {
@@ -56,15 +57,9 @@ func metadataResource() *schema.Resource {
5657
Description: "A URL to an icon that will display in the dashboard. View built-in " +
5758
"icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " +
5859
"built-in icon with `\"${data.coder_workspace.me.access_url}/icon/<path>\"`.",
59-
ForceNew: true,
60-
Optional: true,
61-
ValidateFunc: func(i interface{}, s string) ([]string, []error) {
62-
_, err := url.Parse(s)
63-
if err != nil {
64-
return nil, []error{err}
65-
}
66-
return nil, nil
67-
},
60+
ForceNew: true,
61+
Optional: true,
62+
ValidateFunc: helpers.ValidateURL,
6863
},
6964
"daily_cost": {
7065
Type: schema.TypeInt,

‎provider/parameter.go

Lines changed: 8 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ import (
66
"encoding/hex"
77
"encoding/json"
88
"fmt"
9-
"net/url"
109
"os"
1110
"regexp"
1211
"strconv"
@@ -19,6 +18,8 @@ import (
1918
"github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation"
2019
"github.com/mitchellh/mapstructure"
2120
"golang.org/x/xerrors"
21+
22+
"github.com/coder/terraform-provider-coder/v2/provider/helpers"
2223
)
2324

2425
var (
@@ -223,15 +224,9 @@ func parameterDataSource() *schema.Resource {
223224
Description: "A URL to an icon that will display in the dashboard. View built-in " +
224225
"icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " +
225226
"built-in icon with `\"${data.coder_workspace.me.access_url}/icon/<path>\"`.",
226-
ForceNew: true,
227-
Optional: true,
228-
ValidateFunc: func(i interface{}, s string) ([]string, []error) {
229-
_, err := url.Parse(s)
230-
if err != nil {
231-
return nil, []error{err}
232-
}
233-
return nil, nil
234-
},
227+
ForceNew: true,
228+
Optional: true,
229+
ValidateFunc: helpers.ValidateURL,
235230
},
236231
"option": {
237232
Type: schema.TypeList,
@@ -263,15 +258,9 @@ func parameterDataSource() *schema.Resource {
263258
Description: "A URL to an icon that will display in the dashboard. View built-in " +
264259
"icons [here](https://github.com/coder/coder/tree/main/site/static/icon). Use a " +
265260
"built-in icon with `\"${data.coder_workspace.me.access_url}/icon/<path>\"`.",
266-
ForceNew: true,
267-
Optional: true,
268-
ValidateFunc: func(i interface{}, s string) ([]string, []error) {
269-
_, err := url.Parse(s)
270-
if err != nil {
271-
return nil, []error{err}
272-
}
273-
return nil, nil
274-
},
261+
ForceNew: true,
262+
Optional: true,
263+
ValidateFunc: helpers.ValidateURL,
275264
},
276265
},
277266
},

‎provider/parameter_test.go

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -665,7 +665,33 @@ data "coder_parameter" "region" {
665665
}
666666
`,
667667
ExpectError: regexp.MustCompile("ephemeral parameter requires the default property"),
668-
}} {
668+
}, {
669+
Name: "InvalidIconURL",
670+
Config: `
671+
data "coder_parameter" "region" {
672+
name = "Region"
673+
type = "string"
674+
icon = "/icon%.svg"
675+
}
676+
`,
677+
ExpectError: regexp.MustCompile("invalid URL escape"),
678+
}, {
679+
Name: "OptionInvalidIconURL",
680+
Config: `
681+
data "coder_parameter" "region" {
682+
name = "Region"
683+
type = "string"
684+
option {
685+
name = "1"
686+
value = "1"
687+
icon = "/icon%.svg"
688+
description = "Something!"
689+
}
690+
}
691+
`,
692+
ExpectError: regexp.MustCompile("invalid URL escape"),
693+
},
694+
} {
669695
tc := tc
670696
t.Run(tc.Name, func(t *testing.T) {
671697
t.Parallel()

0 commit comments

Comments
 (0)