From b7c4058810b851f5e2cfe22542fe6776d86f9de8 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Tue, 17 Mar 2020 19:42:53 +0100 Subject: [PATCH 1/9] Validate transaction hash IDs as 64 lowecase hex chars Also, sneak in some minor improvements in `handler.go` --- .../horizon/internal/actions_transaction_test.go | 2 +- services/horizon/internal/handler.go | 13 +++++++++---- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/services/horizon/internal/actions_transaction_test.go b/services/horizon/internal/actions_transaction_test.go index d75bd59d6f..789e5dc72c 100644 --- a/services/horizon/internal/actions_transaction_test.go +++ b/services/horizon/internal/actions_transaction_test.go @@ -31,7 +31,7 @@ func TestTransactionActions_Show(t *testing.T) { // missing tx w = ht.Get("/transactions/not_real") - ht.Assert.Equal(404, w.Code) + ht.Assert.Equal(400, w.Code) } func TestTransactionActions_Show_Failed(t *testing.T) { diff --git a/services/horizon/internal/handler.go b/services/horizon/internal/handler.go index b0bd1fe566..3e4440eb3e 100644 --- a/services/horizon/internal/handler.go +++ b/services/horizon/internal/handler.go @@ -5,6 +5,7 @@ import ( "context" "crypto/sha256" "database/sql" + "encoding/hex" "encoding/json" "net/http" "strings" @@ -203,7 +204,7 @@ func (we *web) streamIndexActionHandler(jfn interface{}, sfn streamFunc) http.Ha // showActionHandler handles all non-streamable endpoints. func showActionHandler(jfn interface{}) http.HandlerFunc { - return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + return func(w http.ResponseWriter, r *http.Request) { ctx := r.Context() contentType := render.Negotiate(r) if jfn == nil || (contentType != render.MimeHal && contentType != render.MimeJSON) { @@ -223,7 +224,7 @@ func showActionHandler(jfn interface{}) http.HandlerFunc { } h.ServeHTTP(w, r) - }) + } } // getAccountID retrieves the account id by the provided key. The key is @@ -240,8 +241,7 @@ func getAccountID(r *http.Request, key string, required bool) (string, error) { return val, nil } - _, err = strkey.Decode(strkey.VersionByteAccountID, val) - if err != nil { + if _, err = strkey.Decode(strkey.VersionByteAccountID, val); err != nil { // TODO: add errInvalidValue return "", problem.MakeInvalidFieldProblem(key, errors.New("invalid address")) } @@ -252,6 +252,11 @@ func getAccountID(r *http.Request, key string, required bool) (string, error) { // getShowActionQueryParams gets the available query params for all non-indexable endpoints. func getShowActionQueryParams(r *http.Request, requireAccountID bool) (*showActionQueryParams, error) { txHash, err := hchi.GetStringFromURL(r, "tx_id") + if err == nil { + if _, err = hex.DecodeString(txHash); err != nil || len(txHash) != 64 || strings.ToLower(txHash) != txHash { + err = problem.MakeInvalidFieldProblem("tx_id", errors.New("invalid hash format")) + } + } if err != nil { return nil, errors.Wrap(err, "getting tx id") } From e5ccf7f89cc368b1ce0289566f2db362f8ec3c1e Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Tue, 17 Mar 2020 19:52:50 +0100 Subject: [PATCH 2/9] Clarify that transaction hashes are lowercase --- .../docs/reference/endpoints/effects-for-transaction.md | 2 +- .../docs/reference/endpoints/operations-for-transaction.md | 2 +- .../docs/reference/endpoints/payments-for-transaction.md | 2 +- .../internal/docs/reference/endpoints/transactions-create.md | 2 +- .../internal/docs/reference/endpoints/transactions-single.md | 2 +- services/horizon/internal/docs/reference/resources/ledger.md | 2 +- .../horizon/internal/docs/reference/resources/transaction.md | 2 +- 7 files changed, 7 insertions(+), 7 deletions(-) diff --git a/services/horizon/internal/docs/reference/endpoints/effects-for-transaction.md b/services/horizon/internal/docs/reference/endpoints/effects-for-transaction.md index 35112446fe..0c5991bd09 100644 --- a/services/horizon/internal/docs/reference/endpoints/effects-for-transaction.md +++ b/services/horizon/internal/docs/reference/endpoints/effects-for-transaction.md @@ -16,7 +16,7 @@ GET /transactions/{hash}/effects{?cursor,limit,order} | name | notes | description | example | | ---- | ----- | ----------- | ------- | -| `hash` | required, string | A transaction hash, hex-encoded | `7e2050abc676003efc3eaadd623c927f753b7a6c37f50864bf284f4e1510d088` | +| `hash` | required, string | A transaction hash, hex-encoded, lowercase. | `7e2050abc676003efc3eaadd623c927f753b7a6c37f50864bf284f4e1510d088` | | `?cursor` | optional, default _null_ | A paging token, specifying where to start returning records from. | `12884905984` | | `?order` | optional, string, default `asc` | The order in which to return rows, "asc" or "desc". | `asc` | | `?limit` | optional, number, default `10` | Maximum number of records to return. | `200` | diff --git a/services/horizon/internal/docs/reference/endpoints/operations-for-transaction.md b/services/horizon/internal/docs/reference/endpoints/operations-for-transaction.md index f0e78ea695..bcd7dd1ee1 100644 --- a/services/horizon/internal/docs/reference/endpoints/operations-for-transaction.md +++ b/services/horizon/internal/docs/reference/endpoints/operations-for-transaction.md @@ -22,7 +22,7 @@ GET /transactions/{hash}/operations{?cursor,limit,order} | name | notes | description | example | | ---- | ----- | ----------- | ------- | -| `hash` | required, string | A transaction hash, hex-encoded | `4a3365180521e16b478d9f0c9198b97a9434fc9cb07b34f83ecc32fc54d0ca8a` | +| `hash` | required, string | A transaction hash, hex-encoded, lowercase. | `4a3365180521e16b478d9f0c9198b97a9434fc9cb07b34f83ecc32fc54d0ca8a` | | `?cursor` | optional, default _null_ | A paging token, specifying where to start returning records from. | `12884905984` | | `?order` | optional, string, default `asc` | The order in which to return rows, "asc" or "desc". | `asc` | | `?limit` | optional, number, default `10` | Maximum number of records to return. | `200` | diff --git a/services/horizon/internal/docs/reference/endpoints/payments-for-transaction.md b/services/horizon/internal/docs/reference/endpoints/payments-for-transaction.md index 447bc2c0f9..29b557eb4c 100644 --- a/services/horizon/internal/docs/reference/endpoints/payments-for-transaction.md +++ b/services/horizon/internal/docs/reference/endpoints/payments-for-transaction.md @@ -29,7 +29,7 @@ GET /transactions/{hash}/payments{?cursor,limit,order} | name | notes | description | example | | ---- | ----- | ----------- | ------- | -| `hash` | required, string | A transaction hash, hex-encoded | `f65278b36875d170e865853838da400515f59ca23836f072e8d62cac18b803e5` | +| `hash` | required, string | A transaction hash, hex-encoded, lowercase. | `f65278b36875d170e865853838da400515f59ca23836f072e8d62cac18b803e5` | | `?cursor` | optional, default _null_ | A paging token, specifying where to start returning records from. | `12884905984` | | `?order` | optional, string, default `asc` | The order in which to return rows, "asc" or "desc". | `asc` | | `?limit` | optional, number, default `10` | Maximum number of records to return. | `200` | diff --git a/services/horizon/internal/docs/reference/endpoints/transactions-create.md b/services/horizon/internal/docs/reference/endpoints/transactions-create.md index c63df49b3c..97b8baa126 100644 --- a/services/horizon/internal/docs/reference/endpoints/transactions-create.md +++ b/services/horizon/internal/docs/reference/endpoints/transactions-create.md @@ -71,7 +71,7 @@ If the transaction failed or errored, then an error response will be returned. P | Name | Type | | |-------------------|--------|-----------------------------------------------------------------------| -| `hash` | string | A hex-encoded hash of the submitted transaction. | +| `hash` | string | A hex-encoded, lowercase hash of the submitted transaction. | | `ledger` | number | The ledger number that the submitted transaction was included in. | | `envelope_xdr` | string | A base64 encoded `TransactionEnvelope` [XDR](../xdr.md) object. | | `result_xdr` | string | A base64 encoded `TransactionResult` [XDR](../xdr.md) object. | diff --git a/services/horizon/internal/docs/reference/endpoints/transactions-single.md b/services/horizon/internal/docs/reference/endpoints/transactions-single.md index da48891b2f..f9d48d19b9 100644 --- a/services/horizon/internal/docs/reference/endpoints/transactions-single.md +++ b/services/horizon/internal/docs/reference/endpoints/transactions-single.md @@ -23,7 +23,7 @@ GET /transactions/{hash} | name | notes | description | example | | ------ | ------- | ----------- | ------- | -| `hash` | required, string | A transaction hash, hex-encoded. | 264226cb06af3b86299031884175155e67a02e0a8ad0b3ab3a88b409a8c09d5c | +| `hash` | required, string | A transaction hash, hex-encoded, lowercase. | 264226cb06af3b86299031884175155e67a02e0a8ad0b3ab3a88b409a8c09d5c | ### curl Example Request diff --git a/services/horizon/internal/docs/reference/resources/ledger.md b/services/horizon/internal/docs/reference/resources/ledger.md index a505b3d41c..c974f700d0 100644 --- a/services/horizon/internal/docs/reference/resources/ledger.md +++ b/services/horizon/internal/docs/reference/resources/ledger.md @@ -12,7 +12,7 @@ To learn more about the concept of ledgers in the Stellar network, take a look a |------------------------------|--------|------------------------------------------------------------------------------------------------------------------------------| | id | string | The id is a unique identifier for this ledger. | | paging_token | number | A [paging token](./page.md) suitable for use as a `cursor` parameter. | -| hash | string | A hex-encoded SHA-256 hash of the ledger's [XDR](../../learn/xdr.md)-encoded form. | +| hash | string | A hex-encoded, lowercase SHA-256 hash of the ledger's [XDR](../../learn/xdr.md)-encoded form. | | prev_hash | string | The hash of the ledger that chronologically came before this one. | | sequence | number | Sequence number of this ledger, suitable for use as the as the :id parameter for url templates that require a ledger number. | | successful_transaction_count | number | The number of successful transactions in this ledger. | diff --git a/services/horizon/internal/docs/reference/resources/transaction.md b/services/horizon/internal/docs/reference/resources/transaction.md index 11239d32fd..a1c21f5fe9 100644 --- a/services/horizon/internal/docs/reference/resources/transaction.md +++ b/services/horizon/internal/docs/reference/resources/transaction.md @@ -15,7 +15,7 @@ To learn more about the concept of transactions in the Stellar network, take a l | id | string | The canonical id of this transaction, suitable for use as the :id parameter for url templates that require a transaction's ID. | | paging_token | string | A [paging token](./page.md) suitable for use as the `cursor` parameter to transaction collection resources. | | successful | bool | Indicates if transaction was successful or not. | -| hash | string | A hex-encoded SHA-256 hash of the transaction's [XDR](../../learn/xdr.md)-encoded form. | +| hash | string | A hex-encoded, lowercase SHA-256 hash of the transaction's [XDR](../../learn/xdr.md)-encoded form. | | ledger | number | Sequence number of the ledger in which this transaction was applied. | | created_at | ISO8601 string | | | source_account | string | | From 71d2b47a91641514c40678f5352cbabad07421a7 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Tue, 17 Mar 2020 20:28:07 +0100 Subject: [PATCH 3/9] Update CHANGELOG --- services/horizon/CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/services/horizon/CHANGELOG.md b/services/horizon/CHANGELOG.md index 082f46cce8..e5478f1937 100644 --- a/services/horizon/CHANGELOG.md +++ b/services/horizon/CHANGELOG.md @@ -5,6 +5,8 @@ file. This project adheres to [Semantic Versioning](http://semver.org/). ## Unreleased +* Validate transaction hash IDs as 64 lowercase hex chars. As such, wrongly-formatted parameters which used to cause 404 (`Not found`) errors will now cause 400 (`Bad request`) HTTP errors. + ## v1.0.1 ### Fixed From 345b5ca3c5b18007ef5359abc128d5cb4bff584c Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 18 Mar 2020 13:32:56 +0100 Subject: [PATCH 4/9] Extend hash validation to all operations --- services/horizon/internal/actions/helpers.go | 67 ++++++++++++------ .../horizon/internal/actions/helpers_test.go | 68 +++++++++++++------ services/horizon/internal/actions_effects.go | 2 +- .../horizon/internal/actions_operation.go | 2 +- services/horizon/internal/handler.go | 8 +-- 5 files changed, 97 insertions(+), 50 deletions(-) diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index e559e271b5..6d2aa7f726 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -2,12 +2,14 @@ package actions import ( "context" + "encoding/hex" "fmt" "mime" "net/http" "net/url" "reflect" "strconv" + "strings" "unicode/utf8" "github.com/asaskevich/govalidator" @@ -94,14 +96,6 @@ func GetCursor(r *http.Request, name string) (string, error) { return cursor, nil } -// checkUTF8 checks if value is a valid UTF-8 string, otherwise sets -// error to `action.Err`. -func (base *Base) checkUTF8(name, value string) { - if err := checkUTF8(name, value); err != nil { - base.SetInvalidField(name, err) - } -} - func checkUTF8(name, value string) error { if !utf8.ValidString(value) { return problem.MakeInvalidFieldProblem(name, errors.New("invalid value")) @@ -110,24 +104,32 @@ func checkUTF8(name, value string) error { } // GetStringFromURLParam retrieves a string from the URLParams. -func (base *Base) GetStringFromURLParam(name string) string { - if base.Err != nil { - return "" - } - - fromURL, ok := base.GetURLParam(name) +func GetStringFromURLParam(r *http.Request, name string) (string, error) { + fromURL, ok := GetURLParam(r, name) if ok { ret, err := url.PathUnescape(fromURL) if err != nil { - base.SetInvalidField(name, err) - return "" + return "", problem.MakeInvalidFieldProblem(name, err) } - base.checkUTF8(name, ret) - return ret + if err := checkUTF8(name, ret); err != nil { + return "", err + } + return ret, nil } - return "" + return "", nil +} + +// GetStringFromURLParam retrieves a string from the URLParams. +func (base *Base) GetStringFromURLParam(name string) string { + if base.Err != nil { + return "" + } + + var ret string + ret, base.Err = GetString(base.R, name) + return ret } // GetString retrieves a string from either the URLParams, form or query string. @@ -376,6 +378,33 @@ func (base *Base) GetAddress(name string, opts ...Opt) (result string) { return result } +// GetTransactionID retireves a transaction identifier by attempting to decode an hex-encoded, +// 64-digit lowercase string at the provided name. +func GetTransactionID(r *http.Request, name string) (string, error) { + value, err := GetStringFromURLParam(r, name) + if err != nil { + return "", err + } + + if _, err = hex.DecodeString(value); err != nil || len(value) != 64 || strings.ToLower(value) != value { + return "", problem.MakeInvalidFieldProblem(name, errors.New("invalid hash format")) + } + + return value, nil +} + +// GetTransactionID retireves a transaction identifier by attempting to decode an hex-encoded, +// 64-digit lowercase string at the provided name. +func (base *Base) GetTransactionID(name string) string { + if base.Err != nil { + return "" + } + + var res string + res, base.Err = GetTransactionID(base.R, name) + return res +} + // GetAccountID retireves an xdr.AccountID by attempting to decode a stellar // address at the provided name. func GetAccountID(r *http.Request, name string) (xdr.AccountId, error) { diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go index ed0b1a298f..82e0e93530 100644 --- a/services/horizon/internal/actions/helpers_test.go +++ b/services/horizon/internal/actions/helpers_test.go @@ -34,6 +34,27 @@ func TestGetAccountID(t *testing.T) { ) } +func TestGetTransactionID(t *testing.T) { + tt := test.Start(t) + defer tt.Finish() + action := makeTestAction() + + txID := action.GetTransactionID("valid_tx_id") + tt.Assert.NoError(action.Err) + tt.Assert.Equal( + "aa168f12124b7c196c0adaee7c73a64d37f99428cacb59a91ff389626845e7cf", + txID, + ) + + txID = action.GetTransactionID("invalid_uppercase_tx_id") + tt.Assert.Error(action.Err) + + action.Err = nil + txID = action.GetTransactionID("invalid_too_short_tx_id") + tt.Assert.Error(action.Err) + +} + func TestGetAsset(t *testing.T) { tt := test.Start(t) defer tt.Finish() @@ -711,28 +732,31 @@ func makeAction(path string, body map[string]string) *Base { func testURLParams() map[string]string { return map[string]string{ - "blank": "", - "minus_one": "-1", - "zero": "0", - "two": "2", - "twenty": "20", - "32min": fmt.Sprint(math.MinInt32), - "32max": fmt.Sprint(math.MaxInt32), - "64min": fmt.Sprint(math.MinInt64), - "64max": fmt.Sprint(math.MaxInt64), - "native_asset_type": "native", - "4_asset_type": "credit_alphanum4", - "4_asset_code": "USD", - "4_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", - "12_asset_type": "credit_alphanum12", - "12_asset_code": "USD", - "12_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", - "long_4_asset_type": "credit_alphanum4", - "long_4_asset_code": "SPOOON", - "long_4_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", - "long_12_asset_type": "credit_alphanum12", - "long_12_asset_code": "OHMYGODITSSOLONG", - "long_12_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "blank": "", + "minus_one": "-1", + "zero": "0", + "two": "2", + "twenty": "20", + "32min": fmt.Sprint(math.MinInt32), + "32max": fmt.Sprint(math.MaxInt32), + "64min": fmt.Sprint(math.MinInt64), + "64max": fmt.Sprint(math.MaxInt64), + "native_asset_type": "native", + "4_asset_type": "credit_alphanum4", + "4_asset_code": "USD", + "4_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "12_asset_type": "credit_alphanum12", + "12_asset_code": "USD", + "12_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "long_4_asset_type": "credit_alphanum4", + "long_4_asset_code": "SPOOON", + "long_4_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "long_12_asset_type": "credit_alphanum12", + "long_12_asset_code": "OHMYGODITSSOLONG", + "long_12_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "valid_tx_id": "aa168f12124b7c196c0adaee7c73a64d37f99428cacb59a91ff389626845e7cf", + "invalid_upppercase_tx_id": "AA168F12124B7C196C0ADAEE7C73A64D37F99428CACB59A91FF389626845E7CF", + "invalid_too_short_tx_id": "aa168f12124b7c196c0adaee7c73a64d37f99428cacb59a91ff389626845e7", } } diff --git a/services/horizon/internal/actions_effects.go b/services/horizon/internal/actions_effects.go index a9f84f2edb..6182170549 100644 --- a/services/horizon/internal/actions_effects.go +++ b/services/horizon/internal/actions_effects.go @@ -109,7 +109,7 @@ func (action *EffectIndexAction) loadParams() { action.PagingParams = action.GetPageQuery() action.AccountFilter = action.GetAddress("account_id") action.LedgerFilter = action.GetInt32("ledger_id") - action.TransactionFilter = action.GetString("tx_id") + action.TransactionFilter = action.GetTransactionID("tx_id") action.OperationFilter = action.GetInt64("op_id") filters, err := countNonEmpty( diff --git a/services/horizon/internal/actions_operation.go b/services/horizon/internal/actions_operation.go index 64bfe21564..9d80ff81b4 100644 --- a/services/horizon/internal/actions_operation.go +++ b/services/horizon/internal/actions_operation.go @@ -131,7 +131,7 @@ func (action *OperationIndexAction) loadParams() { action.ValidateCursorAsDefault() action.AccountFilter = action.GetAddress("account_id") action.LedgerFilter = action.GetInt32("ledger_id") - action.TransactionFilter = action.GetStringFromURLParam("tx_id") + action.TransactionFilter = action.GetTransactionID("tx_id") action.PagingParams = action.GetPageQuery() action.IncludeFailed = action.GetBool("include_failed") parsed, err := parseJoinField(&action.Action.Base) diff --git a/services/horizon/internal/handler.go b/services/horizon/internal/handler.go index 3e4440eb3e..9ec88f6e90 100644 --- a/services/horizon/internal/handler.go +++ b/services/horizon/internal/handler.go @@ -5,7 +5,6 @@ import ( "context" "crypto/sha256" "database/sql" - "encoding/hex" "encoding/json" "net/http" "strings" @@ -251,12 +250,7 @@ func getAccountID(r *http.Request, key string, required bool) (string, error) { // getShowActionQueryParams gets the available query params for all non-indexable endpoints. func getShowActionQueryParams(r *http.Request, requireAccountID bool) (*showActionQueryParams, error) { - txHash, err := hchi.GetStringFromURL(r, "tx_id") - if err == nil { - if _, err = hex.DecodeString(txHash); err != nil || len(txHash) != 64 || strings.ToLower(txHash) != txHash { - err = problem.MakeInvalidFieldProblem("tx_id", errors.New("invalid hash format")) - } - } + txHash, err := actions.GetTransactionID(r, "tx_id") if err != nil { return nil, errors.Wrap(err, "getting tx id") } From fc9d99934cea378de87bc2c128e9928b30b87230 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 18 Mar 2020 14:36:46 +0100 Subject: [PATCH 5/9] Extend transaction tests --- services/horizon/internal/actions_transaction_test.go | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/services/horizon/internal/actions_transaction_test.go b/services/horizon/internal/actions_transaction_test.go index 789e5dc72c..b3a3108b48 100644 --- a/services/horizon/internal/actions_transaction_test.go +++ b/services/horizon/internal/actions_transaction_test.go @@ -30,7 +30,15 @@ func TestTransactionActions_Show(t *testing.T) { } // missing tx - w = ht.Get("/transactions/not_real") + w = ht.Get("/transactions/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff") + ht.Assert.Equal(404, w.Code) + + // uppercase tx hash not accepted + w = ht.Get("/transactions/2374E99349B9EF7DBA9A5DB3339B78FDA8F34777B1AF33BA468AD5C0DF946D4D") + ht.Assert.Equal(400, w.Code) + + // badly formated tx hash not accepted + w = ht.Get("/transactions/%00%1E4%5E%EF%BF%BD%EF%BF%BD%EF%BF%BDpVP%EF%BF%BDI&R%0BK%EF%BF%BD%1D%EF%BF%BD%EF%BF%BD=%EF%BF%BD%3F%23%EF%BF%BD%EF%BF%BDl%EF%BF%BD%1El%EF%BF%BD%EF%BF%BD") ht.Assert.Equal(400, w.Code) } From 6c8cf28867f127c7eade51bfd00b3aa5e02f4cfe Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 18 Mar 2020 15:51:41 +0100 Subject: [PATCH 6/9] Accept empty hashes This is needed since actions try to decode all parameters even if not present --- services/horizon/internal/actions/helpers.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index 6d2aa7f726..139ee879ea 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -386,8 +386,10 @@ func GetTransactionID(r *http.Request, name string) (string, error) { return "", err } - if _, err = hex.DecodeString(value); err != nil || len(value) != 64 || strings.ToLower(value) != value { - return "", problem.MakeInvalidFieldProblem(name, errors.New("invalid hash format")) + if value != "" { + if _, err = hex.DecodeString(value); err != nil || len(value) != 64 || strings.ToLower(value) != value { + return "", problem.MakeInvalidFieldProblem(name, errors.New("invalid hash format")) + } } return value, nil From ddf5898ed9e3dd37594c5dd6527a132b0b635189 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 18 Mar 2020 15:52:16 +0100 Subject: [PATCH 7/9] Fix typo --- .../horizon/internal/actions/helpers_test.go | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go index 82e0e93530..e643bbc6d7 100644 --- a/services/horizon/internal/actions/helpers_test.go +++ b/services/horizon/internal/actions/helpers_test.go @@ -732,31 +732,31 @@ func makeAction(path string, body map[string]string) *Base { func testURLParams() map[string]string { return map[string]string{ - "blank": "", - "minus_one": "-1", - "zero": "0", - "two": "2", - "twenty": "20", - "32min": fmt.Sprint(math.MinInt32), - "32max": fmt.Sprint(math.MaxInt32), - "64min": fmt.Sprint(math.MinInt64), - "64max": fmt.Sprint(math.MaxInt64), - "native_asset_type": "native", - "4_asset_type": "credit_alphanum4", - "4_asset_code": "USD", - "4_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", - "12_asset_type": "credit_alphanum12", - "12_asset_code": "USD", - "12_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", - "long_4_asset_type": "credit_alphanum4", - "long_4_asset_code": "SPOOON", - "long_4_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", - "long_12_asset_type": "credit_alphanum12", - "long_12_asset_code": "OHMYGODITSSOLONG", - "long_12_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", - "valid_tx_id": "aa168f12124b7c196c0adaee7c73a64d37f99428cacb59a91ff389626845e7cf", - "invalid_upppercase_tx_id": "AA168F12124B7C196C0ADAEE7C73A64D37F99428CACB59A91FF389626845E7CF", - "invalid_too_short_tx_id": "aa168f12124b7c196c0adaee7c73a64d37f99428cacb59a91ff389626845e7", + "blank": "", + "minus_one": "-1", + "zero": "0", + "two": "2", + "twenty": "20", + "32min": fmt.Sprint(math.MinInt32), + "32max": fmt.Sprint(math.MaxInt32), + "64min": fmt.Sprint(math.MinInt64), + "64max": fmt.Sprint(math.MaxInt64), + "native_asset_type": "native", + "4_asset_type": "credit_alphanum4", + "4_asset_code": "USD", + "4_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "12_asset_type": "credit_alphanum12", + "12_asset_code": "USD", + "12_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "long_4_asset_type": "credit_alphanum4", + "long_4_asset_code": "SPOOON", + "long_4_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "long_12_asset_type": "credit_alphanum12", + "long_12_asset_code": "OHMYGODITSSOLONG", + "long_12_asset_issuer": "GBRPYHIL2CI3FNQ4BXLFMNDLFJUNPU2HY3ZMFSHONUCEOASW7QC7OX2H", + "valid_tx_id": "aa168f12124b7c196c0adaee7c73a64d37f99428cacb59a91ff389626845e7cf", + "invalid_uppercase_tx_id": "AA168F12124B7C196C0ADAEE7C73A64D37F99428CACB59A91FF389626845E7CF", + "invalid_too_short_tx_id": "aa168f12124b7c196c0adaee7c73a64d37f99428cacb59a91ff389626845e7", } } From f7b0c509cd18506ea5dc97a73b930a13c98a5100 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 18 Mar 2020 19:49:36 +0100 Subject: [PATCH 8/9] Get rid of (*Base)GetTransactionID() --- services/horizon/internal/actions/helpers.go | 12 ------------ services/horizon/internal/actions/helpers_test.go | 14 ++++++-------- services/horizon/internal/actions_effects.go | 7 ++++++- services/horizon/internal/actions_operation.go | 7 ++++++- 4 files changed, 18 insertions(+), 22 deletions(-) diff --git a/services/horizon/internal/actions/helpers.go b/services/horizon/internal/actions/helpers.go index 139ee879ea..47ea4de4ad 100644 --- a/services/horizon/internal/actions/helpers.go +++ b/services/horizon/internal/actions/helpers.go @@ -395,18 +395,6 @@ func GetTransactionID(r *http.Request, name string) (string, error) { return value, nil } -// GetTransactionID retireves a transaction identifier by attempting to decode an hex-encoded, -// 64-digit lowercase string at the provided name. -func (base *Base) GetTransactionID(name string) string { - if base.Err != nil { - return "" - } - - var res string - res, base.Err = GetTransactionID(base.R, name) - return res -} - // GetAccountID retireves an xdr.AccountID by attempting to decode a stellar // address at the provided name. func GetAccountID(r *http.Request, name string) (xdr.AccountId, error) { diff --git a/services/horizon/internal/actions/helpers_test.go b/services/horizon/internal/actions/helpers_test.go index e643bbc6d7..035ec72272 100644 --- a/services/horizon/internal/actions/helpers_test.go +++ b/services/horizon/internal/actions/helpers_test.go @@ -39,20 +39,18 @@ func TestGetTransactionID(t *testing.T) { defer tt.Finish() action := makeTestAction() - txID := action.GetTransactionID("valid_tx_id") - tt.Assert.NoError(action.Err) + txID, err := GetTransactionID(action.R, "valid_tx_id") + tt.Assert.NoError(err) tt.Assert.Equal( "aa168f12124b7c196c0adaee7c73a64d37f99428cacb59a91ff389626845e7cf", txID, ) - txID = action.GetTransactionID("invalid_uppercase_tx_id") - tt.Assert.Error(action.Err) - - action.Err = nil - txID = action.GetTransactionID("invalid_too_short_tx_id") - tt.Assert.Error(action.Err) + txID, err = GetTransactionID(action.R, "invalid_uppercase_tx_id") + tt.Assert.Error(err) + txID, err = GetTransactionID(action.R, "invalid_too_short_tx_id") + tt.Assert.Error(err) } func TestGetAsset(t *testing.T) { diff --git a/services/horizon/internal/actions_effects.go b/services/horizon/internal/actions_effects.go index 6182170549..fa7ade8985 100644 --- a/services/horizon/internal/actions_effects.go +++ b/services/horizon/internal/actions_effects.go @@ -109,7 +109,12 @@ func (action *EffectIndexAction) loadParams() { action.PagingParams = action.GetPageQuery() action.AccountFilter = action.GetAddress("account_id") action.LedgerFilter = action.GetInt32("ledger_id") - action.TransactionFilter = action.GetTransactionID("tx_id") + var err error + action.TransactionFilter, err = actions.GetTransactionID(action.R, "tx_id") + if err != nil { + action.Err = err + return + } action.OperationFilter = action.GetInt64("op_id") filters, err := countNonEmpty( diff --git a/services/horizon/internal/actions_operation.go b/services/horizon/internal/actions_operation.go index 9d80ff81b4..6c23ee15d9 100644 --- a/services/horizon/internal/actions_operation.go +++ b/services/horizon/internal/actions_operation.go @@ -131,7 +131,12 @@ func (action *OperationIndexAction) loadParams() { action.ValidateCursorAsDefault() action.AccountFilter = action.GetAddress("account_id") action.LedgerFilter = action.GetInt32("ledger_id") - action.TransactionFilter = action.GetTransactionID("tx_id") + var err error + action.TransactionFilter, err = actions.GetTransactionID(action.R, "tx_id") + if err != nil { + action.Err = err + return + } action.PagingParams = action.GetPageQuery() action.IncludeFailed = action.GetBool("include_failed") parsed, err := parseJoinField(&action.Action.Base) From cd6eff28483f738fecbeb24471d0b3192b9a1fa1 Mon Sep 17 00:00:00 2001 From: Alfonso Acosta Date: Wed, 18 Mar 2020 20:00:16 +0100 Subject: [PATCH 9/9] Extend tx hash test to all endpoints --- services/horizon/internal/actions_effects_test.go | 14 ++++++++++++++ .../horizon/internal/actions_operation_test.go | 9 +++++++++ services/horizon/internal/actions_payment_test.go | 9 +++++++++ 3 files changed, 32 insertions(+) diff --git a/services/horizon/internal/actions_effects_test.go b/services/horizon/internal/actions_effects_test.go index 25d973e338..42479630b0 100644 --- a/services/horizon/internal/actions_effects_test.go +++ b/services/horizon/internal/actions_effects_test.go @@ -69,6 +69,20 @@ func TestEffectActions_Index(t *testing.T) { } // filtered by transaction + w = ht.Get("/transactions/2374e99349b9ef7dba9a5db3339b78fda8f34777b1af33ba468ad5c0df946d4d/effects") + if ht.Assert.Equal(200, w.Code) { + ht.Assert.PageOf(3, w.Body) + } + // missing tx + w = ht.Get("/transactions/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/effects") + ht.Assert.Equal(404, w.Code) + // uppercase tx hash not accepted + w = ht.Get("/transactions/2374E99349B9EF7DBA9A5DB3339B78FDA8F34777B1AF33BA468AD5C0DF946D4D/effects") + ht.Assert.Equal(400, w.Code) + // badly formated tx hash not accepted + w = ht.Get("/transactions/%00%1E4%5E%EF%BF%BD%EF%BF%BD%EF%BF%BDpVP%EF%BF%BDI&R%0BK%EF%BF%BD%1D%EF%BF%BD%EF%BF%BD=%EF%BF%BD%3F%23%EF%BF%BD%EF%BF%BDl%EF%BF%BD%1El%EF%BF%BD%EF%BF%BD/effects") + ht.Assert.Equal(400, w.Code) + w = ht.Get("/transactions/2374e99349b9ef7dba9a5db3339b78fda8f34777b1af33ba468ad5c0df946d4d/effects") if ht.Assert.Equal(200, w.Code) { ht.Assert.PageOf(3, w.Body) diff --git a/services/horizon/internal/actions_operation_test.go b/services/horizon/internal/actions_operation_test.go index 17e8f85526..252d47d5e2 100644 --- a/services/horizon/internal/actions_operation_test.go +++ b/services/horizon/internal/actions_operation_test.go @@ -62,6 +62,15 @@ func TestOperationActions_Index(t *testing.T) { if ht.Assert.Equal(200, w.Code) { ht.Assert.PageOf(1, w.Body) } + // missing tx + w = ht.Get("/transactions/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/operations") + ht.Assert.Equal(404, w.Code) + // uppercase tx hash not accepted + w = ht.Get("/transactions/2374E99349B9EF7DBA9A5DB3339B78FDA8F34777B1AF33BA468AD5C0DF946D4D/operations") + ht.Assert.Equal(400, w.Code) + // badly formated tx hash not accepted + w = ht.Get("/transactions/%00%1E4%5E%EF%BF%BD%EF%BF%BD%EF%BF%BDpVP%EF%BF%BDI&R%0BK%EF%BF%BD%1D%EF%BF%BD%EF%BF%BD=%EF%BF%BD%3F%23%EF%BF%BD%EF%BF%BDl%EF%BF%BD%1El%EF%BF%BD%EF%BF%BD/operations") + ht.Assert.Equal(400, w.Code) w = ht.Get("/transactions/164a5064eba64f2cdbadb856bf3448485fc626247ada3ed39cddf0f6902133b6/operations") if ht.Assert.Equal(200, w.Code) { diff --git a/services/horizon/internal/actions_payment_test.go b/services/horizon/internal/actions_payment_test.go index b7897680da..ef0da466ad 100644 --- a/services/horizon/internal/actions_payment_test.go +++ b/services/horizon/internal/actions_payment_test.go @@ -56,6 +56,15 @@ func TestPaymentActions(t *testing.T) { if ht.Assert.Equal(200, w.Code) { ht.Assert.PageOf(0, w.Body) } + // missing tx + w = ht.Get("/transactions/ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff/payments") + ht.Assert.Equal(404, w.Code) + // uppercase tx hash not accepted + w = ht.Get("/transactions/2374E99349B9EF7DBA9A5DB3339B78FDA8F34777B1AF33BA468AD5C0DF946D4D/payments") + ht.Assert.Equal(400, w.Code) + // badly formated tx hash not accepted + w = ht.Get("/transactions/%00%1E4%5E%EF%BF%BD%EF%BF%BD%EF%BF%BDpVP%EF%BF%BDI&R%0BK%EF%BF%BD%1D%EF%BF%BD%EF%BF%BD=%EF%BF%BD%3F%23%EF%BF%BD%EF%BF%BDl%EF%BF%BD%1El%EF%BF%BD%EF%BF%BD/payments") + ht.Assert.Equal(400, w.Code) // 400 for invalid tx hash w = ht.Get("/transactions/ /payments")