search

OctopusDeploy / Octostache

| Public | The variable substitution syntax for Octopus Deploy
Other
5 stars 24 forks source link

TemplateParser.ParseTemplate() crashes if the template contains a reference to a variable with "special" characters in name #53

Open jjanuszkiewicz opened 3 years ago

jjanuszkiewicz commented 3 years ago

A call to TemplateParser.ParseTemplate(foo) throws an exception on some valid input such as #{foo!}:

Parsing failure: unexpected '#'; expected end of input (Line 1, Column 1); recently consumed: 
at Sprache.ParserExtensions.Parse[T](Parser`1 parser, String input)
at Octostache.Templates.TemplateParser.ParseTemplate(String template)
at IHS.Castle.Tools.OctopusUtilities.WebApi.Program.Main(String[] args) in S:\Connect.BuildTools.TestAndSources\src\IHS.Castle.Tools.OctopusUtilities.WebApi\Program.cs:line 14

Octopus Deploy allows to create variables with all kinds of characters in the name, even crazy ones like !@#$%^&*()_+-={}[]:";'<>?,./ (yes, I was able to create a variable called just that for a test), which while not the greatest practice, should be supported by this library. My team had one legitimate case where one of those characters was used, but this crashed TemplateParser.

Looking at the code of TemplateParser, it only allows the following non-alphanumerical, non-whitespace characters: _-:/~().

I could create a PR to change this, but I can't find any documentation on what characters are actually supported by Octopus Deploy in variable names.

droyad commented 3 years ago

@jjanuszkiewicz Octopus does not impose any limitation on the variable name. It also supports spaces.

My main concern with a parser change like this is performance on big documents (e.g. some customers do variable substitutions on files megabytes in size.

I like the idea of extending the parsing, but I think having some exclusions would be a good idea. Off the top of my head, space, { and } would make sense to me.

We've taken the stance that variable names could be anything, but if you want to use octostache substitution, then they have to do X, Y and Z.

jjanuszkiewicz commented 3 years ago

@droyad Thanks for the response. I'm not sure I follow, though. Isn't Octostache the official way for doing the substitution? The home page states "Octostache is the variable substitution syntax for Octopus Deploy", so I expected it to handle any valid Octopus variables, but maybe I understood it wrong.

Is Octopus itself using Octostache internally? Curiously, in our case, when a variable had ? in the name, the substitution worked just fine during deployment by Octopus. It was only our custom tool that uses Octostache that crashed.

I'm just not sure in which direction you'd like this to move - can the rules be completely relaxed to handle all valid variable names, as long as performance is not affected? Are there some unit tests/benchmarks for those big document cases in the repo?