Skip to content

Commit

Permalink
Fix a few import problems: snippets and import literals.
Browse files Browse the repository at this point in the history
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 #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 #2204
  • Loading branch information
augustoroman committed Jun 16, 2018
1 parent 6965075 commit def73ff
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 36 deletions.
31 changes: 4 additions & 27 deletions caddyfile/parse.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
Expand Down Expand Up @@ -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...)
}
}
Expand Down
96 changes: 87 additions & 9 deletions caddyfile/parse_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -561,3 +561,81 @@ 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) {
file2 := writeStringToTempFileOrDie(t, `
http://example.com {
# This isn't an import directive, it's just an arg with value 'import'
gzip import foo
}
`)
// Parse the root file that defines (common) and then imports the other one.
p := testParser(`import ` + file2)
blocks, err := p.parseAll()
if err != nil {
t.Fatal(err)
}
for _, b := range blocks {
t.Log(b.Keys)
t.Log(b.Tokens)
}
gzip := blocks[0].Tokens["gzip"]
line := gzip[0].Text + " " + gzip[1].Text + " " + gzip[2].Text
if line != "gzip import foo" {
// Previously, it would be 'gzip import /path/to/test/dir/foo' referencing
// a file that (probably) doesn't exist.
t.Errorf("Expected gzip tokens to be 'gzip import foo' but got %#q", line)
}
}

func TestSnippetAcrossMultipleFiles(t *testing.T) {
// Make the derived Caddyfile that expects (common) to be defined.
file2 := 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 ` + file2 + `
`)

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)
}
}

0 comments on commit def73ff

Please sign in to comment.