Skip to content
This repository has been archived by the owner on Apr 9, 2024. It is now read-only.

Scan components to find asset folders to serve as static resources #22

Merged
merged 6 commits into from Apr 16, 2019
Merged

Scan components to find asset folders to serve as static resources #22

merged 6 commits into from Apr 16, 2019

Conversation

umaar
Copy link
Contributor

@umaar umaar commented Apr 12, 2019

Also:

  • Updates the readme to mention the service_path
  • Updates the route static tests to use tap, which has a compatible API, but provides better async support than tape
  • Updates tests

To expand upon my change of tape to tap: tape didn't have great async support, whereas tap does. While I think there are even better test packages out there, using tap is fairly unobtrusive as it has a compatible API, and the test command doesn't even need modifying. All that changes is the require statement at the top of the file...we can do this incrementally. Tests can go from:

test('When a file that does not exist within a static route is requested', t => {
  const app = callRoutesStatic()

  request(app)
    .get('/static.txt')
    .end((err, res) => {
      t.equals(err, null, 'it should not invoke an error')
      t.equals(res.status, 404, 'it should return 404')
      t.end()
    })
})

To this:

test('When a file that does not exist within a static route is requested', async t => {
  const app = await callRoutesStatic()
  const {error, status} = await request(app).get('/static.txt')
  t.ok(error, null, 'it should invoke an error')
  t.equals(status, 404, 'it should return 404')
})

Which is better IMO.

Also converted the method signature to use objects rather than function(arg1, arg2, arg3) etc. As I think it's more readable...ideally though, if we agree, we can add such rules to a linter at some point!

Also:

- Updates the readme to mention the service_path
- Updates the route static tests to use tap, which has a compatible API, but provides better async support than tape
- Updates tests
@umaar
Copy link
Contributor Author

umaar commented Apr 12, 2019

@solidgoldpig setting this as a draft PR since this is my first. Let me know of any feedback!

/cc @asmega @theKHutDeveloper for visibility

@umaar
Copy link
Contributor Author

umaar commented Apr 12, 2019

Also heads up: the tests were failing for me in master and caused a bunch of confusion as a different branch (the refinement branch) had some fixes...would be good to keep master clean - having a proper PR process, where our PRs are continuously tested for linting errors, tests failures etc. and then rejected if necessary would be a good idea!

@umaar
Copy link
Contributor Author

umaar commented Apr 15, 2019

Code review feedback so far:

  • Fix linting
  • Don't use experimental Node.js features for the async stuff, but rather use Util.promisify

…de-modules-for-asset-folders

# Conflicts:
#	lib/middleware/routes-static/routes-static.test.govuk-frontend.unit.spec.js
#	lib/middleware/routes-static/routes-static.test.multiple-paths.unit.spec.js
#	lib/middleware/routes-static/routes-static.test.no-paths.unit.spec.js
#	lib/middleware/routes-static/routes-static.unit.spec.js
#	package-lock.json
- Address code review feedback
- Remove the serviceDir from route static
- Removes uneeded test
- Updates package lock file
@umaar umaar marked this pull request as ready for review April 16, 2019 09:51
@umaar
Copy link
Contributor Author

umaar commented Apr 16, 2019

Hey @solidgoldpig thanks for looking at this! Have pulled in your changes, resolved conflicts, addressed code review feedback and now using Util.promisify instead of experimental Node.js features. If you want to have another look that'd be useful.

Also if you think it's worth doing the ava stuff in this branch, can do that, otherwise will save it for next time

@solidgoldpig
Copy link
Contributor

solidgoldpig commented Apr 16, 2019

All good.

I think leave the ava-ification for another (but soonish) time

The introduction of tap I see has now triggered a bunch of audit warnings.

Locally I ran npm audit fix and it sorted 8 out of the 9 issues, but tap's handlebars dependency (via istanbul-reports) is still complaining

┌───────────────┬──────────────────────────────────────────────────────────────┐
│ High          │ Prototype Pollution                                          │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Package       │ handlebars                                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Patched in    │ >=4.0.14 <4.1.0 || >=4.1.2                                   │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Dependency of │ tap [dev]                                                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ Path          │ tap > nyc > istanbul-reports > handlebars                    │
├───────────────┼──────────────────────────────────────────────────────────────┤
│ More info     │ https://nodesecurity.io/advisories/755                       │
└───────────────┴──────────────────────────────────────────────────────────────┘

@umaar
Copy link
Contributor Author

umaar commented Apr 16, 2019

For that particular remaining advisory, fortunately we are immune since the attack vector is from user supplied input. In our case, tap is only used as a dev dependency and is not exposed to the user.

Also, looking at this: handlebars-lang/handlebars.js#1495 (comment) they have fixed the issue. The version they list as fixed was the same version we had installed, I assume the NPM advisory hasn't been updated to take into account backported fixes.

I updated the package lock file anyway though

@umaar
Copy link
Contributor Author

umaar commented Apr 16, 2019

Update: Just to be certain, I verified 4.1.0 (which we would have installed):

  • Includes the security fix
  • Has been published on NPM

@solidgoldpig
Copy link
Contributor

Good to go

@umaar umaar merged commit cd0fed4 into ministryofjustice:master Apr 16, 2019
@umaar umaar deleted the scan-node-modules-for-asset-folders branch April 16, 2019 12:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants