From 7dedd1486c252133c9fb0d2d28992c928ffc451d Mon Sep 17 00:00:00 2001 From: prettysunflower Date: Wed, 15 Apr 2026 02:58:53 -0400 Subject: [PATCH] fix(caddyfile): {block} in snippet (#7558) * fix(caddyfile): {block} in snippet Resolve issue #7557 So, here is the situation: - Pull request #7206 included some changes to the doImport's function of Caddyfile's parser. What it does is that if there is no token within a block that follows the import, and the import contains `{block}`, then the `{block}` token is discarded. - After this pull request: - Issue #7518 noticed that in cases that `{block}` was not imported, a runtime error was raised due to the assumption that tokens were always added to `tokensCopy` on every iteration of `importedTokens`. This was fixed by pull request #7543. - Issue #7557 notices that {block} can be ignored when imported from a certain file. There, it's again an issue with how the import works. When `import snippets` is called, this import instruction doesn't contains any nested blocks. And when the argument replacer that is the `importedTokens` loop is called and finds `{block}`, it uses the block from the file's import (which in this case is nothing), `{block}` is erased, and unavailable when the import directive is called for the imported snippet. The changed in this commit addresses the second issue by checking before replacing `{block}` if we're currently in a snippet definition, and appending the `{block}` token to `tokensCopy` if we are. With this changes, when importing those snippets, the `{block}` token will be available to be replaced by the nested blocks in `tokensToAdd` if needed, or erased if there are no nested blocks and `tokensToAdd` is empty. Tests added in pull requests #7206 and #7543 passes with this new implementation, confirming that unused `{block}` are accepted if nothing is passed to `import`, as well as the other usual tests. A new test was also added based on issue #7557 reporting, and also passes. Signed-off-by: prettysunflower * caddyfile: add imported snippet block placeholder coverage --------- Signed-off-by: prettysunflower Co-authored-by: Zen Dodd --- caddyconfig/caddyfile/parse.go | 6 +- caddyconfig/caddyfile/parse_test.go | 71 +++++++++++++++++++ ...snippet_invalid_subdirective.caddyfiletest | 15 ++++ ...sue_7557_invalid_subdirective_snippet.conf | 7 ++ 4 files changed, 98 insertions(+), 1 deletion(-) create mode 100644 caddytest/integration/caddyfile_adapt/import_block_snippet_invalid_subdirective.caddyfiletest create mode 100644 caddytest/integration/testdata/issue_7557_invalid_subdirective_snippet.conf diff --git a/caddyconfig/caddyfile/parse.go b/caddyconfig/caddyfile/parse.go index 6a4db5bbb..58d9b272c 100644 --- a/caddyconfig/caddyfile/parse.go +++ b/caddyconfig/caddyfile/parse.go @@ -550,7 +550,11 @@ func (p *parser) doImport(nesting int) error { } if foundBlockDirective { - tokensCopy = append(tokensCopy, tokensToAdd...) + if maybeSnippet { + tokensCopy = append(tokensCopy, token) + } else { + tokensCopy = append(tokensCopy, tokensToAdd...) + } continue } diff --git a/caddyconfig/caddyfile/parse_test.go b/caddyconfig/caddyfile/parse_test.go index 516b7dfd9..9403f7ac3 100644 --- a/caddyconfig/caddyfile/parse_test.go +++ b/caddyconfig/caddyfile/parse_test.go @@ -960,6 +960,77 @@ import `+importFile2+` } } +func TestImportedSnippetDefinitionRetainsBlockPlaceholder(t *testing.T) { + tempDir := t.TempDir() + importFile := filepath.Join(tempDir, "snippets.caddy") + + err := os.WriteFile(importFile, []byte(` + (site) { + http://{args[0]} { + respond "before" + {block} + respond "after" + } + } + `), 0o644) + if err != nil { + t.Fatalf("writing imported snippet file: %v", err) + } + + for _, tc := range []struct { + name string + input string + expectedDirectives []string + }{ + { + name: "with nested block", + input: ` + import ` + importFile + ` + + import site example.com { + redir https://example.net + } + `, + expectedDirectives: []string{"respond", "redir", "respond"}, + }, + { + name: "without nested block", + input: ` + import ` + importFile + ` + + import site example.com + `, + expectedDirectives: []string{"respond", "respond"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + p := testParser(tc.input) + blocks, err := p.parseAll() + if err != nil { + t.Fatalf("parseAll: %v", err) + } + + if len(blocks) != 1 { + t.Fatalf("expected exactly one server block, got %d", len(blocks)) + } + + if actual := blocks[0].GetKeysText(); len(actual) != 1 || actual[0] != "http://example.com" { + t.Fatalf("expected server block key http://example.com, got %v", actual) + } + + if len(blocks[0].Segments) != len(tc.expectedDirectives) { + t.Fatalf("expected %d segments, got %d", len(tc.expectedDirectives), len(blocks[0].Segments)) + } + + for i, directive := range tc.expectedDirectives { + if actual := blocks[0].Segments[i].Directive(); actual != directive { + t.Fatalf("segment %d: expected directive %q, got %q", i, directive, actual) + } + } + }) + } +} + func testParser(input string) parser { return parser{Dispenser: NewTestDispenser(input)} } diff --git a/caddytest/integration/caddyfile_adapt/import_block_snippet_invalid_subdirective.caddyfiletest b/caddytest/integration/caddyfile_adapt/import_block_snippet_invalid_subdirective.caddyfiletest new file mode 100644 index 000000000..6936bada1 --- /dev/null +++ b/caddytest/integration/caddyfile_adapt/import_block_snippet_invalid_subdirective.caddyfiletest @@ -0,0 +1,15 @@ +{ + admin off + auto_https off +} + +import testdata/issue_7557_invalid_subdirective_snippet.conf + +:8080 { + import test { + this_is_nonsense + } +} + +---------- +parsing caddyfile tokens for 'reverse_proxy': unrecognized subdirective this_is_nonsense \ No newline at end of file diff --git a/caddytest/integration/testdata/issue_7557_invalid_subdirective_snippet.conf b/caddytest/integration/testdata/issue_7557_invalid_subdirective_snippet.conf new file mode 100644 index 000000000..d7cb0c9ff --- /dev/null +++ b/caddytest/integration/testdata/issue_7557_invalid_subdirective_snippet.conf @@ -0,0 +1,7 @@ +# Used by import_block_snippet_invalid_subdirective.caddyfiletest + +(test) { + reverse_proxy { + {block} + } +} \ No newline at end of file