Validate updatedAt on saves and deletes

.eslintignore

@@ -1 +0,0 @@
-test/*

.eslintrc.js

@@ -9,6 +9,10 @@ module.exports = {
   parserOptions: {
     ecmaVersion: 2018,
     sourceType: 'module',
+    project: {
+      extends: './tsconfig.json',
+      include: ['src/**/*', 'test/**/*'],
+    },
   },
   env: {
     'jest/globals': true,

CHANGELOG.md

@@ -1,5 +1,11 @@
 # Radiks-Server Changelog
 
+## 0.3.0-beta.1 - November 18th, 2019
+
+Fixes for [#29](https://github.com/blockstack/radiks-server/issues/29)
+
+Radiks-server was not properly validating the `updatedAt` attribute for model updates and deletes. This could potentially lead to signature jacking. Radiks-server now validated that the `updatedAt` field is greater than previous `updatedAt`s. This acts similarly to an `nonce` in blockchains.
+
 ## 0.2.3 - September 11th, 2019
 
 - Dependency updates from @dependabot:

package.json

@@ -1,6 +1,6 @@
 {
   "name": "radiks-server",
-  "version": "0.2.3",
+  "version": "0.3.0-beta.1",
   "description": "An express plugin for building a Radiks server",
   "main": "app/index.js",
   "types": "typed",
@@ -15,7 +15,7 @@
     "compile-watch": "babel src -d app --watch --extensions '.ts,.js'",
     "typescript": "tsc",
     "prettier": "prettier --write '**/*.{ts,tsx,json}'",
-    "eslint": "eslint 'src/**/*'",
+    "eslint": "eslint 'src/**/*' 'test/**/*' --ext .ts",
     "test": "jest --verbose=true --ci --runInBand test/",
     "test-watch": "jest --verbose=true --ci --runInBand --watch test/",
     "prepublishOnly": "yarn build"

src/controllers/ModelsController.ts

@@ -90,8 +90,18 @@ const makeModelsController = (
         _id: attrs.signingKeyId,
         radiksType: 'SigningKey',
       });
-      const message = `${attrs._id}-${attrs.updatedAt}`;
-      if (verifyECDSA(message, publicKey, req.query.signature)) {
+      const {
+        signature,
+        updatedAt,
+      }: { signature: string; updatedAt: string } = req.query;
+      if (!updatedAt || parseInt(updatedAt, 10) <= attrs.updatedAt) {
+        return res.json({
+          success: false,
+          error: 'Invalid `updatedAt` request query',
+        });
+      }
+      const message = `${attrs._id}-${updatedAt}`;
+      if (verifyECDSA(message, publicKey, signature)) {
         await radiksCollection.deleteOne({ _id: req.params.id });
         return res.json({
           success: true,

src/lib/validator.ts

@@ -6,11 +6,11 @@ const errorMessage = (message: string) => {
 };
 
 class Validator {
-  private db: Collection;
+  db: Collection;
 
-  private attrs: any;
+  attrs: any
 
-  private previous: any;
+  previous: any;
 
   constructor(db: Collection, attrs: any) {
     this.db = db;
@@ -22,6 +22,7 @@ class Validator {
     await this.fetchPrevious();
     await this.validateSignature();
     await this.validatePrevious();
+    await this.validateUpdatedAt();
     return true;
   }
 
@@ -79,6 +80,22 @@ class Validator {
     }
   }
 
+  validateUpdatedAt() {
+    if (!this.previous) {
+      return true;
+    }
+    if (typeof this.previous.updatedAt !== "number") {
+      errorMessage("This model's previous `updatedAt` is not a number");
+    }
+    if (typeof this.attrs.updatedAt !== "number") {
+      errorMessage("This model's `updatedAt` is not a number");
+    }
+    if (this.attrs.updatedAt <= this.previous.updatedAt) {
+      errorMessage("Model's `updatedAt` has not been increased");
+    }
+    return true;
+  }
+
   validatePresent(key: string) {
     if (!this.attrs[key]) {
       errorMessage(`No '${key}' attribute, which is required.`);

test/controllers/models-controller.test.ts

@@ -1,5 +1,6 @@
 import '../setup';
 import request from 'supertest';
+import { Express } from 'express';
 import { signECDSA } from 'blockstack/lib/encryption';
 import { makeECPrivateKey } from 'blockstack/lib/keys';
 import getApp from '../test-server';
@@ -12,6 +13,7 @@ jest.mock(
   '../../src/lib/validator',
   () =>
     class FakeValidator {
+      // eslint-disable-next-line class-methods-use-this
       validate() {
         return true;
       }
@@ -44,7 +46,7 @@ test('it can save the same model twice', async () => {
   expect(response.body.success).toEqual(true);
 });
 
-const getDocs = async (app, query) => {
+const getDocs = async (app: Express, query: any) => {
   const req = request(app)
     .get('/radiks/models/find')
     .query(query);
@@ -99,13 +101,14 @@ test('it can delete a model', async () => {
   const radiksData = db.collection(constants.COLLECTION);
   await signer.save(db);
   await radiksData.insertOne(model);
+  signer.sign(model);
   const { signature } = signECDSA(
     signer.privateKey,
     `${model._id}-${model.updatedAt}`
   );
   const response = await request(app)
     .del(`/radiks/models/${model._id}`)
-    .query({ signature });
+    .query({ signature, updatedAt: model.updatedAt });
   expect(response.body.success).toEqual(true);
   const dbModel = await radiksData.findOne({ _id: model._id });
   expect(dbModel).toBeNull();
@@ -120,14 +123,38 @@ test('it cannot delete with an invalid signature', async () => {
   const radiksData = db.collection(constants.COLLECTION);
   await signer.save(db);
   await radiksData.insertOne(model);
+  const updatedAt = (model.updatedAt as number) + 1;
   const { signature } = signECDSA(
     makeECPrivateKey(),
+    `${model._id}-${updatedAt}`
+  );
+  const response = await request(app)
+    .del(`/radiks/models/${model._id}`)
+    .query({ signature, updatedAt });
+  expect(response.body.success).toEqual(false);
+  expect(response.body.error).toEqual('Invalid signature');
+  const dbModel = await radiksData.findOne({ _id: model._id });
+  expect(dbModel).not.toBeNull();
+});
+
+test('it cannot update with an older updatedAt', async () => {
+  const app = await getApp();
+  const model = { ...models.test1 };
+  const signer = new Signer();
+  signer.sign(model);
+  const db = await getDB();
+  const radiksData = db.collection(constants.COLLECTION);
+  await signer.save(db);
+  await radiksData.insertOne(model);
+  const { signature } = signECDSA(
+    signer.privateKey,
     `${model._id}-${model.updatedAt}`
   );
   const response = await request(app)
     .del(`/radiks/models/${model._id}`)
-    .query({ signature });
+    .query({ signature, updatedAt: model.updatedAt });
   expect(response.body.success).toEqual(false);
+  expect(response.body.error).toEqual('Invalid `updatedAt` request query');
   const dbModel = await radiksData.findOne({ _id: model._id });
   expect(dbModel).not.toBeNull();
 });

test/mocks.ts

@@ -4,7 +4,19 @@ import constants from '../src/lib/constants';
 
 const userGroupId = faker.random.uuid();
 
-const models = {
+interface MockModel {
+  _id: string;
+  updatedAt?: number;
+  createdAt?: number;
+  signingKeyId?: string;
+  [key: string]: any;
+}
+
+interface Models {
+  [key: string]: MockModel;
+}
+
+const models: Models = {
   test1: {
     name: faker.name.findName(),
     email: faker.internet.email(),

test/setup.ts

@@ -7,7 +7,11 @@ dotenv.config({
   path: path.resolve(process.cwd(), '.env.test'),
 });
 
-jest.mock('request-promise', () => options => {
+interface RequestPromiseArgs {
+  uri: string;
+}
+
+jest.mock('request-promise', () => (options: RequestPromiseArgs) => {
   const { models } = require('./mocks'); // eslint-disable-line
   const { uri } = options;
   return Promise.resolve(models[uri]);

test/signer.ts

@@ -1,20 +1,24 @@
+/* eslint-disable no-param-reassign */
 import { makeECPrivateKey, getPublicKeyFromPrivate } from 'blockstack/lib/keys';
 import { signECDSA } from 'blockstack/lib/encryption';
 import uuid from 'uuid/v4';
+import { Db } from 'mongodb';
 import constants from '../src/lib/constants';
 
 export default class Signer {
-  private _id: string;
-  private privateKey: string;
-  private publicKey: string;
+  _id: string;
+
+  privateKey: string;
+
+  publicKey: string;
 
   constructor(privateKey?: string) {
     this.privateKey = privateKey || makeECPrivateKey();
     this.publicKey = getPublicKeyFromPrivate(this.privateKey);
     this._id = uuid();
   }
 
-  save(db) {
+  save(db: Db) {
     const { _id, privateKey, publicKey } = this;
     return db.collection(constants.COLLECTION).insertOne({
       _id,
@@ -24,7 +28,7 @@ export default class Signer {
     });
   }
 
-  sign(doc) {
+  sign(doc: any) {
     const now = new Date().getTime();
     doc.updatedAt = now;
     doc.signingKeyId = doc.signingKeyId || this._id;

test/validator.test.ts

@@ -29,9 +29,9 @@ test('it doesnt allow mismatched signingKeyId', async () => {
     ...models.hank,
   };
   signer.sign(model);
-  await db.collection(constants.COLLECTION).insertOne(model);
   let validator = new Validator(db.collection(constants.COLLECTION), model);
   expect(await validator.validate()).toEqual(true);
+  await db.collection(constants.COLLECTION).insertOne(model);
 
   const secondSigner = new Signer();
   await secondSigner.save(db);
@@ -61,7 +61,7 @@ test('it allows changing the signing key if signed with previous signing key', a
   expect(await validator.validate()).toEqual(true);
 });
 
-test('it doesnt allow older updatedAt', async () => {
+test('it doesnt allow updating if `updatable` is false', async () => {
   const model = {
     ...models.notUpdatable,
   };
@@ -148,3 +148,20 @@ test('allows users to use personal signing key', async () => {
   const validator = new Validator(db.collection(constants.COLLECTION), user);
   expect(await validator.validate()).toEqual(true);
 });
+
+test('doesnt allow updating if updatedAt hasnt changed', async () => {
+  const model = {
+    ...models.hank,
+  };
+  const signer = new Signer();
+  const db = await getDB();
+  await signer.save(db);
+  signer.sign(model);
+  let validator = new Validator(db.collection(constants.COLLECTION), model);
+  expect(await validator.validate()).toBe(true);
+  await db.collection(constants.COLLECTION).insertOne(model);
+  validator = new Validator(db.collection(constants.COLLECTION), model);
+  await expect(validator.validate()).rejects.toThrow(
+    "Model's `updatedAt` has not been increased"
+  );
+});