Skip to content

Security review fixes#154

Merged
abbas-dependable-naqvi merged 5 commits intomasterfrom
security_review_fixes
Jun 3, 2025
Merged

Security review fixes#154
abbas-dependable-naqvi merged 5 commits intomasterfrom
security_review_fixes

Conversation

@Kshitij-Katiyar
Copy link
Copy Markdown
Contributor

@Kshitij-Katiyar Kshitij-Katiyar commented May 12, 2025

@Kshitij-Katiyar Kshitij-Katiyar requested a review from Copilot May 12, 2025 08:40
@Kshitij-Katiyar Kshitij-Katiyar self-assigned this May 12, 2025
@Kshitij-Katiyar Kshitij-Katiyar added 2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester labels May 12, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR introduces several security fixes for handling panic situations, improving channel access validations for subscription-related endpoints, and making minor updates to error handling and logging.

  • Added a helper method for checking channel access.
  • Updated endpoints (save, get, edit, and delete subscription) to validate user access.
  • Updated error handling for JSON decoding in Confluence Cloud event processing.

Reviewed Changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.

Show a summary per file
File Description
server/user.go Added a helper for channel access validation.
server/service/delete_subscription.go Minor changes to delete subscription handling.
server/serializer/confluence_cloud.go Updated JSON decoding to return an error and fixed error logging message.
server/save_subscription.go Added channel access validation and connection checks.
server/get_subscriptions.go Added channel ID validation via API call.
server/get_subscription.go Added user access validation and connection check.
server/edit_subscription.go Added user access validation and connection check.
server/confluence_server.go Minor code formatting improvements.
server/confluence_cloud.go Updated error handling/logging in webhook payload processing.
server/command.go Updated command handlers to enforce channel access checks.
Comments suppressed due to low confidence (1)

server/serializer/confluence_cloud.go:66

  • The error log message incorrectly refers to 'ConfluenceServerEvent' instead of 'ConfluenceCloudEvent'. Please update the message to reflect the correct event type.
config.Mattermost.LogError("Unable to decode JSON for ConfluenceServerEvent.", "Error", err.Error())

@wiggin77
Copy link
Copy Markdown
Member

@Kshitij-Katiyar please update the Jira ticket statuses for each of these. They should be Submitted.

@Kshitij-Katiyar
Copy link
Copy Markdown
Contributor Author

@Kshitij-Katiyar please update the Jira ticket statuses for each of these. They should be Submitted.

@wiggin77 Sure

@enzowritescode
Copy link
Copy Markdown
Contributor

I updated the list of Jira issues in the description. The original was extremely confusing.
In some cases it had both the security ticket and the bug ticket, and in some cases it was one or the other. Now it just has the bug tickets.

Copy link
Copy Markdown
Contributor

@enzowritescode enzowritescode left a comment

Choose a reason for hiding this comment

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

The PR description says MM-64171 was fixed, but I don't see that in this PR.
Feel free to just fix that in another PR and update the description.

In the future please do several things:

  1. Less tickets per PR (ideally only 1 - or closely related issues)
  2. Make sure what you are saying is fixed is actually fixed
  3. Only include the bug ticket in the description (not some bug tickets, some security tickets, and some with both the security ticket and the associated bug ticket).

@Kshitij-Katiyar
Copy link
Copy Markdown
Contributor Author

I updated the list of Jira issues in the description. The original was extremely confusing. In some cases it had both the security ticket and the bug ticket, and in some cases it was one or the other. Now it just has the bug tickets.

thanks @enzowritescode

@Kshitij-Katiyar
Copy link
Copy Markdown
Contributor Author

The PR description says MM-64171 was fixed, but I don't see that in this PR. Feel free to just fix that in another PR and update the description.

In the future please do several things:

  1. Less tickets per PR (ideally only 1 - or closely related issues)
  2. Make sure what you are saying is fixed is actually fixed
  3. Only include the bug ticket in the description (not some bug tickets, some security tickets, and some with both the security ticket and the associated bug ticket).

Sure, will keep that in mind.
You are right, MM-64171 was fixed in #151

@enzowritescode
Copy link
Copy Markdown
Contributor

@Kshitij-Katiyar I updated the description for you by removing MM-64171. Next time please do that yourself.

@wiggin77
Copy link
Copy Markdown
Member

@Kshitij-Katiyar are you ready to merge this?

@Kshitij-Katiyar
Copy link
Copy Markdown
Contributor Author

Kshitij-Katiyar commented May 30, 2025

@wiggin77 We are planning ot get this QA tested once testing of MsCalendar RC is done.
We can merge all three PRs for security fixes into an epic and then have it QA-tested all at once.

@abbas-dependable-naqvi abbas-dependable-naqvi merged commit 39f67d3 into master Jun 3, 2025
6 checks passed
@abbas-dependable-naqvi abbas-dependable-naqvi deleted the security_review_fixes branch June 3, 2025 07:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2: Dev Review Requires review by a core committer 3: QA Review Requires review by a QA tester

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants