From 2aa7a958cb370a5138ea4068ddbe446d8ae9db76 Mon Sep 17 00:00:00 2001 From: Evan Schwartz Date: Thu, 28 Jul 2016 15:17:25 +0200 Subject: [PATCH 1/3] feat: expect request as full ilp packet before https://github.com/interledger/five-bells-connector/pull/195 the connector would strip the ilp packet and only pass on the data field. this change makes the receiver expect and validate the full request packet. it also keeps the old behavior because *.ilpdemo.org has not yet been updated with the latest connector fixes https://github.com/interledger/js-ilp/issues/15 --- src/lib/receiver.js | 44 ++++++++++++++++++++------------- test/data/paymentParams.json | 2 +- test/data/paymentRequest.json | 2 +- test/data/transferIncoming.json | 14 ++++++++--- test/receiverSpec.js | 36 +++++++++++++++++++++------ test/senderSpec.js | 2 +- 6 files changed, 70 insertions(+), 30 deletions(-) diff --git a/src/lib/receiver.js b/src/lib/receiver.js index fadfc9b..59649ee 100644 --- a/src/lib/receiver.js +++ b/src/lib/receiver.js @@ -8,6 +8,7 @@ const Client = require('ilp-core').Client const cc = require('five-bells-condition') const EventEmitter = require('eventemitter2') const debug = require('debug')('ilp-itp:receiver') +const BigNumber = require('bignumber.js') /** * @module Receiver @@ -97,27 +98,36 @@ function createReceiver (opts) { return 'no-execution' } - // TODO look for the request in transfer.data.ilp_header after https://github.com/interledger/five-bells-connector/pull/195 is merged - // if (!transfer.data || !transfer.data.ilp_header) { - // debug('got notification of transfer without ilp packet', transfer) - // return false - // } - const request = { - amount: String(transfer.amount), - ledger: client.getPlugin().id, - account: client.getPlugin().getAccount(), - data: { - expires_at: transfer.data.expires_at, - request_id: transfer.data.request_id + // The request is the ilp_header + let request = transfer.data && transfer.data.ilp_header + if (request && request.data && request.data.execution_condition) { + delete request.data.execution_condition + } + + // For now support the old connector behavior that only passes on the ILP packet data field + if (!request && transfer.data.request_id) { + debug('using old behavior for when connector only passes on the ilp packet data field') + request = { + amount: (new BigNumber(transfer.amount)).toString(), + ledger: client.getPlugin().id, + account: client.getPlugin().getAccount(), + data: { + expires_at: transfer.data.expires_at, + request_id: transfer.data.request_id + } } } - // TODO re-enable this when we aren't using the transfer's amount + if (!request) { + debug('got notification of transfer with no request attached') + return 'no-packet' + } + // TODO also allow receiver to disallow amounts greater than requested - // if ((new BigNumber(transfer.amount)).lessThan(request.amount)) { - // debug('got notification of transfer where amount is less than expected (' + request.amount + ')', transfer) - // return 'insufficient' - // } + if ((new BigNumber(transfer.amount)).lessThan(request.amount)) { + debug('got notification of transfer where amount is less than expected (' + request.amount + ')', transfer) + return 'insufficient' + } if (request.data.expires_at && moment().isAfter(request.data.expires_at)) { debug('got notification of transfer with expired request packet', transfer) diff --git a/test/data/paymentParams.json b/test/data/paymentParams.json index 800e809..dcfffbc 100644 --- a/test/data/paymentParams.json +++ b/test/data/paymentParams.json @@ -8,6 +8,6 @@ "request_id": "22e315dc-3f99-4f89-9914-1987ceaa906d", "expires_at": "1970-01-01T00:00:10Z" }, - "executionCondition": "cc:0:3:crwqC6igtxv1DsIFiKv91roUp7qQxToKutwGELGQZIE:32", + "executionCondition": "cc:0:3:4l2SBwP_i-oCEzaD2IGRpwdywCfaBmrxJcpJ_3PXv6o:32", "expiresAt": "1970-01-01T00:00:10.000Z" } diff --git a/test/data/paymentRequest.json b/test/data/paymentRequest.json index 3fd4a38..5003db4 100644 --- a/test/data/paymentRequest.json +++ b/test/data/paymentRequest.json @@ -5,6 +5,6 @@ "data": { "request_id": "22e315dc-3f99-4f89-9914-1987ceaa906d", "expires_at": "1970-01-01T00:00:10Z", - "execution_condition": "cc:0:3:crwqC6igtxv1DsIFiKv91roUp7qQxToKutwGELGQZIE:32" + "execution_condition": "cc:0:3:4l2SBwP_i-oCEzaD2IGRpwdywCfaBmrxJcpJ_3PXv6o:32" } } diff --git a/test/data/transferIncoming.json b/test/data/transferIncoming.json index 18f8f3f..aa98068 100644 --- a/test/data/transferIncoming.json +++ b/test/data/transferIncoming.json @@ -4,9 +4,17 @@ "account": "https://blue.ilpdemo.org/ledger/accounts/connie", "amount": "1.0000", "data":{ - "request_id": "22e315dc-3f99-4f89-9914-1987ceaa906d", - "expires_at": "1970-01-01T00:00:10Z" + "ilp_header": { + "ledger": "https://blue.ilpdemo.org/ledger", + "account": "https://blue.ilpdemo.org/ledger/accounts/bob", + "amount": "1", + "data": { + "request_id": "22e315dc-3f99-4f89-9914-1987ceaa906d", + "expires_at": "1970-01-01T00:00:10Z", + "execution_condition": "cc:0:3:4l2SBwP_i-oCEzaD2IGRpwdywCfaBmrxJcpJ_3PXv6o:32" + } + } }, - "executionCondition": "cc:0:3:crwqC6igtxv1DsIFiKv91roUp7qQxToKutwGELGQZIE:32", + "executionCondition": "cc:0:3:4l2SBwP_i-oCEzaD2IGRpwdywCfaBmrxJcpJ_3PXv6o:32", "expiresAt": "1970-01-01T00:00:10.000Z" } diff --git a/test/receiverSpec.js b/test/receiverSpec.js index 52e9d06..be4758b 100644 --- a/test/receiverSpec.js +++ b/test/receiverSpec.js @@ -182,24 +182,30 @@ describe('Receiver Module', function () { expect(results).to.deep.equal(['no-execution']) }) - it.skip('should ignore transfers without ilp packets in the data field', function * () { - + it('should ignore transfers without ilp packets in the data field', function * () { + const results = yield this.client.emitAsync('receive', _.assign(this.transfer, { + data: { not: 'a packet' } + })) + expect(results).to.deep.equal(['no-packet']) }) it('should ignore expired packets', function * () { timekeeper.freeze(new Date(10000)) const results = yield this.client.emitAsync('receive', _.merge(this.transfer, { data: { - expires_at: '1970-01-01T00:00:01Z' + ilp_header: { + data: { + expires_at: '1970-01-01T00:00:01Z' + } + } } })) expect(results).to.deep.equal(['expired']) }) - // TODO re-enable this when we aren't using the transfer's amount - it.skip('should ignore transfers where the amount is less than specified in the packet', function * () { + it('should ignore transfers where the amount is less than specified in the packet', function * () { const results = yield this.client.emitAsync('receive', _.merge(this.transfer, { - amount: '1.0000000000001' + amount: '0.999999999' })) expect(results).to.deep.equal(['insufficient']) }) @@ -216,7 +222,23 @@ describe('Receiver Module', function () { }) it('should fulfill the conditions of transfers corresponding to requests generated by the receiver', function * () { - const results = yield this.client.emitAsync('receive', this.transfer) + timekeeper.reset() + const request = this.receiver.createRequest({ + amount: 1 + }) + const results = yield this.client.emitAsync('receive', _.assign(this.transfer, { + data: { + ilp_header: request + }, + executionCondition: request.data.execution_condition + })) + expect(results).to.deep.equal(['sent']) + }) + + it('should (temporarily) support the old behavior where the connector only passes on the ilp packet data field', function * () { + const results = yield this.client.emitAsync('receive', _.assign(this.transfer, { + data: this.transfer.data.ilp_header.data + })) expect(results).to.deep.equal(['sent']) }) }) diff --git a/test/senderSpec.js b/test/senderSpec.js index 539e818..5950f60 100644 --- a/test/senderSpec.js +++ b/test/senderSpec.js @@ -121,7 +121,7 @@ describe('Sender Module', function () { 'request_id': '22e315dc-3f99-4f89-9914-1987ceaa906d', 'expires_at': '1970-01-01T00:00:10Z' }, - 'executionCondition': 'cc:0:3:crwqC6igtxv1DsIFiKv91roUp7qQxToKutwGELGQZIE:32', + 'executionCondition': 'cc:0:3:4l2SBwP_i-oCEzaD2IGRpwdywCfaBmrxJcpJ_3PXv6o:32', 'expiresAt': '1970-01-01T00:00:10.000Z' }) }) From 8f7527a2902219c53e77d79ee78564b0a447f141 Mon Sep 17 00:00:00 2001 From: Evan Schwartz Date: Thu, 28 Jul 2016 15:33:03 +0200 Subject: [PATCH 2/3] feat: add disallowOverPayment option by default the receiver will accept incoming transfers where the amount is greater than requested. this adds an option to require that transfer amounts exactly match the requested amount --- src/lib/receiver.js | 8 +++++++- test/receiverSpec.js | 22 +++++++++++++++++++--- 2 files changed, 26 insertions(+), 4 deletions(-) diff --git a/src/lib/receiver.js b/src/lib/receiver.js index 59649ee..87b7475 100644 --- a/src/lib/receiver.js +++ b/src/lib/receiver.js @@ -24,6 +24,7 @@ const BigNumber = require('bignumber.js') * @param {ilp-core.Client} [opts.client] [ilp-core](https://github.com/interledger/js-ilp-core) Client, which can optionally be supplied instead of the previous options * @param {Buffer} [opts.hmacKey=crypto.randomBytes(32)] 32-byte secret used for generating request conditions * @param {Number} [opts.defaultRequestTimeout=30] Default time in seconds that requests will be valid for + * @param {Boolean} [opts.disallowOverPayment=false] Require that incoming transfer amounts exactly match the requested amount * @return {Receiver} */ function createReceiver (opts) { @@ -39,6 +40,7 @@ function createReceiver (opts) { } const hmacKey = opts.hmacKey || crypto.randomBytes(32) const defaultRequestTimeout = opts.defaultRequestTimeout || 30 + const disallowOverPayment = !!opts.disallowOverPayment /** * Create a payment request @@ -123,12 +125,16 @@ function createReceiver (opts) { return 'no-packet' } - // TODO also allow receiver to disallow amounts greater than requested if ((new BigNumber(transfer.amount)).lessThan(request.amount)) { debug('got notification of transfer where amount is less than expected (' + request.amount + ')', transfer) return 'insufficient' } + if (disallowOverPayment && (new BigNumber(transfer.amount)).greaterThan(request.amount)) { + debug('got notification of transfer where amount is greater than expected (' + request.amount + ')', transfer) + return 'overpayment-disallowed' + } + if (request.data.expires_at && moment().isAfter(request.data.expires_at)) { debug('got notification of transfer with expired request packet', transfer) return 'expired' diff --git a/test/receiverSpec.js b/test/receiverSpec.js index be4758b..6775c55 100644 --- a/test/receiverSpec.js +++ b/test/receiverSpec.js @@ -210,8 +210,18 @@ describe('Receiver Module', function () { expect(results).to.deep.equal(['insufficient']) }) - it.skip('should ignore transfers where the amount is more than requested if disallowOverPayment is set', function () { - + it('should ignore transfers where the amount is more than requested if disallowOverPayment is set', function * () { + const receiver = createReceiver({ + client: this.client, + hmacKey: Buffer.from('+Xd3hhabpygJD6cen+R/eon+acKWvFLzqp65XieY8W0=', 'base64'), + disallowOverPayment: true + }) + yield receiver.listen() + const results = yield this.client.emitAsync('receive', _.merge(this.transfer, { + amount: '1.0000000001' + })) + // because we're instantiating an extra receiver there will actually be two events + expect(results).to.contain('overpayment-disallowed') }) it('should ignore transfers where the executionCondition does not match the generated condition', function * () { @@ -222,7 +232,6 @@ describe('Receiver Module', function () { }) it('should fulfill the conditions of transfers corresponding to requests generated by the receiver', function * () { - timekeeper.reset() const request = this.receiver.createRequest({ amount: 1 }) @@ -235,6 +244,13 @@ describe('Receiver Module', function () { expect(results).to.deep.equal(['sent']) }) + it('should allow overpayment if disallowOverPayment is not set', function * () { + const results = yield this.client.emitAsync('receive', _.assign(this.transfer, { + amount: '10000000000000' // a bit excessive, i know + })) + expect(results).to.deep.equal(['sent']) + }) + it('should (temporarily) support the old behavior where the connector only passes on the ilp packet data field', function * () { const results = yield this.client.emitAsync('receive', _.assign(this.transfer, { data: this.transfer.data.ilp_header.data From fbd2628a7de146be5e8aac333d229b7aea757100 Mon Sep 17 00:00:00 2001 From: Evan Schwartz Date: Thu, 28 Jul 2016 15:33:03 +0200 Subject: [PATCH 3/3] feat: BREAKING disallow overpayment by default add allowOverPayment option to allow incoming transfers where the amount is greater than requested --- src/lib/receiver.js | 6 +++--- test/receiverSpec.js | 22 +++++++++++----------- 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/src/lib/receiver.js b/src/lib/receiver.js index 87b7475..fef0c00 100644 --- a/src/lib/receiver.js +++ b/src/lib/receiver.js @@ -24,7 +24,7 @@ const BigNumber = require('bignumber.js') * @param {ilp-core.Client} [opts.client] [ilp-core](https://github.com/interledger/js-ilp-core) Client, which can optionally be supplied instead of the previous options * @param {Buffer} [opts.hmacKey=crypto.randomBytes(32)] 32-byte secret used for generating request conditions * @param {Number} [opts.defaultRequestTimeout=30] Default time in seconds that requests will be valid for - * @param {Boolean} [opts.disallowOverPayment=false] Require that incoming transfer amounts exactly match the requested amount + * @param {Boolean} [opts.allowOverPayment=false] Allow transfers where the amount is greater than requested * @return {Receiver} */ function createReceiver (opts) { @@ -40,7 +40,7 @@ function createReceiver (opts) { } const hmacKey = opts.hmacKey || crypto.randomBytes(32) const defaultRequestTimeout = opts.defaultRequestTimeout || 30 - const disallowOverPayment = !!opts.disallowOverPayment + const allowOverPayment = !!opts.allowOverPayment /** * Create a payment request @@ -130,7 +130,7 @@ function createReceiver (opts) { return 'insufficient' } - if (disallowOverPayment && (new BigNumber(transfer.amount)).greaterThan(request.amount)) { + if (!allowOverPayment && (new BigNumber(transfer.amount)).greaterThan(request.amount)) { debug('got notification of transfer where amount is greater than expected (' + request.amount + ')', transfer) return 'overpayment-disallowed' } diff --git a/test/receiverSpec.js b/test/receiverSpec.js index 6775c55..7d0a6b0 100644 --- a/test/receiverSpec.js +++ b/test/receiverSpec.js @@ -210,18 +210,11 @@ describe('Receiver Module', function () { expect(results).to.deep.equal(['insufficient']) }) - it('should ignore transfers where the amount is more than requested if disallowOverPayment is set', function * () { - const receiver = createReceiver({ - client: this.client, - hmacKey: Buffer.from('+Xd3hhabpygJD6cen+R/eon+acKWvFLzqp65XieY8W0=', 'base64'), - disallowOverPayment: true - }) - yield receiver.listen() + it('should ignore transfers where the amount is more than requested', function * () { const results = yield this.client.emitAsync('receive', _.merge(this.transfer, { amount: '1.0000000001' })) - // because we're instantiating an extra receiver there will actually be two events - expect(results).to.contain('overpayment-disallowed') + expect(results).to.deep.equal(['overpayment-disallowed']) }) it('should ignore transfers where the executionCondition does not match the generated condition', function * () { @@ -244,11 +237,18 @@ describe('Receiver Module', function () { expect(results).to.deep.equal(['sent']) }) - it('should allow overpayment if disallowOverPayment is not set', function * () { + it('should allow overpayment if allowOverPayment is set', function * () { + const receiver = createReceiver({ + client: this.client, + hmacKey: Buffer.from('+Xd3hhabpygJD6cen+R/eon+acKWvFLzqp65XieY8W0=', 'base64'), + allowOverPayment: true + }) + yield receiver.listen() const results = yield this.client.emitAsync('receive', _.assign(this.transfer, { amount: '10000000000000' // a bit excessive, i know })) - expect(results).to.deep.equal(['sent']) + // because we're instantiating an extra receiver there will actually be two events + expect(results).to.contain('sent') }) it('should (temporarily) support the old behavior where the connector only passes on the ilp packet data field', function * () {