Create FIPS complaint boards plugin#120
Conversation
esarafianou
left a comment
esarafianou
left a comment
There was a problem hiding this comment.
Thanks Stavro, a few comments:
harshilsharma63
left a comment
harshilsharma63
left a comment
There was a problem hiding this comment.
Only change required is not returning that error there as in comment
|
Deferring the Makefile changes related to FIPS to @esarafianou , but boards changes look good. |
esarafianou
left a comment
esarafianou
left a comment
There was a problem hiding this comment.
@stafot great work, I didn't know we were not building the Boards plugin during CI at all.
Just one comment for naming. It should stay Boards to distinguish itself from the community supported focalboard.
13ffe4c to
cd67501
Compare
Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>
Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>
Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>
Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>
Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>
Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>
Signed-off-by: Stavros Foteinopoulos <stafot@gmail.com>
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
Merged up to the |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
Merged up to the |
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
Merged up to the |
|
Merged up to the |
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request updates the build and CI infrastructure to use Go 1.24.6 and golangci-lint v2.1.6, introduces FIPS build support in the Makefile, refactors CI workflows with inline job execution and artifact handling, standardizes error-value handling across the codebase, updates linting rules, and applies control-flow improvements (if/else to switch statements) while fixing category board lookup logic. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
This pull request introduces a high-severity authorization bypass in
🟠 Potential Authorization Bypass in Board Member management in
|
| Vulnerability | Potential Authorization Bypass in Board Member management |
|---|---|
| Description | The modified permission check allows users with PermissionManageBoardProperties to add members to Open boards. However, within the handleAddMember function, the new member's role (Admin/Editor/Viewer) is directly taken from the request payload. The function lacks a check to ensure that the user adding the new member has PermissionManageBoardRoles before granting them administrative privileges, or that the assigned roles do not exceed the privileges the adder is permitted to grant. As such, a user who is only supposed to have ManageBoardProperties for an Open board can, via this API, grant themselves or others Admin roles. |
mattermost-plugin-boards/server/api/members.go
Lines 149 to 155 in f4fc5d6
Comment to provide feedback on these findings.
Report false positive: @dryrunsecurity fp [FINDING ID] [FEEDBACK]
Report low-impact: @dryrunsecurity nit [FINDING ID] [FEEDBACK]
Example: @dryrunsecurity fp drs_90eda195 This code is not user-facing
All finding details can be found in the DryRun Security Dashboard.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/services/store/generators/main.go (1)
126-134:⚠️ Potential issue | 🟡 MinorClose the opened file via
deferto avoid leaks on read errors.If
io.ReadAllfails, the current flow returns before Line 134 and leaves the descriptor open.💡 Proposed fix
file, err := os.Open("store.go") if err != nil { return nil, fmt.Errorf("unable to open store/store.go file: %w", err) } + defer func() { _ = file.Close() }() src, err := io.ReadAll(file) if err != nil { return nil, err } - _ = file.Close()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/generators/main.go` around lines 126 - 134, The file descriptor opened with os.Open("store.go") is closed only after ReadAll, so if io.ReadAll returns an error the descriptor leaks; fix by calling defer file.Close() immediately after the successful os.Open (i.e., right after the err check for os.Open) to ensure closure on all return paths, and remove the later manual _ = file.Close() to avoid double-close confusion.server/.golangci.yml (1)
1-47:⚠️ Potential issue | 🟠 MajorConfig uses deprecated golangci-lint v1 syntax in v2; exclusions will not work.
With
version: 2, this configuration has three critical issues:
Top-level
linters-settings:(lines 6–15) is ignored. In v2, linter settings must be nested underlinters.settings:. Thestaticcheckconfiguration here will not be applied, making the//nolint:staticcheck // QF1008suppressions scattered in the codebase the only active enforcement.
issues.exclude-filesandissues.exclude-rules(lines 32–47) have been removed in v2. These v1 keys are not recognized. Exclusions must now be configured underlinters.exclusions:with the new structure:
issues.exclude-files→linters.exclusions.paths:issues.exclude-rules→linters.exclusions.rules:Duplicate
govetsettings. Lines 6–9 (top-level) and lines 19–22 (nested) both configuregovet, but only the nested version is read.The correct v2 structure consolidates all linter settings under
linters.settings:and exclusions underlinters.exclusions::♻️ Corrected v2 configuration
version: 2 run: timeout: 5m linters: disable-all: true settings: govet: enable: - shadow disable: - fieldalignment staticcheck: checks: - all - "-QF1008" enable: - errcheck - govet - ineffassign - staticcheck - unused - misspell -issues: - exclude-files: - - product/boards_product.go - - services/store/sqlstore/migrations - exclude-rules: - - path: server/manifest.go - linters: - - unused - - path: server/configuration.go - linters: - - unused - - path: _test\.go - linters: - - bodyclose - - linters: - - staticcheck - source: QF1008 +linters: + exclusions: + paths: + - product/boards_product.go + - services/store/sqlstore/migrations + rules: + - path: server/manifest.go + linters: + - unused + - path: server/configuration.go + linters: + - unused + - path: _test\.go + linters: + - bodyclose + - linters: + - staticcheck + source: QF1008Use
golangci-lint migrateto convert this configuration to v2 format automatically.Minor: trailing whitespace on lines 30 and 39.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/.golangci.yml` around lines 1 - 47, The config uses deprecated v1 keys so linters settings and exclusions are ignored; move the top-level "linters-settings:" block into "linters.settings:" and consolidate the duplicate "govet" entries under that nested "linters.settings.govet", and replace "issues.exclude-files" and "issues.exclude-rules" with the v2 structure under "linters.exclusions" (use "linters.exclusions.paths:" for the former and "linters.exclusions.rules:" for the latter, preserving the same paths and rule entries and sources like QF1008); run "golangci-lint migrate" or manually restructure the YAML accordingly and remove trailing whitespace on the blank lines.
🧹 Nitpick comments (11)
server/services/store/sqlstore/migrate.go (1)
146-151: Minor: defer assigns to outererr, which is harmless here but fragile.
err = db.Close()on line 148 reassigns the enclosing function-scopederr(declared at line 129). SinceMigrate()has unnamed returns, this does not change the returned value — but if someone later converts the signature to a named return (err error), this deferredClosewould silently overwrite the real migration error. Using:=/ a local variable makes the intent explicit and forward-safe.Proposed tweak
defer func() { s.logger.Debug("Closing migrations connection") - if err = db.Close(); err != nil { - s.logger.Error("Error closing migrations connection", mlog.Err(err)) + if cErr := db.Close(); cErr != nil { + s.logger.Error("Error closing migrations connection", mlog.Err(cErr)) } }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/sqlstore/migrate.go` around lines 146 - 151, The defer in Migrate() currently does "if err = db.Close(); err != nil { ... }" which reassigns the outer variable err; change it to capture the Close error in a new local variable (e.g. localErr or cerr) using := so you don't overwrite the function-scoped err—update the defer block around db.Close() to use a distinct local variable and log that local error via s.logger.Error(...) without assigning to the outer err.server/integrationtests/board_test.go (1)
2064-2071: Optional: break once found.The
.Category.ID→.IDchange is a no-op refactor (embedded field promotion). Separately, the loop continues scanning all categories/metadata after a match; adding a labeledbreakwould make intent clearer and avoid unnecessary iterations, though correctness isn't affected since each board should belong to only one category.Proposed tweak
var duplicateBoardCategoryID string + outer: for _, categoryBoard := range userCategoryBoards { for _, boardMetadata := range categoryBoard.BoardMetadata { if boardMetadata.BoardID == duplicateBoard.ID { duplicateBoardCategoryID = categoryBoard.ID + break outer } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/integrationtests/board_test.go` around lines 2064 - 2071, The code loops through userCategoryBoards and their BoardMetadata to set duplicateBoardCategoryID when boardMetadata.BoardID == duplicateBoard.ID but continues scanning unnecessarily; modify the nested loops (iterating userCategoryBoards and categoryBoard.BoardMetadata) to exit early once a match is found by using a labeled break (or setting a flag and breaking both loops) so that after assigning duplicateBoardCategoryID the outer loop stops immediately, improving clarity and avoiding extra iterations.server/services/store/sqlstore/migrationstests/migration_18_test.go (1)
100-100: Prefer asserting the unmarshal error over silencing it.Using
_ = json.Unmarshal(...)trades a lint warning for a silent failure mode: if the JSON is malformed, the subsequentrequire.NotNil(fields["columnCalculations"])will report a misleading failure instead of pointing at the real cause. Assert it instead.Proposed fix
- _ = json.Unmarshal([]byte(view.Fields), &fields) + require.NoError(t, json.Unmarshal([]byte(view.Fields), &fields))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/services/store/sqlstore/migrationstests/migration_18_test.go` at line 100, The current test silences JSON unmarshal errors by using `_ = json.Unmarshal([]byte(view.Fields), &fields)` which hides parse failures; change this to capture the error from json.Unmarshal on view.Fields into a variable (err) and assert it is nil using the test helper (e.g., require.NoError(t, err) or require.Nil(t, err)) before accessing fields["columnCalculations"] so any malformed JSON fails with the actual unmarshal error.server/client/client.go (1)
768-768: Consider logging/propagating themultipart.Writer.Close()error instead of silently discarding it.
writer.Close()finalizes the multipart body by writing the trailing boundary. If it fails, the server will receive a truncated/malformed multipart body and produce a confusing error far from the real root cause. Since both methods already return a*Responsewith anErrorfield, returning the close error would give callers useful diagnostics.Minor — not a correctness bug in practice (writing to
bytes.Bufferrarely fails), but discarding the error loses debuggability.♻️ Suggested change
- _ = writer.Close() + if err := writer.Close(); err != nil { + return nil, &Response{Error: err} // (or `return &Response{Error: err}` in ImportArchive) + }Also applies to: 883-883
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/client/client.go` at line 768, The multipart.Writer.Close() error is being discarded; capture its returned error from writer.Close() in the relevant methods (those that return *Response with an Error field) and propagate it into the Response.Error instead of ignoring it. Locate the call to writer.Close() in server/client/client.go (and the other occurrence) and if err := writer.Close(); err != nil { return &Response{Error: err} } (or set resp.Error = err before returning) so callers receive the actual close error for better diagnostics.build/manifest/main.go (1)
217-228: Recommended: usefilepath.Joinand assert the output directory exists.Two small refinements:
- Building paths with
fmt.Sprintf("%s/%s/plugin.json", ...)works on POSIX but is non-idiomatic; preferfilepath.Joinfor portability and clarity.os.WriteFilewill fail if<outputDir>/<manifest.Id>/doesn't exist. Today the Makefilebundle/bundle-fipstargets pre-create it, so there's an implicit ordering contract — anos.MkdirAllhere would make the helper robust when invoked directly.♻️ Suggested refactor
func distManifest(manifest *model.Manifest, outputDir string) error { manifestBytes, err := json.MarshalIndent(manifest, "", " ") if err != nil { return err } - if err := os.WriteFile(fmt.Sprintf("%s/%s/plugin.json", outputDir, manifest.Id), manifestBytes, 0600); err != nil { + dir := filepath.Join(outputDir, manifest.Id) + if err := os.MkdirAll(dir, 0755); err != nil { + return errors.Wrap(err, "failed to create output directory") + } + if err := os.WriteFile(filepath.Join(dir, "plugin.json"), manifestBytes, 0600); err != nil { return errors.Wrap(err, "failed to write plugin.json") } return nil }Requires adding
"path/filepath"to the imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build/manifest/main.go` around lines 217 - 228, In distManifest, replace the manual path fmt.Sprintf("%s/%s/plugin.json", outputDir, manifest.Id) with filepath.Join(outputDir, manifest.Id, "plugin.json") and ensure the directory exists before writing by calling os.MkdirAll(filepath.Join(outputDir, manifest.Id), 0755) (or similar) then call os.WriteFile; also add the "path/filepath" import. This makes distManifest robust when the target directory is missing and portable across platforms.Makefile (3)
259-265: Factor the FIPS archive name into a variable (parity withBUNDLE_NAME).
$(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gzis repeated three times (Lines 260, 262, 265) and cannot be overridden by callers the wayBUNDLE_NAME ?= ...can. Define it once alongsideBUNDLE_NAMEso release tooling / downstream callers can override it consistently.♻️ Suggested
Near Line 61:
BUNDLE_NAME ?= $(PLUGIN_ID)-$(PLUGIN_VERSION).tar.gz +FIPS_BUNDLE_NAME ?= $(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gzThen in
bundle-fips:ifeq ($(shell uname),Darwin) - cd dist-fips && tar --disable-copyfile -cvzf $(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gz $(PLUGIN_ID) + cd dist-fips && tar --disable-copyfile -cvzf $(FIPS_BUNDLE_NAME) $(PLUGIN_ID) else - cd dist-fips && tar -cvzf $(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gz $(PLUGIN_ID) + cd dist-fips && tar -cvzf $(FIPS_BUNDLE_NAME) $(PLUGIN_ID) endif - `@echo` "==> FIPS plugin built at: dist-fips/$(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gz" + `@echo` "==> FIPS plugin built at: dist-fips/$(FIPS_BUNDLE_NAME)"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 259 - 265, The Makefile repeats the literal archive name "$(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gz" in the bundle-fips target; define a single overridable variable (e.g., FIPS_BUNDLE_NAME ?= $(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gz) alongside BUNDLE_NAME near where BUNDLE_NAME is declared and then replace the three occurrences in the bundle-fips target (the tar and the echo) to use $(FIPS_BUNDLE_NAME) instead of the repeated literal so callers can override it consistently.
237-246: Redundant copy ofplugin-linux-amd64-fips.Lines 237–239 explicitly copy
plugin-linux-amd64-fips→plugin-linux-amd64. The glob loop on Lines 241–246 matchesplugin-*-fips*which also matchesplugin-linux-amd64-fipsand will copy it again to the same destination. Harmless but confusing. Either drop the explicit block and rely on the loop, or drop the loop if only amd64 is ever produced (see my earlier comment about FIPS arch coverage).♻️ Simplified
- # Copy FIPS binaries but rename them to standard names for server compatibility - if [ -f server/dist-fips/plugin-linux-amd64-fips ]; then \ - cp server/dist-fips/plugin-linux-amd64-fips dist-fips/$(PLUGIN_ID)/server/dist/plugin-linux-amd64; \ - fi - # Copy any other FIPS binaries and rename them - for file in server/dist-fips/plugin-*-fips*; do \ + # Copy FIPS binaries and strip the -fips suffix so server loader finds them by OS/arch. + for file in server/dist-fips/plugin-*-fips*; do \ if [ -f "$$file" ]; then \ target=$$(basename "$$file" | sed 's/-fips//g'); \ cp "$$file" "dist-fips/$(PLUGIN_ID)/server/dist/$$target"; \ fi; \ done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 237 - 246, The Makefile currently copies plugin-linux-amd64-fips explicitly and the for-loop over server/dist-fips/plugin-*-fips* will copy it again, causing a redundant duplicate; fix by removing the explicit block that handles plugin-linux-amd64-fips (the lines that cp server/dist-fips/plugin-linux-amd64-fips ...) and rely on the for-loop that sets target=$$(basename "$$file" | sed 's/-fips//g') to handle all plugin-*-fips* files, or alternatively adjust the for-loop to skip plugin-linux-amd64-fips if you prefer keeping the explicit copy (refer to the for file in the loop and the target variable to locate the code).
465-475:deploy-to-mattermost-directoryunconditionally copiespublic/and assets.Unlike
bundle(which gatespublic/behind$(HAS_PUBLIC)viacopy_bundle_files), this target always runscp -r public $(BOARD_PLUGIN_PATH)/andcp -r $(ASSETS_DIR) …. This plugin has both directories today so it works, but for parity with the bundler and future-proofing consider the same conditional guards. Low priority since this target is local-dev only.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` around lines 465 - 475, The deploy-to-mattermost-directory target unconditionally copies public and assets; change it to mirror the bundler guards by wrapping the public and ASSETS_DIR copies in the same conditional checks used by copy_bundle_files/$(HAS_PUBLIC) (or use directory-existence checks) so cp -r public $(BOARD_PLUGIN_PATH)/ and cp -r $(ASSETS_DIR) $(BOARD_PLUGIN_PATH)/ only run when public and ASSETS_DIR are present/allowed; update the Makefile block around the deploy-to-mattermost-directory target and reference BOARD_PLUGIN_PATH, ASSETS_DIR, and $(HAS_PUBLIC)/copy_bundle_files so it behaves the same as bundle..github/workflows/ci.yml (2)
31-34: Pick a single source of truth for the Go version across jobs.
webapp-testresolves Go fromfocalboard/go.mod(Line 34) whiledistuses the workflow-levelGO_VERSION: 1.24.6(Line 66). Ifgo.modever drifts from1.24.6, the two jobs compile with different toolchains and this PR's FIPS binary can get built on a different minor than what lint/test validated.♻️ Suggested alignment
Either read from
go.modin both jobs, or usego-version: "${{ env.GO_VERSION }}"in both. Usinggo-version-filein both is simplest and removes theGO_VERSIONenv entirely:- - name: Set up Go - uses: actions/setup-go@3041bf56c941b39c61721a86cd11f3bb1338122a # v5.2.0 - with: - go-version: "${{ env.GO_VERSION }}" - cache: true + - name: Set up Go + uses: actions/setup-go@3041bf56c941b39c61721a86cd11f3bb1338122a # v5.2.0 + with: + go-version-file: focalboard/go.mod + cache: trueAlso applies to: 63-67
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 31 - 34, The workflow uses two different sources for the Go version—webapp-test uses actions/setup-go with go-version-file: focalboard/go.mod while dist relies on the workflow-level GO_VERSION env (1.24.6); unify them by choosing one source of truth and update the other job: either set both jobs' actions/setup-go to use go-version-file: focalboard/go.mod (remove GO_VERSION env) or set both to use go-version: "${{ env.GO_VERSION }}" (replace go-version-file in webapp-test), ensuring you update the actions/setup-go invocations in the webapp-test and dist jobs and remove or align the GO_VERSION variable accordingly.
92-95: Pinsetup-chainctlby commit SHA, like the other actions in this file.Every other
uses:in this workflow is pinned to a 40-char SHA with the tag as a comment (v4.2.2,v5.2.0,v4.1.0,v4.2.3,v4.5.0).chainguard-dev/setup-chainctl@v0.3.2relies on tag mutability, which weakens the supply-chain posture of the FIPS build — the very thing this PR is trying to strengthen. Usechainguard-dev/setup-chainctl@f4ed65b781b048c44d4f033ae854c025c5531c19 # v0.3.2instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 92 - 95, Replace the mutable tag reference in the workflow step that currently reads "uses: chainguard-dev/setup-chainctl@v0.3.2" with the pinned commit SHA and add the original tag as a trailing comment (e.g., use "chainguard-dev/setup-chainctl@f4ed65b781b048c44d4f033ae854c025c5531c19 # v0.3.2"); update the step named "ci/setup-chainctl" to use the SHA-pinned ref while preserving the existing "with: identity: ${{ secrets.CHAINGUARD_IDENTITY }}" input..github/workflows/lint-server.yml (1)
41-41: Consider pinning theinstall.shref to the same version tag.The install script is currently fetched from
master, then piped intosh. While the tool version (v2.1.6) is pinned, the installer itself floats with whatever is onmasterat run time. For improved supply-chain security, reference the same version tag for the script (and apply the same change in.github/workflows/ci.ymlat line 45):🔧 Suggested change
- run: curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/master/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.1.6 + run: curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/v2.1.6/install.sh | sh -s -- -b $(go env GOPATH)/bin v2.1.6🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/lint-server.yml at line 41, Replace the floating installer URL that pipes the install.sh from master with a tag-pinned URL that matches the tool version (change the curl target from raw.githubusercontent.com/.../master/install.sh to the tag-specific path e.g. raw.githubusercontent.com/.../v2.1.6/install.sh) so the installer script is pinned to v2.1.6; apply the same replacement to the identical curl line in the other CI workflow where the installer is fetched.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 76-108: The CI currently caches and installs focalboard/webapp
node_modules in the steps named "Cache webapp node modules" and "Setup webapp
npm deps", but those are always deleted because the Makefile target dist-all
declares clean as a prerequisite (target dist-all -> clean) and clean removes
webapp/node_modules; fix by removing clean from the dist-all prerequisites in
the Makefile (edit the dist-all target to no longer depend on clean) so the
cached node_modules survive the make dist-all run, or alternatively remove the
two CI steps ("Cache webapp node modules" and "Setup webapp npm deps") from
.github/workflows/ci.yml if you prefer to keep clean in dist-all.
In `@Makefile`:
- Around line 140-152: The fallback Docker-login branch in the Makefile is using
Makefile expansion ($(CHAINGUARD_DEV_USERNAME)/$(CHAINGUARD_DEV_TOKEN)) instead
of reading the CI shell environment, so the tests [ -n
"$(CHAINGUARD_DEV_USERNAME)" ] always fail; change those Make-variable
references to shell env references by escaping the dollar sign (use
$$CHAINGUARD_DEV_USERNAME and $$CHAINGUARD_DEV_TOKEN wherever they appear in the
conditional and the docker login command) so the shell invoked by the recipe
reads the CI-provided environment variables at runtime.
- Around line 283-296: The dist-all target currently claims a parallel build but
invokes $(MAKE) dist and $(MAKE) dist-fips sequentially and causes the webapp to
be built twice because both dist and dist-fips depend on the webapp target; fix
by either (A) adjusting the echo to remove the "parallel" claim and keep
sequential invocation, or (B) avoid duplicate webapp builds by removing webapp
from the prerequisites of the dist-fips target (so dist-all runs dist then
dist-fips reusing the already-built webapp) or by making the webapp target
idempotent; update the dist-all message string accordingly and ensure references
to webapp/pack and server/dist remain correct when you remove the duplicate
prerequisite.
- Around line 132-172: The server-fips target produces only an amd64 binary
(server/dist-fips/plugin-linux-amd64-fips) because the FIPS Docker image
(FIPS_IMAGE) is amd64-only; add a short comment above the server-fips target
documenting this amd64-only constraint and why cross-compilation isn't
supported. Also add an explicit host-architecture check in the server-fips
recipe (use uname -m or similar) and abort early with a clear error message if
the host is not x86_64, writing the FIPS_BUILD_FAILED.txt placeholder and
exiting non-zero so admins installing the bundle see a deterministic failure;
reference the server-fips target, FIPS_IMAGE, output filename, and
FIPS_BUILD_FAILED.txt when making the changes.
In `@server/api/api.go`:
- Line 214: The line containing "fmt.Fprint(w, message)" has extra leading tabs
and should be indented to match the surrounding function block (single tab per
indent level); locate the statement (fmt.Fprint(w, message)) in
server/api/api.go and replace the extra tabs so it uses the same one-tab
indentation as the adjacent lines, then run gofmt/goimports (or ensure
formatting matches gofmt style) so the file's indentation is consistent.
In `@server/api/archive.go`:
- Line 155: Fix the mis-indented error write that uses fmt.Fprintf(w, "%v", err)
so it aligns with the surrounding if/return block and is gofmt-compliant; also
set a proper response status before writing (e.g., call
w.WriteHeader(http.StatusBadRequest) prior to the fmt.Fprintf) or replace the
raw write with the existing helper (e.g., a.errorResponse(w, req,
http.StatusBadRequest, err.Error())) so the client gets a non-200 status for the
error.
In `@server/boards/boardsapp_util.go`:
- Line 63: The assignment for enableBoardsDeletion currently checks only that
mmconfig.DataRetentionSettings.EnableBoardsDeletion is non-nil, which treats a
false value as enabled; change the logic to check the pointer's boolean value
(e.g., require mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil AND
*mmconfig.DataRetentionSettings.EnableBoardsDeletion == true) so
enableBoardsDeletion is true only when the setting is explicitly true.
In `@server/boards/configuration.go`:
- Line 90: The code treats DataRetentionSettings.EnableBoardsDeletion as enabled
if the pointer is non-nil, causing false to be treated as true; change the logic
that sets enableBoardsDeletion (and any use assigning EnableDataRetention) to
check both that mmconfig.DataRetentionSettings.EnableBoardsDeletion is non-nil
and its dereferenced value is true (i.e., use a nil-safe boolean check like
mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil &&
*mmconfig.DataRetentionSettings.EnableBoardsDeletion). Update any related
assignments or flags that rely on enableBoardsDeletion to use this corrected
check.
In `@server/integrationtests/permissions_test.go`:
- Around line 283-300: The per-branch defers call response.Body.Close()
unguarded and can panic if the request failed and response is nil; update each
branch (where DoAPIGet/DoAPIPost/DoAPIPatch/DoAPIPut/DoAPIDelete are called) to
only defer response.Body.Close() after verifying response != nil (or after
checking err == nil), e.g. call the API, check err/response and then if response
!= nil defer response.Body.Close(), before asserting tc.expectedStatusCode using
response.StatusCode.
In
`@server/services/store/sqlstore/migrationstests/deleted_membership_boards_migration_test.go`:
- Around line 34-49: The test has a copy/paste bug: both DB queries for
boardDirectMessage use id = 'board-group-channel' so boardDirectMessage and
boardGroupChannel are identical; update the second th.f.DB().Get call that fills
boardDirectMessage (the one after th.f.RunInterceptor(18)) to query the correct
direct-message board id (e.g., "board-direct-message") so that
boardDirectMessage reflects the direct message board and the subsequent
assertions on boardDirectMessage.Created_By and Team_ID validate the intended
board type.
In `@server/services/telemetry/telemetry_test.go`:
- Line 55: The test sets only RUDDER_DATAPLANE_URL which leaves telemetry
missing RUDDER_KEY and can make checkMockRudderServer block; set a dummy
RUDDER_KEY environment variable in the test (alongside RUDDER_DATAPLANE_URL) so
telemetry is initialized and the checkMockRudderServer() path can proceed;
update telemetry_test.go to set os.Setenv("RUDDER_KEY", "<dummy-key>") before
invoking checkMockRudderServer (and optionally restore or unset the env var
after the test).
In `@server/services/webhook/webhook.go`:
- Around line 29-31: The http.Post call in the webhook sender ignores its
returned error and immediately dereferences resp.Body (the http.Post call and
the subsequent io.ReadAll/resp.Body.Close), which can nil-deref if the POST
fails; update the code around the http.Post call so you capture and handle the
returned error, check that resp != nil before reading/closing the body, log or
return/continue on error, and only call io.ReadAll and defer resp.Body.Close
after confirming resp is non-nil (place the defer immediately after the
nil-check). Ensure you update the symbols in this file: the http.Post call, the
resp variable, io.ReadAll usage, and resp.Body.Close handling inside the webhook
sender loop/function.
---
Outside diff comments:
In `@server/.golangci.yml`:
- Around line 1-47: The config uses deprecated v1 keys so linters settings and
exclusions are ignored; move the top-level "linters-settings:" block into
"linters.settings:" and consolidate the duplicate "govet" entries under that
nested "linters.settings.govet", and replace "issues.exclude-files" and
"issues.exclude-rules" with the v2 structure under "linters.exclusions" (use
"linters.exclusions.paths:" for the former and "linters.exclusions.rules:" for
the latter, preserving the same paths and rule entries and sources like QF1008);
run "golangci-lint migrate" or manually restructure the YAML accordingly and
remove trailing whitespace on the blank lines.
In `@server/services/store/generators/main.go`:
- Around line 126-134: The file descriptor opened with os.Open("store.go") is
closed only after ReadAll, so if io.ReadAll returns an error the descriptor
leaks; fix by calling defer file.Close() immediately after the successful
os.Open (i.e., right after the err check for os.Open) to ensure closure on all
return paths, and remove the later manual _ = file.Close() to avoid double-close
confusion.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 31-34: The workflow uses two different sources for the Go
version—webapp-test uses actions/setup-go with go-version-file:
focalboard/go.mod while dist relies on the workflow-level GO_VERSION env
(1.24.6); unify them by choosing one source of truth and update the other job:
either set both jobs' actions/setup-go to use go-version-file: focalboard/go.mod
(remove GO_VERSION env) or set both to use go-version: "${{ env.GO_VERSION }}"
(replace go-version-file in webapp-test), ensuring you update the
actions/setup-go invocations in the webapp-test and dist jobs and remove or
align the GO_VERSION variable accordingly.
- Around line 92-95: Replace the mutable tag reference in the workflow step that
currently reads "uses: chainguard-dev/setup-chainctl@v0.3.2" with the pinned
commit SHA and add the original tag as a trailing comment (e.g., use
"chainguard-dev/setup-chainctl@f4ed65b781b048c44d4f033ae854c025c5531c19 #
v0.3.2"); update the step named "ci/setup-chainctl" to use the SHA-pinned ref
while preserving the existing "with: identity: ${{ secrets.CHAINGUARD_IDENTITY
}}" input.
In @.github/workflows/lint-server.yml:
- Line 41: Replace the floating installer URL that pipes the install.sh from
master with a tag-pinned URL that matches the tool version (change the curl
target from raw.githubusercontent.com/.../master/install.sh to the tag-specific
path e.g. raw.githubusercontent.com/.../v2.1.6/install.sh) so the installer
script is pinned to v2.1.6; apply the same replacement to the identical curl
line in the other CI workflow where the installer is fetched.
In `@build/manifest/main.go`:
- Around line 217-228: In distManifest, replace the manual path
fmt.Sprintf("%s/%s/plugin.json", outputDir, manifest.Id) with
filepath.Join(outputDir, manifest.Id, "plugin.json") and ensure the directory
exists before writing by calling os.MkdirAll(filepath.Join(outputDir,
manifest.Id), 0755) (or similar) then call os.WriteFile; also add the
"path/filepath" import. This makes distManifest robust when the target directory
is missing and portable across platforms.
In `@Makefile`:
- Around line 259-265: The Makefile repeats the literal archive name
"$(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gz" in the bundle-fips target; define a
single overridable variable (e.g., FIPS_BUNDLE_NAME ?=
$(PLUGIN_ID)-$(PLUGIN_VERSION)-fips.tar.gz) alongside BUNDLE_NAME near where
BUNDLE_NAME is declared and then replace the three occurrences in the
bundle-fips target (the tar and the echo) to use $(FIPS_BUNDLE_NAME) instead of
the repeated literal so callers can override it consistently.
- Around line 237-246: The Makefile currently copies plugin-linux-amd64-fips
explicitly and the for-loop over server/dist-fips/plugin-*-fips* will copy it
again, causing a redundant duplicate; fix by removing the explicit block that
handles plugin-linux-amd64-fips (the lines that cp
server/dist-fips/plugin-linux-amd64-fips ...) and rely on the for-loop that sets
target=$$(basename "$$file" | sed 's/-fips//g') to handle all plugin-*-fips*
files, or alternatively adjust the for-loop to skip plugin-linux-amd64-fips if
you prefer keeping the explicit copy (refer to the for file in the loop and the
target variable to locate the code).
- Around line 465-475: The deploy-to-mattermost-directory target unconditionally
copies public and assets; change it to mirror the bundler guards by wrapping the
public and ASSETS_DIR copies in the same conditional checks used by
copy_bundle_files/$(HAS_PUBLIC) (or use directory-existence checks) so cp -r
public $(BOARD_PLUGIN_PATH)/ and cp -r $(ASSETS_DIR) $(BOARD_PLUGIN_PATH)/ only
run when public and ASSETS_DIR are present/allowed; update the Makefile block
around the deploy-to-mattermost-directory target and reference
BOARD_PLUGIN_PATH, ASSETS_DIR, and $(HAS_PUBLIC)/copy_bundle_files so it behaves
the same as bundle.
In `@server/client/client.go`:
- Line 768: The multipart.Writer.Close() error is being discarded; capture its
returned error from writer.Close() in the relevant methods (those that return
*Response with an Error field) and propagate it into the Response.Error instead
of ignoring it. Locate the call to writer.Close() in server/client/client.go
(and the other occurrence) and if err := writer.Close(); err != nil { return
&Response{Error: err} } (or set resp.Error = err before returning) so callers
receive the actual close error for better diagnostics.
In `@server/integrationtests/board_test.go`:
- Around line 2064-2071: The code loops through userCategoryBoards and their
BoardMetadata to set duplicateBoardCategoryID when boardMetadata.BoardID ==
duplicateBoard.ID but continues scanning unnecessarily; modify the nested loops
(iterating userCategoryBoards and categoryBoard.BoardMetadata) to exit early
once a match is found by using a labeled break (or setting a flag and breaking
both loops) so that after assigning duplicateBoardCategoryID the outer loop
stops immediately, improving clarity and avoiding extra iterations.
In `@server/services/store/sqlstore/migrate.go`:
- Around line 146-151: The defer in Migrate() currently does "if err =
db.Close(); err != nil { ... }" which reassigns the outer variable err; change
it to capture the Close error in a new local variable (e.g. localErr or cerr)
using := so you don't overwrite the function-scoped err—update the defer block
around db.Close() to use a distinct local variable and log that local error via
s.logger.Error(...) without assigning to the outer err.
In `@server/services/store/sqlstore/migrationstests/migration_18_test.go`:
- Line 100: The current test silences JSON unmarshal errors by using `_ =
json.Unmarshal([]byte(view.Fields), &fields)` which hides parse failures; change
this to capture the error from json.Unmarshal on view.Fields into a variable
(err) and assert it is nil using the test helper (e.g., require.NoError(t, err)
or require.Nil(t, err)) before accessing fields["columnCalculations"] so any
malformed JSON fails with the actual unmarshal error.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: c861d99f-f167-4ddb-9e70-e9b5d11814e2
📒 Files selected for processing (45)
.github/workflows/ci.yml.github/workflows/lint-server.yml.gitignoreMakefilebuild/manifest/main.goserver/.golangci.ymlserver/api/api.goserver/api/api_test.goserver/api/archive.goserver/api/files.goserver/api/members.goserver/app/boards.goserver/app/category_boards.goserver/app/export.goserver/assets/build-template-archive/main.goserver/boards/boardsapp_test.goserver/boards/boardsapp_util.goserver/boards/configuration.goserver/boards/data_retention_test.goserver/client/client.goserver/integrationtests/board_test.goserver/integrationtests/clienttestlib.goserver/integrationtests/compliance_test.goserver/integrationtests/permissions_test.goserver/integrationtests/pluginteststore.goserver/model/properties_test.goserver/model/team.goserver/plugin.goserver/services/metrics/service.goserver/services/store/generators/main.goserver/services/store/sqlstore/data_migrations.goserver/services/store/sqlstore/data_migrations_test.goserver/services/store/sqlstore/helpers_test.goserver/services/store/sqlstore/migrate.goserver/services/store/sqlstore/migrationstests/boards_migrator_test.goserver/services/store/sqlstore/migrationstests/deleted_membership_boards_migration_test.goserver/services/store/sqlstore/migrationstests/migrate_34_test.goserver/services/store/sqlstore/migrationstests/migration36_test.goserver/services/store/sqlstore/migrationstests/migration_18_test.goserver/services/store/sqlstore/util.goserver/services/telemetry/telemetry_test.goserver/services/webhook/webhook.goserver/web/webserver.goserver/web/webserver_test.goserver/ws/server.go
| - name: Cache webapp node modules | ||
| id: cache-webapp-node-modules | ||
| uses: actions/cache@5a3ec84eff668545956fd18022155c47e93e2684 # v4.2.3 | ||
| with: | ||
| path: focalboard/webapp/node_modules | ||
| key: ${{ runner.os }}-webapp-node-modules-${{ hashFiles('focalboard/webapp/package-lock.json') }} | ||
| restore-keys: ${{ runner.os }}-webapp-node-modules- | ||
|
|
||
| - name: Setup webapp npm deps | ||
| if: steps.cache-webapp-node-modules.outputs.cache-hit != 'true' | ||
| env: | ||
| NODE_ENV: development | ||
| run: | | ||
| cd focalboard/webapp | ||
| npm install --ignore-scripts --no-save | ||
|
|
||
| - name: ci/setup-chainctl | ||
| uses: chainguard-dev/setup-chainctl@v0.3.2 | ||
| with: | ||
| identity: ${{ secrets.CHAINGUARD_IDENTITY }} | ||
|
|
||
| - name: ci/setup-build-tools | ||
| run: | | ||
| echo "Setting up build tools..." | ||
| cd focalboard | ||
| mkdir -p build/bin | ||
| cd build/manifest && go build -o ../bin/manifest | ||
| cd ../pluginctl && go build -o ../bin/pluginctl | ||
|
|
||
| - name: Build both distributions | ||
| env: | ||
| GO_VERSION: ${{ env.GO_VERSION }} | ||
| run: cd focalboard; make dist-all |
There was a problem hiding this comment.
Webapp cache + install steps are nullified by make dist-all’s clean target.
dist-all declares clean as a prerequisite (see Makefile Line 285), and clean does rm -fr webapp/node_modules (Makefile Line 434). So the Cache webapp node modules and Setup webapp npm deps steps here (Lines 76–90) are always thrown away before the actual build — dist → webapp → webapp/node_modules triggers a fresh npm install anyway.
Either drop these two steps, or remove clean from dist-all’s prerequisites (preferable — see my comment on the Makefile) so the cache actually buys you something.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 76 - 108, The CI currently caches and
installs focalboard/webapp node_modules in the steps named "Cache webapp node
modules" and "Setup webapp npm deps", but those are always deleted because the
Makefile target dist-all declares clean as a prerequisite (target dist-all ->
clean) and clean removes webapp/node_modules; fix by removing clean from the
dist-all prerequisites in the Makefile (edit the dist-all target to no longer
depend on clean) so the cached node_modules survive the make dist-all run, or
alternatively remove the two CI steps ("Cache webapp node modules" and "Setup
webapp npm deps") from .github/workflows/ci.yml if you prefer to keep clean in
dist-all.
| ## Builds the server with FIPS compliance using Docker (requires Docker) | ||
| .PHONY: server-fips | ||
| server-fips: templates-archive | ||
| ifneq ($(HAS_SERVER),) | ||
| @echo Building FIPS-compliant plugin server binaries | ||
| mkdir -p server/dist-fips | ||
| @echo "Setting up FIPS build environment..." | ||
|
|
||
| # Docker authentication is handled by CI (setup-chainctl) | ||
| @if ! docker manifest inspect $(FIPS_IMAGE) >/dev/null 2>&1; then \ | ||
| echo "Docker authentication failed. Ensure setup-chainctl configured Docker authentication."; \ | ||
| echo "Trying fallback authentication if credentials are available..."; \ | ||
| if [ -n "$(CHAINGUARD_DEV_USERNAME)" ] && [ -n "$(CHAINGUARD_DEV_TOKEN)" ]; then \ | ||
| echo "Using username/token authentication..."; \ | ||
| echo "$(CHAINGUARD_DEV_TOKEN)" | docker login cgr.dev --username "$(CHAINGUARD_DEV_USERNAME)" --password-stdin; \ | ||
| else \ | ||
| echo "Warning: No authentication available. FIPS build may fail."; \ | ||
| fi; \ | ||
| else \ | ||
| echo "✅ Docker authentication is working"; \ | ||
| fi | ||
|
|
||
| # Create local cache directory for CI/ACT compatibility | ||
| mkdir -p $(PWD)/.build-cache | ||
|
|
||
| # Try FIPS build with error handling | ||
| @if docker run --rm \ | ||
| --entrypoint="" \ | ||
| -v $(PWD):/plugin \ | ||
| -v $(PWD)/.build-cache:/root/.cache \ | ||
| -w /plugin/server \ | ||
| $(FIPS_IMAGE) \ | ||
| sh -c "CGO_ENABLED=1 GOOS=linux GOARCH=amd64 go build -tags fips -ldflags '$(LDFLAGS)' -trimpath -buildvcs=false -o dist-fips/plugin-linux-amd64-fips"; then \ | ||
| echo "FIPS plugin server build completed: server/dist-fips/plugin-linux-amd64-fips"; \ | ||
| else \ | ||
| echo "FIPS build failed - likely authentication issue with $(FIPS_IMAGE)"; \ | ||
| echo "Creating placeholder to indicate FIPS build was attempted but failed"; \ | ||
| echo "FIPS_BUILD_FAILED" > server/dist-fips/FIPS_BUILD_FAILED.txt; \ | ||
| exit 1; \ | ||
| fi | ||
| endif |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does cgr.dev/mattermost.com/go-msft-fips:1.24 support cross-compilation to linux/arm64 with CGO enabled, or is it amd64-only?
💡 Result:
No, cgr.dev/mattermost.com/go-msft-fips:1.24 does not support cross-compilation to linux/arm64 with CGO enabled. It is amd64-only. The image is used as a builder in Mattermost FIPS Dockerfiles with CGO_ENABLED=1 explicitly set for FIPS compliance via Microsoft's Go build with systemcrypto and OpenSSL CGO bindings. Mattermost's FIPS builds explicitly limit to linux/amd64 ("only build-linux-amd64 for fips", "skip arm for FIPS"), skipping arm64. Chainguard FIPS images like mattermost-fips list sizes only for x86_64 (amd64), with no arm64 variants. FIPS Go requires CGO for OpenSSL, and cross-compilation with CGO to arm64 needs cross-compiled OpenSSL libraries, which this specialized Chainguard image likely lacks—consistent with amd64-only builds in practice.
Citations:
- 1: https://github.com/mattermost/mattermost-operator/blob/master/Dockerfile.fips
- 2: Introduce build-server-image-fips mattermost#33670
- 3: MM-64878: FIPS Build mattermost#33809
- 4: https://github.com/mattermost/mattermost/actions/runs/20798185206
- 5: https://github.com/microsoft/go-images
- 6: https://images.chainguard.dev/directory/image/mattermost-fips/overview
- 7: https://images.chainguard.dev/directory/image/mattermost-fips/specifications
- 8: https://images.chainguard.dev/directory/image/mattermost-fips/versions
FIPS build scope is intentionally limited to linux-amd64 by the FIPS tooling; document this constraint and improve error messaging.
The server-fips target only produces plugin-linux-amd64-fips because the underlying FIPS Docker image (cgr.dev/mattermost.com/go-msft-fips:1.24) is amd64-only. Cross-compilation to arm64 is not supported by this image—FIPS Go requires CGO bindings for OpenSSL/systemcrypto, and the image lacks cross-compiled libraries for non-x86_64 platforms. This is a known architectural decision in Mattermost FIPS builds.
To prevent silent failures when admins install the FIPS bundle on non-amd64 hosts:
- Document the amd64-only scope in a comment above this target or in the PR description.
- Ensure the server surfaces a clear error if the host architecture doesn't match the available FIPS binary.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 132 - 172, The server-fips target produces only an
amd64 binary (server/dist-fips/plugin-linux-amd64-fips) because the FIPS Docker
image (FIPS_IMAGE) is amd64-only; add a short comment above the server-fips
target documenting this amd64-only constraint and why cross-compilation isn't
supported. Also add an explicit host-architecture check in the server-fips
recipe (use uname -m or similar) and abort early with a clear error message if
the host is not x86_64, writing the FIPS_BUILD_FAILED.txt placeholder and
exiting non-zero so admins installing the bundle see a deterministic failure;
reference the server-fips target, FIPS_IMAGE, output filename, and
FIPS_BUILD_FAILED.txt when making the changes.
| # Docker authentication is handled by CI (setup-chainctl) | ||
| @if ! docker manifest inspect $(FIPS_IMAGE) >/dev/null 2>&1; then \ | ||
| echo "Docker authentication failed. Ensure setup-chainctl configured Docker authentication."; \ | ||
| echo "Trying fallback authentication if credentials are available..."; \ | ||
| if [ -n "$(CHAINGUARD_DEV_USERNAME)" ] && [ -n "$(CHAINGUARD_DEV_TOKEN)" ]; then \ | ||
| echo "Using username/token authentication..."; \ | ||
| echo "$(CHAINGUARD_DEV_TOKEN)" | docker login cgr.dev --username "$(CHAINGUARD_DEV_USERNAME)" --password-stdin; \ | ||
| else \ | ||
| echo "Warning: No authentication available. FIPS build may fail."; \ | ||
| fi; \ | ||
| else \ | ||
| echo "✅ Docker authentication is working"; \ | ||
| fi |
There was a problem hiding this comment.
Fallback Docker login expands as Make variables, not shell env.
$(CHAINGUARD_DEV_USERNAME) and $(CHAINGUARD_DEV_TOKEN) are Make variables and will be expanded at parse time — if they aren't passed on the make command line they evaluate to empty strings, so both [ -n "" ] tests always fail silently and the "Warning: No authentication available" branch is taken regardless of what's in the CI environment. To read the CI-provided env vars, escape with $$:
🛡️ Suggested fix
- if [ -n "$(CHAINGUARD_DEV_USERNAME)" ] && [ -n "$(CHAINGUARD_DEV_TOKEN)" ]; then \
+ if [ -n "$$CHAINGUARD_DEV_USERNAME" ] && [ -n "$$CHAINGUARD_DEV_TOKEN" ]; then \
echo "Using username/token authentication..."; \
- echo "$(CHAINGUARD_DEV_TOKEN)" | docker login cgr.dev --username "$(CHAINGUARD_DEV_USERNAME)" --password-stdin; \
+ echo "$$CHAINGUARD_DEV_TOKEN" | docker login cgr.dev --username "$$CHAINGUARD_DEV_USERNAME" --password-stdin; \
else \Since CI is expected to use setup-chainctl for auth, this is low severity — but as-written the fallback is dead code.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Docker authentication is handled by CI (setup-chainctl) | |
| @if ! docker manifest inspect $(FIPS_IMAGE) >/dev/null 2>&1; then \ | |
| echo "Docker authentication failed. Ensure setup-chainctl configured Docker authentication."; \ | |
| echo "Trying fallback authentication if credentials are available..."; \ | |
| if [ -n "$(CHAINGUARD_DEV_USERNAME)" ] && [ -n "$(CHAINGUARD_DEV_TOKEN)" ]; then \ | |
| echo "Using username/token authentication..."; \ | |
| echo "$(CHAINGUARD_DEV_TOKEN)" | docker login cgr.dev --username "$(CHAINGUARD_DEV_USERNAME)" --password-stdin; \ | |
| else \ | |
| echo "Warning: No authentication available. FIPS build may fail."; \ | |
| fi; \ | |
| else \ | |
| echo "✅ Docker authentication is working"; \ | |
| fi | |
| # Docker authentication is handled by CI (setup-chainctl) | |
| `@if` ! docker manifest inspect $(FIPS_IMAGE) >/dev/null 2>&1; then \ | |
| echo "Docker authentication failed. Ensure setup-chainctl configured Docker authentication."; \ | |
| echo "Trying fallback authentication if credentials are available..."; \ | |
| if [ -n "$$CHAINGUARD_DEV_USERNAME" ] && [ -n "$$CHAINGUARD_DEV_TOKEN" ]; then \ | |
| echo "Using username/token authentication..."; \ | |
| echo "$$CHAINGUARD_DEV_TOKEN" | docker login cgr.dev --username "$$CHAINGUARD_DEV_USERNAME" --password-stdin; \ | |
| else \ | |
| echo "Warning: No authentication available. FIPS build may fail."; \ | |
| fi; \ | |
| else \ | |
| echo "✅ Docker authentication is working"; \ | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 140 - 152, The fallback Docker-login branch in the
Makefile is using Makefile expansion
($(CHAINGUARD_DEV_USERNAME)/$(CHAINGUARD_DEV_TOKEN)) instead of reading the CI
shell environment, so the tests [ -n "$(CHAINGUARD_DEV_USERNAME)" ] always fail;
change those Make-variable references to shell env references by escaping the
dollar sign (use $$CHAINGUARD_DEV_USERNAME and $$CHAINGUARD_DEV_TOKEN wherever
they appear in the conditional and the docker login command) so the shell
invoked by the recipe reads the CI-provided environment variables at runtime.
| ## Builds both normal and FIPS distributions. | ||
| .PHONY: dist-all | ||
| dist-all: clean | ||
| @echo "==> Building both normal and FIPS distributions in parallel..." | ||
| $(MAKE) dist | ||
| @if $(MAKE) dist-fips; then \ | ||
| echo "==> Both distributions built successfully:"; \ | ||
| echo " Normal: dist/$$(./build/bin/manifest id)-$$(./build/bin/manifest version).tar.gz"; \ | ||
| echo " FIPS: dist-fips/$$(./build/bin/manifest id)-$$(./build/bin/manifest version)-fips.tar.gz"; \ | ||
| else \ | ||
| echo "==> FIPS build failed, continuing with normal distribution only:"; \ | ||
| echo " Normal: dist/$$(./build/bin/manifest id)-$$(./build/bin/manifest version).tar.gz"; \ | ||
| echo " FIPS: Build failed - check Docker/credentials"; \ | ||
| fi |
There was a problem hiding this comment.
dist-all builds the webapp twice and mislabels work as parallel.
Two issues in this recipe:
- Sequential, not parallel. The comment says "Building both normal and FIPS distributions in parallel…" but
$(MAKE) distand$(MAKE) dist-fipsare invoked back-to-back. Update the message, or run them with-jif parallel is really desired (harder — both targets mutatewebapp/packandserver/dist). - Duplicate webapp build.
distdepends onwebapp, anddist-fipsalso depends onwebapp(Line 281). Thewebapptarget has no staleness check, sonpm run build+npm run packrun twice on everydist-all. Sincedist-all: cleanhas already wipedwebapp/node_modules, you also pay onenpm installcost. On CI this roughly doubles the webapp portion of the build.
Recommended: drop webapp from dist-fips's prerequisites and rely on dist-all ordering, or refactor the webapp target to be idempotent. And update the echo:
♻️ Suggested
-## Builds and bundles the FIPS plugin.
-.PHONY: dist-fips
-dist-fips: apply server-fips webapp bundle-fips
+## Builds and bundles the FIPS plugin. Assumes `webapp` has already been built
+## (e.g. when invoked via `dist-all`). Run `make dist` first if invoking standalone.
+.PHONY: dist-fips
+dist-fips: apply server-fips bundle-fips- `@echo` "==> Building both normal and FIPS distributions in parallel..."
+ `@echo` "==> Building normal and FIPS distributions sequentially..."
$(MAKE) dist
`@if` $(MAKE) dist-fips; then \Also note: because dist-all: clean removes webapp/node_modules, the Cache webapp node modules / Setup webapp npm deps steps in ci.yml are effectively no-ops — see my comment on ci.yml Lines 76–108.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Makefile` around lines 283 - 296, The dist-all target currently claims a
parallel build but invokes $(MAKE) dist and $(MAKE) dist-fips sequentially and
causes the webapp to be built twice because both dist and dist-fips depend on
the webapp target; fix by either (A) adjusting the echo to remove the "parallel"
claim and keep sequential invocation, or (B) avoid duplicate webapp builds by
removing webapp from the prerequisites of the dist-fips target (so dist-all runs
dist then dist-fips reusing the already-built webapp) or by making the webapp
target idempotent; update the dist-all message string accordingly and ensure
references to webapp/pack and server/dist remain correct when you remove the
duplicate prerequisite.
| setResponseHeader(w, "Content-Type", "application/json") | ||
| w.WriteHeader(code) | ||
| fmt.Fprint(w, message) | ||
| _, _ = fmt.Fprint(w, message) |
There was a problem hiding this comment.
Indentation is off on the changed line.
Line 214 uses extra leading tabs compared to surrounding lines in the same function, breaking formatting. Since gofmt/goimports were removed from server/.golangci.yml in this PR, this is no longer caught by CI — please restore the normal one-tab indent.
🔧 Proposed fix
- _, _ = fmt.Fprint(w, message)
+ _, _ = fmt.Fprint(w, message)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/api/api.go` at line 214, The line containing "fmt.Fprint(w, message)"
has extra leading tabs and should be indented to match the surrounding function
block (single tab per indent level); locate the statement (fmt.Fprint(w,
message)) in server/api/api.go and replace the extra tabs so it uses the same
one-tab indentation as the adjacent lines, then run gofmt/goimports (or ensure
formatting matches gofmt style) so the file's indentation is consistent.
| if mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil { | ||
| enableBoardsDeletion = true | ||
| } | ||
| enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil |
There was a problem hiding this comment.
Line 90: Data retention is enabled by pointer presence, not configured value (risk of unintended deletions).
EnableBoardsDeletion != nil becomes true even when the setting is explicitly false. This can incorrectly set EnableDataRetention to true.
Suggested fix
- enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil
+ enableBoardsDeletion := mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil &&
+ *mmconfig.DataRetentionSettings.EnableBoardsDeletion📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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` at line 90, The code treats
DataRetentionSettings.EnableBoardsDeletion as enabled if the pointer is non-nil,
causing false to be treated as true; change the logic that sets
enableBoardsDeletion (and any use assigning EnableDataRetention) to check both
that mmconfig.DataRetentionSettings.EnableBoardsDeletion is non-nil and its
dereferenced value is true (i.e., use a nil-safe boolean check like
mmconfig.DataRetentionSettings.EnableBoardsDeletion != nil &&
*mmconfig.DataRetentionSettings.EnableBoardsDeletion). Update any related
assignments or flags that rely on enableBoardsDeletion to use this corrected
check.
| case methodGet: | ||
| response, err = reqClient.DoAPIGet(url, "") | ||
| defer response.Body.Close() | ||
| defer func() { _ = response.Body.Close() }() | ||
| case methodPost: | ||
| response, err = reqClient.DoAPIPost(url, tc.body) | ||
| defer response.Body.Close() | ||
| defer func() { _ = response.Body.Close() }() | ||
| case methodPatch: | ||
| response, err = reqClient.DoAPIPatch(url, tc.body) | ||
| defer response.Body.Close() | ||
| defer func() { _ = response.Body.Close() }() | ||
| case methodPut: | ||
| response, err = reqClient.DoAPIPut(url, tc.body) | ||
| defer response.Body.Close() | ||
| defer func() { _ = response.Body.Close() }() | ||
| case methodDelete: | ||
| response, err = reqClient.DoAPIDelete(url, tc.body) | ||
| defer response.Body.Close() | ||
| defer func() { _ = response.Body.Close() }() | ||
| } | ||
|
|
||
| require.Equal(t, tc.expectedStatusCode, response.StatusCode, tc.identifier()) |
There was a problem hiding this comment.
Guard response before deferring body close to avoid nil-pointer panics.
If a request fails with response == nil, each branch-level defer can panic and hide the real failure.
💡 Proposed fix
switch tc.method {
case methodGet:
response, err = reqClient.DoAPIGet(url, "")
- defer func() { _ = response.Body.Close() }()
case methodPost:
response, err = reqClient.DoAPIPost(url, tc.body)
- defer func() { _ = response.Body.Close() }()
case methodPatch:
response, err = reqClient.DoAPIPatch(url, tc.body)
- defer func() { _ = response.Body.Close() }()
case methodPut:
response, err = reqClient.DoAPIPut(url, tc.body)
- defer func() { _ = response.Body.Close() }()
case methodDelete:
response, err = reqClient.DoAPIDelete(url, tc.body)
- defer func() { _ = response.Body.Close() }()
}
+ require.NotNil(t, response, tc.identifier())
+ defer func() { _ = response.Body.Close() }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/integrationtests/permissions_test.go` around lines 283 - 300, The
per-branch defers call response.Body.Close() unguarded and can panic if the
request failed and response is nil; update each branch (where
DoAPIGet/DoAPIPost/DoAPIPatch/DoAPIPut/DoAPIDelete are called) to only defer
response.Body.Close() after verifying response != nil (or after checking err ==
nil), e.g. call the API, check err/response and then if response != nil defer
response.Body.Close(), before asserting tc.expectedStatusCode using
response.StatusCode.
| err = th.f.DB().Get(&boardDirectMessage, "SELECT created_by, team_id FROM focalboard_boards WHERE id = 'board-group-channel'") | ||
| require.NoError(t, err) | ||
| require.Equal(t, "user-one", boardDirectMessage.Created_By) | ||
| require.Equal(t, "team-one", boardDirectMessage.Team_ID) | ||
|
|
||
| th.f.RunInterceptor(18) | ||
|
|
||
| th.f.DB().Get(&boardGroupChannel, "SELECT created_by, team_id FROM focalboard_boards WHERE id = 'board-group-channel'") | ||
| err = th.f.DB().Get(&boardGroupChannel, "SELECT created_by, team_id FROM focalboard_boards WHERE id = 'board-group-channel'") | ||
| require.NoError(t, err) | ||
| require.Equal(t, "user-one", boardGroupChannel.Created_By) | ||
| require.Equal(t, "team-three", boardGroupChannel.Team_ID) | ||
|
|
||
| th.f.DB().Get(&boardDirectMessage, "SELECT created_by, team_id FROM focalboard_boards WHERE id = 'board-group-channel'") | ||
| err = th.f.DB().Get(&boardDirectMessage, "SELECT created_by, team_id FROM focalboard_boards WHERE id = 'board-group-channel'") | ||
| require.NoError(t, err) | ||
| require.Equal(t, "user-one", boardDirectMessage.Created_By) | ||
| require.Equal(t, "team-three", boardDirectMessage.Team_ID) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP "board-direct-message|board-group-channel" server/services/store/sqlstore/migrationstests/Repository: mattermost/mattermost-plugin-boards
Length of output: 1361
Fix copy/paste bug: boardDirectMessage should query a different board.
Lines 34 and 46 both query WHERE id = 'board-group-channel', so boardDirectMessage holds the same data as boardGroupChannel. The variable name suggests this should test a distinct direct message board, but currently the second set of assertions (lines 46–49) provide no meaningful coverage. Update the second query to use the correct board ID (likely 'board-direct-message' or similar) to properly exercise both board types.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@server/services/store/sqlstore/migrationstests/deleted_membership_boards_migration_test.go`
around lines 34 - 49, The test has a copy/paste bug: both DB queries for
boardDirectMessage use id = 'board-group-channel' so boardDirectMessage and
boardGroupChannel are identical; update the second th.f.DB().Get call that fills
boardDirectMessage (the one after th.f.RunInterceptor(18)) to query the correct
direct-message board id (e.g., "board-direct-message") so that
boardDirectMessage reflects the direct message board and the subsequent
assertions on boardDirectMessage.Created_By and Team_ID validate the intended
board type.
|
|
||
| os.Setenv("RUDDER_KEY", "mock-test-rudder-key") | ||
| os.Setenv("RUDDER_DATAPLANE_URL", server.URL) | ||
| _ = os.Setenv("RUDDER_DATAPLANE_URL", server.URL) |
There was a problem hiding this comment.
Missing RUDDER_KEY can prevent telemetry from sending and hang this test.
Line 55 sets only RUDDER_DATAPLANE_URL, but telemetry config requires both values; otherwise checkMockRudderServer can block waiting for an event.
💡 Proposed fix
func TestTelemetry(t *testing.T) {
receiveChan, server := mockServer()
+ defer server.Close()
- _ = os.Setenv("RUDDER_DATAPLANE_URL", server.URL)
+ t.Setenv("RUDDER_KEY", "test-rudder-key")
+ t.Setenv("RUDDER_DATAPLANE_URL", server.URL)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _ = os.Setenv("RUDDER_DATAPLANE_URL", server.URL) | |
| func TestTelemetry(t *testing.T) { | |
| receiveChan, server := mockServer() | |
| defer server.Close() | |
| t.Setenv("RUDDER_KEY", "test-rudder-key") | |
| t.Setenv("RUDDER_DATAPLANE_URL", server.URL) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/services/telemetry/telemetry_test.go` at line 55, The test sets only
RUDDER_DATAPLANE_URL which leaves telemetry missing RUDDER_KEY and can make
checkMockRudderServer block; set a dummy RUDDER_KEY environment variable in the
test (alongside RUDDER_DATAPLANE_URL) so telemetry is initialized and the
checkMockRudderServer() path can proceed; update telemetry_test.go to set
os.Setenv("RUDDER_KEY", "<dummy-key>") before invoking checkMockRudderServer
(and optionally restore or unset the env var after the test).
| resp, _ := http.Post(url, "application/json", bytes.NewBuffer(json)) //nolint:gosec | ||
| _, _ = io.ReadAll(resp.Body) | ||
| resp.Body.Close() | ||
| _ = resp.Body.Close() |
There was a problem hiding this comment.
Pre-existing nil-deref risk around the touched line.
http.Post ignores the returned error on line 29 and then unconditionally dereferences resp.Body on lines 30–31. If the POST fails (DNS, connection refused, timeout, malformed URL), resp will be nil and this will panic inside the webhook loop, killing the caller's goroutine.
Since this close is being modified anyway, consider handling the error and nil-checking resp while you're here.
🛡️ Suggested fix
for _, url := range wh.config.WebhookUpdate {
- resp, _ := http.Post(url, "application/json", bytes.NewBuffer(json)) //nolint:gosec
- _, _ = io.ReadAll(resp.Body)
- _ = resp.Body.Close()
-
- wh.logger.Debug("webhook.NotifyUpdate", mlog.String("url", url))
+ resp, err := http.Post(url, "application/json", bytes.NewBuffer(json)) //nolint:gosec
+ if err != nil {
+ wh.logger.Error("webhook.NotifyUpdate: POST failed", mlog.String("url", url), mlog.Err(err))
+ continue
+ }
+ _, _ = io.ReadAll(resp.Body)
+ _ = resp.Body.Close()
+
+ wh.logger.Debug("webhook.NotifyUpdate", mlog.String("url", url))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| resp, _ := http.Post(url, "application/json", bytes.NewBuffer(json)) //nolint:gosec | |
| _, _ = io.ReadAll(resp.Body) | |
| resp.Body.Close() | |
| _ = resp.Body.Close() | |
| resp, err := http.Post(url, "application/json", bytes.NewBuffer(json)) //nolint:gosec | |
| if err != nil { | |
| wh.logger.Error("webhook.NotifyUpdate: POST failed", mlog.String("url", url), mlog.Err(err)) | |
| continue | |
| } | |
| _, _ = io.ReadAll(resp.Body) | |
| _ = resp.Body.Close() | |
| wh.logger.Debug("webhook.NotifyUpdate", mlog.String("url", url)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/services/webhook/webhook.go` around lines 29 - 31, The http.Post call
in the webhook sender ignores its returned error and immediately dereferences
resp.Body (the http.Post call and the subsequent io.ReadAll/resp.Body.Close),
which can nil-deref if the POST fails; update the code around the http.Post call
so you capture and handle the returned error, check that resp != nil before
reading/closing the body, log or return/continue on error, and only call
io.ReadAll and defer resp.Body.Close after confirming resp is non-nil (place the
defer immediately after the nil-check). Ensure you update the symbols in this
file: the http.Post call, the resp variable, io.ReadAll usage, and
resp.Body.Close handling inside the webhook sender loop/function.
|
@CodeRabbit ignore |
✅ Actions performedReviews paused. |
|
@coderabbitai ignore |
✅ Actions performedReviews paused. |
Bumping mattermost-plugin-boards to Patch version 9.2.5 # Conflicts: # .github/workflows/ci.yml # server/app/export.go # server/integrationtests/clienttestlib.go # server/integrationtests/permissions_test.go # server/model/team.go
7 participants
🔒 Add FIPS 140-2 Compliance Support for Mattermost Boards Plugin
Summary
This PR implements FIPS (Federal Information Processing Standards) 140-2 compliance for the Mattermost Boards plugin, enabling secure deployment in government and enterprise environments that require cryptographic validation.
Changes Made
🛠️ FIPS Build Infrastructure
Added FIPS build targets: server-fips, bundle-fips, dist-fips, dist-all
FIPS Docker integration: Uses cgr.dev/mattermost.com/go-msft-fips:1.24.6 for FIPS-validated Go compilation
Dual distribution support: Builds both regular and FIPS-compliant plugin bundles
🔧 Plugin Compatibility Fixes
Binary naming standardization: FIPS bundles now contain correctly named binaries (plugin-linux-amd64) for server compatibility
Fixed broken variable references: Resolved FOCALBOARD_PLUGIN_PATH → BOARD_PLUGIN_PATH inconsistencies
Updated Go version: Upgraded to Go 1.24.6 for latest security patches
Testing
✅ FIPS server compatibility: Tested with FIPS-enabled Mattermost server
✅ Plugin loading: Verified successful installation and startup
✅ Functionality: Confirmed full plugin operation in FIPS mode
✅ Security compliance: Maintains FIPS 140-2 cryptographic standards
Breaking Changes
None - fully backward compatible with existing installations.
Deployment
FIPS-compliant organizations can now deploy the Boards plugin using the -fips bundle variant while maintaining full functionality and security compliance.
Related: Part of broader FIPS compliance initiative across Mattermost plugin ecosystem.
Ticket Link
https://mattermost.atlassian.net/browse/CLD-9417
Change Impact: 🟡 Medium
Regression Risk: The changes introduce moderate regression risk, though several mitigating factors reduce it:
if/elsechains toswitchstatements and rewrite boolean expressions (De Morgan transformations). These are stated to be behaviorally equivalent but introduce code path verification requirements.categoryBoard.Category.IDtocategoryBoard.IDappears to fix a structural access issue, reducing rather than increasing regression risk. The grep results confirmcategoryBoard.IDis the correct field pattern used elsewhere in the codebase.server/api,server/app,server/model, and test files, affecting board management, permissions, and category operations—all critical paths.dist-fips,bundle-fips) and Docker-based compilation paths are additive and isolated to the build system, reducing impact on runtime behavior.FOCALBOARD_PLUGIN_PATH→BOARD_PLUGIN_PATHandPLUGIN_NAME→PLUGIN_IDin artifact naming requires coordinated deployment updates.QA Recommendation: Standard automated test execution should be sufficient as a primary gate, but recommended focused manual validation on:
Generated by CodeRabbitAI