Open Redirect

diff --git a/pkg/api/short_url.go b/pkg/api/short_url.go index 316c8bc183ea0..22f399baf3e9f 100644 --- a/pkg/api/short_url.go +++ b/pkg/api/short_url.go @@ -2,12 +2,13 @@ package api import ( "encoding/json" + "errors" "fmt" "net/http" "net/url" "strings" - "k8s.io/apimachinery/pkg/api/errors" + k8serrors "k8s.io/apimachinery/pkg/api/errors" v1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/client-go/dynamic" @@ -162,13 +163,14 @@ func (sk8s *shortURLK8sHandler) getKubernetesRedirectFromShortURL(c *contextmode uid := web.Params(c.Req)[":uid"] if !util.IsValidShortUID(uid) { c.Logger.Warn("Invalid short URL UID format", "uid", uid) - c.Redirect(sk8s.cfg.AppURL, http.StatusFound) + c.Redirect(sk8s.cfg.AppURL, http.StatusPermanentRedirect) return } client, err := kubernetes.NewForConfig(sk8s.clientConfigProvider.GetDirectRestConfig(c)) if err != nil { - c.JsonApiErr(500, "client", err) + c.Logger.Error("Short URL redirection error: failed to create kubernetes client", "err", err) + c.Redirect(sk8s.cfg.AppURL, http.StatusTemporaryRedirect) return } @@ -182,23 +184,41 @@ func (sk8s *shortURLK8sHandler) getKubernetesRedirectFromShortURL(c *contextmode Do(c.Req.Context()) if err = result.Error(); err != nil { - c.JsonApiErr(500, "goto", err) + // Only emit a 308 when the 404 is from our API group. errors.IsNotFound + // matches any 404 from the apiserver, including infrastructure problems + // (CRD not registered, wrong API path) — those surface with + // Details.Group="meta.k8s.io" and must not be cached as a permanent + // redirect, or a client would keep bypassing a working link after the + // backend issue is fixed. + var statusErr *k8serrors.StatusError + if errors.As(err, &statusErr) && k8serrors.IsNotFound(err) && + statusErr.ErrStatus.Details != nil && + statusErr.ErrStatus.Details.Group == v1beta1.APIGroup { + c.Logger.Debug("Not redirecting short URL since not found", "uid", uid) + c.Redirect(sk8s.cfg.AppURL, http.StatusPermanentRedirect) + return + } + c.Logger.Error("Short URL redirection error: goto subresource call failed", "uid", uid, "err", err) + c.Redirect(sk8s.cfg.AppURL, http.StatusTemporaryRedirect) return } body, err := result.Raw() if err != nil { - c.JsonApiErr(500, "body", err) + c.Logger.Error("Short URL redirection error: failed to read response body", "uid", uid, "err", err) + c.Redirect(sk8s.cfg.AppURL, http.StatusTemporaryRedirect) return } value := &v1beta1.GetGotoResponse{} if err = json.Unmarshal(body, value); err != nil { - c.JsonApiErr(500, "unmarshal", err) + c.Logger.Error("Short URL redirection error: failed to unmarshal response", "uid", uid, "err", err) + c.Redirect(sk8s.cfg.AppURL, http.StatusTemporaryRedirect) return } if value.Url == "" { - c.JsonApiErr(500, "invalid", fmt.Errorf("expected url")) + c.Logger.Error("Short URL redirection error: empty url in response", "uid", uid) + c.Redirect(sk8s.cfg.AppURL, http.StatusTemporaryRedirect) return } @@ -207,7 +227,8 @@ func (sk8s *shortURLK8sHandler) getKubernetesRedirectFromShortURL(c *contextmode // so we parse it and validate the path component. parsedURL, parseErr := url.Parse(value.Url) if parseErr != nil { - c.JsonApiErr(500, "invalid redirect URL", parseErr) + c.Logger.Error("Short URL redirection error: invalid redirect URL", "uid", uid, "url", value.Url, "err", parseErr) + c.Redirect(sk8s.cfg.AppURL, http.StatusTemporaryRedirect) return } // Ensure the redirect URL is relative to this server by checking it has no scheme @@ -215,7 +236,8 @@ func (sk8s *shortURLK8sHandler) getKubernetesRedirectFromShortURL(c *contextmode // as appURL + "/" + path, we just need to verify it's not pointing externally. appParsed, appParseErr := url.Parse(sk8s.cfg.AppURL) if appParseErr != nil || appParsed.Host == "" { - c.JsonApiErr(500, "invalid app URL configuration", appParseErr) + c.Logger.Error("Short URL redirection error: invalid app URL configuration", "err", appParseErr) + c.Redirect(sk8s.cfg.AppURL, http.StatusTemporaryRedirect) return } if parsedURL.Host != "" && !strings.EqualFold(parsedURL.Host, appParsed.Host) { @@ -272,7 +294,7 @@ func (sk8s *shortURLK8sHandler) getClient(c *contextmodel.ReqContext) (dynamic.R func (sk8s *shortURLK8sHandler) writeError(c *contextmodel.ReqContext, err error) { //nolint:errorlint - statusError, ok := err.(*errors.StatusError) + statusError, ok := err.(*k8serrors.StatusError) if ok { c.JsonApiErr(int(statusError.Status().Code), statusError.Status().Message, err) return diff --git a/pkg/api/short_url_k8s_test.go b/pkg/api/short_url_k8s_test.go new file mode 100644 index 0000000000000..0bd4232af2e07 --- /dev/null +++ b/pkg/api/short_url_k8s_test.go @@ -0,0 +1,142 @@ +package api + +import ( + "encoding/json" + "net/http" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + "github.com/grafana/grafana/apps/shorturl/pkg/apis/shorturl/v1beta1" + "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" + "github.com/grafana/grafana/pkg/setting" +) + +func TestGetKubernetesRedirectFromShortURL(t *testing.T) { + const appURL = "http://localhost:3000/" + const validUID = "abcdef1234" + + tests := []struct { + name string + uid string + statusCode int + responseBody []byte + wantStatus int + wantLocation string + }{ + { + name: "invalid uid redirects to AppURL with 308", + uid: "!!!invalid", + wantStatus: http.StatusPermanentRedirect, + wantLocation: appURL, + }, + { + name: "happy path redirects to goto Url with 302", + uid: validUID, + statusCode: http.StatusOK, + responseBody: mustMarshal(t, v1beta1.GetGotoResponse{Url: appURL + "explore"}), + wantStatus: http.StatusFound, + wantLocation: appURL + "explore", + }, + { + name: "ShortURL resource not found redirects to AppURL with 308", + uid: validUID, + statusCode: http.StatusNotFound, + responseBody: mustMarshal(t, metav1.Status{ + TypeMeta: metav1.TypeMeta{Kind: "Status", APIVersion: "v1"}, + Status: metav1.StatusFailure, + Reason: metav1.StatusReasonNotFound, + Code: http.StatusNotFound, + Details: &metav1.StatusDetails{ + Group: v1beta1.APIGroup, + Kind: v1beta1.ShortURLKind().Plural(), + Name: validUID, + }, + }), + wantStatus: http.StatusPermanentRedirect, + wantLocation: appURL, + }, + { + name: "404 from unrelated resource (e.g. missing CRD) redirects to AppURL with 307", + uid: validUID, + statusCode: http.StatusNotFound, + responseBody: mustMarshal(t, metav1.Status{ + TypeMeta: metav1.TypeMeta{Kind: "Status", APIVersion: "v1"}, + Status: metav1.StatusFailure, + Reason: metav1.StatusReasonNotFound, + Code: http.StatusNotFound, + Details: &metav1.StatusDetails{ + Group: "other.grafana.app", + Kind: "widgets", + Name: validUID, + }, + }), + wantStatus: http.StatusTemporaryRedirect, + wantLocation: appURL, + }, + { + name: "server error redirects to AppURL with 307", + uid: validUID, + statusCode: http.StatusInternalServerError, + responseBody: mustMarshal(t, metav1.Status{Status: metav1.StatusFailure, Reason: metav1.StatusReasonInternalError, Code: http.StatusInternalServerError}), + wantStatus: http.StatusTemporaryRedirect, + wantLocation: appURL, + }, + { + name: "unmarshalable response redirects to AppURL with 307", + uid: validUID, + statusCode: http.StatusOK, + responseBody: []byte("not json"), + wantStatus: http.StatusTemporaryRedirect, + wantLocation: appURL, + }, + { + name: "empty url in response redirects to AppURL with 307", + uid: validUID, + statusCode: http.StatusOK, + responseBody: mustMarshal(t, v1beta1.GetGotoResponse{Url: ""}), + wantStatus: http.StatusTemporaryRedirect, + wantLocation: appURL, + }, + { + name: "external host in response redirects to AppURL with 302", + uid: validUID, + statusCode: http.StatusOK, + responseBody: mustMarshal(t, v1beta1.GetGotoResponse{Url: "http://attacker.example.com/explore"}), + wantStatus: http.StatusFound, + wantLocation: appURL, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cfg := setting.NewCfg() + cfg.AppURL = appURL + + handler := &shortURLK8sHandler{ + gvr: v1beta1.ShortURLKind().GroupVersionResource(), + namespacer: request.GetNamespaceMapper(cfg), + clientConfigProvider: &mockDirectRestConfigProvider{ + host: "http://localhost", + transport: &mockRoundTripper{statusCode: tt.statusCode, responseBody: tt.responseBody}, + }, + cfg: cfg, + } + + ctx, recorder := newTestContext(t, http.MethodGet, "/goto/"+tt.uid, map[string]string{":uid": tt.uid}) + handler.getKubernetesRedirectFromShortURL(ctx) + + assert.Equal(t, tt.wantStatus, recorder.Code) + assert.Equal(t, tt.wantLocation, recorder.Header().Get("Location")) + }) + } +} + +func mustMarshal(t *testing.T, v any) []byte { + t.Helper() + b, err := json.Marshal(v) + require.NoError(t, err) + return b +}