Conversation
- Address https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/18 https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/16 https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/15 https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/14 https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/6 - Add dependabot like in mattermost
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing touches🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Comment |
NARSimoes
force-pushed
the
nasimoes-check-deps
branch
from
0066db5 to
ee1415e
Compare
NARSimoes
force-pushed
the
nasimoes-check-deps
branch
from
ee1415e to
7a100fa
Compare
Consider preallocating boardsAdded (prealloc)
QF1003: could use tagged switch on userID (staticcheck) QF1002: could use tagged switch on teamID (staticcheck) QF1008: could remove embedded field "Category" from selector (staticcheck) QF1008: could remove embedded field "Server" from selector (staticcheck)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@server/.golangci.yml`:
- Around line 80-82: The goimports local-prefixes entry currently uses the
template value "github.com/mattermost/mattermost-starter-template" which will
mis-group local imports; update the local-prefixes list in .golangci.yml to use
this repository's actual Go module path (replace the template string under the
local-prefixes key) so goimports groups local imports correctly and avoids
formatter churn.
- Around line 63-65: Remove the now-unsupported "scopelint" entry from the
linters list in the golangci-lint config (the block that lists "linters:" with
"bodyclose" under the path matching `_test\.go`); leave "bodyclose" and other
entries intact and rely on the already-enabled "copyloopvar" for loop-variable
checks. Ensure you delete the `scopelint` token and its comment so the config
validates with golangci-lint v2.8.0.
- Line 1: The golangci-lint config has two problems: remove the deprecated rule
name "scopelint" from the exclusions list (it was removed in v2 and is
unnecessary since "copyloopvar" is present) and update the goimports
local-prefixes value from the template residue
"github.com/mattermost/mattermost-starter-template" to the repository-specific
"github.com/mattermost/mattermost-plugin-boards" so import grouping works
correctly; locate the entries named "scopelint" and "goimports.local-prefixes"
in .golangci.yml and delete the former and replace the latter with the correct
module path.
| local-prefixes: | ||
| - github.com/mattermost/mattermost-starter-template | ||
| exclusions: |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get the module path from go.mod
rg -n '^module ' go.modRepository: mattermost/mattermost-plugin-boards
Length of output: 134
🏁 Script executed:
#!/bin/bash
# Check if server/go.mod exists and what its module path is
if [ -f server/go.mod ]; then
echo "=== server/go.mod found ==="
rg -n '^module ' server/go.mod
else
echo "=== server/go.mod not found ==="
echo "Checking directory structure:"
ls -la server/ | head -20
fiRepository: mattermost/mattermost-plugin-boards
Length of output: 1260
🏁 Script executed:
#!/bin/bash
# Verify the current content of server/.golangci.yml around the goimports section
rg -B5 -A5 'goimports' server/.golangci.ymlRepository: mattermost/mattermost-plugin-boards
Length of output: 348
Update goimports local-prefixes to this repo's module path.
github.com/mattermost/mattermost-starter-template is a template default that will mis-group local imports, causing formatter churn.
♻️ Suggested adjustment
goimports:
local-prefixes:
- - github.com/mattermost/mattermost-starter-template
+ - github.com/mattermost/mattermost-plugin-boards🤖 Prompt for AI Agents
In `@server/.golangci.yml` around lines 80 - 82, The goimports local-prefixes
entry currently uses the template value
"github.com/mattermost/mattermost-starter-template" which will mis-group local
imports; update the local-prefixes list in .golangci.yml to use this
repository's actual Go module path (replace the template string under the
local-prefixes key) so goimports groups local imports correctly and avoids
formatter churn.
There was a problem hiding this comment.
@NARSimoes, understood! Since this will be addressed in a follow-up, would you like me to create a GitHub issue to track this configuration update? That way it won't get lost.
QF1001: could apply De Morgan's law (staticcheck) QF1003: could use tagged switch on boardType (staticcheck) QF1007: could merge conditional assignment into variable declaration (staticcheck) QF1008: could remove embedded field "Category" from selector (staticcheck)
The format must be done in a follow up because the current update already includes many things.
NARSimoes
dismissed
coderabbitai[bot]’s stale review
Addressed and explained part of that will be done in a follow up.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/boards/configuration.go (1)
81-91:⚠️ Potential issue | 🟠 Major
enableBoardsDeletionignores an explicitfalsevalue.
This enables data retention even when the config setsEnableBoardsDeletion=false.🛠️ Proposed fix
- enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil + enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil && + *mmconfig.DataRetentionSettings.EnableBoardsDeletion
🤖 Fix all issues with AI agents
In `@server/api/members.go`:
- Around line 133-135: The permission conditional in members.go incorrectly uses
&& causing open boards to bypass both ManageBoardRoles and
ManageBoardProperties; update the conditional in the block that calls
a.errorResponse to use the original OR semantics so the check reads: if not
HasPermissionToBoard(userID, boardID, PermissionManageBoardRoles) && (board.Type
!= BoardTypeOpen || not HasPermissionToBoard(userID, boardID,
PermissionManageBoardProperties)) — i.e., restore the logical operator between
the open-board check and the PermissionManageBoardProperties call so open boards
don’t unintentionally bypass permissions (refer to
a.permissions.HasPermissionToBoard, board.Type,
model.PermissionManageBoardRoles, model.PermissionManageBoardProperties,
model.BoardTypeOpen).
In `@server/boards/boardsapp_util.go`:
- Around line 61-63: The current check for enableBoardsDeletion only tests for a
non-nil pointer so an explicit false is treated as enabled; change the logic
that assigns enableBoardsDeletion (the variable referencing
mmconfig.DataRetentionSettings.EnableBoardsDeletion) to both check for non-nil
and that the dereferenced boolean is true (e.g., enableBoardsDeletion should be
true only when mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil &&
*mmconfig.DataRetentionSettings.EnableBoardsDeletion == true) so an explicit
false disables deletion as intended.
In `@server/model/team.go`:
- Around line 55-58: The validation logic in ValidateTeamID allows malformed IDs
when isTemplate is true; change the conditional to reject when the ID is neither
valid nor the allowed GlobalTeamID-for-templates. Replace the current if that
uses "!mm_model.IsValidId(teamID) && (!isTemplate && teamID != GlobalTeamID)"
with a check that returns an error when "not IsValidId(teamID) AND not
(isTemplate && teamID == GlobalTeamID)", i.e. use mm_model.IsValidId, isTemplate
and GlobalTeamID to allow only valid IDs or the GlobalTeamID in template mode.
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
40-44: Consider: Potential redundancy withlint-server.yml.Both
ci.ymlandlint-server.ymlrun golangci-lint on the same code paths with identical triggers (push to main/release branches, PRs). This may be intentional for different reporting contexts, but if not, you could consolidate to reduce CI time.
NARSimoes
dismissed
coderabbitai[bot]’s stale review
addressed requested updates ...
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/.golangci.yml (1)
40-41:llllinter configured but not enabled.The
lll(line length) linter has settings defined but is not included in theenablelist (lines 6-34). Either addlllto the enabled linters or remove the unused settings block.♻️ Suggested fix to enable lll
- revive - staticcheck - unconvert - unparam - unused - whitespace + - lll settings:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/.golangci.yml` around lines 40 - 41, The config defines lll with "line-length: 150" but the linter is not enabled; either add "lll" to the existing enable list under the "enable" key so the line-length setting is applied, or remove the lll block entirely to avoid dead configuration—update the "enable" list (or delete the "lll:" block) to resolve the mismatch..github/workflows/lint-server.yml (1)
5-7: Consider consolidating lint workflows to avoid duplicate runs.Both
lint-server.ymlandci.ymlrungolangci-lintwith identical configurations (v2.8.0, working-directory:focalboard/server). On overlapping triggers—specifically PRs tomainandrelease-**branches—linting executes twice, consuming CI resources unnecessarily.Additionally, the branch patterns differ:
ci.ymlusesreleases-**(with 's') whilelint-server.ymlusesrelease-**. Verify this inconsistency is intentional, or align both workflows to use the same pattern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint-server.yml around lines 5 - 7, Consolidate duplicate golangci-lint runs by removing or disabling the redundant job in one workflow (either lint-server.yml or ci.yml) so only a single workflow runs golangci-lint (the job referencing golangci-lint v2.8.0 and working-directory: focalboard/server); ensure the remaining workflow keeps the correct configuration and triggers. Also align the branch patterns between the two workflows by choosing a single pattern (either release-** or releases-**) and updating the other workflow to match, so PR triggers for main and release branches are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@server/boards/configuration.go`:
- Around line 90-92: The current assignment uses only a nil-check on
mmconfig.DataRetentionSettings.EnableBoardsDeletion (via enableBoardsDeletion),
which treats an explicit false as enabled; change the logic so
EnableDataRetention is true only when EnableBoardsDeletion is non-nil AND its
boolean value is true. Update the evaluation before setting
b.server.Config().EnableDataRetention to respect the dereferenced boolean value
rather than just pointer presence.
---
Duplicate comments:
In `@server/.golangci.yml`:
- Around line 100-103: Update the goimports local-prefixes entry to use this
repository's actual Go module path instead of the template value; locate the
goimports configuration block (goimports: local-prefixes:) in .golangci.yml and
replace the placeholder "github.com/mattermost/mattermost-starter-template" with
the correct module name for this project so imports are grouped correctly.
---
Nitpick comments:
In @.github/workflows/lint-server.yml:
- Around line 5-7: Consolidate duplicate golangci-lint runs by removing or
disabling the redundant job in one workflow (either lint-server.yml or ci.yml)
so only a single workflow runs golangci-lint (the job referencing golangci-lint
v2.8.0 and working-directory: focalboard/server); ensure the remaining workflow
keeps the correct configuration and triggers. Also align the branch patterns
between the two workflows by choosing a single pattern (either release-** or
releases-**) and updating the other workflow to match, so PR triggers for main
and release branches are consistent.
In `@server/.golangci.yml`:
- Around line 40-41: The config defines lll with "line-length: 150" but the
linter is not enabled; either add "lll" to the existing enable list under the
"enable" key so the line-length setting is applied, or remove the lll block
entirely to avoid dead configuration—update the "enable" list (or delete the
"lll:" block) to resolve the mismatch.
| enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil | ||
|
|
||
| b.server.Config().EnableDataRetention = enableBoardsDeletion |
There was a problem hiding this comment.
Explicit false now incorrectly enables retention.
A nil-check alone turns a configured false into true. Keep the nil guard but also respect the actual boolean value.
🛠️ Proposed fix
- enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil
+ enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil &&
+ *mmconfig.DataRetentionSettings.EnableBoardsDeletion🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/boards/configuration.go` around lines 90 - 92, The current assignment
uses only a nil-check on mmconfig.DataRetentionSettings.EnableBoardsDeletion
(via enableBoardsDeletion), which treats an explicit false as enabled; change
the logic so EnableDataRetention is true only when EnableBoardsDeletion is
non-nil AND its boolean value is true. Update the evaluation before setting
b.server.Config().EnableDataRetention to respect the dereferenced boolean value
rather than just pointer presence.
There was a problem hiding this comment.
@coderabbitai This how it was before, see the diff. The setting will show up in the configuration when it's enabled but granted that if it is later changed to false it will incorrectly never disable it
There was a problem hiding this comment.
@marianunez, thank you for the clarification! You're right that the nil-check pattern matches the original behavior. I appreciate you acknowledging the edge case where an explicit false value would be incorrectly treated as enabled.
My suggested fix would resolve that scenario by respecting the actual boolean value:
enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil &&
*mmconfig.DataRetentionSettings.EnableBoardsDeletionThis ensures that:
- If the pointer is
nil,enableBoardsDeletionisfalse(no setting configured) - If the pointer points to
true,enableBoardsDeletionistrue(explicitly enabled) - If the pointer points to
false,enableBoardsDeletionisfalse(explicitly disabled)
Let me know if you'd like to address this now or track it separately for a future fix.
There was a problem hiding this comment.
@NARSimoes coderabbit has a point, this was probably a bug.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
2 participants
Summary
Address https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/18 https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/16 https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/15 https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/14 https://github.com/mattermost/mattermost-plugin-boards/security/code-scanning/6
Add dependabot like in mattermost
Fix lint issues in tests check 4df5202 and 15815b8
Fix issues in server/ 7b6a639 and ignored a few issues to don't add many changes in this. Must be revisited later.
Ticket Link
TODO
Summary by CodeRabbit
Chores
Refactor
Tests