Merge pull request jamestelfer#25 from jamestelfer/handler-validation

fix: improve validation of Git credentials request

handlers.go

@@ -4,7 +4,7 @@ import (
 	"encoding/json"
 	"io"
 	"net/http"
-	"net/url"
+	"strings"
 
 	"github.com/jamestelfer/chinmina-bridge/internal/credentialhandler"
 	"github.com/jamestelfer/chinmina-bridge/internal/jwt"
@@ -59,18 +59,14 @@ func handlePostGitCredentials(tokenVendor vendor.PipelineTokenVendor) http.Handl
 			return
 		}
 
-		u, _ := url.Parse("https://github.com")
-		if protocol, ok := requestedRepo.Lookup("protocol"); ok {
-			u.Scheme = protocol
-		}
-		if host, ok := requestedRepo.Lookup("host"); ok {
-			u.Host = host
-		}
-		if path, ok := requestedRepo.Lookup("path"); ok {
-			u.Path = path
+		requestedRepoURL, err := credentialhandler.ConstructRepositoryURL(requestedRepo)
+		if err != nil {
+			log.Info().Msgf("invalid request parameters %v\n", err)
+			requestError(w, http.StatusBadRequest)
+			return
 		}
 
-		tokenResponse, err := tokenVendor(r.Context(), claims, u.String())
+		tokenResponse, err := tokenVendor(r.Context(), claims, requestedRepoURL)
 		if err != nil {
 			log.Info().Msgf("token creation failed %v\n", err)
 			requestError(w, http.StatusInternalServerError)
@@ -79,10 +75,13 @@ func handlePostGitCredentials(tokenVendor vendor.PipelineTokenVendor) http.Handl
 
 		w.Header().Set("Content-Type", "text/plain")
 
-		// repo mismatch: empty return
+		// Given repository doesn't match the pipeline: empty return this means
+		// that we understand the request but cannot fulfil it: this is a
+		// successful case for a credential helper, so we successfully return
+		// but don't offer credentials.
 		if tokenResponse == nil {
-			w.WriteHeader(http.StatusOK)
 			w.Header().Add("Content-Length", "0")
+			w.WriteHeader(http.StatusOK)
 
 			return
 		}
@@ -98,7 +97,7 @@ func handlePostGitCredentials(tokenVendor vendor.PipelineTokenVendor) http.Handl
 		props := credentialhandler.NewMap(6)
 		props.Set("protocol", tokenURL.Scheme)
 		props.Set("host", tokenURL.Host)
-		props.Set("path", tokenURL.Path)
+		props.Set("path", strings.TrimPrefix(tokenURL.Path, "/"))
 		props.Set("username", "x-access-token")
 		props.Set("password", tokenResponse.Token)
 		props.Set("password_expiry_utc", tokenResponse.ExpiryUnix())

handlers_test.go

@@ -12,6 +12,7 @@ import (
 	"time"
 
 	"github.com/auth0/go-jwt-middleware/v2/validator"
+	"github.com/jamestelfer/chinmina-bridge/internal/credentialhandler"
 	"github.com/jamestelfer/chinmina-bridge/internal/jwt"
 	"github.com/jamestelfer/chinmina-bridge/internal/vendor"
 	"github.com/stretchr/testify/assert"
@@ -54,16 +55,7 @@ func TestHandlers_RequireClaims(t *testing.T) {
 func TestHandlePostToken_ReturnsTokenOnSuccess(t *testing.T) {
 	tokenVendor := tv("expected-token-value")
 
-	ctx := context.Background()
-	ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{
-		RegisteredClaims: validator.RegisteredClaims{
-			Issuer: "https://buildkite.com",
-		},
-		CustomClaims: &jwt.BuildkiteClaims{
-			OrganizationSlug: "organization-slug",
-			PipelineSlug:     "pipeline-slug",
-		},
-	})
+	ctx := claimsContext()
 
 	req, err := http.NewRequest("POST", "/token", nil)
 	require.NoError(t, err)
@@ -93,16 +85,7 @@ func TestHandlePostToken_ReturnsTokenOnSuccess(t *testing.T) {
 func TestHandlePostToken_ReturnsFailureOnVendorFailure(t *testing.T) {
 	tokenVendor := tvFails(errors.New("vendor failure"))
 
-	ctx := context.Background()
-	ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{
-		RegisteredClaims: validator.RegisteredClaims{
-			Issuer: "https://buildkite.com",
-		},
-		CustomClaims: &jwt.BuildkiteClaims{
-			OrganizationSlug: "organization-slug",
-			PipelineSlug:     "pipeline-slug",
-		},
-	})
+	ctx := claimsContext()
 
 	req, err := http.NewRequest("POST", "/token", nil)
 	require.NoError(t, err)
@@ -123,18 +106,15 @@ func TestHandlePostToken_ReturnsFailureOnVendorFailure(t *testing.T) {
 func TestHandlePostGitCredentials_ReturnsTokenOnSuccess(t *testing.T) {
 	tokenVendor := tv("expected-token-value")
 
-	ctx := context.Background()
-	ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{
-		RegisteredClaims: validator.RegisteredClaims{
-			Issuer: "https://buildkite.com",
-		},
-		CustomClaims: &jwt.BuildkiteClaims{
-			OrganizationSlug: "organization-slug",
-			PipelineSlug:     "pipeline-slug",
-		},
-	})
+	ctx := claimsContext()
+
+	m := credentialhandler.NewMap(10)
+	m.Set("protocol", "https")
+	m.Set("host", "github.com")
+	m.Set("path", "org/repo")
 
-	body := bytes.NewBufferString("\n\n\n\nuseless-content")
+	body := &bytes.Buffer{}
+	credentialhandler.WriteProperties(m, body)
 	req, err := http.NewRequest("POST", "/git-credentials", body)
 	require.NoError(t, err)
 
@@ -150,24 +130,109 @@ func TestHandlePostGitCredentials_ReturnsTokenOnSuccess(t *testing.T) {
 	assert.Equal(t, "text/plain", rr.Header().Get("Content-Type"))
 
 	respBody := rr.Body.String()
-	assert.Equal(t, "protocol=https\nhost=github.com\npath=\nusername=x-access-token\npassword=expected-token-value\npassword_expiry_utc=1715104776\n\n", respBody)
+	assert.Equal(t, "protocol=https\nhost=github.com\npath=org/repo\nusername=x-access-token\npassword=expected-token-value\npassword_expiry_utc=1715104776\n\n", respBody)
+}
+
+func TestHandlePostGitCredentials_ReturnsEmptySuccessWhenNoToken(t *testing.T) {
+	tokenVendor := vendor.PipelineTokenVendor(func(_ context.Context, claims jwt.BuildkiteClaims, repoUrl string) (*vendor.PipelineRepositoryToken, error) {
+		return nil, nil
+	})
+
+	ctx := claimsContext()
+
+	m := credentialhandler.NewMap(10)
+	m.Set("protocol", "https")
+	m.Set("host", "github.com")
+	m.Set("path", "org/repo")
+
+	body := &bytes.Buffer{}
+	credentialhandler.WriteProperties(m, body)
+	req, err := http.NewRequest("POST", "/git-credentials", body)
+	require.NoError(t, err)
+
+	req = req.WithContext(ctx)
+	rr := httptest.NewRecorder()
+
+	// act
+	handler := handlePostGitCredentials(tokenVendor)
+	handler.ServeHTTP(rr, req)
+
+	// assert
+	r := rr.Result()
+	assert.Equal(t, http.StatusOK, r.StatusCode)
+	assert.Equal(t, "text/plain", r.Header.Get("Content-Type"))
+	assert.Equal(t, "0", r.Header.Get("Content-Length"))
+	assert.Equal(t, int64(0), r.ContentLength)
+
+	respBody := rr.Body.String()
+	assert.Equal(t, "", respBody)
+}
+func TestHandlePostGitCredentials_ReturnsFailureOnInvalidRequest(t *testing.T) {
+	tokenVendor := tv("expected-token-value")
+
+	ctx := claimsContext()
+
+	req, err := http.NewRequest("POST", "/git-credentials", nil)
+	require.NoError(t, err)
+
+	req = req.WithContext(ctx)
+	rr := httptest.NewRecorder()
+
+	// act
+	handler := handlePostGitCredentials(tokenVendor)
+	handler.ServeHTTP(rr, req)
+
+	// assert
+	assert.Equal(t, http.StatusBadRequest, rr.Code)
+	// important to know that internal details aren't part of the error response
+	assert.Equal(t, "Bad Request\n", rr.Body.String())
+}
+
+func TestHandlePostGitCredentials_ReturnsFailureOnReadFailure(t *testing.T) {
+	tokenVendor := tv("expected-token-value")
+
+	ctx := claimsContext()
+
+	m := credentialhandler.NewMap(10)
+	m.Set("protocol", "https")
+	m.Set("host", "github.com")
+	m.Set("path", "org/repo")
+
+	body := &bytes.Buffer{}
+	credentialhandler.WriteProperties(m, body)
+
+	req, err := http.NewRequest("POST", "/git-credentials", body)
+	require.NoError(t, err)
+
+	req = req.WithContext(ctx)
+	rr := httptest.NewRecorder()
+
+	// act
+	handler := maxRequestSize(1)(
+		// use the request size limit to force an error in the credentials handler
+		handlePostGitCredentials(tokenVendor),
+	)
+	handler.ServeHTTP(rr, req)
+
+	// assert
+	assert.Equal(t, http.StatusInternalServerError, rr.Code)
+	// important to know that internal details aren't part of the error response
+	assert.Equal(t, "Internal Server Error\n", rr.Body.String())
 }
 
 func TestHandlePostGitCredentials_ReturnsFailureOnVendorFailure(t *testing.T) {
 	tokenVendor := tvFails(errors.New("vendor failure"))
 
-	ctx := context.Background()
-	ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{
-		RegisteredClaims: validator.RegisteredClaims{
-			Issuer: "https://buildkite.com",
-		},
-		CustomClaims: &jwt.BuildkiteClaims{
-			OrganizationSlug: "organization-slug",
-			PipelineSlug:     "pipeline-slug",
-		},
-	})
+	ctx := claimsContext()
 
-	req, err := http.NewRequest("POST", "/token", nil)
+	m := credentialhandler.NewMap(10)
+	m.Set("protocol", "https")
+	m.Set("host", "github.com")
+	m.Set("path", "org/repo")
+
+	body := &bytes.Buffer{}
+	credentialhandler.WriteProperties(m, body)
+	req, err := http.NewRequest("POST", "/git-credentials", body)
 	require.NoError(t, err)
 
 	req = req.WithContext(ctx)
@@ -201,6 +266,22 @@ func tvFails(err error) vendor.PipelineTokenVendor {
 	})
 }
 
+func claimsContext() context.Context {
+	ctx := context.Background()
+
+	ctx = jwt.ContextWithClaims(ctx, &validator.ValidatedClaims{
+		RegisteredClaims: validator.RegisteredClaims{
+			Issuer: "https://buildkite.com",
+		},
+		CustomClaims: &jwt.BuildkiteClaims{
+			OrganizationSlug: "organization-slug",
+			PipelineSlug:     "pipeline-slug",
+		},
+	})
+
+	return ctx
+}
+
 func TestMaxRequestSizeMiddleware(t *testing.T) {
 
 	mw := maxRequestSize(10)

internal/credentialhandler/marshal.go

@@ -5,6 +5,7 @@ import (
 	"bytes"
 	"errors"
 	"io"
+	"net/url"
 	"strings"
 )
 
@@ -74,3 +75,28 @@ func WriteProperties(props *ArrayMap, w io.Writer) error {
 
 	return nil
 }
+
+func ConstructRepositoryURL(props *ArrayMap) (string, error) {
+	u := &url.URL{}
+
+	protocol, ok := props.Lookup("protocol")
+	if !ok {
+		return "", errors.New("protocol/scheme must be present")
+	}
+
+	host, ok := props.Lookup("host")
+	if !ok {
+		return "", errors.New("host must be present")
+	}
+
+	path, ok := props.Lookup("path")
+	if !ok {
+		return "", errors.New("path must be present")
+	}
+
+	u.Scheme = protocol
+	u.Host = host
+	u.Path = path
+
+	return u.String(), nil
+}