Fixing Torq hostname identifying logic#194003
Fixing Torq hostname identifying logic#194003igorkheif wants to merge 13 commits intoelastic:mainfrom
Conversation
|
💚 CLA has been signed |
|
Pinging @elastic/security-detection-engine (Team:Detection Alerts) |
|
Is there an issue open for this? Not seeing one ... |
|
I've added |
| }; | ||
| if (!isUrl(value)) return error; | ||
| const hostname = new URL(value).hostname; | ||
| const isTorqHostname = /^hooks(\.[a-z0-9]+)*\.torq\.io$/.test(hostname); |
There was a problem hiding this comment.
Looks like this was the only functional change in this PR. It looks good and the tests look great, thank you for adding them. Will leave the final review and approval to the @elastic/response-ops team as I'm not familiar with the torq.io url naming patterns. Thanks for the improvement @igorkheif !
|
@michaelolo24 @igorkheif Before reviewing we need the security solution team to review, test, and approve the PR. After that, we will do a high-level review. Could you also please update the description so reviewers unfamiliar with Torq will better understand what this PR is about? Thanks! |
|
@igorkheif As Christos also mentioned above, could you please add more details to the description and also some steps/instructions for how to test the PR so reviewers unfamiliar with Torq will have a better understanding ? |
|
@igorkheif - Thanks for making these changes. We were able to test it, and while the validation on the UI is working as expected, you'll also need to modify the server side validation as well: and please update the tests as well |
michaelolo24
left a comment
There was a problem hiding this comment.
Please update the validation on the server as well:
@michaelolo24 Thanks, good catch! Done! |
|
@elasticmachine merge upstream |
Zacqary
left a comment
There was a problem hiding this comment.
LGTM from Response Ops side, thanks @igorkheif
|
Pinging @elastic/response-ops (Team:ResponseOps) |
|
@elasticmachine merge upstream |
|
This has already been implemented in #212563. Closing this one. |
Pull request was closed
Summary
This change allows the connector to validate all of the Torq subdomains such as
hooks.eu.torq.ioinstead of just the defaulthooks.torq.io.This need arises as Torq now has several environments in various locations, and they all use different subdomains.
Checklist
Delete any items that are not applicable to this PR.
Risk Matrix
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:
For maintainers