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

Verify Slack webhook tokens #776

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

tadzik
Copy link
Collaborator

@tadzik tadzik commented Mar 28, 2024

This will unfortunately require relinking all existing channels that use webhooks.

I pondered making this optional, but in the spirit of being secure by default, it's probably best if we mandate this.

Existing rooms will have to be unlinked and linked again (or have their database entries updated manually).

This also increases SlackHookHandler's test coverage by ~18% (from 9.39% to 27.51%) :)

Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com>
Signed-off-by: Tadeusz „tadzik” Sośnierz <tadeusz@sosnierz.com>
@@ -85,7 +85,7 @@ export class SlackHookHandler extends BaseSlackHandler {
}
}

private onRequest(req: IncomingMessage, res: ServerResponse) {
public _onRequest(req: IncomingMessage, res: ServerResponse) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems quite cheeky. If you're mocking stuff, why not hookHandler["onRequest"] to get around the public/private check.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Getting around the check seems dirtier to me, to be honest. I won't fight for it though, I'm fine with either.

@@ -67,7 +67,7 @@ export class SlackHookHandler extends BaseSlackHandler {
createServer = (cb) => httpsCreate(tlsOptions, cb);
}
return new Promise<void>((resolve, reject) => {
const srv = createServer(this.onRequest.bind(this));
const srv = createServer(this._onRequest.bind(this));
Copy link
Contributor

Choose a reason for hiding this comment

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

Not a huge fan of the underscore prefix, smells like snakes

Copy link
Collaborator Author

@tadzik tadzik Jun 5, 2024

Choose a reason for hiding this comment

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

let room = new BridgedRoom(harness.main as unknown as Main, {
matrix_room_id: '!foo:bar.baz',
inbound_id: randomstring.generate(32),
slack_webhook_token: randomstring.generate(24),
Copy link
Contributor

Choose a reason for hiding this comment

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

What about no token at all :). Or null, or true etc etc :)

Copy link
Collaborator Author

@tadzik tadzik Jun 5, 2024

Choose a reason for hiding this comment

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

Added a few more in tadzik@4d312a3?diff=unified&w=1 :)

@@ -0,0 +1 @@
Verify Slack webhook tokens.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I figured bugfix is the closest this comes to, since it was arguably a bug that we never did this before.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants