Fix account ownership check #383

godexsoft · 2022-11-03T21:09:25Z

Fixes #222

When recreating the cursor from a marker in account_objects we call out to 'isOwnedByAccount' which seems to be a safety net of sorts.
If the marker happens to land on an entry where the accountID we check against is the destination the check will fail.
The fix just adds a check for this case.

On a second thought tho, maybe we don't need this isOwnedByAccount check at all?

cjcobb23 · 2022-11-03T21:18:37Z

So I think we can probably remove this ownership check. All we are doing is trying to make sure the marker is a valid point to start iteration. I think we can do that check in here:

clio/src/rpc/RPCHelpers.cpp

Line 712 in 8cc2de5

if (hexMarker.isNonZero())

and it would be much less likely to be buggy. Specifically, if hintDir is not found, or if it is but hexMarker is not found in sfIndexes, return an error.

godexsoft · 2022-11-04T01:32:02Z

Specifically, if hintDir is not found, or if it is but hexMarker is not found in sfIndexes, return an error.

Take a look if it's what you wanted. I was not sure what to put as the error strings so if you have better suggestions - let me know.

cjcobb23

What is rippled's behavior when you pass an invalid marker?

godexsoft · 2022-11-08T14:47:52Z

What is rippled's behavior when you pass an invalid marker?

@cjcobb23 This is what i got sending an invalid marker:

{
  "result": {
    "error": "invalidParams",
    "error_code": 31,
    "error_message": "Invalid field 'marker'.",
    "request": {
      "account": "raeR1nAGkMm18hHpUz1ZP6wGKLA4WCh8rX",
      "command": "account_objects",
      "deletion_blockers_only": false,
      "limit": 10,
      "marker": "1111",
      "validated": true
    },
    "status": "error"
  }
}

If i use a valid marker but change one character in it i get this:

{
  "result": {
    "account": "raeR1nAGkMm18hHpUz1ZP6wGKLA4WCh8rX",
    "account_objects": [],
    "ledger_current_index": 23293951,
    "status": "success",
    "validated": false
  }
}

For the same type of change, with clio i get this:

{
  "result": {
    "error": "invalidParams",
    "error_code": 31,
    "error_message": "Malformed cursor",
    "status": "error",
    "type": "response",
    "request": {
      "method": "account_objects",
      "params": [
        {
          "marker": "17D55F149C997B7A15F683F9C568A2087DCEAD913962689F9E0A7A56E5F423C5,0",
          "account": "raeR1nAGkMm18hHpUz1ZP6wGKLA4WCh8rX",
          "deletion_blockers_only": false,
          "limit": 10,
          "validated": true
        }
      ]
    }
  },
  "warnings": [
    {
      "id": 2001,
      "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request"
    }
  ]
}

I also get exactly the same error for any other completely malformed marker.

So i guess the takeaway is that we need to distinguish between invalid format of a marker and a marker that does not yield a cursor, right?

cjcobb23 · 2022-11-14T18:25:27Z

What is rippled's behavior when you pass an invalid marker?

@cjcobb23 This is what i got sending an invalid marker:

{
  "result": {
    "error": "invalidParams",
    "error_code": 31,
    "error_message": "Invalid field 'marker'.",
    "request": {
      "account": "raeR1nAGkMm18hHpUz1ZP6wGKLA4WCh8rX",
      "command": "account_objects",
      "deletion_blockers_only": false,
      "limit": 10,
      "marker": "1111",
      "validated": true
    },
    "status": "error"
  }
}

If i use a valid marker but change one character in it i get this:

{
  "result": {
    "account": "raeR1nAGkMm18hHpUz1ZP6wGKLA4WCh8rX",
    "account_objects": [],
    "ledger_current_index": 23293951,
    "status": "success",
    "validated": false
  }
}

For the same type of change, with clio i get this:

{
  "result": {
    "error": "invalidParams",
    "error_code": 31,
    "error_message": "Malformed cursor",
    "status": "error",
    "type": "response",
    "request": {
      "method": "account_objects",
      "params": [
        {
          "marker": "17D55F149C997B7A15F683F9C568A2087DCEAD913962689F9E0A7A56E5F423C5,0",
          "account": "raeR1nAGkMm18hHpUz1ZP6wGKLA4WCh8rX",
          "deletion_blockers_only": false,
          "limit": 10,
          "validated": true
        }
      ]
    }
  },
  "warnings": [
    {
      "id": 2001,
      "message": "This is a clio server. clio only serves validated data. If you want to talk to rippled, include 'ledger_index':'current' in your request"
    }
  ]
}

I also get exactly the same error for any other completely malformed marker.

So i guess the takeaway is that we need to distinguish between invalid format of a marker and a marker that does not yield a cursor, right?

Yeah one is malformed, and one is sort of invalid but well formed at least. It looks like rippled is not returning an error for the invalid case, just returning nothing. So we should just copy that.

src/rpc/Errors.h

src/rpc/RPCHelpers.cpp

src/rpc/handlers/Subscribe.cpp

src/rpc/RPCHelpers.cpp

cjcobb23

LGTM

Fix account ownership check [FOLD] Apply changes suggested in review [FOLD] Implement workaround for empty account_objects [FOLD] Return empty resultset for account_objects with bogus marker [FOLD] Patch up double return due to clang-format bug [FOLD] Put back ledger_index in response as per docs

Fixes XRPLF#222

* Implement logging abstraction (#371) Fixes #290 * Fix pre-commit to only check staged files * Implement account ownership check and fix paging (#383) Fixes #222 * Remove the github action package signing step This will be done elsewhere. * include searched_all in error response of tx (#407) * helper function for subscribe to ensure cleanup (#402) * Add closed to header for all paths of ledger_data (#416) Fixes #219 * Add custom error for malformed owner and request (#417) Fixes #274 * Use custom malformedAddress error in ledger_entry (#419) Fixes #272 * Return lgrIdxsInvalid error for ledger_max_index less than ledger_min_index (#339) Fixes #263 * Update headers to use #pragma once * Add custom error for malformed request (#414) Fixes #276 * Return srcCurMalformed on invalid taker_pays in book_offers (#413) Fixes #267 * Fix source_location issue on MacOSX and Debug build (#431) Fixes #428 * Implement always adding git ref to version string (#430) Fixes #427 * add connection counting (#433) * Fix malformed output format over ws rpc (#426) Fixes #405 * Remove branch name from version string (#437) Fixes a bug from #430 * Implement cli parsing using boost::po (#436) Fixes #367 * Update documentation and config with ssl_cert_file and ssl_key_file (#443) Fixes #424 * Fix gateway balances to match rippled output (#441) Fixes #271 * Update README and example config to describe start_sequence (#438) Fixes #250 * Add copyright to top of each source file (#444) Fixes #411 * Increase file descriptor limit (#449) * Update readme with more log configurations (#447) Fixes #446 * Document dos_guard in example config. Log when client surpasses rate limit (#451) * Add unit tests for DOSGuard (#453) Fixes #452 * Build macOS and Ubuntu 22.04 (#456) build release/x.y.z branches * Add time measurement profiler (#458) Rebase * Match format to rippled error code (#461) Fixes #263 * Change error message to match rippled (#463) Fixes #263 * Add requests limit to DosGuard (#462) Fixing #448 * Set version to 1.0.4-rc2 Co-authored-by: Alex Kremer <[email protected]> Co-authored-by: CJ Cobb <[email protected]> Co-authored-by: Francis Mendoza <[email protected]> Co-authored-by: cyan317 <[email protected]>

* add nft_history and mark certain APIs as clio-only to improve error (XRPLF#255) * Database read throttle (XRPLF#242) Track current outstanding read requests to the database. When the configured limit is exceeded, reject new RPCs and return rpcTOO_BUSY * add work queue output to server_info (XRPLF#322) * Build Clio with CentOS 7 * Implement subscription for book_changes (XRPLF#315) Fixes XRPLF#315 * Remove useless mutex from BackendInterface and its usage from CassandraBackend (XRPLF#326) Fixes XRPLF#304 * Fix ProbingETL toJson to serialize underlying source states (XRPLF#325) Fixes XRPLF#323 * Throw error if server bind or listen fails (XRPLF#309) * Throw error if server bind or listen fails * Implement unique taging of incoming requests (XRPLF#311) Fixes XRPLF#212 * Add rpcLGR_IDX_MALFORMED error messages to ledger sequence min and max out of range conditionals (XRPLF#336) * Add rpcSRC_CUR_MALFORMED to badTakerPaysCurrency and rpcDST_AMT_MALFORMED to badTakerGetsCurrency (XRPLF#268) (XRPLF#333) * Mark package release's version string (XRPLF#317) * Add LimitRange to noripple_check (XRPLF#324) * Add rpcDST_ISR_MALFORMED to taker_gets conditionals (XRPLF#341) * Don't return marker in account_tx when past user specified window (XRPLF#282) * Allow server to download cache from another clio server (XRPLF#246) * Allow server to download cache from another clio server * Config takes an array of clio peers. If any of these peers have a full cache, clio picks a peer at random to download the cache from. Otherwise, fall back to downloading cache from the database. * Remove postgres support from clio (XRPLF#327) Fixes XRPLF#310 * Add malformedAddress in conditionals (XRPLF#343) Fixes XRPLF#272 * Return malformedOwner in ticket.owner for ledger_entry (XRPLF#344) Fixes XRPLF#274 * Return malformedOwner for deposit_preauth.owner in ledger_entry (XRPLF#345) Fixes XRPLF#273 * Return badMarket for same currency in taker_gets and taker_pays in book_offers (XRPLF#357) Fixes XRPLF#269 * Fix bug on cache download from peer when ledger not found (XRPLF#370) * Implement an abstraction for the config (XRPLF#358) Fixes XRPLF#321 * Rename NFT offer index to "nft_offer_index" (XRPLF#377) Fixes XRPLF#380 * Fix nft_sell_offers/nft_buy_offers limit and marker correctness (XRPLF#342) Fixes XRPLF#335 * Add offers to the response regardless of it being empty (XRPLF#389) Fixes XRPLF#351 * Return account malformed error for invalid accounts (XRPLF#388) Fixes XRPLF#349 * Add checks for empty array in accounts/accounts_proposed subscriptions (XRPLF#387) Fixes XRPLF#347 * Implement a simple check to suppress 'validated' flag output (XRPLF#393) Fixes XRPLF#354 * Remove checks for a valid subscription in subscribe/unsubscribe rpc to match rippled (XRPLF#386) Fixes XRPLF#348 * Port ignore_default support for account_lines rpc (XRPLF#391) Fixes XRPLF#375 * Return actNotFound for non-existent account in traverseOwnedNodes (XRPLF#382) * put peers in correct spot in example config (XRPLF#376) * Return account malformed error from account_tx when account is malformed (XRPLF#319) * Return correct error on subscription to non-existing stream (XRPLF#390) Fixes XRPLF#353 * return error on negative limit (XRPLF#394) * Add clang-format git hook (XRPLF#395) Fixes XRPLF#392 * Add limit in four additional files (XRPLF#328) Fixes XRPLF#221 * Add Doxyfile and comments for BackendInterface.h (XRPLF#307) Fixes XRPLF#285 * remove accountFromSeed (XRPLF#399) * handle invalidHotWallet in gateway_balances (XRPLF#384) * Better handle markers in nft_buy_offers and nft_sell_offers (XRPLF#400) * Use rpcINVALID_PARAMETERS for invalid ledger_index in ledger command (XRPLF#279) Fixes XRPLF#279 * Use rpcBAD_ISSUER for empty taker in subscribe cmd (XRPLF#352) Fixes XRPLF#352 * Return srcIsrMalformed for taker_gets issuer in book_offers (XRPLF#266) Fixes XRPLF#266 * Add error code extension mechanism and use malformed currency code (XRPLF#396) Fixes XRPLF#275 * Add rpcNOT_SUPPORTED due to Clio disparity with Rippled (XRPLF#360) Mitigates XRPLF#280 * Implement logging abstraction (XRPLF#371) Fixes XRPLF#290 * Fix pre-commit to only check staged files * Implement account ownership check and fix paging (XRPLF#383) Fixes XRPLF#222 * Remove the github action package signing step This will be done elsewhere. * include searched_all in error response of tx (XRPLF#407) * helper function for subscribe to ensure cleanup (XRPLF#402) * Add closed to header for all paths of ledger_data (XRPLF#416) Fixes XRPLF#219 * Add custom error for malformed owner and request (XRPLF#417) Fixes XRPLF#274 * Use custom malformedAddress error in ledger_entry (XRPLF#419) Fixes XRPLF#272 * Return lgrIdxsInvalid error for ledger_max_index less than ledger_min_index (XRPLF#339) Fixes XRPLF#263 * Update headers to use #pragma once * Add custom error for malformed request (XRPLF#414) Fixes XRPLF#276 * Return srcCurMalformed on invalid taker_pays in book_offers (XRPLF#413) Fixes XRPLF#267 * Fix source_location issue on MacOSX and Debug build (XRPLF#431) Fixes XRPLF#428 * Implement always adding git ref to version string (XRPLF#430) Fixes XRPLF#427 * add connection counting (XRPLF#433) * Fix malformed output format over ws rpc (XRPLF#426) Fixes XRPLF#405 * Remove branch name from version string (XRPLF#437) Fixes a bug from XRPLF#430 * Implement cli parsing using boost::po (XRPLF#436) Fixes XRPLF#367 * Update documentation and config with ssl_cert_file and ssl_key_file (XRPLF#443) Fixes XRPLF#424 * Fix gateway balances to match rippled output (XRPLF#441) Fixes XRPLF#271 * Update README and example config to describe start_sequence (XRPLF#438) Fixes XRPLF#250 * Add copyright to top of each source file (XRPLF#444) Fixes XRPLF#411 * Increase file descriptor limit (XRPLF#449) * Update readme with more log configurations (XRPLF#447) Fixes XRPLF#446 * Document dos_guard in example config. Log when client surpasses rate limit (XRPLF#451) * Add unit tests for DOSGuard (XRPLF#453) Fixes XRPLF#452 * Build macOS and Ubuntu 22.04 (XRPLF#456) build release/x.y.z branches * Add time measurement profiler (XRPLF#458) Rebase * Match format to rippled error code (XRPLF#461) Fixes XRPLF#263 * Change error message to match rippled (XRPLF#463) Fixes XRPLF#263 * Add requests limit to DosGuard (XRPLF#462) Fixing XRPLF#448 * Write Clio version file from template (XRPLF#457) * Set build version from git * disallow untagged commits to master * remove clang-format ingore around versionString * config not postional parameter anymore. * add heavy runner label * fake clio build * use my fork * skip every other step * maincpp * no tests * no testsfr * revert cmakelists --------- Co-authored-by: ledhed2222 <[email protected]> Co-authored-by: CJ Cobb <[email protected]> Co-authored-by: Alex Kremer <[email protected]> Co-authored-by: Francis Mendoza <[email protected]> Co-authored-by: Shawn <[email protected]> Co-authored-by: manojsdoshi <[email protected]> Co-authored-by: cyan317 <[email protected]>

godexsoft requested a review from cjcobb23 November 3, 2022 21:10

cjcobb23 reviewed Nov 8, 2022

View reviewed changes

godexsoft force-pushed the bugfix/account-objects-paging branch from fc5b50c to 221ceac Compare November 16, 2022 00:20

godexsoft commented Nov 16, 2022

View reviewed changes

src/rpc/Errors.h Outdated Show resolved Hide resolved

godexsoft force-pushed the bugfix/account-objects-paging branch from 221ceac to 161a3f9 Compare November 16, 2022 00:23

godexsoft requested a review from cjcobb23 November 16, 2022 00:23

cjcobb23 suggested changes Nov 16, 2022

View reviewed changes

src/rpc/RPCHelpers.cpp Outdated Show resolved Hide resolved

godexsoft commented Nov 16, 2022

View reviewed changes

src/rpc/handlers/Subscribe.cpp Show resolved Hide resolved

godexsoft requested a review from cjcobb23 November 16, 2022 22:43

cjcobb23 reviewed Nov 17, 2022

View reviewed changes

src/rpc/RPCHelpers.cpp Show resolved Hide resolved

cjcobb23 previously approved these changes Nov 18, 2022

View reviewed changes

godexsoft force-pushed the bugfix/account-objects-paging branch from cd44fa5 to b397d1e Compare November 18, 2022 17:39

godexsoft dismissed cjcobb23’s stale review via 8d55fb6 November 18, 2022 17:44

godexsoft force-pushed the bugfix/account-objects-paging branch 2 times, most recently from 8d55fb6 to 9ee2139 Compare November 18, 2022 17:49

cjcobb23 previously approved these changes Nov 18, 2022

View reviewed changes

godexsoft dismissed cjcobb23’s stale review via e8c1a92 November 18, 2022 17:49

godexsoft force-pushed the bugfix/account-objects-paging branch from 9ee2139 to e8c1a92 Compare November 18, 2022 17:49

godexsoft requested a review from cjcobb23 November 18, 2022 17:50

cjcobb23 approved these changes Nov 18, 2022

View reviewed changes

godexsoft merged commit 041aba9 into XRPLF:develop Nov 18, 2022

legleux pushed a commit to legleux/clio that referenced this pull request Jan 10, 2023

Implement account ownership check and fix paging (XRPLF#383)

ba5c7f0

Fixes XRPLF#222

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix account ownership check #383

Fix account ownership check #383

Uh oh!

godexsoft commented Nov 3, 2022

Uh oh!

cjcobb23 commented Nov 3, 2022 •

edited

Loading

Uh oh!

godexsoft commented Nov 4, 2022

Uh oh!

cjcobb23 left a comment

Uh oh!

godexsoft commented Nov 8, 2022 •

edited

Loading

Uh oh!

cjcobb23 commented Nov 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjcobb23 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Fix account ownership check #383

Fix account ownership check #383

Uh oh!

Conversation

godexsoft commented Nov 3, 2022

Uh oh!

cjcobb23 commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

godexsoft commented Nov 4, 2022

Uh oh!

cjcobb23 left a comment

Choose a reason for hiding this comment

Uh oh!

godexsoft commented Nov 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cjcobb23 commented Nov 14, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cjcobb23 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cjcobb23 commented Nov 3, 2022 •

edited

Loading

godexsoft commented Nov 8, 2022 •

edited

Loading