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

Add "Risk Matrix" section to the PR template #100649

Merged
merged 15 commits into from
Jun 2, 2021
Merged
20 changes: 20 additions & 0 deletions .github/PULL_REQUEST_TEMPLATE.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

Summarize your PR. If it involves visual changes include a screenshot or gif.


### Checklist

Delete any items that are not applicable to this PR.
Expand All @@ -15,6 +16,25 @@ Delete any items that are not applicable to this PR.
- [ ] This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [ ] This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)


### Risk Matrix
streamich marked this conversation as resolved.
Show resolved Hide resolved

Delete this section if it is not applicable to this PR.

Before closing this PR, invite QA, stakeholders, and other developers to
identify risks that should be tested prior to the change/feature release.

When forming the risk matrix, consider some of the following examples and how
they may potentially impact the change:

| Risk | Probability | Severity | Mitigation/Notes |
|---------------------------|-------------|----------|-------------------------|
| Multiple Spaces—unexpected behavior in non-default Kibana Space. | Low | High | Integration tests will verify that all features are still supported in non-default Kibana Space and when user switches between spaces. |
| Multiple nodes—Elasticsearch polling might have race conditions when multiple Kibana nodes are polling for the same tasks. | High | Low | Tasks are idempotent, so executing them multiple times will not result in logical error, but will degrade performance. To test for this case we add plenty of unit tests around this logic and document manual testing procedure. |
| Code should gracefully handle cases when feature X or plugin Y are disabled. | Medium | High | Unit tests will verify that any feature flag or plugin combination still results in our service operational. |
| [See more potential risk examples](../RISK_MATRIX.md) |


### For maintainers

- [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)
61 changes: 61 additions & 0 deletions RISK_MATRIX.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
# Risk consideration
streamich marked this conversation as resolved.
Show resolved Hide resolved

When merging a new feature of considerable size or modifying an existing one,
consider adding a *Risk Matrix* section to your PR in collaboration with other
developers on your team and the QA team.

Below are some general themes to consider for the *Risk Matrix*:
streamich marked this conversation as resolved.
Show resolved Hide resolved


## General risks

- What happens when your feature is used in a non-default space or a custom
space?
- What happens when Kibana is run in a cluster mode?
streamich marked this conversation as resolved.
Show resolved Hide resolved
- What happens when plugin you depend on is disabled?
streamich marked this conversation as resolved.
Show resolved Hide resolved
- What happens when feature you depend on is disabled?
streamich marked this conversation as resolved.
Show resolved Hide resolved
- Is your change working correctly regardless of `kibana.yml` configuration or
UI Setting configuration? (For example, does it support both
`state:storeInSessionStorage` UI setting states?)
- What happens when a third party integration you depend on is not responding?
- How is authentication handled with third party services?
- Does the feature work in Elastic Cloud?
- Does the feautre create a setting that needs to be exposed, or configured
streamich marked this conversation as resolved.
Show resolved Hide resolved
differently than the default, on the Elastic Cloud?
- Is there a significant performance impact that may affect Cloud Kibana
instances?
- Does your feature need to be aware of running in a container?
- Does the feature Work with security disabled, or fails gracefully?
streamich marked this conversation as resolved.
Show resolved Hide resolved
- Are there performance risks associated with your feature? Does it access:
streamich marked this conversation as resolved.
Show resolved Hide resolved
(1) many fields; (2) many indices; (3) lots of data; (4) lots of saved
objects; (5) large saved objects.
- Will leaving browser running overnight not have negative impacts ot the page
streamich marked this conversation as resolved.
Show resolved Hide resolved
and server performance?
streamich marked this conversation as resolved.
Show resolved Hide resolved
- Will your feature still work if Kibana is run behind a reverse proxy?
- Does your feature affect other plugins?
- If you write to the file system, what happens if Kibana node goes down? What
happens if Kibana is run in cluster mode?
streamich marked this conversation as resolved.
Show resolved Hide resolved
- Are migrations handled gracefully? Does the feature affect old indices or
saved objects?
- Are you using any technologies, protocols, techniques, conventions, libraries,
NPM modules, etc. that may be new or unprecedented in Kibana?


## Security risks
streamich marked this conversation as resolved.
Show resolved Hide resolved

- XSS attacks—can user inject unescaped HTML on the page? (For example through
React's `dangerouslySetInnerHtml`, `Element.innerHTML`, `Element.outerHTML`).
Is all user input escaped?
- CSRF attacks—are you not using the default Kibana HTTP service to
communicate with the server? If not ensure you configure correctly CSRF header
and talk with security team to review.
- Remote code execution attacks—is your code doing something "highly"
dynamic? Such as: (1) usage of `eval` function; (2) usage of `vm` or
`child_process` Node.js modules; (3) usage of dynamic requires; (4) running
template interpolation such as Handlebars or Lodash `_.template`.
- [Prototype pollution attacks](https://docs.google.com/document/d/19V-d9sb6IF-fbzF4iyiPpAropQNydCnoJApzSX5FdcI/edit?usp=sharing).
- User input validation—are you validating user input beyond the default
HTTP server schema validation? Are you validating complex user input, such
as expression language or KQL?
- Check for accidental reveal of sensitive information. It could be printed to
console through logs or errors.