Added UI validation when creating a Webhook connector with invalid URL by YulNaumenko · Pull Request #70025 · elastic/kibana

YulNaumenko · 2020-06-26T00:15:22Z

Resolved by adding placeholder info for Webhook URL and proper URL client side validation on input:

gmmorris · 2020-06-29T08:23:25Z

This is a good approach to validating the Front End input, but I don't think this fully addresses the issue.

The main problem, as I can see, is that the input is throwing an error as if it isn't whitelisted - which isn't accurate, because the whitelist is * by default (allow all).
Looking at the code in isWhitelistedHostnameInUri I noticed that the reason we think it isn't whitelisted is that parsing example.com/do-something as a URI fails on line 68:

kibana/x-pack/plugins/actions/server/actions_config.ts

Lines 66 to 73 in 40ff82d

    
           function isWhitelistedHostnameInUri(config: ActionsConfigType, uri: string): boolean { 
        
             return pipe( 
        
               tryCatch(() => new URL(uri)), 
        
               map((url) => url.hostname), 
        
               mapNullable((hostname) => isWhitelisted(config, hostname)), 
        
               getOrElse<boolean>(() => false) 
        
             ); 
        
           }

What I think this PR should also do (in addition to what's already done) is validate the URL in the schema on the server side before checking the whitelist.

kibana/x-pack/plugins/actions/server/builtin_action_types/webhook.ts

Lines 73 to 79 in 40ff82d

    
           validate: { 
        
             config: schema.object(configSchemaProps, { 
        
               validate: curry(validateActionTypeConfig)(configurationUtilities), 
        
             }), 
        
             secrets: SecretsSchema, 
        
             params: ParamsSchema, 
        
           },

This will achieve two things:

Creating a webhook via the http API will result in the same errors that the UI shows (the current PR will not catch this error on the server)
It will correct the whitelist validation so that it will pass for example.com/do-something as it should have done if the format were correct.

pmuellr

Noted that I think we should use the built-in URL parser to check for validity, rather than a regexp.

x-pack/plugins/triggers_actions_ui/public/application/lib/value_validators.ts

…webhook-url-validation # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.

YulNaumenko · 2020-06-30T16:39:27Z

isWhitelistedHostnameInUri

Added server API validation for URL.

pmuellr

LGTM, but made note about the server-side error message being generated.

x-pack/plugins/actions/server/builtin_action_types/webhook.ts

gmmorris

LGTM, thanks for making the changes 👍

elasticmachine · 2020-07-06T21:33:17Z

Pinging @elastic/ingest-management (Team:Ingest Management)

kibanamachine · 2020-07-07T00:35:11Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request
Commit: 5143df6

Build metrics

@kbn/optimizer bundle module count

id	value	diff	baseline
triggers_actions_ui	147	+1	146

History

💔 Build #59182 failed f412fe8
💔 Build #59170 failed d99cf6b
💔 Build #57945 failed c54ce78
💔 Build #57666 failed f3c998a
💔 Build #57590 failed e725ed9

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

elastic#70025) * Added UI validation when creating a Webhook connector with invalid URL * fixed tests * Fixed due to comments * fixed type check and extended error message for invalid URL * Fixed whitelisting of URL * fixed failing tests * fixed str

* master: (53 commits) [Composable template] Details panel + delete functionality (elastic#70814) [Uptime] Ping list body scroll (elastic#70781) moving indexPattern.delete() to indexPatterns.delete(indexPattern) (elastic#70430) Adapt expected response of advanced settings feature control for cloud tests (elastic#70793) skip flaky suite (elastic#70885) skip flaky suite (elastic#67814) skip flaky suite (elastic#70906) Revert "reenable regression and classification functional tests (elastic#70661)" (elastic#70908) Added UI validation when creating a Webhook connector with invalid URL (elastic#70025) [Security Solution] Change default index pattern (elastic#70797) ServiceNow push to Incident generic implementation (supporting both Case specific and generic Alerts) (elastic#68464) add button link to ingest (elastic#70142) reenable regression and classification functional tests (elastic#70661) [Component templates] Form wizard (elastic#69732) [Ingest Manager] Copy changes (elastic#70828) Adding test user to maps functional tests - PR 1 (elastic#70649) [Ingest Manager] Support limiting integrations on an agent config (elastic#70542) skip flaky suite (elastic#70880) [Metrics UI] Fix a bug in Metric Threshold query filter construction (elastic#70672) upgrade caniuse-lite database (elastic#70833) ...

* master: (46 commits) [Composable template] Details panel + delete functionality (elastic#70814) [Uptime] Ping list body scroll (elastic#70781) moving indexPattern.delete() to indexPatterns.delete(indexPattern) (elastic#70430) Adapt expected response of advanced settings feature control for cloud tests (elastic#70793) skip flaky suite (elastic#70885) skip flaky suite (elastic#67814) skip flaky suite (elastic#70906) Revert "reenable regression and classification functional tests (elastic#70661)" (elastic#70908) Added UI validation when creating a Webhook connector with invalid URL (elastic#70025) [Security Solution] Change default index pattern (elastic#70797) ServiceNow push to Incident generic implementation (supporting both Case specific and generic Alerts) (elastic#68464) add button link to ingest (elastic#70142) reenable regression and classification functional tests (elastic#70661) [Component templates] Form wizard (elastic#69732) [Ingest Manager] Copy changes (elastic#70828) Adding test user to maps functional tests - PR 1 (elastic#70649) [Ingest Manager] Support limiting integrations on an agent config (elastic#70542) skip flaky suite (elastic#70880) [Metrics UI] Fix a bug in Metric Threshold query filter construction (elastic#70672) upgrade caniuse-lite database (elastic#70833) ...

* actions/feature: (46 commits) [Composable template] Details panel + delete functionality (elastic#70814) [Uptime] Ping list body scroll (elastic#70781) moving indexPattern.delete() to indexPatterns.delete(indexPattern) (elastic#70430) Adapt expected response of advanced settings feature control for cloud tests (elastic#70793) skip flaky suite (elastic#70885) skip flaky suite (elastic#67814) skip flaky suite (elastic#70906) Revert "reenable regression and classification functional tests (elastic#70661)" (elastic#70908) Added UI validation when creating a Webhook connector with invalid URL (elastic#70025) [Security Solution] Change default index pattern (elastic#70797) ServiceNow push to Incident generic implementation (supporting both Case specific and generic Alerts) (elastic#68464) add button link to ingest (elastic#70142) reenable regression and classification functional tests (elastic#70661) [Component templates] Form wizard (elastic#69732) [Ingest Manager] Copy changes (elastic#70828) Adding test user to maps functional tests - PR 1 (elastic#70649) [Ingest Manager] Support limiting integrations on an agent config (elastic#70542) skip flaky suite (elastic#70880) [Metrics UI] Fix a bug in Metric Threshold query filter construction (elastic#70672) upgrade caniuse-lite database (elastic#70833) ...

…lid URL (#70025) (#70905) * Added UI validation when creating a Webhook connector with invalid URL (#70025) * Added UI validation when creating a Webhook connector with invalid URL * fixed tests * Fixed due to comments * fixed type check and extended error message for invalid URL * Fixed whitelisting of URL * fixed failing tests * fixed str * fixed merge issue

Added UI validation when creating a Webhook connector with invalid URL

ac8e18f

YulNaumenko added Feature:Alerting v8.0.0 release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Platform ResponseOps team (formerly the Cases and Alerting teams) t// v7.9.0 labels Jun 26, 2020

YulNaumenko requested a review from a team as a code owner June 26, 2020 00:15

YulNaumenko self-assigned this Jun 26, 2020

fixed tests

a4f8fd6

gmmorris self-requested a review June 29, 2020 08:23

pmuellr requested changes Jun 29, 2020

View reviewed changes

x-pack/plugins/triggers_actions_ui/public/application/lib/value_validators.ts Outdated Show resolved Hide resolved

pmuellr mentioned this pull request Jun 29, 2020

ServiceNow push to Incident generic implementation (supporting both Case specific and generic Alerts) #68464

Merged

YulNaumenko added 2 commits June 30, 2020 08:42

Fixed due to comments

e725ed9

YulNaumenko requested a review from pmuellr June 30, 2020 16:37

pmuellr approved these changes Jun 30, 2020

View reviewed changes

x-pack/plugins/actions/server/builtin_action_types/webhook.ts Outdated Show resolved Hide resolved

YulNaumenko added 2 commits June 30, 2020 12:41

fixed type check and extended error message for invalid URL

f3c998a

Fixed whitelisting of URL

c54ce78

gmmorris approved these changes Jul 2, 2020

View reviewed changes

YulNaumenko requested a review from a team July 6, 2020 21:33

YulNaumenko requested review from a team as code owners July 6, 2020 21:33

botelastic bot added Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:Fleet Team label for Observability Data Collection Fleet team labels Jul 6, 2020

YulNaumenko force-pushed the alerting-provide-webhook-url-validation branch from 7a4906f to c54ce78 Compare July 6, 2020 21:46

YulNaumenko removed request for a team July 6, 2020 21:47

YulNaumenko added 3 commits July 6, 2020 14:49

fixed failing tests

d99cf6b

Merge master

f412fe8

fixed str

5143df6

YulNaumenko merged commit 438e905 into elastic:master Jul 7, 2020

YulNaumenko mentioned this pull request Jul 7, 2020

[7.x] Added UI validation when creating a Webhook connector with invalid URL (#70025) #70905

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added UI validation when creating a Webhook connector with invalid URL#70025

Added UI validation when creating a Webhook connector with invalid URL#70025
YulNaumenko merged 9 commits intoelastic:masterfrom
YulNaumenko:alerting-provide-webhook-url-validation

YulNaumenko commented Jun 26, 2020 •

edited

Loading

Uh oh!

gmmorris commented Jun 29, 2020 •

edited

Loading

Uh oh!

pmuellr left a comment

Uh oh!

Uh oh!

YulNaumenko commented Jun 30, 2020

Uh oh!

pmuellr left a comment

Uh oh!

Uh oh!

gmmorris left a comment

Uh oh!

elasticmachine commented Jul 6, 2020

Uh oh!

kibanamachine commented Jul 7, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

YulNaumenko commented Jun 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmmorris commented Jun 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

YulNaumenko commented Jun 30, 2020

Uh oh!

pmuellr left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

gmmorris left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 6, 2020

Uh oh!

kibanamachine commented Jul 7, 2020

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

History

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

YulNaumenko commented Jun 26, 2020 •

edited

Loading

gmmorris commented Jun 29, 2020 •

edited

Loading