strimzi / strimzi-ui

Strimzi UI
Apache License 2.0
26 stars 27 forks source link

fix: Sort express mountpoints to avoid requests getting hijacked #133

Closed ajborley closed 3 years ago

ajborley commented 3 years ago

Contributes to: #42

Signed-off-by: Andrew Borley borley@uk.ibm.com

github-actions[bot] commented 3 years ago

PR Report

Bundle Sizes

View bundle sizes for 'client'
|Bundle|New Size|Original Size|Increase/Decrease|File| |---|---|---|---|---| |1.bundle.js|631.33KB|631.33KB|0%|1.bundle.js| |Strimzi UI JS Bundle|11.08KB|11.08KB|0%|main.bundle.js| ##### Overall bundle size change: 0%
View bundle sizes for 'server'
|Bundle|New Size|Original Size|Increase/Decrease|File| |---|---|---|---|---| |Strimzi UI Server JS Bundle|26.07KB|25.83KB|1%|main.js| ##### Overall bundle size change: 0.94%

Test Coverage

View test coverage
| File | Lines | Statement | Functions | Branches | | --- | --- | --- | --- | --- | | Total | 100% | 100% | 100% | 100% | | client/Bootstrap/Navigation/useRouteConfig/useRouteConfig.hook.ts | 100% | 100% | 100% | 100% | | client/Contexts/ConfigFeatureFlag/Context.tsx | 100% | 100% | 100% | 100% | | client/Contexts/ConfigFeatureFlag/FeatureFlag.view.tsx | 100% | 100% | 100% | 100% | | client/Contexts/Introspect/Introspection.ts | 100% | 100% | 100% | 100% | | client/Contexts/Logging/Context.tsx | 100% | 100% | 100% | 100% | | client/Hooks/useConfigFeatureFlag/useConfigFeatureFlag.ts | 100% | 100% | 100% | 100% | | client/Hooks/useLogger/Hook.ts | 100% | 100% | 100% | 100% | | client/Panels/Home/Home.tsx | 100% | 100% | 100% | 100% | | client/Utils/sanitise/sanitise.ts | 100% | 100% | 100% | 100% | | client/Utils/window/window.ts | 100% | 100% | 100% | 100% | | File | Lines | Statement | Functions | Branches | | --- | --- | --- | --- | --- | | Total | 100% | 100% | 100% | 100% | | server/api/controller.ts | 100% | 100% | 100% | 100% | | server/api/router.ts | 100% | 100% | 100% | 100% | | server/client/controller.ts | 100% | 100% | 100% | 100% | | server/client/router.ts | 100% | 100% | 100% | 100% | | server/config/controller.ts | 100% | 100% | 100% | 100% | | server/config/router.ts | 100% | 100% | 100% | 100% | | server/core/app.ts | 100% | 100% | 100% | 100% | | server/core/modules.ts | 100% | 100% | 100% | 100% | | server/log/router.ts | 100% | 100% | 100% | 100% | | server/mockapi/data.ts | 100% | 100% | 100% | 100% | | server/mockapi/router.ts | 100% | 100% | 100% | 100% |

Triggered by commit: e4499460c95472438309a6544084879e079ce27e

matthew-chirgwin commented 3 years ago

Hi @ajborley - am I right in thinking (given the change made), this is to stop https://github.com/strimzi/strimzi-ui/blob/e4499460c95472438309a6544084879e079ce27e/server/client/router.ts#L48 responding to all requests?

ajborley commented 3 years ago

@matthew-chirgwin - Yep, the ordering in https://github.com/strimzi/strimzi-ui/blob/master/server/core/modules.ts meant that the express router was mounting the client module on / before the config, log or mockapi modules. That means that calls to GET /log or GET /config were being serviced by the client module.

This PR reverse-sorts the list based on the mountpoint so that the order of mounting is now:

It also means, for example, that if we have a 'topics' module mounted at /api/topics in the future, calls to that endpoint will get serviced by the topics module rather than the api module.