fix!: enforce regex for agent names (#16641)

Underscores and double hyphens are now blocked. The regex is almost the
exact same as the `coder_app` `slug` regex, but uppercase characters are
still permitted.
This commit is contained in:
Dean Sheather
2025-02-20 16:09:26 +11:00
committed by GitHub
parent 92870f0642
commit 9469b78290
17 changed files with 368 additions and 104 deletions
+2 -1
View File
@@ -3918,7 +3918,8 @@ func (s *MethodTestSuite) TestSystemFunctions() {
s.Run("InsertWorkspaceAgent", s.Subtest(func(db database.Store, check *expects) {
dbtestutil.DisableForeignKeysAndTriggers(s.T(), db)
check.Args(database.InsertWorkspaceAgentParams{
ID: uuid.New(),
ID: uuid.New(),
Name: "dev",
}).Asserts(rbac.ResourceSystem, policy.ActionCreate)
}))
s.Run("InsertWorkspaceApp", s.Subtest(func(db database.Store, check *expects) {
+2 -1
View File
@@ -91,7 +91,8 @@ func (b WorkspaceBuildBuilder) WithAgent(mutations ...func([]*sdkproto.Agent) []
//nolint: revive // returns modified struct
b.agentToken = uuid.NewString()
agents := []*sdkproto.Agent{{
Id: uuid.NewString(),
Id: uuid.NewString(),
Name: "dev",
Auth: &sdkproto.Agent_Token{
Token: b.agentToken,
},
@@ -1891,6 +1891,19 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid.
appSlugs = make(map[string]struct{})
)
for _, prAgent := range protoResource.Agents {
// Similar logic is duplicated in terraform/resources.go.
if prAgent.Name == "" {
return xerrors.Errorf("agent name cannot be empty")
}
// In 2025-02 we removed support for underscores in agent names. To
// provide a nicer error message, we check the regex first and check
// for underscores if it fails.
if !provisioner.AgentNameRegex.MatchString(prAgent.Name) {
if strings.Contains(prAgent.Name, "_") {
return xerrors.Errorf("agent name %q contains underscores which are no longer supported, please use hyphens instead (regex: %q)", prAgent.Name, provisioner.AgentNameRegex.String())
}
return xerrors.Errorf("agent name %q does not match regex %q", prAgent.Name, provisioner.AgentNameRegex.String())
}
// Agent names must be case-insensitive-unique, to be unambiguous in
// `coder_app`s and CoderVPN DNS names.
if _, ok := agentNames[strings.ToLower(prAgent.Name)]; ok {
@@ -2070,10 +2083,13 @@ func InsertWorkspaceResource(ctx context.Context, db database.Store, jobID uuid.
}
for _, app := range prAgent.Apps {
// Similar logic is duplicated in terraform/resources.go.
slug := app.Slug
if slug == "" {
return xerrors.Errorf("app must have a slug or name set")
}
// Contrary to agent names above, app slugs were never permitted to
// contain uppercase letters or underscores.
if !provisioner.AppSlugRegex.MatchString(slug) {
return xerrors.Errorf("app slug %q does not match regex %q", slug, provisioner.AppSlugRegex.String())
}
@@ -1883,6 +1883,7 @@ func TestInsertWorkspaceResource(t *testing.T) {
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev",
Auth: &sdkproto.Agent_Token{
Token: "bananas",
},
@@ -1896,6 +1897,7 @@ func TestInsertWorkspaceResource(t *testing.T) {
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev",
Apps: []*sdkproto.App{{
Slug: "a",
}, {
@@ -1903,7 +1905,61 @@ func TestInsertWorkspaceResource(t *testing.T) {
}},
}},
})
require.ErrorContains(t, err, "duplicate app slug")
require.ErrorContains(t, err, `duplicate app slug, must be unique per template: "a"`)
err = insert(dbmem.New(), uuid.New(), &sdkproto.Resource{
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev1",
Apps: []*sdkproto.App{{
Slug: "a",
}},
}, {
Name: "dev2",
Apps: []*sdkproto.App{{
Slug: "a",
}},
}},
})
require.ErrorContains(t, err, `duplicate app slug, must be unique per template: "a"`)
})
t.Run("AppSlugInvalid", func(t *testing.T) {
t.Parallel()
db := dbmem.New()
job := uuid.New()
err := insert(db, job, &sdkproto.Resource{
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev",
Apps: []*sdkproto.App{{
Slug: "dev_1",
}},
}},
})
require.ErrorContains(t, err, `app slug "dev_1" does not match regex`)
err = insert(db, job, &sdkproto.Resource{
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev",
Apps: []*sdkproto.App{{
Slug: "dev--1",
}},
}},
})
require.ErrorContains(t, err, `app slug "dev--1" does not match regex`)
err = insert(db, job, &sdkproto.Resource{
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev",
Apps: []*sdkproto.App{{
Slug: "Dev",
}},
}},
})
require.ErrorContains(t, err, `app slug "Dev" does not match regex`)
})
t.Run("DuplicateAgentNames", func(t *testing.T) {
t.Parallel()
@@ -1931,6 +1987,35 @@ func TestInsertWorkspaceResource(t *testing.T) {
})
require.ErrorContains(t, err, "duplicate agent name")
})
t.Run("AgentNameInvalid", func(t *testing.T) {
t.Parallel()
db := dbmem.New()
job := uuid.New()
err := insert(db, job, &sdkproto.Resource{
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "Dev",
}},
})
require.NoError(t, err) // uppercase is still allowed
err = insert(db, job, &sdkproto.Resource{
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev_1",
}},
})
require.ErrorContains(t, err, `agent name "dev_1" contains underscores`) // custom error for underscores
err = insert(db, job, &sdkproto.Resource{
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev--1",
}},
})
require.ErrorContains(t, err, `agent name "dev--1" does not match regex`)
})
t.Run("Success", func(t *testing.T) {
t.Parallel()
db := dbmem.New()
@@ -2007,6 +2092,7 @@ func TestInsertWorkspaceResource(t *testing.T) {
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev",
DisplayApps: &sdkproto.DisplayApps{
Vscode: true,
VscodeInsiders: true,
@@ -2035,6 +2121,7 @@ func TestInsertWorkspaceResource(t *testing.T) {
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev",
DisplayApps: &sdkproto.DisplayApps{},
}},
})
@@ -2059,6 +2146,7 @@ func TestInsertWorkspaceResource(t *testing.T) {
Name: "something",
Type: "aws_instance",
Agents: []*sdkproto.Agent{{
Name: "dev",
DisplayApps: &sdkproto.DisplayApps{},
ResourcesMonitoring: &sdkproto.ResourcesMonitoring{
Memory: &sdkproto.MemoryResourceMonitor{
+3 -1
View File
@@ -829,6 +829,7 @@ func TestTemplateVersionResources(t *testing.T) {
Type: "example",
Agents: []*proto.Agent{{
Id: "something",
Name: "dev",
Auth: &proto.Agent_Token{},
}},
}, {
@@ -875,7 +876,8 @@ func TestTemplateVersionLogs(t *testing.T) {
Name: "some",
Type: "example",
Agents: []*proto.Agent{{
Id: "something",
Id: "something",
Name: "dev",
Auth: &proto.Agent_Token{
Token: uuid.NewString(),
},
+2 -1
View File
@@ -393,7 +393,8 @@ func TestWorkspaceAgentConnectRPC(t *testing.T) {
Name: "example",
Type: "aws_instance",
Agents: []*proto.Agent{{
Id: uuid.NewString(),
Id: uuid.NewString(),
Name: "dev",
Auth: &proto.Agent_Token{
Token: uuid.NewString(),
},
+1
View File
@@ -720,6 +720,7 @@ func TestWorkspaceBuildLogs(t *testing.T) {
Type: "example",
Agents: []*proto.Agent{{
Id: "something",
Name: "dev",
Auth: &proto.Agent_Token{},
}},
}, {
+3
View File
@@ -33,6 +33,7 @@ func TestPostWorkspaceAuthAzureInstanceIdentity(t *testing.T) {
Name: "somename",
Type: "someinstance",
Agents: []*proto.Agent{{
Name: "dev",
Auth: &proto.Agent_InstanceId{
InstanceId: instanceID,
},
@@ -78,6 +79,7 @@ func TestPostWorkspaceAuthAWSInstanceIdentity(t *testing.T) {
Name: "somename",
Type: "someinstance",
Agents: []*proto.Agent{{
Name: "dev",
Auth: &proto.Agent_InstanceId{
InstanceId: instanceID,
},
@@ -164,6 +166,7 @@ func TestPostWorkspaceAuthGoogleInstanceIdentity(t *testing.T) {
Name: "somename",
Type: "someinstance",
Agents: []*proto.Agent{{
Name: "dev",
Auth: &proto.Agent_InstanceId{
InstanceId: instanceID,
},
+9 -2
View File
@@ -219,6 +219,7 @@ func TestWorkspace(t *testing.T) {
Type: "example",
Agents: []*proto.Agent{{
Id: uuid.NewString(),
Name: "dev",
Auth: &proto.Agent_Token{},
}},
}},
@@ -259,6 +260,7 @@ func TestWorkspace(t *testing.T) {
Type: "example",
Agents: []*proto.Agent{{
Id: uuid.NewString(),
Name: "dev",
Auth: &proto.Agent_Token{},
ConnectionTimeoutSeconds: 1,
}},
@@ -1722,7 +1724,8 @@ func TestWorkspaceFilterManual(t *testing.T) {
Name: "example",
Type: "aws_instance",
Agents: []*proto.Agent{{
Id: uuid.NewString(),
Id: uuid.NewString(),
Name: "dev",
Auth: &proto.Agent_Token{
Token: authToken,
},
@@ -2729,7 +2732,8 @@ func TestWorkspaceWatcher(t *testing.T) {
Name: "example",
Type: "aws_instance",
Agents: []*proto.Agent{{
Id: uuid.NewString(),
Id: uuid.NewString(),
Name: "dev",
Auth: &proto.Agent_Token{
Token: authToken,
},
@@ -2951,6 +2955,7 @@ func TestWorkspaceResource(t *testing.T) {
Type: "example",
Agents: []*proto.Agent{{
Id: "something",
Name: "dev",
Auth: &proto.Agent_Token{},
Apps: apps,
}},
@@ -3025,6 +3030,7 @@ func TestWorkspaceResource(t *testing.T) {
Type: "example",
Agents: []*proto.Agent{{
Id: "something",
Name: "dev",
Auth: &proto.Agent_Token{},
Apps: apps,
}},
@@ -3068,6 +3074,7 @@ func TestWorkspaceResource(t *testing.T) {
Type: "example",
Agents: []*proto.Agent{{
Id: "something",
Name: "dev",
Auth: &proto.Agent_Token{},
}},
Metadata: []*proto.Resource_Metadata{{
+2 -2
View File
@@ -161,11 +161,11 @@ func TestTemplates(t *testing.T) {
Name: "some",
Type: "example",
Agents: []*proto.Agent{{
Id: "something",
Id: "something",
Name: "test",
Auth: &proto.Agent_Token{
Token: uuid.NewString(),
},
Name: "test",
}},
}, {
Name: "another",
-13
View File
@@ -1,13 +0,0 @@
package provisioner
import "regexp"
// AppSlugRegex is the regex used to validate the slug of a coder_app
// resource. It must be a valid hostname and cannot contain two consecutive
// hyphens or start/end with a hyphen.
//
// This regex is duplicated in the terraform provider code, so make sure to
// update it there as well.
//
// There are test cases for this regex in appslug_test.go.
var AppSlugRegex = regexp.MustCompile(`^[a-z0-9](-?[a-z0-9])*$`)
-64
View File
@@ -1,64 +0,0 @@
package provisioner_test
import (
"testing"
"github.com/stretchr/testify/require"
"github.com/coder/coder/v2/provisioner"
)
func TestValidAppSlugRegex(t *testing.T) {
t.Parallel()
t.Run("Valid", func(t *testing.T) {
t.Parallel()
validStrings := []string{
"a",
"1",
"a1",
"1a",
"1a1",
"1-1",
"a-a",
"ab-cd",
"ab-cd-ef",
"abc-123",
"a-123",
"abc-1",
"ab-c",
"a-bc",
}
for _, s := range validStrings {
require.True(t, provisioner.AppSlugRegex.MatchString(s), s)
}
})
t.Run("Invalid", func(t *testing.T) {
t.Parallel()
invalidStrings := []string{
"",
"-",
"-abc",
"abc-",
"ab--cd",
"a--bc",
"ab--c",
"_",
"ab_cd",
"_abc",
"abc_",
" ",
"abc ",
" abc",
"ab cd",
}
for _, s := range invalidStrings {
require.False(t, provisioner.AppSlugRegex.MatchString(s), s)
}
})
}
+31
View File
@@ -0,0 +1,31 @@
package provisioner
import "regexp"
var (
// AgentNameRegex is the regex used to validate the name of a coder_agent
// resource. It must be a valid hostname and cannot contain two consecutive
// hyphens or start/end with a hyphen. Uppercase characters ARE permitted,
// although duplicate agent names with different casing will be rejected.
//
// Previously, underscores were permitted, but this was changed in 2025-02.
// App URLs never supported underscores, and proxy requests to apps on
// agents with underscores in the name always failed.
//
// Due to terraform limitations, this cannot be validated at the provider
// level as resource names cannot be read from the provider API, so this is
// not duplicated in the terraform provider code.
//
// There are test cases for this regex in regexes_test.go.
AgentNameRegex = regexp.MustCompile(`(?i)^[a-z0-9](-?[a-z0-9])*$`)
// AppSlugRegex is the regex used to validate the slug of a coder_app
// resource. It must be a valid hostname and cannot contain two consecutive
// hyphens or start/end with a hyphen.
//
// This regex is duplicated in the terraform provider code, so make sure to
// update it there as well.
//
// There are test cases for this regex in regexes_test.go.
AppSlugRegex = regexp.MustCompile(`^[a-z0-9](-?[a-z0-9])*$`)
)
+88
View File
@@ -0,0 +1,88 @@
package provisioner_test
import (
"testing"
"github.com/stretchr/testify/require"
"github.com/coder/coder/v2/provisioner"
)
var (
validStrings = []string{
"a",
"1",
"a1",
"1a",
"1a1",
"1-1",
"a-a",
"ab-cd",
"ab-cd-ef",
"abc-123",
"a-123",
"abc-1",
"ab-c",
"a-bc",
}
invalidStrings = []string{
"",
"-",
"-abc",
"abc-",
"ab--cd",
"a--bc",
"ab--c",
"_",
"ab_cd",
"_abc",
"abc_",
" ",
"abc ",
" abc",
"ab cd",
}
uppercaseStrings = []string{
"A",
"A1",
"1A",
}
)
func TestAgentNameRegex(t *testing.T) {
t.Parallel()
t.Run("Valid", func(t *testing.T) {
t.Parallel()
for _, s := range append(validStrings, uppercaseStrings...) {
require.True(t, provisioner.AgentNameRegex.MatchString(s), s)
}
})
t.Run("Invalid", func(t *testing.T) {
t.Parallel()
for _, s := range invalidStrings {
require.False(t, provisioner.AgentNameRegex.MatchString(s), s)
}
})
}
func TestAppSlugRegex(t *testing.T) {
t.Parallel()
t.Run("Valid", func(t *testing.T) {
t.Parallel()
for _, s := range validStrings {
require.True(t, provisioner.AppSlugRegex.MatchString(s), s)
}
})
t.Run("Invalid", func(t *testing.T) {
t.Parallel()
for _, s := range append(invalidStrings, uppercaseStrings...) {
require.False(t, provisioner.AppSlugRegex.MatchString(s), s)
}
})
}
+17 -1
View File
@@ -215,6 +215,19 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
return nil, xerrors.Errorf("decode agent attributes: %w", err)
}
// Similar logic is duplicated in terraform/resources.go.
if tfResource.Name == "" {
return nil, xerrors.Errorf("agent name cannot be empty")
}
// In 2025-02 we removed support for underscores in agent names. To
// provide a nicer error message, we check the regex first and check
// for underscores if it fails.
if !provisioner.AgentNameRegex.MatchString(tfResource.Name) {
if strings.Contains(tfResource.Name, "_") {
return nil, xerrors.Errorf("agent name %q contains underscores which are no longer supported, please use hyphens instead (regex: %q)", tfResource.Name, provisioner.AgentNameRegex.String())
}
return nil, xerrors.Errorf("agent name %q does not match regex %q", tfResource.Name, provisioner.AgentNameRegex.String())
}
// Agent names must be case-insensitive-unique, to be unambiguous in
// `coder_app`s and CoderVPN DNS names.
if _, ok := agentNames[strings.ToLower(tfResource.Name)]; ok {
@@ -443,6 +456,7 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
if attrs.Slug == "" {
attrs.Slug = resource.Name
}
// Similar logic is duplicated in terraform/resources.go.
if attrs.DisplayName == "" {
if attrs.Name != "" {
// Name is deprecated but still accepted.
@@ -452,8 +466,10 @@ func ConvertState(ctx context.Context, modules []*tfjson.StateModule, rawGraph s
}
}
// Contrary to agent names above, app slugs were never permitted to
// contain uppercase letters or underscores.
if !provisioner.AppSlugRegex.MatchString(attrs.Slug) {
return nil, xerrors.Errorf("invalid app slug %q, please update your coder/coder provider to the latest version and specify the slug property on each coder_app", attrs.Slug)
return nil, xerrors.Errorf("app slug %q does not match regex %q", attrs.Slug, provisioner.AppSlugRegex.String())
}
if _, exists := appSlugs[attrs.Slug]; exists {
+102 -16
View File
@@ -984,6 +984,7 @@ func TestInvalidTerraformAddress(t *testing.T) {
require.Equal(t, state.Resources[0].ModulePath, "invalid terraform address")
}
//nolint:tparallel
func TestAppSlugValidation(t *testing.T) {
t.Parallel()
ctx, logger := ctxAndLogger(t)
@@ -1001,31 +1002,116 @@ func TestAppSlugValidation(t *testing.T) {
tfPlanGraph, err := os.ReadFile(filepath.Join(dir, "multiple-apps.tfplan.dot"))
require.NoError(t, err)
// Change all slugs to be invalid.
cases := []struct {
slug string
errContains string
}{
{slug: "$$$ invalid slug $$$", errContains: "does not match regex"},
{slug: "invalid--slug", errContains: "does not match regex"},
{slug: "invalid_slug", errContains: "does not match regex"},
{slug: "Invalid-slug", errContains: "does not match regex"},
{slug: "valid", errContains: ""},
}
//nolint:paralleltest
for i, c := range cases {
c := c
t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) {
// Change the first app slug to match the current case.
for _, resource := range tfPlan.PlannedValues.RootModule.Resources {
if resource.Type == "coder_app" {
resource.AttributeValues["slug"] = c.slug
break
}
}
_, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
if c.errContains != "" {
require.ErrorContains(t, err, c.errContains)
} else {
require.NoError(t, err)
}
})
}
}
func TestAppSlugDuplicate(t *testing.T) {
t.Parallel()
ctx, logger := ctxAndLogger(t)
// nolint:dogsled
_, filename, _, _ := runtime.Caller(0)
dir := filepath.Join(filepath.Dir(filename), "testdata", "multiple-apps")
tfPlanRaw, err := os.ReadFile(filepath.Join(dir, "multiple-apps.tfplan.json"))
require.NoError(t, err)
var tfPlan tfjson.Plan
err = json.Unmarshal(tfPlanRaw, &tfPlan)
require.NoError(t, err)
tfPlanGraph, err := os.ReadFile(filepath.Join(dir, "multiple-apps.tfplan.dot"))
require.NoError(t, err)
for _, resource := range tfPlan.PlannedValues.RootModule.Resources {
if resource.Type == "coder_app" {
resource.AttributeValues["slug"] = "$$$ invalid slug $$$"
resource.AttributeValues["slug"] = "dev"
}
}
state, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
require.Nil(t, state)
require.Error(t, err)
require.ErrorContains(t, err, "invalid app slug")
// Change all slugs to be identical and valid.
for _, resource := range tfPlan.PlannedValues.RootModule.Resources {
if resource.Type == "coder_app" {
resource.AttributeValues["slug"] = "valid"
}
}
state, err = terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
require.Nil(t, state)
_, err = terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
require.Error(t, err)
require.ErrorContains(t, err, "duplicate app slug")
}
//nolint:tparallel
func TestAgentNameInvalid(t *testing.T) {
t.Parallel()
ctx, logger := ctxAndLogger(t)
// nolint:dogsled
_, filename, _, _ := runtime.Caller(0)
dir := filepath.Join(filepath.Dir(filename), "testdata", "multiple-agents")
tfPlanRaw, err := os.ReadFile(filepath.Join(dir, "multiple-agents.tfplan.json"))
require.NoError(t, err)
var tfPlan tfjson.Plan
err = json.Unmarshal(tfPlanRaw, &tfPlan)
require.NoError(t, err)
tfPlanGraph, err := os.ReadFile(filepath.Join(dir, "multiple-agents.tfplan.dot"))
require.NoError(t, err)
cases := []struct {
name string
errContains string
}{
{name: "bad--name", errContains: "does not match regex"},
{name: "bad_name", errContains: "contains underscores"}, // custom error for underscores
{name: "valid-name-123", errContains: ""},
{name: "valid", errContains: ""},
{name: "UppercaseValid", errContains: ""},
}
//nolint:paralleltest
for i, c := range cases {
c := c
t.Run(fmt.Sprintf("case-%d", i), func(t *testing.T) {
// Change the first agent name to match the current case.
for _, resource := range tfPlan.PlannedValues.RootModule.Resources {
if resource.Type == "coder_agent" {
resource.Name = c.name
break
}
}
_, err := terraform.ConvertState(ctx, []*tfjson.StateModule{tfPlan.PlannedValues.RootModule}, string(tfPlanGraph), logger)
if c.errContains != "" {
require.ErrorContains(t, err, c.errContains)
} else {
require.NoError(t, err)
}
})
}
}
func TestAgentNameDuplicate(t *testing.T) {
t.Parallel()
ctx, logger := ctxAndLogger(t)
+1 -1
View File
@@ -166,7 +166,7 @@ func New(opts *Options) *Handler {
handler.installScript, err = parseInstallScript(opts.SiteFS, opts.BuildInfo)
if err != nil {
_, _ = fmt.Fprintf(os.Stderr, "install.sh will be unavailable: %v", err.Error())
opts.Logger.Warn(context.Background(), "could not parse install.sh, it will be unavailable", slog.Error(err))
}
return handler