Do not replace import tokens if they are part of a snippet

[WeidiDeng]

As discussed in slack, if an imported file also has a snippet with variadic placeholders and imports this snippet, parsing will fail saying not enough arguments.

Added tests show what the problem is.

When importing, caddy will always replace tokens placeholders. However, when importing a file that contains snippets definition, caddy will replace them. For {args[n]} type token, they will either be replaced or dropped, which is undesirable since they are part of a snippet.

Implement a naive algorithm to skip replacing tokens if they are part of a snippet. It does not check for syntax error, that will be done later when caddy finished importing all the tokens.

[WeidiDeng]

I cannot get rid of the warning. Don't know a good way to hide these false positives. 😞

[francislavoie]

Oh, good, I like that you simplified the fix in that last commit. I thought it was weird that the tokens were passed in.

[mholt]

Thanks for the patch Weidi 😊

[WeidiDeng]

@mholt This patch changes the how variadic placeholders work. Now it cannot be used outside snippets. args is an import feature, not a snippet feature. This patch fixes variadic placeholders by making them not available outside snippets.

The way to fix it requires at least 2 passes and obviously, rewriting how caddy file parsing works. What do you think of it?

[WeidiDeng]

I think the problem is snippets should be exempt from the replace rule but not. Implemented a naive way to check it.

[mholt]

So, to be sure I'm clear on this, does this patch cause something to stop working? Based on the comments above, it sounds like using variadic placeholders, i.e. placeholders like {args[:n]} can only be used in Caddyfile snippets. Were they able to be used other places before? 🤔

[WeidiDeng]

So, to be sure I'm clear on this, does this patch cause something to stop working? Based on the comments above, it sounds like using variadic placeholders, i.e. placeholders like {args[:n]} can only be used in Caddyfile snippets. Were they able to be used other places before? 🤔

Now they can be used in file as well, i.e. like other file imports do. There also won't be false warning about arguments index.

[mholt]

Oh, wonderful -- that sounds like a good improvement. Thank you!

@francislavoie just to double check you're good with this too? If so we can merge it 😊

[francislavoie]

LGTM!

[mholt]

Thanks for the contribution, @WeidiDeng !

[mholt]

I think this has a bug: https://caddy.community/t/problem-after-upgrading-to-2-7-2/20687/6

(matrix) {
    respond `{"m.server":"{args.0}"}`
}

localhost {
    import matrix "example.com:443"
}

With this config, the response will be example.com:443"} instead of {"m.server":"example.com:443"}.

I've observed that this only happens when the value is used in a snippet, and it does NOT happen when the JSON { is escaped (\{).

(Edit: Possibly fixed in #5685 )