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

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

Conversation

YulNaumenko
Copy link
Contributor

@YulNaumenko YulNaumenko commented Jun 26, 2020

Resolve #68662

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

img2

@YulNaumenko YulNaumenko added Feature:Alerting v8.0.0 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.9.0 labels Jun 26, 2020
@YulNaumenko YulNaumenko requested a review from a team as a code owner June 26, 2020 00:15
@YulNaumenko YulNaumenko self-assigned this Jun 26, 2020
@gmmorris
Copy link
Contributor

gmmorris commented Jun 29, 2020

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:

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.

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

This will achieve two things:

  1. 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)
  2. 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.

@gmmorris gmmorris self-requested a review June 29, 2020 08:23
Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

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

…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 YulNaumenko requested a review from pmuellr June 30, 2020 16:37
@YulNaumenko
Copy link
Contributor Author

isWhitelistedHostnameInUri

Added server API validation for URL.

Copy link
Member

@pmuellr pmuellr left a comment

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@gmmorris gmmorris left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for making the changes 👍

@YulNaumenko YulNaumenko requested a review from a team July 6, 2020 21:33
@YulNaumenko YulNaumenko requested review from a team as code owners July 6, 2020 21:33
@botelastic 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
@elasticmachine
Copy link
Contributor

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

@YulNaumenko YulNaumenko force-pushed the alerting-provide-webhook-url-validation branch from 7a4906f to c54ce78 Compare July 6, 2020 21:46
@YulNaumenko YulNaumenko removed request for a team July 6, 2020 21:47
@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

@kbn/optimizer bundle module count

id value diff baseline
triggers_actions_ui 147 +1 146

History

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

@YulNaumenko YulNaumenko merged commit 438e905 into elastic:master Jul 7, 2020
YulNaumenko added a commit to YulNaumenko/kibana that referenced this pull request Jul 7, 2020
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
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 7, 2020
* 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)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 7, 2020
* 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)
  ...
gmmorris added a commit to gmmorris/kibana that referenced this pull request Jul 7, 2020
* 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)
  ...
YulNaumenko added a commit that referenced this pull request Jul 7, 2020

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Alerting Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v7.9.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Misleading error message when creating a webhook connector with invalid URL
5 participants