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

[alerting] encode rule/connector ids in http requests made from alerting UI #97854

Merged
merged 6 commits into from
Apr 23, 2021

Conversation

pmuellr
Copy link
Member

@pmuellr pmuellr commented Apr 21, 2021

resolves: #97852

Summary

Adds encodeURIComponent() wrappers around references to rule, alert, and connector ids. Without this fix, if an alert id (which can contain customer-generated data) contains a character that needs to be URL encoded, the resulting API call from the web UI will fail.

Checklist

Delete any items that are not applicable to this PR.

For maintainers

…ing UI

resolves: elastic#97852

Adds `encodeURIComponent()` wrappers around references to rule, alert, and
connector ids.  Without this fix, if an alert id (which can contain
customer-generated data) contains a character that needs to be URL
encoded, the resulting API call from the web UI will fail.
@pmuellr pmuellr added Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Feature:Actions Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.13.0 labels Apr 21, 2021
@spalger spalger added v7.14.0 and removed v7.13.0 labels Apr 21, 2021
@pmuellr pmuellr marked this pull request as ready for review April 22, 2021 02:53
@pmuellr pmuellr requested a review from a team as a code owner April 22, 2021 02:53
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-alerting-services (Team:Alerting Services)

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@ymao1 ymao1 left a comment

Choose a reason for hiding this comment

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

LGTM!

@pmuellr
Copy link
Member Author

pmuellr commented Apr 22, 2021

@elasticmachine merge upstream

@pmuellr
Copy link
Member Author

pmuellr commented Apr 23, 2021

@elasticmachine merge upstream

@pmuellr
Copy link
Member Author

pmuellr commented Apr 23, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
triggersActionsUi 1.5MB 1.5MB +400.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
triggersActionsUi 106.1KB 106.1KB +60.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@pmuellr pmuellr merged commit 2ebb308 into elastic:master Apr 23, 2021
pmuellr added a commit to pmuellr/kibana that referenced this pull request Apr 23, 2021
…ing UI (elastic#97854)

resolves: elastic#97852

Adds `encodeURIComponent()` wrappers around references to rule, alert, and
connector ids.  Without this fix, if an alert id (which can contain
customer-generated data) contains a character that needs to be URL
encoded, the resulting API call from the web UI will fail.
pmuellr added a commit to pmuellr/kibana that referenced this pull request Apr 23, 2021
…ing UI (elastic#97854)

resolves: elastic#97852

Adds `encodeURIComponent()` wrappers around references to rule, alert, and
connector ids.  Without this fix, if an alert id (which can contain
customer-generated data) contains a character that needs to be URL
encoded, the resulting API call from the web UI will fail.
@pmuellr
Copy link
Member Author

pmuellr commented Apr 23, 2021

Starting a backport for 7.13, after feature freeze this Tuesday, since this is a pretty nasty bug that was easy to fix. Going to attempt a 7.12 backport as well, but not hopeful due to the API naming changes we made in 7.13

pmuellr added a commit to pmuellr/kibana that referenced this pull request Apr 23, 2021
…ing UI (elastic#97854)

resolves: elastic#97852

Adds `encodeURIComponent()` wrappers around references to rule, alert, and
connector ids.  Without this fix, if an alert id (which can contain
customer-generated data) contains a character that needs to be URL
encoded, the resulting API call from the web UI will fail.
# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/jira/api.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/jira/api.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/resilient/api.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/resilient/api.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/servicenow/api.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/servicenow/api.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/delete.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/delete.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/execute.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/execute.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/update.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/update.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/alert_summary.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/alert_summary.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/delete.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/delete.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/disable.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/disable.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/enable.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/enable.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/get_rule.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/get_rule.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/mute.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/mute.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/mute_alert.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/mute_alert.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/unmute.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/unmute.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/unmute_alert.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/unmute_alert.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/update.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/update.ts
#	x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/details.ts
@pmuellr
Copy link
Member Author

pmuellr commented Apr 23, 2021

Ya, the 7.12.2 backport will end up being a re-write of this PR from scratch, due to the API name changes and file name changes. Not clear it's worth it at this point. 7.12.2 is scheduled for May 18, 7.13.0 is scheduled for May 25.

pmuellr added a commit that referenced this pull request Apr 23, 2021
…ing UI (#97854) (#98208)

resolves: #97852

Adds `encodeURIComponent()` wrappers around references to rule, alert, and
connector ids.  Without this fix, if an alert id (which can contain
customer-generated data) contains a character that needs to be URL
encoded, the resulting API call from the web UI will fail.
pmuellr added a commit that referenced this pull request Apr 23, 2021
…ing UI (#97854) (#98209)

resolves: #97852

Adds `encodeURIComponent()` wrappers around references to rule, alert, and
connector ids.  Without this fix, if an alert id (which can contain
customer-generated data) contains a character that needs to be URL
encoded, the resulting API call from the web UI will fail.
pmuellr added a commit that referenced this pull request Apr 26, 2021
…m alerting UI (#97854) (#98211)

* [alerting] encode rule/connector ids in http requests made from alerting UI (#97854)

resolves: #97852

Adds `encodeURIComponent()` wrappers around references to rule, alert, and
connector ids.  Without this fix, if an alert id (which can contain
customer-generated data) contains a character that needs to be URL
encoded, the resulting API call from the web UI will fail.
# Conflicts:
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/jira/api.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/jira/api.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/resilient/api.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/resilient/api.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/servicenow/api.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/components/builtin_action_types/servicenow/api.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/delete.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/delete.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/execute.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/execute.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/update.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/action_connector_api/update.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/alert_summary.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/alert_summary.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/delete.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/delete.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/disable.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/disable.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/enable.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/enable.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/get_rule.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/get_rule.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/mute.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/mute.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/mute_alert.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/mute_alert.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/unmute.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/unmute.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/unmute_alert.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/unmute_alert.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/update.test.ts
#	x-pack/plugins/triggers_actions_ui/public/application/lib/alert_api/update.ts
#	x-pack/test/functional_with_es_ssl/apps/triggers_actions_ui/details.ts

* fix merge conflicts

In 7.13.0, the structure of the connector and rules API libraries in
triggers_actions_ui changed, where in 7.12 they were all in a single
file - one for connectors, one for rules - but in 7.13 they are split
out into separate files in a directory for connectors and one for rules.

To cut down on the noise, I decided to not use the `encodeURIComponent()`
wrappers on rule ids, just connector ids and alert ids, since it's not
possible in 7.12 to have rule ids which are not UUIDs, and so don't need
the encoding.

* fix prettier errors

Co-authored-by: Kibana Machine <42973632+kibanamachine@users.noreply.github.com>
@pmuellr
Copy link
Member Author

pmuellr commented Apr 26, 2021

Went ahead and fixed the 7.12 backport, with some caveats: #98211 (comment) - this should ship in 7.12.2, so added that label.

madirey pushed a commit to madirey/kibana that referenced this pull request May 11, 2021
…ing UI (elastic#97854)

resolves: elastic#97852

Adds `encodeURIComponent()` wrappers around references to rule, alert, and
connector ids.  Without this fix, if an alert id (which can contain
customer-generated data) contains a character that needs to be URL
encoded, the resulting API call from the web UI will fail.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported Feature:Actions Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.12.2 v7.13.0 v7.14.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[alerting] need to use encodeURIComponent() when making http calls with user-data in the path
6 participants