fluxcd / source-controller

The GitOps Toolkit source management component
https://fluxcd.io
Apache License 2.0
231 stars 183 forks source link

Fix incorrect use of format strings with the `conditions` package. #1529

Closed octo closed 7 hours ago

octo commented 1 week ago

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.

octo commented 7 hours ago

LGTM! Thanks. Please rebase.

Thank you so much for the review!

✅ rebased onto main

fluxcdbot commented 7 hours ago

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