services/horizon: Gracefully handle incorrect assets in GET /offers (…

…#2634)
stellar · Jun 4, 2020 · 5a773bb · 5a773bb
1 parent 27bf500
commit 5a773bb
Show file tree

Hide file tree

Showing 6 changed files with 106 additions and 29 deletions.
diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go
@@ -697,7 +697,10 @@ func TestGetParams(t *testing.T) {
 
 	tt.Assert.NoError(err)
 	tt.Assert.Equal(account, qp.Account)
-	tt.Assert.True(usd.Equals(*qp.Selling()))
+	selling, err := qp.Selling()
+	tt.Assert.NoError(err)
+	tt.Assert.NotNil(selling)
+	tt.Assert.True(usd.Equals(*selling))
 
 	urlParams = map[string]string{
 		"account_id":         account,
@@ -710,7 +713,10 @@ func TestGetParams(t *testing.T) {
 
 	tt.Assert.NoError(err)
 	native := xdr.MustNewNativeAsset()
-	tt.Assert.True(native.Equals(*qp.Selling()))
+	selling, err = qp.Selling()
+	tt.Assert.NoError(err)
+	tt.Assert.NotNil(selling)
+	tt.Assert.True(native.Equals(*selling))
 
 	urlParams = map[string]string{"account_id": "1"}
 	r = makeAction("/transactions?limit=2&cursor=123456&order=desc", urlParams).R

diff --git a/services/horizon/internal/actions/offer.go b/services/horizon/internal/actions/offer.go
@@ -90,11 +90,20 @@ func (handler GetOffersHandler) GetResourcePage(
 		return nil, err
 	}
 
+	selling, err := qp.Selling()
+	if err != nil {
+		return nil, err
+	}
+	buying, err := qp.Buying()
+	if err != nil {
+		return nil, err
+	}
+
 	query := history.OffersQuery{
 		PageQuery: pq,
 		SellerID:  qp.Seller,
-		Selling:   qp.Selling(),
-		Buying:    qp.Buying(),
+		Selling:   selling,
+		Buying:    buying,
 	}
 
 	historyQ, err := HistoryQFromRequest(r)

diff --git a/services/horizon/internal/actions/offer_test.go b/services/horizon/internal/actions/offer_test.go
@@ -393,6 +393,31 @@ func TestGetOffersHandler(t *testing.T) {
 			tt.Assert.Equal(asset, offer.Buying)
 		}
 	})
+
+	t.Run("Wrong buying query parameter", func(t *testing.T) {
+		asset := horizon.Asset{}
+		eurAsset.Extract(&asset.Type, &asset.Code, &asset.Issuer)
+
+		_, err := handler.GetResourcePage(
+			httptest.NewRecorder(),
+			makeRequest(
+				t,
+				map[string]string{
+					"buying": `native\\u0026cursor=\\u0026limit=10\\u0026order=asc\\u0026selling=BTC:GAEDZ7BHMDYEMU6IJT3CTTGDUSLZWS5CQWZHGP4XUOIDG5ISH3AFAEK2`,
+				},
+				map[string]string{},
+				q.Session,
+			),
+		)
+		tt.Assert.Error(err)
+		p, ok := err.(*problem.P)
+		if tt.Assert.True(ok) {
+			tt.Assert.Equal(400, p.Status)
+			tt.Assert.NotNil(p.Extras)
+			tt.Assert.Equal(p.Extras["invalid_field"], "buying")
+			tt.Assert.Equal(p.Extras["reason"], "Asset code length is invalid")
+		}
+	})
 }
 
 func TestGetAccountOffersHandler(t *testing.T) {

diff --git a/services/horizon/internal/actions/query_params.go b/services/horizon/internal/actions/query_params.go
@@ -54,22 +54,33 @@ func (q SellingBuyingAssetQueryParams) Validate() error {
 }
 
 // Selling returns an xdr.Asset representing the selling side of the offer.
-func (q SellingBuyingAssetQueryParams) Selling() *xdr.Asset {
+func (q SellingBuyingAssetQueryParams) Selling() (*xdr.Asset, error) {
 	if len(q.SellingAsset) > 0 {
 		switch q.SellingAsset {
 		case "native":
 			asset := xdr.MustNewNativeAsset()
-			return &asset
+			return &asset, nil
 		default:
 			parts := strings.Split(q.SellingAsset, ":")
-			asset := xdr.MustNewCreditAsset(parts[0], parts[1])
-
-			return &asset
+			if len(parts) != 2 {
+				return nil, problem.MakeInvalidFieldProblem(
+					"selling",
+					errors.New("missing colon"),
+				)
+			}
+			asset, err := xdr.NewCreditAsset(parts[0], parts[1])
+			if err != nil {
+				return nil, problem.MakeInvalidFieldProblem(
+					"selling",
+					err,
+				)
+			}
+			return &asset, err
 		}
 	}
 
 	if len(q.SellingAssetType) == 0 {
-		return nil
+		return nil, nil
 	}
 
 	selling, err := xdr.BuildAsset(
@@ -79,29 +90,42 @@ func (q SellingBuyingAssetQueryParams) Selling() *xdr.Asset {
 	)
 
 	if err != nil {
-		panic(err)
+		p := problem.BadRequest
+		p.Extras = map[string]interface{}{"reason": fmt.Sprintf("bad selling asset: %s", err.Error())}
+		return nil, p
 	}
 
-	return &selling
+	return &selling, nil
 }
 
 // Buying returns an *xdr.Asset representing the buying side of the offer.
-func (q SellingBuyingAssetQueryParams) Buying() *xdr.Asset {
+func (q SellingBuyingAssetQueryParams) Buying() (*xdr.Asset, error) {
 	if len(q.BuyingAsset) > 0 {
 		switch q.BuyingAsset {
 		case "native":
 			asset := xdr.MustNewNativeAsset()
-			return &asset
+			return &asset, nil
 		default:
 			parts := strings.Split(q.BuyingAsset, ":")
-			asset := xdr.MustNewCreditAsset(parts[0], parts[1])
-
-			return &asset
+			if len(parts) != 2 {
+				return nil, problem.MakeInvalidFieldProblem(
+					"buying",
+					errors.New("missing colon"),
+				)
+			}
+			asset, err := xdr.NewCreditAsset(parts[0], parts[1])
+			if err != nil {
+				return nil, problem.MakeInvalidFieldProblem(
+					"buying",
+					err,
+				)
+			}
+			return &asset, err
 		}
 	}
 
 	if len(q.BuyingAssetType) == 0 {
-		return nil
+		return nil, nil
 	}
 
 	buying, err := xdr.BuildAsset(
@@ -111,8 +135,10 @@ func (q SellingBuyingAssetQueryParams) Buying() *xdr.Asset {
 	)
 
 	if err != nil {
-		panic(err)
+		p := problem.BadRequest
+		p.Extras = map[string]interface{}{"reason": fmt.Sprintf("bad buying asset: %s", err.Error())}
+		return nil, p
 	}
 
-	return &buying
+	return &buying, nil
 }
diff --git a/services/horizon/internal/actions/query_params_test.go b/services/horizon/internal/actions/query_params_test.go
@@ -307,8 +307,12 @@ func TestSellingBuyingAssetQueryParamsWithCanonicalRepresenation(t *testing.T) {
 
 			if len(tc.expectedInvalidField) == 0 {
 				tt.NoError(err)
-				tt.Equal(tc.expectedBuying, qp.Buying())
-				tt.Equal(tc.expectedSelling, qp.Selling())
+				selling, sellingErr := qp.Selling()
+				tt.NoError(sellingErr)
+				buying, buyingErr := qp.Buying()
+				tt.NoError(buyingErr)
+				tt.Equal(tc.expectedBuying, buying)
+				tt.Equal(tc.expectedSelling, selling)
 			} else {
 				if tt.IsType(&problem.P{}, err) {
 					p := err.(*problem.P)

diff --git a/xdr/asset.go b/xdr/asset.go
@@ -37,19 +37,26 @@ func MustNewNativeAsset() Asset {
 
 // MustNewCreditAsset returns a new general asset, panicking if it can't.
 func MustNewCreditAsset(code string, issuer string) Asset {
-	a := Asset{}
-	accountID := AccountId{}
-	err := accountID.SetAddress(issuer)
-	if err != nil {
-		panic(err)
-	}
-	err = a.SetCredit(code, accountID)
+	a, err := NewCreditAsset(code, issuer)
 	if err != nil {
 		panic(err)
 	}
 	return a
 }
 
+// NewCreditAsset returns a new general asset, returning an error if it can't.
+func NewCreditAsset(code string, issuer string) (Asset, error) {
+	a := Asset{}
+	accountID := AccountId{}
+	if err := accountID.SetAddress(issuer); err != nil {
+		return Asset{}, err
+	}
+	if err := a.SetCredit(code, accountID); err != nil {
+		return Asset{}, err
+	}
+	return a, nil
+}
+
 // BuildAsset creates a new asset from a given `assetType`, `code`, and `issuer`.
 //
 // Valid assetTypes are: