caddyfile: Fix multi-file snippets and import literals. (caddyserver#2205)

* Fix a few import problems: snippets and import literals.

Two problems are fixed by this code simplification:
1. Snippets defined in one import file are strangely not available in
   another.
2. If an imported file had a directive with an argument "import", then
   the rest of the tokens on the line would be converted to absolute
   filepaths.

An example of caddyserver#2 would be the following directive in an imported file:
    basicauth / import secret

In this case, the password would actually be an absolute path to the
file 'secret' (whether or not it exists) in the directory of the imported
Caddyfile.

The problem was the blind token processing to fix import paths in the
imported tokens without considering the context of the 'import' token.

My first inclination was to just add more context (detect 'import' tokens
at the beginning of lines and check the value tokens against defined
snippets), however I eventually realized that we already do all of this
in the parser, so the code was redundant. Instead we just use the current
token's File property when importing. This works fine with imported tokens
since they already have the absolute path to the imported file!

Fixes caddyserver#2204

* renamed file2 -> fileName

* Fix copy/pasted comment in test.

* Change gzip example to basicauth example.

This makes it more clear how the import side effect is detrimental.

Author: augustoroman

caddyfile/parse.go

@@ -251,9 +251,10 @@ func (p *parser) doImport() error {
 	if p.definedSnippets != nil && p.definedSnippets[importPattern] != nil {
 		importedTokens = p.definedSnippets[importPattern]
 	} else {
-		// make path relative to Caddyfile rather than current working directory (issue #867)
-		// and then use glob to get list of matching filenames
-		absFile, err := filepath.Abs(p.Dispenser.filename)
+		// make path relative to the file of the _token_ being processed rather
+		// than current working directory (issue #867) and then use glob to get
+		// list of matching filenames
+		absFile, err := filepath.Abs(p.Dispenser.File())
 		if err != nil {
 			return p.Errf("Failed to get absolute path of file: %s: %v", p.Dispenser.filename, err)
 		}
@@ -290,30 +291,6 @@ func (p *parser) doImport() error {
 			if err != nil {
 				return err
 			}
-
-			var importLine int
-			for i, token := range newTokens {
-				if token.Text == "import" {
-					importLine = token.Line
-					continue
-				}
-				if token.Line == importLine {
-					var abs string
-					if filepath.IsAbs(token.Text) {
-						abs = token.Text
-					} else if !filepath.IsAbs(importFile) {
-						abs = filepath.Join(filepath.Dir(absFile), token.Text)
-					} else {
-						abs = filepath.Join(filepath.Dir(importFile), token.Text)
-					}
-					newTokens[i] = Token{
-						Text: abs,
-						Line: token.Line,
-						File: token.File,
-					}
-				}
-			}
-
 			importedTokens = append(importedTokens, newTokens...)
 		}
 	}

caddyfile/parse_test.go

@@ -527,15 +527,15 @@ func testParser(input string) parser {
 }
 
 func TestSnippets(t *testing.T) {
-	p := testParser(`(common) {
-					gzip foo
-					errors stderr
-					
-				}	
-				http://example.com {
-					import common
-				}
-			`)
+	p := testParser(`
+		(common) {
+			gzip foo
+			errors stderr
+		}
+		http://example.com {
+			import common
+		}
+	`)
 	blocks, err := p.parseAll()
 	if err != nil {
 		t.Fatal(err)
@@ -561,3 +561,83 @@ func TestSnippets(t *testing.T) {
 	}
 
 }
+
+func writeStringToTempFileOrDie(t *testing.T, str string) (pathToFile string) {
+	file, err := ioutil.TempFile("", t.Name())
+	if err != nil {
+		panic(err) // get a stack trace so we know where this was called from.
+	}
+	if _, err := file.WriteString(str); err != nil {
+		panic(err)
+	}
+	if err := file.Close(); err != nil {
+		panic(err)
+	}
+	return file.Name()
+}
+
+func TestImportedFilesIgnoreNonDirectiveImportTokens(t *testing.T) {
+	fileName := writeStringToTempFileOrDie(t, `
+		http://example.com {
+			# This isn't an import directive, it's just an arg with value 'import'
+			basicauth / import password
+		}
+	`)
+	// Parse the root file that imports the other one.
+	p := testParser(`import ` + fileName)
+	blocks, err := p.parseAll()
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, b := range blocks {
+		t.Log(b.Keys)
+		t.Log(b.Tokens)
+	}
+	auth := blocks[0].Tokens["basicauth"]
+	line := auth[0].Text + " " + auth[1].Text + " " + auth[2].Text + " " + auth[3].Text
+	if line != "basicauth / import password" {
+		// Previously, it would be changed to:
+		//   basicauth / import /path/to/test/dir/password
+		// referencing a file that (probably) doesn't exist and changing the
+		// password!
+		t.Errorf("Expected basicauth tokens to be 'basicauth / import password' but got %#q", line)
+	}
+}
+
+func TestSnippetAcrossMultipleFiles(t *testing.T) {
+	// Make the derived Caddyfile that expects (common) to be defined.
+	fileName := writeStringToTempFileOrDie(t, `
+		http://example.com {
+			import common
+		}
+	`)
+
+	// Parse the root file that defines (common) and then imports the other one.
+	p := testParser(`
+		(common) {
+			gzip foo
+		}
+		import ` + fileName + `
+	`)
+
+	blocks, err := p.parseAll()
+	if err != nil {
+		t.Fatal(err)
+	}
+	for _, b := range blocks {
+		t.Log(b.Keys)
+		t.Log(b.Tokens)
+	}
+	if len(blocks) != 1 {
+		t.Fatalf("Expect exactly one server block. Got %d.", len(blocks))
+	}
+	if actual, expected := blocks[0].Keys[0], "http://example.com"; expected != actual {
+		t.Errorf("Expected server name to be '%s' but was '%s'", expected, actual)
+	}
+	if len(blocks[0].Tokens) != 1 {
+		t.Fatalf("Server block should have tokens from import")
+	}
+	if actual, expected := blocks[0].Tokens["gzip"][0].Text, "gzip"; expected != actual {
+		t.Errorf("Expected argument to be '%s' but was '%s'", expected, actual)
+	}
+}