Conversation
nang2049
added
2: Dev Review
📝 WalkthroughWalkthroughCard and board property payloads now receive server-side schema validation before patch application, with API and model tests covering rejected malformed values. Frontend helpers normalize invalid property names and values across board, card, table, and URL rendering. ChangesCard property integrity
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/model/block.go (1)
203-207: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winValidate property values on full block validation.
This path only verifies the outer map type. Unlike
BlockPatch, it can still persist{"properties":{"prop":{}}}through full block creation/update validation. CallValidateCardPropertyValuesafter the type assertion.Proposed fix
if propsIface, present := b.Fields[BlockFieldProperties]; present { - if _, ok := propsIface.(map[string]interface{}); !ok { + props, ok := propsIface.(map[string]interface{}) + if !ok { return ErrBlockPropertiesInvalidType } + if err := ValidateCardPropertyValues(props); err != nil { + return err + } }🤖 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 `@server/model/block.go` around lines 203 - 207, Update full block validation in the block validation method containing the BlockFieldProperties type check to call ValidateCardPropertyValues after the properties map type assertion succeeds. Preserve ErrBlockPropertiesInvalidType for invalid outer values, and reject invalid nested property values before full block creation or update proceeds.
🤖 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 `@server/api/cards.go`:
- Around line 300-303: In the handler flow before calling patch.CheckValid,
detect when the unmarshaled patch is nil, including a JSON null body, and return
a 400 Bad Request through a.errorResponse. Keep the existing validation and
error handling unchanged for non-nil patches.
In `@webapp/src/properties/url/url.tsx`:
- Around line 29-30: Initialize the URL editor’s value from the sanitized
propertyValue rather than the raw card property, ensuring Editable always
receives a string. Update the relevant URL editor state initialization near
safePropertyString, and add a regression test covering edit mode with a
malformed persisted URL object or array.
---
Outside diff comments:
In `@server/model/block.go`:
- Around line 203-207: Update full block validation in the block validation
method containing the BlockFieldProperties type check to call
ValidateCardPropertyValues after the properties map type assertion succeeds.
Preserve ErrBlockPropertiesInvalidType for invalid outer values, and reject
invalid nested property values before full block creation or update proceeds.
🪄 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: 3c3949e1-5f7d-4140-9cf5-072e5c700a49
📒 Files selected for processing (14)
server/api/cards.goserver/integrationtests/board_test.goserver/integrationtests/cards_test.goserver/model/block.goserver/model/board.goserver/model/card.goserver/model/card_property.goserver/model/card_property_test.gowebapp/src/blocks/board.test.tswebapp/src/blocks/board.tswebapp/src/components/cardDetail/cardDetailProperties.tsxwebapp/src/components/propertyValueElement.tsxwebapp/src/components/table/tableHeaders.tsxwebapp/src/properties/url/url.tsx
Comment on lines
+300
to
+303
| if err = patch.CheckValid(); err != nil { | ||
| a.errorResponse(w, r, model.NewErrBadRequest(err.Error())) | ||
| return | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## cards.go excerpt\n'
nl -ba server/api/cards.go | sed -n '250,320p'
printf '\n## model patch type and methods\n'
rg -n "type CardPatch|func \\(.*CardPatch.*CheckValid|CheckValid\\(" server model -g '*.go'
printf '\n## surrounding model file excerpt\n'
file=$(rg -l "type CardPatch" server model -g '*.go' | head -n 1)
if [ -n "${file:-}" ]; then
nl -ba "$file" | sed -n '1,220p'
fiRepository: mattermost/mattermost-plugin-boards
Length of output: 234
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## cards.go excerpt\n'
python3 - <<'PY'
from pathlib import Path
p = Path('server/api/cards.go')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 250 <= i <= 320:
print(f"{i:4d}: {line}")
PY
printf '\n## model patch type and methods\n'
rg -n "type CardPatch|func \\(.*CardPatch.*CheckValid|CheckValid\\(" server model -g '*.go'
printf '\n## candidate file contents\n'
file=$(rg -l "type CardPatch" server model -g '*.go' | head -n 1)
if [ -n "${file:-}" ]; then
python3 - <<'PY'
from pathlib import Path
import os
file = os.environ['file']
for i, line in enumerate(Path(file).read_text().splitlines(), 1):
if i <= 220:
print(f"{i:4d}: {line}")
PY
fiRepository: mattermost/mattermost-plugin-boards
Length of output: 3351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## cards.go excerpt\n'
python3 - <<'PY'
from pathlib import Path
p = Path('server/api/cards.go')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 250 <= i <= 320:
print(f"{i:4d}: {line}")
PY
printf '\n## model patch type and methods\n'
rg -n "type CardPatch|func \\(.*CardPatch.*CheckValid|CheckValid\\(" server model -g '*.go'
printf '\n## candidate file contents\n'
file=$(rg -l "type CardPatch" server model -g '*.go' | head -n 1)
if [ -n "${file:-}" ]; then
FILE="$file" python3 - <<'PY'
from pathlib import Path
import os
file = os.environ['FILE']
for i, line in enumerate(Path(file).read_text().splitlines(), 1):
if i <= 220:
print(f"{i:4d}: {line}")
PY
fiRepository: mattermost/mattermost-plugin-boards
Length of output: 3351
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('server/model/card.go')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 145 <= i <= 230 or 280 <= i <= 320:
print(f"{i:4d}: {line}")
PYRepository: mattermost/mattermost-plugin-boards
Length of output: 4260
Reject null before validating the patch.
json.Unmarshal leaves patch nil for a JSON null, so patch.CheckValid() panics here. Return 400 Bad Request when the body is null.
🤖 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 `@server/api/cards.go` around lines 300 - 303, In the handler flow before
calling patch.CheckValid, detect when the unmarshaled patch is nil, including a
JSON null body, and return a 400 Bad Request through a.errorResponse. Keep the
existing validation and error handling unchanged for non-nil patches.
Comment on lines
+29
to
+30
| const propertyValue = safePropertyString(props.propertyValue) | ||
| const isEmpty = !propertyValue.trim() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Initialize the URL editor state from the sanitized value.
propertyValue is safe, but value is still initialized from the raw card property immediately above. A corrupted object or array can therefore reach Editable as a non-string and bypass the new safety boundary.
Proposed fix
- const [value, setValue] = useState(props.card.fields.properties[props.propertyTemplate.id || ''] || '')
+ const [value, setValue] = useState(safePropertyString(props.propertyValue))Add a regression test for entering edit mode with a malformed persisted URL value.
🤖 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 `@webapp/src/properties/url/url.tsx` around lines 29 - 30, Initialize the URL
editor’s value from the sanitized propertyValue rather than the raw card
property, ensuring Editable always receives a string. Update the relevant URL
editor state initialization near safePropertyString, and add a regression test
covering edit mode with a malformed persisted URL object or array.
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
1 participant
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
A board Editor could persist a non-string value into a card property name or a card property value which the
webapp then rendered directly as a React child. The result was a permanent crash for every user opening the affected board or card. This PR adds one shared validator in
server/model/card_property.gothat requires property values to be strings or arrays of strings, and property templates to have a non-empty stringidplus stringname,typeandoptionsfields.BoardPatch.IsValid/Patchthe originally reported endpointCardPatch.CheckValid/PatchandhandlePatchCardthe instance found during review now returning 400 instead of 500ValidateBlockPatchPATCH /boards/{id}/blocks/{id}, the endpoint the webapp itself uses to edit property values, otherwise the card fix is trivially bypassableBoard.IsValid/Card.CheckValidthe matching create endpointsAs defense in depth, and to make boards that are corrupted openable again, the webapp coerces property names and values before rendering them via
safePropertyString/safePropertyValueinblocks/board.ts, applied centrally inpropertyValueElement.tsxpluscardDetailProperties.tsx,tableHeaders.tsxandurl.tsx.Manual QA Steps
Setup: User A (system admin) creates a board with a text property, adds a card, and shares the board with User B (regular user) as Editor. All requests below are sent as User B.
1. Board card property name is rejected
Expect 400 Bad Request (was 200). Confirm with a
GETon the board thatnameis stillthe original string, then open the board as User A: it loads normally in table and card view.
2. Card property value via the cards API is rejected
Expect 400 Bad Request.
GETthe card and confirm the previous value is intact.3. Card property value via the blocks API is rejected
Expect 400 Bad Request.
Ticket Link
https://mattermost.atlassian.net/browse/MM-69978
Change Impact: 🟡 Medium
Regression Risk: Validation affects shared persistence and rendering paths across server and webapp layers; malformed data handling is covered, but valid edge cases could regress.
QA Recommendation: Manual QA can be skipped given comprehensive automated coverage; targeted smoke testing is optional.
Generated by CodeRabbitAI