Update dependencies (Go + webapp) - #28
Merged
Merged
Conversation
- Update golangci-lint to v2.9.0, gotestsum to v1.13.0 - Update Node.js to 24.13.1 (from 20.11) - Update Go to 1.25 (from 1.23.0), run go mod tidy - Adopt new linters from template: bodyclose, gocritic, gosec, modernize, nakedret, unqueryvet - Add gofmt and gofumpt formatters to golangci.yml - Update mock tool from golang/mock to go.uber.org/mock - Update semver Makefile targets with LATEST_RELEASE_TAG logic - Fix .editorconfig glob syntax (remove spaces in patterns) - Update build/manifest/main.go: use strings.FieldsSeq, url.JoinPath, fmt.Appendf - Fix all linter issues: modernize loops, gosec permissions, gocritic patterns
- Upgrade mattermost/server/public v0.1.10 → v0.4.2 and regenerate mocks (plugin.API gained CountPropertyFields; switch mock tool import to go.uber.org/mock/gomock in matrix_util_test and thread_mapping_test) - Upgrade testcontainers-go v0.37.0 → v0.42.0 - Upgrade Go toolchain 1.25 → 1.26.3 (via go get -u) - Upgrade various transitive deps (grpc, otel, x/crypto, x/net, x/sys, …) - Fix flaky rate-limit timing test: measure total elapsed from first call start instead of second call start, matching the rate limiter's own clock
- Babel 7.25.x → 7.29.7: fixes arbitrary file read (CVE) and ReDoS - @typescript-eslint/parser 7.0.2 → 7.18.0: fixes minimatch ReDoS, aligns with eslint-plugin version - webpack 5.95.0 → 5.107.2: fixes SSRF vulnerability - Remaining 23 vulns require jest/jest-junit major version bumps (skip)
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR upgrades the Go toolchain to 1.26.3, Node.js to 24.13.1, and numerous direct and indirect dependencies. It migrates the mock library from ChangesTooling Upgrade, Linter Compliance, and Go Modernization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
Makefile (1)
436-436: ⚡ Quick winUse
$(GO)for mockgen install to keep toolchain selection consistent.Line 436 bypasses the configured
GOvariable and can break environments that overrideGO.Suggested patch
- go install go.uber.org/mock/mockgen@v0.6.0 + $(GO) install go.uber.org/mock/mockgen@v0.6.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Makefile` at line 436, The mockgen installation command on line 436 uses the direct go command instead of the configured GO variable, which breaks environments that override the GO toolchain variable for consistency. Replace the direct go install command with $(GO) install to use the same Go toolchain variable that is configured elsewhere in the Makefile. This ensures that the mockgen@v0.6.0 installation respects the toolchain selection configuration.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.golangci.yml:
- Around line 42-45: The mock exclusion regex pattern in the paths section is
incorrectly specified as "mock.*", which only matches files at the root level
starting with "mock" and fails to exclude the actual mock files stored in
server/mocks/ and server/command/mocks/ directories. Replace the "mock.*"
pattern with a regex that correctly matches these nested mock directories, such
as "server/.*/mocks/" or ".*mocks/.*" to ensure the linter properly excludes all
generated mock files from the repository.
In `@go.mod`:
- Around line 7-12: Remove the unused legacy dependency github.com/golang/mock
v1.6.0 from the go.mod file. This dependency is no longer used in the codebase
and has been replaced by go.uber.org/mock v0.6.0. Delete the line containing
github.com/golang/mock v1.6.0 to eliminate unnecessary dependency bloat and
avoid migration confusion.
---
Nitpick comments:
In `@Makefile`:
- Line 436: The mockgen installation command on line 436 uses the direct go
command instead of the configured GO variable, which breaks environments that
override the GO toolchain variable for consistency. Replace the direct go
install command with $(GO) install to use the same Go toolchain variable that is
configured elsewhere in the Makefile. This ensures that the mockgen@v0.6.0
installation respects the toolchain selection configuration.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ab424e70-4afc-46ed-b97e-d3642400d88d
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumwebapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (45)
.editorconfig.golangci.yml.nvmrcMakefilebuild/manifest/main.gobuild/pluginctl/main.gogo.modserver/bridge_utils.goserver/bridge_utils_test.goserver/command/command.goserver/command/command_test.goserver/command/mocks/mock_commands.goserver/configuration.goserver/dm_room_creation_test.goserver/logr.goserver/matrix/client_ratelimit_test.goserver/matrix/ratelimit.goserver/matrix/ratelimit_load_test.goserver/matrix/ratelimit_test.goserver/matrix/test/client_test.goserver/matrix/test/ratelimit_test.goserver/matrix_mentions_integration_test.goserver/matrix_util.goserver/matrix_util_test.goserver/matrix_webhook.goserver/matrix_webhook_test.goserver/migrations.goserver/migrations_test.goserver/mocks/mock_kvstore.goserver/mocks/mock_plugin_api.goserver/plugin.goserver/plugin_integration_test.goserver/post_tracker_test.goserver/sync_to_matrix.goserver/sync_to_matrix_integration_test.goserver/sync_to_matrix_test.goserver/sync_to_mattermost.goserver/sync_to_mattermost_test.goserver/testhelpers_test.goserver/thread_mapping_test.goserver/user_remote_detection_test.gotestcontainers/matrix/container.gotestcontainers/matrix/utils.gotools/emoji_generator.gowebapp/package.json
- Remove github.com/golang/mock from go.mod (migrated to go.uber.org/mock) - Fix goroutine leak in CreateTransactionLogger: call logger.Shutdown() on all error paths after logr.New() to avoid orphaned background goroutines - Add unit tests for CreateTransactionLogger covering no-op (empty env), absolute path, relative path, bare filename, and relative ".." traversal rejection via os.Root.MkdirAll
- Fix golangci.yml mock exclusion regex: mock.* only matched root-level paths; replace with .*/mocks/.* to cover server/mocks/ and server/command/mocks/ directories - Use $(GO) instead of go in mockgen install target for consistent toolchain selection in environments that override the GO variable
Strip characters invalid in Matrix alias local parts (only [a-z0-9-] are allowed) so room names with quotes or other special characters don't produce malformed aliases like #_mattermost_"name":server. Also strip surrounding quotes from the /matrix create roomName argument since users may wrap room names in quotes when the name contains spaces.
The condition was backwards: it returned an error when shared channels was enabled rather than when it was disabled, so the plugin refused to load in any correctly configured environment.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docker-compose.yml (2)
3-3: Pin container images to specific versions instead oflatest.Using
latestforalpineand Synapse makes local environments non-reproducible and increases supply-chain drift risk. Pin explicit versions so updates remain intentional.Suggested change
- image: alpine:latest + image: alpine:3.23 ... - image: matrixdotorg/synapse:latest + image: matrixdotorg/synapse:v1.154.0🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` at line 3, In the docker-compose.yml file, replace the `alpine:latest` image tag with a specific version number (for example, `alpine:3.18` or `alpine:3.19`) to ensure reproducible builds and reduce supply chain drift risk. Using pinned versions makes the local environment predictable and keeps image updates intentional rather than automatic.
27-29: Gate Synapse startup on Postgres health, not just container start.
depends_onwithout health conditions can still race DB readiness and cause transient startup failures. Add a Postgres healthcheck and depend onservice_healthy. Theservice_completed_successfullyandservice_healthyconditions are officially supported in current Docker Compose versions and align with best practices for startup orchestration.Suggested change
synapse: @@ - depends_on: - - synapse-init - - postgres + depends_on: + synapse-init: + condition: service_completed_successfully + postgres: + condition: service_healthy @@ postgres: image: postgres:14 @@ environment: @@ - POSTGRES_INITDB_ARGS=--encoding=UTF-8 --lc-collate=C --lc-ctype=C + healthcheck: + test: ["CMD-SHELL", "pg_isready -U synapse -d synapse"] + interval: 10s + timeout: 5s + retries: 10🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose.yml` around lines 27 - 29, The depends_on section in the docker-compose.yml file currently lacks health conditions, which can cause Synapse to start before Postgres is actually ready. Convert the depends_on list format to an object format with explicit conditions: change synapse-init to use service_completed_successfully condition and postgres to use service_healthy condition. Additionally, ensure the postgres service in the docker-compose.yml includes a healthcheck definition that Docker Compose can monitor (such as checking if the database port is responding) so that the service_healthy condition can be properly evaluated before Synapse attempts to connect.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose.yml`:
- Around line 4-8: The chmod -R 755 command applies overly permissive
world-readable and executable permissions to all files and directories under
/data, which exposes sensitive Synapse signing keys and configuration files.
Replace the recursive chmod 755 with more restrictive permissions that align
with Synapse's security design (such as 0750 for directories and 0640 for
sensitive files, or remove the chmod entirely if Synapse properly sets
permissions during initialization). Ensure the chown command to 991:991 remains
in place to set the correct ownership, but tighten the permission model to
prevent unauthorized access to sensitive configuration and signing key files.
In `@server/configuration.go`:
- Line 88: The condition at line 88 dereferences the pointer
ConnectedWorkspacesSettings without first checking if it is nil, which will
cause a panic. Add a nil check for ConnectedWorkspacesSettings before attempting
to access its EnableSharedChannels field. Modify the condition to first verify
that p.GetPluginAPI().GetConfig().ConnectedWorkspacesSettings is not nil, then
check if EnableSharedChannels is nil or false within that guard.
In `@server/matrix/client.go`:
- Around line 841-843: The fallback alias assignment in the block where alias is
empty (if alias == "") uses a static value "room" which causes collisions when
multiple sanitized aliases result in empty strings. Replace this static fallback
with a deterministic unique identifier by appending a sanitized fragment of the
mattermostChannelID to the fallback string, ensuring each room gets a unique
alias even when the original alias sanitizes to empty.
---
Nitpick comments:
In `@docker-compose.yml`:
- Line 3: In the docker-compose.yml file, replace the `alpine:latest` image tag
with a specific version number (for example, `alpine:3.18` or `alpine:3.19`) to
ensure reproducible builds and reduce supply chain drift risk. Using pinned
versions makes the local environment predictable and keeps image updates
intentional rather than automatic.
- Around line 27-29: The depends_on section in the docker-compose.yml file
currently lacks health conditions, which can cause Synapse to start before
Postgres is actually ready. Convert the depends_on list format to an object
format with explicit conditions: change synapse-init to use
service_completed_successfully condition and postgres to use service_healthy
condition. Additionally, ensure the postgres service in the docker-compose.yml
includes a healthcheck definition that Docker Compose can monitor (such as
checking if the database port is responding) so that the service_healthy
condition can be properly evaluated before Synapse attempts to connect.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 192eb91a-fb72-485e-8912-ec8722bea8be
📒 Files selected for processing (8)
.golangci.ymlMakefileREADME.mddocker-compose.ymlserver/command/command.goserver/command/command_test.goserver/configuration.goserver/matrix/client.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (3)
- .golangci.yml
- server/command/command.go
- Makefile
…to empty Use mattermostChannelID as a deterministic suffix instead of the static "room" fallback, preventing alias-in-use errors when multiple channels produce an empty sanitized alias.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Update Go and webapp (npm) dependencies to latest compatible versions, fixing security vulnerabilities and adopting build tooling improvements from the starter template.
Go dependencies (
go get -u ./...+go mod tidy):go.uber.org/mock v0.6.0(replaces deprecatedgithub.com/golang/mock)mattermost/server/publicaddedCountPropertyFieldstoplugin.APIWebapp dependencies (npm):
@babel/*7.25.x → 7.29.7: fixes arbitrary file read and ReDoS vulnerabilities@typescript-eslint/parser7.0.2 → 7.18.0: fixes minimatch ReDoS, aligns witheslint-pluginversionwebpack5.95.0 → 5.107.2: fixes SSRF vulnerabilityStarter template sync (separate commit):
bodyclose,gocritic,gosec,modernize,nakedret,unconvertvetos.MkdirAll+//nolint:gosecwithos.OpenRoot-based secure directory creation inlogr.goBug fixes found during local integration testing:
OnConfigurationChangethat caused the plugin to refuse loading in any correctly configured environment (Connected Workspaces shared channels check was backwards)[a-z0-9-]are allowed), producing malformed aliases like#_mattermost_"name":serverwhen room names contained quotes or other special characters/matrix createargument parsing:strings.Fieldsdoes not strip shell-style quotes, so room names passed as"my room"retained their surrounding quotes and ended up in the aliasTicket Link