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.

Sign up for GitHub

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

Jump to bottom

[Actions] Removed double parsing when passing action url for validation #87928

Merged
merged 6 commits into from
Jan 19, 2021
Merged

[Actions] Removed double parsing when passing action url for validation #87928

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6f30d57
Removed double parsing when passing action url for validation
ymao1 Jan 11, 2021
8480fb9
Fixing functional test
ymao1 Jan 12, 2021
144633f
Merge branch 'master' into actions/allowed-hosts
kibanamachine Jan 13, 2021
670b033
Merge branch 'master' of https://github.com/elastic/kibana into actio…
ymao1 Jan 19, 2021
28e4e23
PR fixes
ymao1 Jan 19, 2021
c48f216
PR fixes
ymao1 Jan 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions x-pack/plugins/actions/server/actions_config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@

import { i18n } from '@kbn/i18n';
import { tryCatch, map, mapNullable, getOrElse } from 'fp-ts/lib/Option';
import { URL } from 'url';
import url from 'url';
import { curry } from 'lodash';
import { pipe } from 'fp-ts/lib/pipeable';

Expand All @@ -22,7 +22,7 @@ export enum EnabledActionTypes {
}

enum AllowListingField {
url = 'url',
URL = 'url',
hostname = 'hostname',
}

Expand Down Expand Up @@ -56,17 +56,17 @@ function disabledActionTypeErrorMessage(actionType: string) {
});
}

function isAllowed({ allowedHosts }: ActionsConfigType, hostname: string): boolean {
function isAllowed({ allowedHosts }: ActionsConfigType, hostname: string | null): boolean {
const allowed = new Set(allowedHosts);
if (allowed.has(AllowedHosts.Any)) return true;
if (allowed.has(hostname)) return true;
if (hostname && allowed.has(hostname)) return true;
ymao1 marked this conversation as resolved.
Show resolved Hide resolved
return false;
}

function isHostnameAllowedInUri(config: ActionsConfigType, uri: string): boolean {
return pipe(
tryCatch(() => new URL(uri)),
map((url) => url.hostname),
tryCatch(() => url.parse(uri)),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to match how axios handles urls https://github.com/axios/axios/blob/59ab559386273a185be18857a12ab0305b753e50/lib/adapters/http.js#L91.

map((parsedUrl) => parsedUrl.hostname),
mapNullable((hostname) => isAllowed(config, hostname)),
getOrElse<boolean>(() => false)
);
Expand Down Expand Up @@ -94,7 +94,7 @@ export function getActionsConfigurationUtilities(
isActionTypeEnabled,
ensureUriAllowed(uri: string) {
if (!isUriAllowed(uri)) {
throw new Error(allowListErrorMessage(AllowListingField.url, uri));
throw new Error(allowListErrorMessage(AllowListingField.URL, uri));
}
},
ensureHostnameAllowed(hostname: string) {
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/actions/server/builtin_action_types/slack.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ describe('validateActionTypeSecrets()', () => {
logger: mockedLogger,
configurationUtilities: {
...actionsConfigMock.create(),
ensureHostnameAllowed: () => {
ensureUriAllowed: () => {
throw new Error(`target hostname is not added to allowedHosts`);
},
},
Expand Down
10 changes: 5 additions & 5 deletions x-pack/plugins/actions/server/builtin_action_types/slack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export function getActionType({
}),
validate: {
secrets: schema.object(secretsSchemaProps, {
validate: curry(valdiateActionTypeConfig)(configurationUtilities),
validate: curry(validateActionTypeConfig)(configurationUtilities),
}),
params: ParamsSchema,
},
Expand All @@ -88,21 +88,21 @@ function renderParameterTemplates(
};
}

function valdiateActionTypeConfig(
function validateActionTypeConfig(
configurationUtilities: ActionsConfigurationUtilities,
secretsObject: ActionTypeSecretsType
) {
let url: URL;
const configuredUrl = secretsObject.webhookUrl;
try {
url = new URL(secretsObject.webhookUrl);
new URL(configuredUrl);
} catch (err) {
return i18n.translate('xpack.actions.builtin.slack.slackConfigurationErrorNoHostname', {
defaultMessage: 'error configuring slack action: unable to parse host name from webhookUrl',
});
}

try {
configurationUtilities.ensureHostnameAllowed(url.hostname);
configurationUtilities.ensureUriAllowed(configuredUrl);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am calling ensureUriAllowed both here and in the teams connector to be consistent with how I updated the webhook action. Pagerduty, Jira, Resilient and Servicenow were already all using ensureUriAllowed

} catch (allowListError) {
return i18n.translate('xpack.actions.builtin.slack.slackConfigurationError', {
defaultMessage: 'error configuring slack action: {message}',
Expand Down
2 changes: 1 addition & 1 deletion x-pack/plugins/actions/server/builtin_action_types/teams.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ describe('validateActionTypeSecrets()', () => {
logger: mockedLogger,
configurationUtilities: {
...actionsConfigMock.create(),
ensureHostnameAllowed: () => {
ensureUriAllowed: () => {
throw new Error(`target hostname is not added to allowedHosts`);
},
},
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/actions/server/builtin_action_types/teams.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,17 +71,17 @@ function validateActionTypeConfig(
configurationUtilities: ActionsConfigurationUtilities,
secretsObject: ActionTypeSecretsType
) {
let url: URL;
const configuredUrl = secretsObject.webhookUrl;
try {
url = new URL(secretsObject.webhookUrl);
new URL(configuredUrl);
} catch (err) {
return i18n.translate('xpack.actions.builtin.teams.teamsConfigurationErrorNoHostname', {
defaultMessage: 'error configuring teams action: unable to parse host name from webhookUrl',
});
}

try {
configurationUtilities.ensureHostnameAllowed(url.hostname);
configurationUtilities.ensureUriAllowed(configuredUrl);
} catch (allowListError) {
return i18n.translate('xpack.actions.builtin.teams.teamsConfigurationError', {
defaultMessage: 'error configuring teams action: {message}',
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugins/actions/server/builtin_action_types/webhook.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,9 +112,9 @@ function validateActionTypeConfig(
configurationUtilities: ActionsConfigurationUtilities,
configObject: ActionTypeConfigType
) {
let url: URL;
const configuredUrl = configObject.url;
try {
url = new URL(configObject.url);
new URL(configuredUrl);
} catch (err) {
return i18n.translate('xpack.actions.builtin.webhook.webhookConfigurationErrorNoHostname', {
defaultMessage: 'error configuring webhook action: unable to parse url: {err}',
Expand All @@ -125,7 +125,7 @@ function validateActionTypeConfig(
}

try {
configurationUtilities.ensureUriAllowed(url.toString());
configurationUtilities.ensureUriAllowed(configuredUrl);
} catch (allowListError) {
return i18n.translate('xpack.actions.builtin.webhook.webhookConfigurationError', {
defaultMessage: 'error configuring webhook action: {message}',
Expand Down
3 changes: 1 addition & 2 deletions .../alerting_api_integration/security_and_spaces/tests/actions/builtin_action_types/slack.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,8 +114,7 @@ export default function slackTest({ getService }: FtrProviderContext) {
expect(resp.body).to.eql({
statusCode: 400,
error: 'Bad Request',
message:
'error validating action type secrets: error configuring slack action: target hostname "slack.mynonexistent.com" is not added to the Kibana config xpack.actions.allowedHosts',
message: `error validating action type secrets: error configuring slack action: target url \"http://slack.mynonexistent.com/other/stuff/in/the/path\" is not added to the Kibana config xpack.actions.allowedHosts`,
});
});
});
Expand Down