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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 27 commits into from
Sep 16, 2020
Merged

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

merged 27 commits into from
Sep 16, 2020

Conversation

tshuli
Copy link
Contributor

@tshuli tshuli commented Aug 31, 2020 •
edited
Loading

Problem

Builds on #160
Closes #180
Bumps helmet from 3.23.1 to 4.1.0.

New dependencies:

  • nocache : helmet.noCache is deprecated and was removed in helmet@4. Replace with nocache npm package.

Tests

  • Check that the following CSPs are present: defaultSrc, imgSrc, fontSrc, scriptSrc, connectSrc, frameSrc, objectSrc, styleSrc, formAction, upgradeInsecureRequests, reportUri
  • imgSrc / connectSrc: check that image can be uploaded correctly for form
  • fontSrc: check that SG government banner renders correctly
  • scriptSrc / frameSrc / styleSrc: check that captcha works and is rendered correctly

@karrui karrui mentioned this pull request Aug 31, 2020
Closed
@tshuli tshuli force-pushed the helmetJS-migration branch 2 times, most recently from 2d3a0f6 to b92d4da Compare September 1, 2020 07:02
@tshuli tshuli marked this pull request as ready for review September 1, 2020 07:34
mantariksh
mantariksh approved these changes Sep 1, 2020
View reviewed changes
Copy link
Contributor

@mantariksh mantariksh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm (after lots of research about helmetJS haha) but roping in @arshadali172 for a review as well since this is clearly high-risk

src/loaders/express/helmet.ts Outdated Show resolved Hide resolved
mantariksh
mantariksh suggested changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@mantariksh mantariksh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

as discussed on Slack, to use types rather than object literals

@tshuli
Copy link
Contributor Author

tshuli commented Sep 2, 2020

@mantariksh changes made, for your re-review

mantariksh
mantariksh approved these changes Sep 2, 2020
View reviewed changes
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

minor quibbles about variable naming, other than that lgtm!

src/loaders/express/helmet.ts Outdated Show resolved Hide resolved
src/loaders/express/helmet.ts Outdated Show resolved Hide resolved
arshadali172
arshadali172 reviewed Sep 2, 2020
View reviewed changes
src/config/config.ts Outdated Show resolved Hide resolved
@mantariksh
Copy link
Contributor

@shuli-ogp yo, what's the status on this? are you making any more changes?

@tshuli
Copy link
Contributor Author

tshuli commented Sep 8, 2020

@mantariksh yup, sorting out the merge conflicts from #190 + writing tests to check the headers

@tshuli
Copy link
Contributor Author

tshuli commented Sep 11, 2020

@mantariksh @arshadali172 I've fixed merge conflicts and added on tests. Appreciate your re-review before I merge!

mantariksh
mantariksh suggested changes Sep 14, 2020
View reviewed changes
src/loaders/express/helmet.ts Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
mantariksh
mantariksh suggested changes Sep 14, 2020
View reviewed changes
src/loaders/express/helmet.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
mantariksh
mantariksh suggested changes Sep 14, 2020
View reviewed changes
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

minor nits remaining!

src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
dependabot bot and others added 3 commits September 15, 2020 11:42
@dependabot @tshuli
fix(deps): bump helmet from 3.23.1 to 4.1.0
44e931a 
Bumps [helmet](https://github.com/helmetjs/helmet) from 3.23.1 to 4.1.0.
- [Release notes](https://github.com/helmetjs/helmet/releases)
- [Changelog](https://github.com/helmetjs/helmet/blob/master/CHANGELOG.md)
- [Commits](helmetjs/helmet@v3.23.1...v4.1.0)

Signed-off-by: dependabot[bot] <support@github.com>
(cherry picked from commit 2437e8dd442a3ef071617b2fe396b667145c5dd9)
@tshuli
chore: import nocache
b2a8bf7
@tshuli
fix: merge conflicts
e87055d
@tshuli
fix: declare type of cspDirective
38b0441
@tshuli
test: check that helmet sets the correct headers
3ba2e40
@tshuli
fix: replace helmet.noCache with nocache
4f7fb12
@tshuli
docs: explain why if statement is necessary for optional directives
1b33872
@tshuli
chore: improve tests
258e255
@tshuli
chore: update styleSrc in tests
0d140b3
@tshuli
docs: link to helmetjs issue on falsey values
d242b18
@tshuli
chore: shift mock implementations to beforeEach
ccafdf3
@tshuli
refactor: use mockReturnValue instead of mockImplementation
2cfe119
@tshuli
chore: remove unnecessary mock function implementation which returns …
ea62812 
…undefined
@tshuli
chore: simplify csp tests and make them stricter
4887025
@tshuli
chore: use beforeAll and clearAllMocks
c742f29
@tshuli
refactor: mockReturnValue instead of mockImplementation
2d965bd
@tshuli
fix: set correct mockFeatureManager.props for !cspReportUri
3cd76f8
@tshuli
refactor: shift cspCoreDirective constant up
602d145
@tshuli
chore: add tests for hsts
d187865
mantariksh
mantariksh suggested changes Sep 15, 2020
View reviewed changes
Copy link
Contributor

@mantariksh mantariksh left a comment •
edited
Loading

Choose a reason for hiding this comment

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

nearly there! just a few adjustments. also, your tests folder should be called __tests__. this is a naming convention for jest.

src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
src/loaders/express/tests/helmet.spec.ts Outdated Show resolved Hide resolved
tests/unit/backend/helpers/jest-express.ts Outdated Show resolved Hide resolved
mantariksh
mantariksh approved these changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@mantariksh mantariksh left a comment

Choose a reason for hiding this comment

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

lgtm! thank you for your patience with all the feedback, and I'm glad that one of our most important modules is now well-tested!

@tshuli tshuli merged commit 0ec85ac into develop Sep 16, 2020
@tshuli tshuli deleted the helmetJS-migration branch September 16, 2020 14:36
@tshuli tshuli mentioned this pull request Sep 22, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mantariksh mantariksh mantariksh approved these changes

@arshadali172 arshadali172 Awaiting requested review from arshadali172

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

HelmetJS v4 migration
3 participants
@tshuli @mantariksh @arshadali172