Sign method - verify accurate encoding (#1026)

* Decode signed transactions and check field integrity

* Add tests (including signing a tx without Flags)

* Update tests to output more descriptive errors on failure

* Update ripple-binary-codec to 0.2.2

package.json

@@ -24,7 +24,7 @@
     "jsonschema": "1.2.2",
     "lodash": "^4.17.4",
     "ripple-address-codec": "^2.0.1",
-    "ripple-binary-codec": "0.2.1",
+    "ripple-binary-codec": "0.2.2",
     "ripple-hashes": "0.3.2",
     "ripple-keypairs": "^0.10.1",
     "ripple-lib-transactionparser": "0.7.1",
@@ -61,7 +61,7 @@
     "doctoc": "doctoc docs/index.md --title '# RippleAPI Reference' --github --maxlevel 2",
     "docgen": "node --harmony scripts/build_docs.js",
     "clean": "rm -rf dist/npm",
-    "compile": "mkdir -p dist/npm/common && cp -r src/common/schemas dist/npm/common/ && tsc --build",
+    "compile": "mkdir -p dist/npm/common/js && cp -r src/common/js/ dist/npm/common/js/ && mkdir -p dist/npm/common && cp -r src/common/schemas dist/npm/common/ && tsc --build",
     "watch": "tsc -w",
     "prepublish": "npm run clean && npm run compile && npm run build",
     "test": "TS_NODE_PROJECT=src/tsconfig.json nyc mocha --exit",

src/transaction/sign.ts

@@ -1,6 +1,7 @@
+import * as isEqual from '../common/js/lodash.isequal'
 import * as utils from './utils'
 import keypairs = require('ripple-keypairs')
-import binary = require('ripple-binary-codec')
+import binaryCodec = require('ripple-binary-codec')
 import {computeBinaryTransactionHash} from 'ripple-hashes'
 import {SignOptions, KeyPair} from './types'
 import {BigNumber} from 'bignumber.js'
@@ -10,8 +11,8 @@ const validate = utils.common.validate
 
 function computeSignature(tx: object, privateKey: string, signAs?: string) {
   const signingData = signAs
-    ? binary.encodeForMultisigning(tx, signAs)
-    : binary.encodeForSigning(tx)
+    ? binaryCodec.encodeForMultisigning(tx, signAs)
+    : binaryCodec.encodeForSigning(tx)
   return keypairs.sign(signingData, privateKey)
 }
 
@@ -32,7 +33,88 @@ function signWithKeypair(
     )
   }
 
-  const fee = new BigNumber(tx.Fee)
+  checkFee(api, tx.Fee)
+
+  const txToSignAndEncode = Object.assign({}, tx)
+
+  txToSignAndEncode.SigningPubKey = options.signAs ? '' : keypair.publicKey
+
+  if (options.signAs) {
+    const signer = {
+      Account: options.signAs,
+      SigningPubKey: keypair.publicKey,
+      TxnSignature: computeSignature(txToSignAndEncode, keypair.privateKey, options.signAs)
+    }
+    txToSignAndEncode.Signers = [{Signer: signer}]
+  } else {
+    txToSignAndEncode.TxnSignature = computeSignature(txToSignAndEncode, keypair.privateKey)
+  }
+
+  const serialized = binaryCodec.encode(txToSignAndEncode)
+
+  checkTxSerialization(serialized, tx)
+
+  return {
+    signedTransaction: serialized,
+    id: computeBinaryTransactionHash(serialized)
+  }
+}
+
+/**
+ *  Decode a serialized transaction, remove the fields that are added during the signing process,
+ *  and verify that it matches the transaction prior to signing.
+ *
+ *  @param {string} serialized A signed and serialized transaction.
+ *  @param {utils.TransactionJSON} tx The transaction prior to signing.
+ *
+ *  @returns {void} This method does not return a value, but throws an error if the check fails.
+ */
+function checkTxSerialization(serialized: string, tx: utils.TransactionJSON): void {
+  // Decode the serialized transaction:
+  const decoded = binaryCodec.decode(serialized)
+
+  // ...And ensure it is equal to the original tx, except:
+  // - It must have a TxnSignature or Signers (multisign).
+  if (!decoded.TxnSignature && !decoded.Signers) {
+    throw new utils.common.errors.ValidationError(
+      'Serialized transaction must have a TxnSignature or Signers property'
+    )
+  }
+  // - We know that the original tx did not have TxnSignature, so we should delete it:
+  delete decoded.TxnSignature
+  // - We know that the original tx did not have Signers, so if it exists, we should delete it:
+  delete decoded.Signers
+
+  // - If SigningPubKey was not in the original tx, then we should delete it.
+  //   But if it was in the original tx, then we should ensure that it has not been changed.
+  if (!tx.SigningPubKey) {
+    delete decoded.SigningPubKey
+  }
+
+  if (!isEqual(decoded, tx)) {
+    const error = new utils.common.errors.ValidationError(
+      'Serialized transaction does not match original txJSON'
+    )
+    error.data = {
+      decoded,
+      tx
+    }
+    throw error
+  }
+}
+
+/**
+ *  Check that a given transaction fee does not exceed maxFeeXRP (in drops).
+ *
+ *  See https://xrpl.org/rippleapi-reference.html#parameters
+ *
+ *  @param {RippleAPI} api A RippleAPI instance.
+ *  @param {string} txFee The transaction fee in drops, encoded as a string.
+ *
+ *  @returns {void} This method does not return a value, but throws an error if the check fails.
+ */
+function checkFee(api: RippleAPI, txFee: string): void {
+  const fee = new BigNumber(txFee)
   const maxFeeDrops = xrpToDrops(api._maxFeeXRP)
   if (fee.greaterThan(maxFeeDrops)) {
     throw new utils.common.errors.ValidationError(
@@ -40,25 +122,6 @@ function signWithKeypair(
       'To use a higher fee, set `maxFeeXRP` in the RippleAPI constructor.'
     )
   }
-
-  tx.SigningPubKey = options.signAs ? '' : keypair.publicKey
-
-  if (options.signAs) {
-    const signer = {
-      Account: options.signAs,
-      SigningPubKey: keypair.publicKey,
-      TxnSignature: computeSignature(tx, keypair.privateKey, options.signAs)
-    }
-    tx.Signers = [{Signer: signer}]
-  } else {
-    tx.TxnSignature = computeSignature(tx, keypair.privateKey)
-  }
-
-  const serialized = binary.encode(tx)
-  return {
-    signedTransaction: serialized,
-    id: computeBinaryTransactionHash(serialized)
-  }
 }
 
 function sign(

test/api-test.js

@@ -2608,6 +2608,115 @@ describe('RippleAPI', function () {
     assert.deepEqual(signature, responses.sign.signAs);
   });
 
+  it('sign - succeeds - prepared payment', async function () {
+    const payment = await this.api.preparePayment('r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59', {
+      source: {
+        address: 'r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59',
+        maxAmount: {
+          value: '1',
+          currency: 'drops'
+        }
+      },
+      destination: {
+        address: 'rQ3PTWGLCbPz8ZCicV5tCX3xuymojTng5r',
+        amount: {
+          value: '1',
+          currency: 'drops'
+        }
+      }
+    });
+    const secret = 'shsWGZcmZz6YsWWmcnpfr6fLTdtFV';
+    const result = this.api.sign(payment.txJSON, secret);
+    const expectedResult = {
+      signedTransaction:
+      '12000022800000002400000017201B008694F261400000000000000168400000000000000C732102F89EAEC7667B30F33D0687BBA86C3FE2A08CCA40A9186C5BDE2DAA6FA97A37D874473045022100A9C91D4CFAE45686146EE0B56D4C53A2E7C2D672FB834D43E0BE2D2E9106519A022075DDA2F92DE552B0C45D83D4E6D35889B3FBF51BFBBD9B25EBF70DE3C96D0D6681145E7B112523F68D2F5E879DB4EAC51C6698A693048314FDB08D07AAA0EB711793A3027304D688E10C3648',
+     id:
+      '88D6B913C66279EA31ADC25C5806C48B2D4E5680261666790A736E1961217700'
+    };
+    assert.deepEqual(result, expectedResult);
+    schemaValidator.schemaValidate('sign', result);
+  });
+
+  it('sign - succeeds - no flags', async function () {
+    const txJSON = '{"TransactionType":"Payment","Account":"r45Rev1EXGxy2hAUmJPCne97KUE7qyrD3j","Destination":"rQ3PTWGLCbPz8ZCicV5tCX3xuymojTng5r","Amount":"20000000","Sequence":1,"Fee":"12"}';
+    const secret = 'shotKgaEotpcYsshSE39vmSnBDRim';
+    const result = this.api.sign(txJSON, secret);
+    const expectedResult = {
+      signedTransaction:
+      '1200002400000001614000000001312D0068400000000000000C7321022B05847086686F9D0499B13136B94AD4323EE1B67D4C429ECC987AB35ACFA34574473045022100C104B7B97C31FACA4597E7D6FCF13BD85BD11375963A62A0AC45B0061236E39802207784F157F6A98DFC85B051CDDF61CC3084C4F5750B82674801C8E9950280D1998114EE3046A5DDF8422C40DDB93F1D522BB4FE6419158314FDB08D07AAA0EB711793A3027304D688E10C3648',
+     id:
+      '0596925967F541BF332FF6756645B2576A9858414B5B363DC3D34915BE8A70D6'
+    };
+    const decoded = binary.decode(result.signedTransaction);
+    assert(decoded.Flags === undefined, `Flags = ${decoded.Flags}, should be undefined`);
+    assert.deepEqual(result, expectedResult);
+    schemaValidator.schemaValidate('sign', result);
+  });
+
+  it('sign - throws when encoded tx does not match decoded tx - prepared payment', async function () {
+    const payment = await this.api.preparePayment('r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59', {
+      source: {
+        address: 'r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59',
+        maxAmount: {
+          value: '1.1234567',
+          currency: 'drops'
+        }
+      },
+      destination: {
+        address: 'rQ3PTWGLCbPz8ZCicV5tCX3xuymojTng5r',
+        amount: {
+          value: '1.1234567',
+          currency: 'drops'
+        }
+      }
+    });
+    const secret = 'shsWGZcmZz6YsWWmcnpfr6fLTdtFV';
+    assert.throws(
+      () => {
+        this.api.sign(payment.txJSON, secret);
+      },
+      /^Error: 1\.1234567 is an illegal amount/
+    );
+  });
+
+  it('sign - throws when encoded tx does not match decoded tx - AccountSet', function () {
+    const secret = 'shsWGZcmZz6YsWWmcnpfr6fLTdtFV';
+    const request = {
+      "txJSON": "{\"Flags\":2147483648,\"TransactionType\":\"AccountSet\",\"Account\":\"r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59\",\"Domain\":\"726970706C652E636F6D\",\"LastLedgerSequence\":8820051,\"Fee\":\"1.2\",\"Sequence\":23,\"SigningPubKey\":\"02F89EAEC7667B30F33D0687BBA86C3FE2A08CCA40A9186C5BDE2DAA6FA97A37D8\"}",
+      "instructions": {
+        "fee": "0.0000012",
+        "sequence": 23,
+        "maxLedgerVersion": 8820051
+      }
+    }
+    
+    assert.throws(
+      () => {
+        this.api.sign(request.txJSON, secret);
+      },
+      /Error: 1\.2 is an illegal amount/
+    );
+  });
+
+  it('sign - throws when encoded tx does not match decoded tx - higher fee', function () {
+    const secret = 'shsWGZcmZz6YsWWmcnpfr6fLTdtFV';
+    const request = {
+      "txJSON": "{\"Flags\":2147483648,\"TransactionType\":\"AccountSet\",\"Account\":\"r9cZA1mLK5R5Am25ArfXFmqgNwjZgnfk59\",\"Domain\":\"726970706C652E636F6D\",\"LastLedgerSequence\":8820051,\"Fee\":\"1123456.7\",\"Sequence\":23,\"SigningPubKey\":\"02F89EAEC7667B30F33D0687BBA86C3FE2A08CCA40A9186C5BDE2DAA6FA97A37D8\"}",
+      "instructions": {
+        "fee": "1.1234567",
+        "sequence": 23,
+        "maxLedgerVersion": 8820051
+      }
+    }
+    
+    assert.throws(
+      () => {
+        this.api.sign(request.txJSON, secret);
+      },
+      /Error: 1123456\.7 is an illegal amount/
+    );
+  });
+
   it('sign - throws when Fee exceeds maxFeeXRP (in drops)', function () {
     const secret = 'shsWGZcmZz6YsWWmcnpfr6fLTdtFV';
     const request = {

yarn.lock

@@ -2839,6 +2839,11 @@ lodash@^4.12.0, lodash@^4.17.11, lodash@^4.17.4:
   resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.13.tgz#0bdc3a6adc873d2f4e0c4bac285df91b64fc7b93"
   integrity sha512-vm3/XWXfWtRua0FkUyEHBZy8kCPjErNBT9fJx8Zvs+U6zjqPbTUOpkaoum3O5uiA8sm+yNMHXfYkTUHFoMxFNA==
 
+lodash@^4.17.15:
+  version "4.17.15"
+  resolved "https://registry.yarnpkg.com/lodash/-/lodash-4.17.15.tgz#b447f6670a0455bbfeedd11392eff330ea097548"
+  integrity sha512-8xOcRHvCjnocdS5cpwXQXVzmmh5e5+saE2QGoeQmbKmRS6J3VQppPOIt0MnmE+4xlZoumy0GPG0D0MVIQbNA1A==
+
 lodash@~1.0.1:
   version "1.0.2"
   resolved "https://registry.yarnpkg.com/lodash/-/lodash-1.0.2.tgz#8f57560c83b59fc270bd3d561b690043430e2551"
@@ -4103,6 +4108,19 @@ ripple-binary-codec@0.2.1:
     lodash "^4.12.0"
     ripple-address-codec "^2.0.1"
 
+ripple-binary-codec@0.2.2:
+  version "0.2.2"
+  resolved "https://registry.yarnpkg.com/ripple-binary-codec/-/ripple-binary-codec-0.2.2.tgz#1d64e1dce536363eeeb636ad1f98c0c5d98a8f48"
+  integrity sha512-kJ1pEIEHKvvNO9IqWgwBCLCL3wwJ/GwYXpJhrmX2kW4x7unHXgFOxhAP9DDlMGwMusSmYrstsXrzhffVEaBoHw==
+  dependencies:
+    babel-runtime "^6.6.1"
+    bn.js "^4.11.3"
+    create-hash "^1.1.2"
+    decimal.js "^5.0.8"
+    inherits "^2.0.1"
+    lodash "^4.17.15"
+    ripple-address-codec "^2.0.1"
+
 ripple-hashes@0.3.2:
   version "0.3.2"
   resolved "https://registry.yarnpkg.com/ripple-hashes/-/ripple-hashes-0.3.2.tgz#f3ac3b1832cec6d0bac07e82acc10a0a6a1cc84e"