Skip to content

Fix slack webhook overriding#856

Merged
gaspergrom merged 5 commits intomainfrom
bugfix/slack-webhook-override
May 12, 2023
Merged

Fix slack webhook overriding#856
gaspergrom merged 5 commits intomainfrom
bugfix/slack-webhook-override

Conversation

@gaspergrom
Copy link
Copy Markdown
Contributor

@gaspergrom gaspergrom commented May 12, 2023

Changes proposed ✍️

What

🤖 Generated by Copilot at a06842e

This pull request adds validation and protection for the slackWebHook field in the settings object. It prevents setting or saving invalid or malicious URLs that could compromise the Slack integration feature.

🤖 Generated by Copilot at a06842e

To send notifications to Slack
We need a valid webhook in the stack
But some URLs are bad
Or could make us look sad
So we check the slackWebHook and roll back

Why

How

🤖 Generated by Copilot at a06842e

  • Validate and sanitize the slackWebHook field in the settings data to prevent errors or security issues when sending notifications to Slack (link, link, link, link)
  • In settingsRepository.ts, check if the slackWebHook field starts with 'https://' and set it to undefined otherwise before saving it to the database (link)
  • In settingsService.ts, initialize the slackWebHook field to undefined when creating a new settings object (link)
  • In settingsService.ts, set the slackWebHook field to undefined when updating the settings object with the data from the request body or the environment variables (link, link)

Checklist ✅

  • Label appropriately with Feature, Improvement, or Bug.
  • Add screehshots to the PR description for relevant FE changes
  • New backend functionality has been unit-tested.
  • API documentation has been updated (if necessary) (see docs on API documentation).
  • Quality standards are met.

@gaspergrom gaspergrom added the Bug Created by Linear-GitHub Sync label May 12, 2023
@gaspergrom gaspergrom requested a review from epipav May 12, 2023 09:52
@gaspergrom gaspergrom self-assigned this May 12, 2023
Copy link
Copy Markdown
Collaborator

@epipav epipav left a comment

Choose a reason for hiding this comment

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

lgtm 👍 Included one comment


data.backgroundImageUrl = _get(data, 'backgroundImages[0].downloadUrl', null)
data.logoUrl = _get(data, 'logos[0].downloadUrl', null)
if (data.slackWebHook && !data.slackWebHook?.startsWith('https://')) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's check this with
if ((typeof data.slackWebhook !== 'string') || (typeof data.slackWebhook === 'string' && !data.slackWebHook?.startsWith('https://'))
so that startsWith() doesn't throw an error when data.slackWebhook === true

@gaspergrom gaspergrom requested a review from epipav May 12, 2023 10:07
@gaspergrom gaspergrom merged commit df4e4b6 into main May 12, 2023
@gaspergrom gaspergrom deleted the bugfix/slack-webhook-override branch May 12, 2023 10:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Bug Created by Linear-GitHub Sync

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants