From 2d214b2727013a79d1e46a21a81e41046cf1ed22 Mon Sep 17 00:00:00 2001
From: Luke Kysow <1034429+lkysow@users.noreply.github.com>
Date: Tue, 30 Oct 2018 13:01:48 -0500
Subject: [PATCH] Restrict possible project names.

We should only allow project names (which are specified in an
atlantis.yaml file) that don't need to be url escaped. The one
exceptional character is '/' which we allow because users like to name
their projects to match the directory they're in. We use the same rule
that Terraform uses for workspace names.

I've also changed the characters that are replaced when writing out the
plan filename to only remove invalid filename characters instead of just
allowing alphanumeric. This is the smallest amount of change required to
ensure the filename is valid.
---
 server/events/runtime/runtime.go       |  5 +++-
 server/events/runtime/runtime_test.go  | 21 ++++++--------
 server/events/yaml/raw/project.go      | 13 +++++++++
 server/events/yaml/raw/project_test.go | 40 ++++++++++++++++++++++++++
 4 files changed, 66 insertions(+), 13 deletions(-)

diff --git a/server/events/runtime/runtime.go b/server/events/runtime/runtime.go
index 6ce340cab3..af89f53af6 100644
--- a/server/events/runtime/runtime.go
+++ b/server/events/runtime/runtime.go
@@ -24,7 +24,10 @@ func MustConstraint(constraint string) version.Constraints {
 	return c
 }
 
-var invalidFilenameChars = regexp.MustCompile(`[^a-zA-Z0-9-_\.]`)
+// invalidFilenameChars matches chars that are invalid for linux and windows
+// filenames.
+// From https://www.oreilly.com/library/view/regular-expressions-cookbook/9781449327453/ch08s25.html
+var invalidFilenameChars = regexp.MustCompile(`[\\/:"*?<>|]`)
 
 // GetPlanFilename returns the filename (not the path) of the generated tf plan
 // given a workspace and maybe a project's config.
diff --git a/server/events/runtime/runtime_test.go b/server/events/runtime/runtime_test.go
index 9173ca9e2b..edefa103b3 100644
--- a/server/events/runtime/runtime_test.go
+++ b/server/events/runtime/runtime_test.go
@@ -20,21 +20,11 @@ func TestGetPlanFilename(t *testing.T) {
 			nil,
 			"workspace.tfplan",
 		},
-		{
-			"workspace with space",
-			nil,
-			"workspace-with-space.tfplan",
-		},
 		{
 			"workspace",
 			&valid.Project{},
 			"workspace.tfplan",
 		},
-		{
-			"workspace with space",
-			&valid.Project{},
-			"workspace-with-space.tfplan",
-		},
 		{
 			"workspace",
 			&valid.Project{
@@ -54,14 +44,21 @@ func TestGetPlanFilename(t *testing.T) {
 			&valid.Project{
 				Name: String("project with space"),
 			},
-			"project-with-space-workspace.tfplan",
+			"project with space-workspace.tfplan",
 		},
 		{
 			"workspace😀",
 			&valid.Project{
 				Name: String("project😀"),
 			},
-			"project--workspace-.tfplan",
+			"project😀-workspace😀.tfplan",
+		},
+		{
+			"default",
+			&valid.Project{
+				Name: String(`all.invalid.chars \/"*?<>`),
+			},
+			"all.invalid.chars --------default.tfplan",
 		},
 	}
 
diff --git a/server/events/yaml/raw/project.go b/server/events/yaml/raw/project.go
index 863995238d..4ba068ccc6 100644
--- a/server/events/yaml/raw/project.go
+++ b/server/events/yaml/raw/project.go
@@ -2,6 +2,7 @@ package raw
 
 import (
 	"fmt"
+	"net/url"
 	"path/filepath"
 	"strings"
 
@@ -58,6 +59,9 @@ func (p Project) Validate() error {
 		if *strPtr == "" {
 			return errors.New("if set cannot be empty")
 		}
+		if !validProjectName(*strPtr) {
+			return fmt.Errorf("%q is not allowed: must contain only URL safe characters", *strPtr)
+		}
 		return nil
 	}
 	return validation.ValidateStruct(&p,
@@ -99,3 +103,12 @@ func (p Project) ToValid() valid.Project {
 
 	return v
 }
+
+// validProjectName returns true if the project name is valid.
+// Since the name might be used in URLs and definitely in files we don't
+// support any characters that must be url escaped *except* for '/' because
+// users like to name their projects to match the directory it's in.
+func validProjectName(name string) bool {
+	nameWithoutSlashes := strings.Replace(name, "/", "-", -1)
+	return nameWithoutSlashes == url.QueryEscape(nameWithoutSlashes)
+}
diff --git a/server/events/yaml/raw/project_test.go b/server/events/yaml/raw/project_test.go
index b2ac198f90..e6211f585a 100644
--- a/server/events/yaml/raw/project_test.go
+++ b/server/events/yaml/raw/project_test.go
@@ -143,6 +143,46 @@ func TestProject_Validate(t *testing.T) {
 			},
 			expErr: "name: if set cannot be empty.",
 		},
+		{
+			description: "project name with slashes",
+			input: raw.Project{
+				Dir:  String("."),
+				Name: String("my/name"),
+			},
+			expErr: "",
+		},
+		{
+			description: "project name with emoji",
+			input: raw.Project{
+				Dir:  String("."),
+				Name: String("😀"),
+			},
+			expErr: "name: \"😀\" is not allowed: must contain only URL safe characters.",
+		},
+		{
+			description: "project name with spaces",
+			input: raw.Project{
+				Dir:  String("."),
+				Name: String("name with spaces"),
+			},
+			expErr: "name: \"name with spaces\" is not allowed: must contain only URL safe characters.",
+		},
+		{
+			description: "project name with +",
+			input: raw.Project{
+				Dir:  String("."),
+				Name: String("namewith+"),
+			},
+			expErr: "name: \"namewith+\" is not allowed: must contain only URL safe characters.",
+		},
+		{
+			description: `project name with \`,
+			input: raw.Project{
+				Dir:  String("."),
+				Name: String(`namewith\`),
+			},
+			expErr: `name: "namewith\\" is not allowed: must contain only URL safe characters.`,
+		},
 	}
 	validation.ErrorTag = "yaml"
 	for _, c := range cases {