Fix incorrect use of format strings with the conditions package.

[octo]

Many of the functions in the conditions package accept a format string and (optional) arguments, just like fmt.Printf and friends.

In many places, the code passed an error message as the format string, causing it to be interpreted by the fmt package. This leads to issues when the message contains percent signs, e.g. URL-encoded values.

Consider the following code:

// internal/controller/ocirepository_controller.go
revision, err := r.getRevision(ref, opts)
if err != nil {
	e := serror.NewGeneric(
		fmt.Errorf("failed to determine artifact digest: %w", err),
		ociv1.OCIPullFailedReason,
	)
	conditions.MarkTrue(obj, sourcev1.FetchFailedCondition, e.Reason, e.Err.Error())
	//                                                                ^^^^^^^^^^^^^
	return sreconcile.ResultEmpty, e
}

Since getRevision() includes the URL in the error message and the error message is used as a format string, the resulting condition reads:

failed to determine artifact digest: GET https://gitlab.com/jwt/auth?scope=repository%!A(MISSING)fforster%!F(MISSING)<REDACTED>%!F(MISSING)k8s-resource-manifests%!A(MISSING)pull&service=container_registry: DENIED: access forbidden

This PR adds an explicit format string and shortens e.Error() and e.Err.Error() to e, which yields the same output.

To the best of my knowledge, Go is safe from format string attacks. I don't think this is a security vulnerability, but I'm also not a security expert.

[darkowlzz]

Thanks for the findings.
The proposed solution seems good to me. I tried the following to understand it better:

func main() {
    msg := "hello %s %A\n"
    foo(msg, nil)
    foo("%s", msg)
}

func foo(format string, args ...interface{}) {
    fmt.Printf(format, args...)
}

Output:

hello %!s(<nil>) %!A(MISSING)
hello %s %A

Adding a simple string verb should be enough.
I've left a few inline comments about the usage of %v which can be replaced with %s as we are mostly dealing with message strings.

[darkowlzz]

LGTM! Thanks.
Please rebase.

[octo]

LGTM! Thanks. Please rebase.

Thank you so much for the review!

✅ rebased onto main

[stefanprodan]

LGTM

Thanks @octo 🥇

PS. This issue is present in all controllers. It would be great If you have time to help us fixing this everywhere. URLs are logged in many places, for example image-reflector-controller.

[fluxcdbot]

Backport failed for release/v1.3.x, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release/v1.3.x
git worktree add -d .worktree/backport-1529-to-release/v1.3.x origin/release/v1.3.x
cd .worktree/backport-1529-to-release/v1.3.x
git switch --create backport-1529-to-release/v1.3.x
git cherry-pick -x 8be37ef1d2dc6655143e6499073643e08ca0b9ba fa3022443ce5aa91aaf959be66a988b0bc93fac0 277e5c1d55b1d430ce5ef4e2c80f1ef45e05ea7b

[octo]

This issue is present in all controllers. It would be great If you have time to help us fixing this everywhere. URLs are logged in many places, for example image-reflector-controller.

@stefanprodan I have opened PRs for:

• notification-controller: Fix incorrect use of format strings with the conditions package. notification-controller#879
• image-automation-controller: Fix incorrect use of format strings with the conditions package. image-automation-controller#711
• image-reflector-controller: Fix incorrect use of format strings with the conditions package. image-reflector-controller#598

Already merged:

• kustomize-controller: Fix incorrect use of format strings with the conditions package. kustomize-controller#1198
• source-controller: Fix incorrect use of format strings with the conditions package. #1529 (this PR)

Did I miss any controllers that might need some TLC?

[stefanprodan]

@octo only helm-controller left. Thanks a lot for all the help!

[octo]

@octo only helm-controller left. Thanks a lot for all the help!

Only missed that in my listing. PR is here: fluxcd/helm-controller#1025