huserben / TfsExtensions

Extensions for TFS 2015+ such as custom Widgets (require TFS 2017) and Build Tasks
MIT License
44 stars 22 forks source link

Universal solution for build parameter escaping #123

Closed dgomsvek closed 4 years ago

dgomsvek commented 4 years ago

There have been a couple different issues and resolutions reported regarding the Trigger Build Task's "build parameters" input and its splitting. However we're still running into issues passing arbitrary values through as parameters.

I propose a once-and-for-all solution which should be able to maintain backwards compatibility:

Can you add a new mode to TfsRestService.buildParameterString()? When the input string value itself appears to be a jsonified string:string hash, just pass that value through as given. For example, as a minimum viable solution, something like this:

private buildParameterString(buildParameters: string): string {
    var buildParametersAsDictionary: { [id: string]: string } = {};

    if (buildParameters === null || buildParameters === undefined) {
        return "";
    }

>    if (buildParameters.startsWith("{"))
>    {
>        return buildParameters;
>    }
huserben commented 4 years ago

Hi @dgomsvek

Thanks for the input. So do I get it right that you want to pass a plain json object? Because that should already be supported when you enclose the value in curly braces (see also https://github.com/huserben/TfsExtensions/blob/master/BuildTasks/overview.md#build-parameters):

An exception to the rules above is if the value is a complete Json object, this will be treated specially. For example the following will be allowed: 01VarName: { "a": 50, "b": 50 }, 02VarName: { "Value1": 12, "OtherObject": { "SomeValue": 13, "Child": { "Name": "MyChild" } } }

Json values are detected only when the value is enclosed by curly braces. More details can be found on the following github issue.

Would that help with your problem or you need something else?

dgomsvek commented 4 years ago

Hi there, thanks for the response!

If I understand correctly, the existing functionality you're describing allows one to supply json as a value for any given parameter. What I'm proposing is to instead accept a json hash for the entire parameter set. So for your example here:

01VarName: ... , 02VarName: ...

One could pass this:

{ "01VarName": "...", "02VarName": "..." }

The reason is that with the existing comma-colon-split parsing, there are still values which are impossible to pass correctly. The option to pass a json hash for the full parameter set is just an expedient way to leverage an existing and well-supported encoding and parsing syntax, while still preserving compatibility with existing consumers. But if you have an alternative suggestion for safely round-tripping arbitrary values, that would of course work too.

huserben commented 4 years ago

Thank you for the clarification, now I understand. No I think is is actually a good proposal as it does not break existing usages.

I will have a look into building it in, however I can currently not work on it so it might take some time till it will end up in the task and be available to you.

I'll keep this thread updated if there is any new information or in case a question comes up.

v-bekif commented 4 years ago

Hi,

I'm running into a related splitting issue that would be solved by the ability to pass in a json string representing the full parameter set:

One of the parameters is a multi-delimited string, like so:

Var1Name,Var1Value;Var2Name,Var2Value;...

However, when the parameter goes through buildParameterString(), it comes out like this:

Var1Name, Var1Value;Var2Name, Var2Value;...

It'd be tricky to try and split on spaces within the sub-build, since the names could potentially have a space in them, but with the fix proposed above, one would be able to construct and pass in the json string without breaking any functionality.

huserben commented 4 years ago

Hi @dgomsvek @v-bekif

I have published a new version of the extension that includes your suggestion. In case a string is passed that is enclosed by '{' and '}' it will be taken "as is":

grafik

I hope this will solve your problems.

Could you verify it and let me know whether it worked?

dgomsvek commented 4 years ago

Thanks! We will get updated and test it.

dgomsvek commented 4 years ago

@huserben good news! This has solved our encoding issues. Thanks again!