refactor: use convict for configuration #190

arshadali172 · 2020-08-21T09:33:27Z

Problem

Closes #92

Solution

Refactoring

Move environment variable default and validation to convict
Refactor configuration values into compulsoryVarsSchema, optionalVarsSchema and prodOnlyVarsSchema to make clear which env vars are needed when
Add validation of AWS bucket Urls
Move config types into types/config.ts
Refactor configuration of nodemailer transport to be based on environment, not on existence of environment variables
Refactor configuration of dbHost to be based on environment and enforce format of mongodb uri

Clean-up

Remove SES_RATEDELTA and SES_RATE which are deprecated from nodemailer
Add default for chromiumBin
Move cspReportUri to sentry bonus feature, enforce url type and only add to helmet if defined

Documentation

Document IS_LOGIN_BANNER which weren't
Document CSP_REPORT_URI under sentry bonus feature
Move other env vars to sections where they should be

Security

Remove default for sessionSecret to prevent accidental non-specification

Tests

Test submission on all fields on email form in staging
Test submission on all fields on encrypt form in staging
Test submission on a SingPass form in staging

arshadali172 · 2020-08-21T09:33:41Z

Discussion Points

DB_HOST should not be allowed to be an empty string. This might lead to production issues. Also, how can we enforce the format?
I don't think SESSION_SECRET should have a default. This may lead to production issues and is encouraging bad practice?Thoughts? @karrui
Can we have a better default for CSP_REPORT_URI? @liangyuanruo
Should we add a default for CHROMIUM_BIN?
Should we make it clearer which environment variables definitely need to be specified?

liangyuanruo · 2020-08-21T16:48:36Z

DB_HOST should not be allowed to be an empty string. This might lead to production issues. Also, how can we enforce the format?

this library might be able to help

I don't think SESSION_SECRET should have a default. This may lead to production issues and is encouraging bad practice?

Yes definitely, please remove and have the server throw an exception if it is not supplied.

Can we have a better default for CSP_REPORT_URI?

Technically this is an optional environment variable, perhaps we can refactor to simply not include it in the CSP headers in the server response, if no value is supplied?

Should we add a default for CHROMIUM_BIN?

Sounds good to me, can provide the default alpine install location.

Should we make it clearer which environment variables definitely need to be specified?

If you mean readme, yes definitely.

arshadali172 · 2020-08-24T07:53:20Z

DB_HOST should not be allowed to be an empty string. This might lead to production issues. Also, how can we enforce the format?

this library might be able to help

I don't think SESSION_SECRET should have a default. This may lead to production issues and is encouraging bad practice?

Yes definitely, please remove and have the server throw an exception if it is not supplied.

Can we have a better default for CSP_REPORT_URI?

Technically this is an optional environment variable, perhaps we can refactor to simply not include it in the CSP headers in the server response, if no value is supplied?

Should we add a default for CHROMIUM_BIN?

Sounds good to me, can provide the default alpine install location.

Should we make it clearer which environment variables definitely need to be specified?

If you mean readme, yes definitely.

Addressed all concerns

src/config/schema.ts

src/config/config.ts

mantariksh

some additional thoughts. I also noticed that there are some env vars is spcp-myinfo.factory.js. Should we be folding this into the new schema as well?

src/config/schema.ts

src/shared/constants.ts

src/config/schema.ts

arshadali172 · 2020-09-02T04:58:21Z

some additional thoughts. I also noticed that there are some env vars is spcp-myinfo.factory.js. Should we be folding this into the new schema as well?

good catch. I didn't even notice them. MyInfo is its own beast? and those env vars should go into spcp-myinfo.config

…in production urls

mantariksh

a few minor nits remaining! also, don't we need to add AWS_ENDPOINT=http://localhost:4572 to docker-compose.yml so the dev environment doesn't break?

docs/DEPLOYMENT_SETUP.md

src/config/config.ts

src/config/schema.ts

Co-authored-by: Antariksh Mahajan <[email protected]>

src/config/schema.ts

mantariksh

lgtm with one minor comment! thank you for accommodating all my requests!

mantariksh · 2020-09-03T05:27:20Z

src/config/schema.ts

+      doc: 'Endpoint for S3 buckets',
+      format: (val) =>
+        validateBucketUrl(val, { isDev, hasTrailingSlash: false, region }),
+      default: 'https://s3.ap-southeast-1.amazonaws.com', // NOTE NO TRAILING / AT THE END OF THIS URL!


I would rather add this as an env var in prod than rely on a default the code, since there is a higher chance of messing up the code. But small matter, up to you

This reverts commit 68a9ea5.

…)" (#285)" This reverts commit 3e61aca.

arshadali172 changed the title ~~Conviction~~ refactor: use convict for configuration Aug 21, 2020

arshadali172 requested review from liangyuanruo and mantariksh August 24, 2020 08:00

arshadali172 force-pushed the conviction branch from 7e5b0b2 to 7da818e Compare August 24, 2020 11:20

mantariksh suggested changes Aug 27, 2020

View reviewed changes

liangyuanruo removed their request for review August 28, 2020 02:25

arshadali172 force-pushed the conviction branch from 7da818e to aa0ab4e Compare September 2, 2020 03:39

arshadali172 requested a review from mantariksh September 2, 2020 03:41

mantariksh suggested changes Sep 2, 2020

View reviewed changes

src/config/schema.ts Outdated Show resolved Hide resolved

src/shared/constants.ts Outdated Show resolved Hide resolved

src/config/schema.ts Show resolved Hide resolved

arshadali172 requested a review from mantariksh September 2, 2020 05:08

arshadali172 force-pushed the conviction branch from 68ac1c0 to 07d602c Compare September 2, 2020 07:07

Arshad Ali added 15 commits September 2, 2020 15:33

Move session secret to config

d2e6efc

Move port to config

bbbe780

Move otpLifeSpan, bounceLifeSpan and submissionsTopUp to config

5fc1503

Move appConfig to config

b115943

Move formsgSdkMode to config

aa7b615

Move awsConfig to config

788995d

Move types out

243cdfe

Move cspReportUri and chromiumBin to config

877b272

Fix bugs with config loading

ee3da06

Move banners and custom watch grp to config

ade9aa7

Move dbHost to config

4cc60a0

Remove deprecated env vars

245f61d

Move ses to convict and enforce that prod ses are defined

72d3c73

Remove repeated code to check bucket url

87b3754

Reorganise banner env vars under banner key/header

13b3e36

Arshad Ali added 7 commits September 2, 2020 15:33

Move aws default endpoint to aws endpoint env var and enforce region …

fee4481

…in production urls

Fix rebase issues

c2472a5

Remove FORMSG_LOCALSTACK_ENDPT

611c4c3

Remove logger from config

ea438f0

Require config in logger now that config does not use logger

1d0098d

Remove aws region default

9a3e3ed

Use config.nodeEnv instead of process.env

38b4292

arshadali172 force-pushed the conviction branch from d80863e to 38b4292 Compare September 2, 2020 07:33

arshadali172 mentioned this pull request Sep 2, 2020

fix(deps): bump helmet from 3.23.1 to 4.1.0 #233

Merged

4 tasks

mantariksh suggested changes Sep 2, 2020

View reviewed changes

arshadali172 and others added 2 commits September 2, 2020 17:33

Update docs/DEPLOYMENT_SETUP.md

2f5c179

Co-authored-by: Antariksh Mahajan <[email protected]>

Update docs/DEPLOYMENT_SETUP.md

8128ff9

Co-authored-by: Antariksh Mahajan <[email protected]>

mantariksh reviewed Sep 2, 2020

View reviewed changes

src/config/schema.ts Show resolved Hide resolved

mantariksh reviewed Sep 2, 2020

View reviewed changes

src/config/schema.ts Show resolved Hide resolved

Arshad Ali added 3 commits September 2, 2020 17:48

Add AWS_ENDPOINT to docker file for dev purposes

eaf0a0b

Add comments to specify how convict defaults work

7af2d49

Add a session secret default for dev environment

958af5a

arshadali172 requested a review from mantariksh September 3, 2020 03:04

Add default for aws endpoint in docker dev file

e6f3c05

mantariksh approved these changes Sep 3, 2020

View reviewed changes

arshadali172 merged commit 68a9ea5 into develop Sep 3, 2020

liangyuanruo deleted the conviction branch September 6, 2020 07:56

liangyuanruo added a commit that referenced this pull request Sep 6, 2020

revert(convict): "refactor: use convict for configuration (#190)"

a3513d4

This reverts commit 68a9ea5.

liangyuanruo added a commit that referenced this pull request Sep 6, 2020

revert(convict): "refactor: use convict for configuration (#190)" (#285)

3e61aca

This reverts commit 68a9ea5.

arshadali172 restored the conviction branch September 7, 2020 02:00

arshadali172 added a commit that referenced this pull request Sep 7, 2020

Revert "revert(convict): "refactor: use convict for configuration (#190…

6549ed6

…)" (#285)" This reverts commit 3e61aca.

This was referenced Sep 7, 2020

revert: reintroduce convict #286

Closed

revert: reintroduce convict #287

Merged

snyk-bot mentioned this pull request Dec 31, 2020

[Snyk] Upgrade neverthrow from 3.0.0 to 3.1.2 #968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use convict for configuration #190

refactor: use convict for configuration #190

arshadali172 commented Aug 21, 2020 •

edited

Loading

arshadali172 commented Aug 21, 2020 •

edited

Loading

liangyuanruo commented Aug 21, 2020

arshadali172 commented Aug 24, 2020

mantariksh left a comment

arshadali172 commented Sep 2, 2020

mantariksh left a comment

mantariksh left a comment

mantariksh Sep 3, 2020

refactor: use convict for configuration #190

refactor: use convict for configuration #190

Conversation

arshadali172 commented Aug 21, 2020 • edited Loading

Problem

Solution

Tests

arshadali172 commented Aug 21, 2020 • edited Loading

Discussion Points

liangyuanruo commented Aug 21, 2020

arshadali172 commented Aug 24, 2020

mantariksh left a comment

Choose a reason for hiding this comment

arshadali172 commented Sep 2, 2020

mantariksh left a comment

Choose a reason for hiding this comment

mantariksh left a comment

Choose a reason for hiding this comment

mantariksh Sep 3, 2020

Choose a reason for hiding this comment

arshadali172 commented Aug 21, 2020 •

edited

Loading

arshadali172 commented Aug 21, 2020 •

edited

Loading