From f781fbcff74638537fa8c4f70844a635a133ee98 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 27 Mar 2023 05:28:03 -0400 Subject: [PATCH 1/7] replacer: Implement `file.*` global replacements --- replacer.go | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/replacer.go b/replacer.go index 2ad5b8bcbbd..7c98838561c 100644 --- a/replacer.go +++ b/replacer.go @@ -24,6 +24,8 @@ import ( "strings" "sync" "time" + + "go.uber.org/zap" ) // NewReplacer returns a new Replacer. @@ -312,6 +314,26 @@ func globalDefaultReplacements(key string) (any, bool) { return os.Getenv(key[len(envPrefix):]), true } + // check files + // TODO: We may want to cache the file contents in case + // this is used in a hot path in a config. But for now, + // we'll just read the file every time, the kernel will + // tend to cache the file contents for us. + const filePrefix = "file." + if strings.HasPrefix(key, filePrefix) { + filename := key[len(filePrefix):] + body, err := os.ReadFile(filename) + if err != nil { + wd, _ := os.Getwd() + Log().Error("placeholder: failed to read file", + zap.String("file", filename), + zap.String("wd", wd), + zap.Error(err)) + return nil, true + } + return body, true + } + switch key { case "system.hostname": // OK if there is an error; just return empty string From 3a52cbbbda48cff78592039f09e2b95403f8fca5 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 27 Mar 2023 15:33:06 -0400 Subject: [PATCH 2/7] Update replacer.go Co-authored-by: Matt Holt --- replacer.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/replacer.go b/replacer.go index 7c98838561c..d7ed7c0eddf 100644 --- a/replacer.go +++ b/replacer.go @@ -327,7 +327,7 @@ func globalDefaultReplacements(key string) (any, bool) { wd, _ := os.Getwd() Log().Error("placeholder: failed to read file", zap.String("file", filename), - zap.String("wd", wd), + zap.String("working_dir", wd), zap.Error(err)) return nil, true } From c019449a4415ceb0c4af5276c154d757f776655a Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Mon, 27 Mar 2023 18:02:07 -0400 Subject: [PATCH 3/7] Read into a buffer of 1MB to limit reading huge files --- replacer.go | 26 +++++++++++++++++++++----- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/replacer.go b/replacer.go index d7ed7c0eddf..95eaba5c9dc 100644 --- a/replacer.go +++ b/replacer.go @@ -16,6 +16,7 @@ package caddy import ( "fmt" + "io" "net/http" "os" "path/filepath" @@ -315,14 +316,11 @@ func globalDefaultReplacements(key string) (any, bool) { } // check files - // TODO: We may want to cache the file contents in case - // this is used in a hot path in a config. But for now, - // we'll just read the file every time, the kernel will - // tend to cache the file contents for us. const filePrefix = "file." if strings.HasPrefix(key, filePrefix) { filename := key[len(filePrefix):] - body, err := os.ReadFile(filename) + maxSize := 1024 * 1024 + body, err := readFileIntoBuffer(filename, maxSize) if err != nil { wd, _ := os.Getwd() Log().Error("placeholder: failed to read file", @@ -369,6 +367,24 @@ func globalDefaultReplacements(key string) (any, bool) { return nil, false } +// readFileIntoBuffer reads the file at filePath into a size limited buffer. +func readFileIntoBuffer(filename string, size int) ([]byte, error) { + file, err := os.Open(filename) + if err != nil { + return nil, err + } + defer file.Close() + + buffer := make([]byte, size) + n, err := file.Read(buffer) + if err != nil && err != io.EOF { + return nil, err + } + + // slice the buffer to the actual size + return buffer[:n], nil +} + // ReplacementFunc is a function that is called when a // replacement is being performed. It receives the // variable (i.e. placeholder name) and the value that From be899f106302349ed8d7338a2ec5952a3c05f57f Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Sat, 14 Oct 2023 18:19:29 -0400 Subject: [PATCH 4/7] Opt out of file placeholders in templates --- modules/caddyhttp/templates/tplcontext.go | 6 +++ replacer.go | 62 ++++++++++++++++------- replacer_test.go | 4 +- 3 files changed, 53 insertions(+), 19 deletions(-) diff --git a/modules/caddyhttp/templates/tplcontext.go b/modules/caddyhttp/templates/tplcontext.go index 8ba64200f2b..4c7c86e13d2 100644 --- a/modules/caddyhttp/templates/tplcontext.go +++ b/modules/caddyhttp/templates/tplcontext.go @@ -249,6 +249,12 @@ func (c *TemplateContext) executeTemplateInBuffer(tplName string, buf *bytes.Buf func (c TemplateContext) funcPlaceholder(name string) string { repl := c.Req.Context().Value(caddy.ReplacerCtxKey).(*caddy.Replacer) + + // For safety, we don't want to allow the file placeholder in + // templates because it could be used to read arbitrary files + // if the template contents were not trusted. + repl = repl.WithoutFile() + value, _ := repl.GetString(name) return value } diff --git a/replacer.go b/replacer.go index 95eaba5c9dc..e5d425c9546 100644 --- a/replacer.go +++ b/replacer.go @@ -37,6 +37,7 @@ func NewReplacer() *Replacer { } rep.providers = []ReplacerFunc{ globalDefaultReplacements, + fileReplacements, rep.fromStatic, } return rep @@ -65,6 +66,24 @@ type Replacer struct { mapMutex *sync.RWMutex } +// WithoutFile returns a copy of the current Replacer +// without support for the {file.*} placeholder, which +// may be unsafe in some contexts. +func (r *Replacer) WithoutFile() *Replacer { + rep := &Replacer{static: r.static} + + // Add a func at the front of the list that + // always skips file placeholders + rep.providers = append( + []ReplacerFunc{func(key string) (any, bool) { + return nil, strings.HasPrefix(key, filePrefix) + }}, + r.providers..., + ) + + return rep +} + // Map adds mapFunc to the list of value providers. // mapFunc will be executed only at replace-time. func (r *Replacer) Map(mapFunc ReplacerFunc) { @@ -308,6 +327,30 @@ func ToString(val any) string { // returned. type ReplacerFunc func(key string) (any, bool) +// fileReplacements handles {file.*} replacements, reading +// a file from disk and replacing with its contents. +func fileReplacements(key string) (any, bool) { + if !strings.HasPrefix(key, filePrefix) { + return nil, false + } + + filename := key[len(filePrefix):] + maxSize := 1024 * 1024 + body, err := readFileIntoBuffer(filename, maxSize) + if err != nil { + wd, _ := os.Getwd() + Log().Error("placeholder: failed to read file", + zap.String("file", filename), + zap.String("working_dir", wd), + zap.Error(err)) + return nil, true + } + return body, true +} + +// globalDefaultReplacements handles replacements that can +// be used in any context, such as system variables, time, +// or environment variables. func globalDefaultReplacements(key string) (any, bool) { // check environment variable const envPrefix = "env." @@ -315,23 +358,6 @@ func globalDefaultReplacements(key string) (any, bool) { return os.Getenv(key[len(envPrefix):]), true } - // check files - const filePrefix = "file." - if strings.HasPrefix(key, filePrefix) { - filename := key[len(filePrefix):] - maxSize := 1024 * 1024 - body, err := readFileIntoBuffer(filename, maxSize) - if err != nil { - wd, _ := os.Getwd() - Log().Error("placeholder: failed to read file", - zap.String("file", filename), - zap.String("working_dir", wd), - zap.Error(err)) - return nil, true - } - return body, true - } - switch key { case "system.hostname": // OK if there is an error; just return empty string @@ -401,3 +427,5 @@ var nowFunc = time.Now const ReplacerCtxKey CtxKey = "replacer" const phOpen, phClose, phEscape = '{', '}', '\\' + +const filePrefix = "file." diff --git a/replacer_test.go b/replacer_test.go index d18ec8eeae6..14cc7e17a0c 100644 --- a/replacer_test.go +++ b/replacer_test.go @@ -374,8 +374,8 @@ func TestReplacerMap(t *testing.T) { func TestReplacerNew(t *testing.T) { rep := NewReplacer() - if len(rep.providers) != 2 { - t.Errorf("Expected providers length '%v' got length '%v'", 2, len(rep.providers)) + if len(rep.providers) != 3 { + t.Errorf("Expected providers length '%v' got length '%v'", 3, len(rep.providers)) } else { // test if default global replacements are added as the first provider hostname, _ := os.Hostname() From f999d903f868182d1a941ad4e691e64398afdd07 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Sat, 14 Oct 2023 19:00:25 -0400 Subject: [PATCH 5/7] Add tests to ensure file placeholder works, and WithoutFile() rejects --- caddytest/integration/testdata/foo.txt | 1 + replacer.go | 2 +- replacer_test.go | 140 +++++++++++++++++-------- 3 files changed, 96 insertions(+), 47 deletions(-) create mode 100644 caddytest/integration/testdata/foo.txt diff --git a/caddytest/integration/testdata/foo.txt b/caddytest/integration/testdata/foo.txt new file mode 100644 index 00000000000..19102815663 --- /dev/null +++ b/caddytest/integration/testdata/foo.txt @@ -0,0 +1 @@ +foo \ No newline at end of file diff --git a/replacer.go b/replacer.go index e5d425c9546..7cbf64bdff4 100644 --- a/replacer.go +++ b/replacer.go @@ -345,7 +345,7 @@ func fileReplacements(key string) (any, bool) { zap.Error(err)) return nil, true } - return body, true + return string(body), true } // globalDefaultReplacements handles replacements that can diff --git a/replacer_test.go b/replacer_test.go index 14cc7e17a0c..56b2f500e1c 100644 --- a/replacer_test.go +++ b/replacer_test.go @@ -372,53 +372,101 @@ func TestReplacerMap(t *testing.T) { } func TestReplacerNew(t *testing.T) { - rep := NewReplacer() - - if len(rep.providers) != 3 { - t.Errorf("Expected providers length '%v' got length '%v'", 3, len(rep.providers)) - } else { - // test if default global replacements are added as the first provider - hostname, _ := os.Hostname() - wd, _ := os.Getwd() - os.Setenv("CADDY_REPLACER_TEST", "envtest") - defer os.Setenv("CADDY_REPLACER_TEST", "") - - for _, tc := range []struct { - variable string - value string - }{ - { - variable: "system.hostname", - value: hostname, - }, - { - variable: "system.slash", - value: string(filepath.Separator), - }, - { - variable: "system.os", - value: runtime.GOOS, - }, - { - variable: "system.arch", - value: runtime.GOARCH, - }, - { - variable: "system.wd", - value: wd, - }, - { - variable: "env.CADDY_REPLACER_TEST", - value: "envtest", - }, - } { - if val, ok := rep.providers[0](tc.variable); ok { - if val != tc.value { - t.Errorf("Expected value '%s' for key '%s' got '%s'", tc.value, tc.variable, val) - } - } else { - t.Errorf("Expected key '%s' to be recognized by first provider", tc.variable) + repl := NewReplacer() + + if len(repl.providers) != 3 { + t.Errorf("Expected providers length '%v' got length '%v'", 3, len(repl.providers)) + } + + // test if default global replacements are added as the first provider + hostname, _ := os.Hostname() + wd, _ := os.Getwd() + os.Setenv("CADDY_REPLACER_TEST", "envtest") + defer os.Setenv("CADDY_REPLACER_TEST", "") + + for _, tc := range []struct { + variable string + value string + }{ + { + variable: "system.hostname", + value: hostname, + }, + { + variable: "system.slash", + value: string(filepath.Separator), + }, + { + variable: "system.os", + value: runtime.GOOS, + }, + { + variable: "system.arch", + value: runtime.GOARCH, + }, + { + variable: "system.wd", + value: wd, + }, + { + variable: "env.CADDY_REPLACER_TEST", + value: "envtest", + }, + } { + if val, ok := repl.providers[0](tc.variable); ok { + if val != tc.value { + t.Errorf("Expected value '%s' for key '%s' got '%s'", tc.value, tc.variable, val) } + } else { + t.Errorf("Expected key '%s' to be recognized by first provider", tc.variable) + } + } + + // test if file provider is added as the second provider + for _, tc := range []struct { + variable string + value string + }{ + { + variable: "file.caddytest/integration/testdata/foo.txt", + value: "foo", + }, + } { + if val, ok := repl.providers[1](tc.variable); ok { + if val != tc.value { + t.Errorf("Expected value '%s' for key '%s' got '%s'", tc.value, tc.variable, val) + } + } else { + t.Errorf("Expected key '%s' to be recognized by second provider", tc.variable) + } + } +} + +func TestReplacerNewWithoutFile(t *testing.T) { + repl := NewReplacer().WithoutFile() + + for _, tc := range []struct { + variable string + value string + expectNil bool + }{ + { + variable: "file.caddytest/integration/testdata/foo.txt", + expectNil: true, + }, + { + variable: "system.os", + value: runtime.GOOS, + }, + } { + if val, ok := repl.Get(tc.variable); ok { + if tc.expectNil && val != nil { + t.Errorf("Expected nil for key '%s' got '%v'", tc.variable, val) + } else if !tc.expectNil && val != tc.value { + t.Errorf("Expected value '%s' for key '%s' got '%s'", tc.value, tc.variable, val) + } + } else { + t.Errorf("Expected key '%s' to be recognized by second provider", tc.variable) } } } From 5af105fa9e97323fb0671b8d8adc7b5ebc55fc1c Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Sun, 15 Oct 2023 22:27:46 -0400 Subject: [PATCH 6/7] Rework replacer types to allow filtering out the file replacer Co-authored-by: Mohammed Al Sahaf --- replacer.go | 73 +++++++++++++++++++++++++++--------------------- replacer_test.go | 36 +++++++++++------------- 2 files changed, 58 insertions(+), 51 deletions(-) diff --git a/replacer.go b/replacer.go index 7cbf64bdff4..17533956012 100644 --- a/replacer.go +++ b/replacer.go @@ -35,10 +35,10 @@ func NewReplacer() *Replacer { static: make(map[string]any), mapMutex: &sync.RWMutex{}, } - rep.providers = []ReplacerFunc{ - globalDefaultReplacements, - fileReplacements, - rep.fromStatic, + rep.providers = []replacementProvider{ + globalDefaultReplacementProvider{}, + fileReplacementProvider{}, + ReplacerFunc(rep.fromStatic), } return rep } @@ -50,8 +50,8 @@ func NewEmptyReplacer() *Replacer { static: make(map[string]any), mapMutex: &sync.RWMutex{}, } - rep.providers = []ReplacerFunc{ - rep.fromStatic, + rep.providers = []replacementProvider{ + ReplacerFunc(rep.fromStatic), } return rep } @@ -60,9 +60,8 @@ func NewEmptyReplacer() *Replacer { // A default/empty Replacer is not valid; // use NewReplacer to make one. type Replacer struct { - providers []ReplacerFunc - - static map[string]any + providers []replacementProvider + static map[string]any mapMutex *sync.RWMutex } @@ -71,16 +70,12 @@ type Replacer struct { // may be unsafe in some contexts. func (r *Replacer) WithoutFile() *Replacer { rep := &Replacer{static: r.static} - - // Add a func at the front of the list that - // always skips file placeholders - rep.providers = append( - []ReplacerFunc{func(key string) (any, bool) { - return nil, strings.HasPrefix(key, filePrefix) - }}, - r.providers..., - ) - + for _, v := range r.providers { + if _, ok := v.(fileReplacementProvider); ok { + continue + } + rep.providers = append(rep.providers, v) + } return rep } @@ -101,7 +96,7 @@ func (r *Replacer) Set(variable string, value any) { // the value and whether the variable was known. func (r *Replacer) Get(variable string) (any, bool) { for _, mapFunc := range r.providers { - if val, ok := mapFunc(variable); ok { + if val, ok := mapFunc.replace(variable); ok { return val, true } } @@ -320,16 +315,28 @@ func ToString(val any) string { } } -// ReplacerFunc is a function that returns a replacement -// for the given key along with true if the function is able -// to service that key (even if the value is blank). If the -// function does not recognize the key, false should be -// returned. +// ReplacerFunc is a function that returns a replacement for the +// given key along with true if the function is able to service +// that key (even if the value is blank). If the function does +// not recognize the key, false should be returned. type ReplacerFunc func(key string) (any, bool) -// fileReplacements handles {file.*} replacements, reading -// a file from disk and replacing with its contents. -func fileReplacements(key string) (any, bool) { +func (f ReplacerFunc) replace(key string) (any, bool) { + return f(key) +} + +// replacementProvider is a type that can provide replacements +// for placeholders. Allows for type assertion to determine +// which type of provider it is. +type replacementProvider interface { + replace(key string) (any, bool) +} + +// fileReplacementsProvider handles {file.*} replacements, +// reading a file from disk and replacing with its contents. +type fileReplacementProvider struct{} + +func (f fileReplacementProvider) replace(key string) (any, bool) { if !strings.HasPrefix(key, filePrefix) { return nil, false } @@ -348,10 +355,12 @@ func fileReplacements(key string) (any, bool) { return string(body), true } -// globalDefaultReplacements handles replacements that can -// be used in any context, such as system variables, time, -// or environment variables. -func globalDefaultReplacements(key string) (any, bool) { +// globalDefaultReplacementsProvider handles replacements +// that can be used in any context, such as system variables, +// time, or environment variables. +type globalDefaultReplacementProvider struct{} + +func (f globalDefaultReplacementProvider) replace(key string) (any, bool) { // check environment variable const envPrefix = "env." if strings.HasPrefix(key, envPrefix) { diff --git a/replacer_test.go b/replacer_test.go index 56b2f500e1c..cf4d321b6ed 100644 --- a/replacer_test.go +++ b/replacer_test.go @@ -240,9 +240,9 @@ func TestReplacerSet(t *testing.T) { func TestReplacerReplaceKnown(t *testing.T) { rep := Replacer{ mapMutex: &sync.RWMutex{}, - providers: []ReplacerFunc{ + providers: []replacementProvider{ // split our possible vars to two functions (to test if both functions are called) - func(key string) (val any, ok bool) { + ReplacerFunc(func(key string) (val any, ok bool) { switch key { case "test1": return "val1", true @@ -255,8 +255,8 @@ func TestReplacerReplaceKnown(t *testing.T) { default: return "NOOO", false } - }, - func(key string) (val any, ok bool) { + }), + ReplacerFunc(func(key string) (val any, ok bool) { switch key { case "1": return "test-123", true @@ -267,7 +267,7 @@ func TestReplacerReplaceKnown(t *testing.T) { default: return "NOOO", false } - }, + }), }, } @@ -413,7 +413,7 @@ func TestReplacerNew(t *testing.T) { value: "envtest", }, } { - if val, ok := repl.providers[0](tc.variable); ok { + if val, ok := repl.providers[0].replace(tc.variable); ok { if val != tc.value { t.Errorf("Expected value '%s' for key '%s' got '%s'", tc.value, tc.variable, val) } @@ -432,7 +432,7 @@ func TestReplacerNew(t *testing.T) { value: "foo", }, } { - if val, ok := repl.providers[1](tc.variable); ok { + if val, ok := repl.providers[1].replace(tc.variable); ok { if val != tc.value { t.Errorf("Expected value '%s' for key '%s' got '%s'", tc.value, tc.variable, val) } @@ -446,27 +446,25 @@ func TestReplacerNewWithoutFile(t *testing.T) { repl := NewReplacer().WithoutFile() for _, tc := range []struct { - variable string - value string - expectNil bool + variable string + value string + notFound bool }{ { - variable: "file.caddytest/integration/testdata/foo.txt", - expectNil: true, + variable: "file.caddytest/integration/testdata/foo.txt", + notFound: true, }, { variable: "system.os", value: runtime.GOOS, }, } { - if val, ok := repl.Get(tc.variable); ok { - if tc.expectNil && val != nil { - t.Errorf("Expected nil for key '%s' got '%v'", tc.variable, val) - } else if !tc.expectNil && val != tc.value { + if val, ok := repl.Get(tc.variable); ok && !tc.notFound { + if val != tc.value { t.Errorf("Expected value '%s' for key '%s' got '%s'", tc.value, tc.variable, val) } - } else { - t.Errorf("Expected key '%s' to be recognized by second provider", tc.variable) + } else if !tc.notFound { + t.Errorf("Expected key '%s' to be recognized", tc.variable) } } } @@ -512,7 +510,7 @@ func BenchmarkReplacer(b *testing.B) { func testReplacer() Replacer { return Replacer{ - providers: make([]ReplacerFunc, 0), + providers: make([]replacementProvider, 0), static: make(map[string]any), mapMutex: &sync.RWMutex{}, } From 877220c2e4f6769ecb5cdc39f70278545ea29bf0 Mon Sep 17 00:00:00 2001 From: Francis Lavoie Date: Wed, 24 Apr 2024 15:15:46 -0400 Subject: [PATCH 7/7] Experimental comment --- replacer.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/replacer.go b/replacer.go index 17533956012..e5d2913e928 100644 --- a/replacer.go +++ b/replacer.go @@ -62,12 +62,14 @@ func NewEmptyReplacer() *Replacer { type Replacer struct { providers []replacementProvider static map[string]any - mapMutex *sync.RWMutex + mapMutex *sync.RWMutex } // WithoutFile returns a copy of the current Replacer // without support for the {file.*} placeholder, which // may be unsafe in some contexts. +// +// EXPERIMENTAL: Subject to change or removal. func (r *Replacer) WithoutFile() *Replacer { rep := &Replacer{static: r.static} for _, v := range r.providers {