Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

FORMS-1563: auto-approve new ext api when same ministry/url already a… #1543

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
30 changes: 26 additions & 4 deletions app/src/forms/admin/service.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const { ExternalAPIStatuses } = require('../common/constants');
const { Form, FormVersion, User, UserFormAccess, FormComponentsProactiveHelp, AdminExternalAPI, ExternalAPI, ExternalAPIStatusCode } = require('../common/models');
const { queryUtils } = require('../common/utils');
const { v4: uuidv4 } = require('uuid');
Expand Down Expand Up @@ -148,14 +149,35 @@ const service = {
upd['userTokenHeader'] = null;
upd['userTokenBearer'] = false;
}

await ExternalAPI.query().patchAndFetchById(id, upd);

return ExternalAPI.query().findById(id);
let trx;
try {
trx = await ExternalAPI.startTransaction();
await ExternalAPI.query(trx).patchAndFetchById(id, upd);
await service._approveMany(id, data, trx);
await trx.commit();
return ExternalAPI.query().findById(id);
} catch (err) {
if (trx) await trx.rollback();
throw err;
}
},
getExternalAPIStatusCodes: async () => {
return ExternalAPIStatusCode.query();
},
_approveMany: async (id, data, trx) => {
// if we are setting to approved, approve all similar endpoints.
// same ministry, same base url...
if (data.code === ExternalAPIStatuses.APPROVED) {
const adminExternalAPI = await AdminExternalAPI.query(trx).findById(id);
const delimiter = '?';
const baseUrl = data.endpointUrl.split(delimiter)[0];
await ExternalAPI.query(trx)
.patch({ code: ExternalAPIStatuses.APPROVED })
.whereRaw(`"formId" in (select id from form where ministry = '${adminExternalAPI.ministry}')`)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is an SQL injection possible with this raw query. It's convoluted, but unknown if/how a bigger problem could be made:

  1. Create a form "A"
  2. Copy your bearer token and curl in a put request for the form data and set the ministry to ' OR 'x'='x, so that the where clause evaluates to true no matter what the ministry is
  3. Create an external API for "A"
  4. Create a form "B" in a different ministry and give it the same URL for an external API
  5. Approve form A's external API, and form B's external API will be automatically approved.

Hopefully the admin will notice this:
image
but perhaps there are ways to obscure it.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That would be a bug in form API. And would be a db issue that we have no integrity on that column. So I can "block" that here but that's not where the bugs are. Even worse, Ministry values are frontend only. See: #1188

Adding a bandaid to prevent sql injection in this query, but bugs should be filed and addressed.

.andWhere('endpointUrl', 'ilike', `${baseUrl}%`)
.andWhere('code', ExternalAPIStatuses.SUBMITTED);
}
},

/**
* @function createFormComponentsProactiveHelp
Expand Down
84 changes: 69 additions & 15 deletions app/src/forms/form/externalApi/service.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const Problem = require('api-problem');
const { v4: uuidv4 } = require('uuid');
const { ExternalAPIStatuses } = require('../../common/constants');

const { ExternalAPI, ExternalAPIStatusCode } = require('../../common/models');
const { ExternalAPI, ExternalAPIStatusCode, Form, AdminExternalAPI } = require('../../common/models');

const { ENCRYPTION_ALGORITHMS } = require('../../../components/encryptionService');

Expand Down Expand Up @@ -41,6 +41,9 @@ const service = {
throw new Problem(422, `'userTokenHeader' is required when 'sendUserToken' is true.`);
}
}
if (!data.endpointUrl || (data.endpointUrl && !(data.endpointUrl.startsWith('https://') || data.endpointUrl.startsWith('http://')))) {
throw new Problem(422, `'endpointUrl' is required and must start with 'http://' or 'https://'`);
}
},

checkAllowSendUserToken: (data, allowSendUserToken) => {
Expand All @@ -60,20 +63,50 @@ const service = {
}
},

_updateAllPreApproved: async (formId, data, trx) => {
let result = 0;
const form = await Form.query().findById(formId);
const delimiter = '?';
const baseUrl = data.endpointUrl.split(delimiter)[0];
// check if there are matching api endpoints for the same ministry as our form that have been previously approved.
const approvedApis = await AdminExternalAPI.query(trx)
.where('endpointUrl', 'ilike', `${baseUrl}%`)
.andWhere('ministry', form.ministry)
.andWhere('code', ExternalAPIStatuses.APPROVED);
if (approvedApis && approvedApis.length) {
// ok, since we've already approved a matching api endpoint, make sure others on this form are approved too.
result = await ExternalAPI.query(trx)
.patch({ code: ExternalAPIStatuses.APPROVED })
.where('endpointUrl', 'ilike', `${baseUrl}%`)
.andWhere('formId', formId)
.andWhere('code', ExternalAPIStatuses.SUBMITTED);
}
return result;
},

createExternalAPI: async (formId, data, currentUser) => {
service.validateExternalAPI(data);

data.id = uuidv4();
// set status to SUBMITTED
// always create as SUBMITTED.
data.code = ExternalAPIStatuses.SUBMITTED;
// ensure that new records don't send user tokens.
service.checkAllowSendUserToken(data, false);
await ExternalAPI.query().insert({
...data,
createdBy: currentUser.usernameIdp,
});

return ExternalAPI.query().findById(data.id);
let trx;
try {
trx = await ExternalAPI.startTransaction();
await ExternalAPI.query(trx).insert({
...data,
createdBy: currentUser.usernameIdp,
});
// any urls on this form pre-approved?
await service._updateAllPreApproved(formId, data, trx);
await trx.commit();
return ExternalAPI.query().findById(data.id);
} catch (err) {
if (trx) await trx.rollback();
throw err;
}
},

updateExternalAPI: async (formId, externalAPIId, data, currentUser) => {
Expand All @@ -82,17 +115,38 @@ const service = {
const existing = await ExternalAPI.query().modify('findByIdAndFormId', externalAPIId, formId).first().throwIfNotFound();

// let's use a different method for the administrators to update status code and allow send user token
// this method should not change the status code.
// this method should not change the status code
data.code = existing.code;
if (existing.endpointUrl.split('?')[0] !== data.endpointUrl.split('?')[0]) {
// url changed, so save as SUBMITTED.
data.code = ExternalAPIStatuses.SUBMITTED;
}
service.checkAllowSendUserToken(data, existing.allowSendUserToken);
await ExternalAPI.query()
.modify('findByIdAndFormId', externalAPIId, formId)
.update({
...data,
let trx;
try {
trx = await ExternalAPI.startTransaction();
await ExternalAPI.query(trx).modify('findByIdAndFormId', externalAPIId, formId).update({
formId: formId,
WalterMoar marked this conversation as resolved.
Show resolved Hide resolved
name: data.name,
code: data.code,
WalterMoar marked this conversation as resolved.
Show resolved Hide resolved
endpointUrl: data.endpointUrl,
sendApiKey: data.sendApiKey,
apiKeyHeader: data.apiKeyHeader,
apiKey: data.apiKey,
allowSendUserToken: data.allowSendUserToken,
sendUserToken: data.sendUserToken,
userTokenHeader: data.userTokenHeader,
userTokenBearer: data.userTokenBearer,
updatedBy: currentUser.usernameIdp,
});

return ExternalAPI.query().findById(externalAPIId);
// any urls on this form pre-approved?
await service._updateAllPreApproved(formId, data, trx);
await trx.commit();
return ExternalAPI.query().findById(data.id);
} catch (err) {
if (trx) await trx.rollback();
throw err;
}
},

deleteExternalAPI: async (formId, externalAPIId) => {
Expand Down
4 changes: 4 additions & 0 deletions app/tests/unit/forms/admin/service.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -137,6 +137,7 @@ describe('Admin service', () => {
});

it('updateExternalAPI should patch and fetch', async () => {
service._approveMany = jest.fn().mockResolvedValueOnce();
await service.updateExternalAPI('id', { code: 'APPROVED', allowSendUserToken: true });
expect(MockModel.query).toBeCalledTimes(3);
expect(MockModel.patchAndFetchById).toBeCalledTimes(1);
Expand All @@ -145,9 +146,11 @@ describe('Admin service', () => {
code: 'APPROVED',
allowSendUserToken: true,
});
expect(service._approveMany).toBeCalledWith('id', { code: 'APPROVED', allowSendUserToken: true }, expect.anything());
});

it('updateExternalAPI should patch and fetch and update user token fields', async () => {
service._approveMany = jest.fn().mockResolvedValueOnce();
await service.updateExternalAPI('id', { code: 'APPROVED', allowSendUserToken: false });
expect(MockModel.query).toBeCalledTimes(3);
expect(MockModel.patchAndFetchById).toBeCalledTimes(1);
Expand All @@ -160,6 +163,7 @@ describe('Admin service', () => {
userTokenHeader: null,
userTokenBearer: false,
});
expect(service._approveMany).toBeCalledWith('id', { code: 'APPROVED', allowSendUserToken: false }, expect.anything());
});

it('getExternalAPIStatusCodes should fetch data', async () => {
Expand Down
38 changes: 34 additions & 4 deletions app/tests/unit/forms/form/externalApi/service.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ describe('createExternalAPI', () => {
});

it('should insert valid data', async () => {
service._updateAllPreApproved = jest.fn().mockResolvedValueOnce(0);
validData.id = null;
validData.code = null;
await service.createExternalAPI(validData.formId, validData, user);
Expand All @@ -164,6 +165,7 @@ describe('createExternalAPI', () => {
code: ExternalAPIStatuses.SUBMITTED,
...validData,
});
expect(service._updateAllPreApproved).toBeCalledWith(validData.formId, validData, expect.anything());
});

it('should raise errors', async () => {
Expand Down Expand Up @@ -205,6 +207,7 @@ describe('updateExternalAPI', () => {
});

it('should update valid data', async () => {
service._updateAllPreApproved = jest.fn().mockResolvedValueOnce(0);
MockModel.throwIfNotFound = jest.fn().mockResolvedValueOnce(Object.assign({}, validData));

// we do not update (status) code - must stay SUBMITTED
Expand All @@ -215,8 +218,18 @@ describe('updateExternalAPI', () => {
expect(MockModel.update).toBeCalledWith({
updatedBy: user.usernameIdp,
code: ExternalAPIStatuses.SUBMITTED,
...validData,
formId: validData.formId,
name: validData.name,
endpointUrl: validData.endpointUrl,
sendApiKey: validData.sendApiKey,
apiKeyHeader: validData.apiKeyHeader,
apiKey: validData.apiKey,
allowSendUserToken: validData.allowSendUserToken,
sendUserToken: validData.sendUserToken,
userTokenHeader: validData.userTokenHeader,
userTokenBearer: validData.userTokenBearer,
});
expect(service._updateAllPreApproved).toBeCalledWith(validData.formId, validData, expect.anything());
});

it('should update user token fields when allowed', async () => {
Expand All @@ -226,33 +239,50 @@ describe('updateExternalAPI', () => {
validData.userTokenHeader = 'Authorization';
validData.userTokenBearer = true;
MockModel.throwIfNotFound = jest.fn().mockResolvedValueOnce(Object.assign({}, validData));
service._updateAllPreApproved = jest.fn().mockResolvedValueOnce(0);

await service.updateExternalAPI(validData.formId, validData.id, validData, user);
expect(MockModel.update).toBeCalledTimes(1);
expect(MockModel.update).toBeCalledWith({
updatedBy: user.usernameIdp,
code: ExternalAPIStatuses.SUBMITTED,
...validData,
formId: validData.formId,
name: validData.name,
endpointUrl: validData.endpointUrl,
sendApiKey: validData.sendApiKey,
apiKeyHeader: validData.apiKeyHeader,
apiKey: validData.apiKey,
allowSendUserToken: validData.allowSendUserToken,
sendUserToken: validData.sendUserToken,
userTokenHeader: validData.userTokenHeader,
userTokenBearer: validData.userTokenBearer,
});
});

it('should blank out user token fields when not allowed', async () => {
// mark as allowed by admin, and set some user token config values...
validData.allowSendUserToken = true;
validData.allowSendUserToken = false;
validData.sendUserToken = false; // don't want to throw a 422...
validData.userTokenHeader = 'Authorization';
validData.userTokenBearer = true;
MockModel.throwIfNotFound = jest.fn().mockResolvedValueOnce(Object.assign({}, validData));
service._updateAllPreApproved = jest.fn().mockResolvedValueOnce(0);

await service.updateExternalAPI(validData.formId, validData.id, validData, user);
expect(MockModel.update).toBeCalledTimes(1);
expect(MockModel.update).toBeCalledWith({
updatedBy: user.usernameIdp,
code: ExternalAPIStatuses.SUBMITTED,
allowSendUserToken: false,
sendUserToken: false,
userTokenHeader: null,
userTokenBearer: false,
...validData,
formId: validData.formId,
name: validData.name,
endpointUrl: validData.endpointUrl,
sendApiKey: validData.sendApiKey,
apiKeyHeader: validData.apiKeyHeader,
apiKey: validData.apiKey,
});
});

Expand Down
Loading