services/horizon: Validate transaction hash IDs as 64 lowecase hex chars

[2opremio]

Fixes #2392

PR Checklist

PR Structure

• This PR has reasonably narrow scope (if not, break it down into smaller PRs).
• This PR avoids mixing refactoring changes with feature changes (split into two PRs
  otherwise).
• This PR's title starts with name of package that is most changed in the PR, ex.
  services/friendbot, or all or doc if the changes are broad or impact many
  packages.

Thoroughness

• This PR adds tests for the most critical parts of the new functionality or fixes.
• I've updated any docs (developer docs, .md
  files, etc... affected by this change). Take a look in the docs folder for a given service,
  like this one.

Release planning

• I've updated the relevant CHANGELOG (here for Horizon) if
  needed with deprecations, added features, breaking changes, and DB schema changes.
• I've decided if this PR requires a new major/minor version according to
  semver, or if it's mainly a patch change. The PR is targeted at the next
  release branch if it's not a patch change.

What

Validate transaction ID URL parameters as 64 lowercase hexadecimal characters

Why

Injecting 0x00 was causing internal errors. Also, it's more intuitive to receive a bad request error than a not-found error when there is a problem in the format.

Known limitations

Hardcoding the length may not be a good idea if we change the hash format/length.

This requires a minor release, since we change the HTTP error returned for badly-formatted hashes.

[leighmcculloch]

Nice, it'll be way better to return 400 instead of 404 for these. Some requests regarding the tests below.

[tamirms]

@2opremio the tx_id parameter is used in the following endpoints:

GET /transactions/{tx_id}
GET /transactions/{tx_id}/effects
GET /transactions/{tx_id}/operations
GET /transactions/{tx_id}/payments

https://github.com/stellar/go/blob/master/services/horizon/internal/web.go#L234-L238

The fix you have implemented only covers the GET /transactions/{tx_id} request.

There are two patterns for implementing HTTP handlers in Horizon. The legacy pattern resembles: https://github.com/stellar/go/blob/master/services/horizon/internal/actions_effects.go
The new pattern, which we eventually want to adopt for all endpoints, is to define all http handlers in the actions package: https://github.com/stellar/go/blob/master/services/horizon/internal/actions

Unfortunately, all the endpoints which have a tx_id parameter are implemented in the legacy style. Eventually, we should refactor those endpoints and move them to the actions package. But this is outside the scope of your PR.

Our strategy for migrating endpoints to the actions package has been to implement functions which parse and validate request parameters in https://github.com/stellar/go/blob/master/services/horizon/internal/actions/helpers.go . Those functions can be used by the legacy handlers which are defined outside the actions package. But, eventually we will move the handlers to the actions package so having the validators live in https://github.com/stellar/go/blob/master/services/horizon/internal/actions/helpers.go makes the migration easier.

My suggestion is to implement a function to extract the transaction hash from a request in helpers.go . Here is a similar function that you can use as a model https://github.com/stellar/go/blob/master/services/horizon/internal/actions/helpers.go#L379-L396

Then you can call the function in each of the handlers for the endpoints I listed above.

[2opremio]

@tamirms Thanks a lot for the comprehensive explanation. I think everything is addressed. PTAL

[tamirms]

looks good to me!