diff mbox series

[v2,3/4] ci/lib: do not interpret escape sequences in `group ()` arguments

Message ID 20241210-pks-ci-section-fixes-v2-3-e087cfd174f4@pks.im (mailing list archive)
State New
Headers show
Series Random improvements to GitLab CI | expand

Commit Message

Patrick Steinhardt Dec. 10, 2024, 12:01 p.m. UTC
We use printf to set up sections with GitLab CI, which requires us to
print a bunch of escape sequences via printf. The group name is
controlled by the user and is expanded directly into the formatting
string, which may cause problems in case the argument controls escape
sequences or formatting directives.

Fix this potential issue by using formatting directives to pass variable
data.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 ci/lib.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Toon Claes Dec. 11, 2024, 12:37 p.m. UTC | #1
Patrick Steinhardt <ps@pks.im> writes:

> We use printf to set up sections with GitLab CI, which requires us to
> print a bunch of escape sequences via printf. The group name is
> controlled by the user and is expanded directly into the formatting
> string, which may cause problems in case the argument controls escape
> sequences or formatting directives.

Could it be you mean "contains" instead of "controls"?

>
> Fix this potential issue by using formatting directives to pass variable
> data.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  ci/lib.sh | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/ci/lib.sh b/ci/lib.sh
> index a54601be923bf475ba1a9cafd98bb1cb71a10255..f15f77f03a06120afbee438cee76ddc2683e1fa2 100755
> --- a/ci/lib.sh
> +++ b/ci/lib.sh
> @@ -18,7 +18,7 @@ elif test true = "$GITLAB_CI"
>  then
>  	begin_group () {
>  		need_to_end_group=t
> -		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n"
> +		printf '\e[0Ksection_start:%s:%s[collapsed=true]\r\e[0K%s\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" "$1"

Personally I find this line rather lengthy and hard to read with all the
single and double quotes. So I would suggest to split the line with a
backslash and put the arguments on a separate line. But I don't think
there's a general guideline on this, so feel free to ignore.

>  		trap "end_group '$1'" EXIT
>  		set -x
>  	}
> @@ -27,7 +27,7 @@ then
>  		test -n "$need_to_end_group" || return 0
>  		set +x
>  		need_to_end_group=
> -		printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n"
> +		printf '\e[0Ksection_end:%s:%s\r\e[0K\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)"

Same here.

But that's all I've got on this patch series. Looking good!

--
Toon
diff mbox series

Patch

diff --git a/ci/lib.sh b/ci/lib.sh
index a54601be923bf475ba1a9cafd98bb1cb71a10255..f15f77f03a06120afbee438cee76ddc2683e1fa2 100755
--- a/ci/lib.sh
+++ b/ci/lib.sh
@@ -18,7 +18,7 @@  elif test true = "$GITLAB_CI"
 then
 	begin_group () {
 		need_to_end_group=t
-		printf "\e[0Ksection_start:$(date +%s):$(echo "$1" | tr ' ' _)[collapsed=true]\r\e[0K$1\n"
+		printf '\e[0Ksection_start:%s:%s[collapsed=true]\r\e[0K%s\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)" "$1"
 		trap "end_group '$1'" EXIT
 		set -x
 	}
@@ -27,7 +27,7 @@  then
 		test -n "$need_to_end_group" || return 0
 		set +x
 		need_to_end_group=
-		printf "\e[0Ksection_end:$(date +%s):$(echo "$1" | tr ' ' _)\r\e[0K\n"
+		printf '\e[0Ksection_end:%s:%s\r\e[0K\n' "$(date +%s)" "$(echo "$1" | tr ' ' _)"
 		trap - EXIT
 	}
 else