regression: channels.list.joined rejects the fields query param - #41979
Conversation
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Looks like this PR is ready to merge! 🎉 |
|
Hacktron Security Check - SkippedReason: OSS PR review limit reached for this approved repository and developer. New OSS PRs for this repository will resume at the start of the next cycle.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
🧰 Additional context used📓 Path-based instructions (2)The main Rocket.Chat Meteor application resides in `apps/meteor/`; place its application code there rather than in other monorepo areas.📄 CodeRabbit inference engine (CLAUDE.md) Files:
Write concise, technical TypeScript/JavaScript with accurate typing in Playwright tests📄 CodeRabbit inference engine (.cursor/rules/playwright.mdc) Files:
🔇 Additional comments (2)
WalkthroughThe ChangesJoined channels projection
Estimated code review effort: 1 (Trivial) | ~5 minutes Merge Risk: ⚪ Minimal · up to This localized fix restores the supported fields parameter for joined-channel requests without broadening channel access or weakening validation, so no actionable merge-blocking risk remains after normal checks and review. Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Full details: Docstring CoverageExplanation No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 2 files. Warning Errors were encountered while retrieving linked issues. Errors (1)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.8.0 #41979 +/- ##
================================================
Coverage ? 69.38%
================================================
Files ? 4254
Lines ? 168901
Branches ? 30150
================================================
Hits ? 117184
Misses ? 46560
Partials ? 5157
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Proposed changes (including videos or screenshots)
GET /api/v1/channels.list.joinedstarted returning HTTP 400 (must NOT have additional properties) whenever thefieldsquery param was provided, even withALLOW_UNSAFE_QUERY_AND_FIELDS_API_PARAMS=true.The typed-route migration in #41415 (b3eee9b) replaced the endpoint's loose params with a strict
ajvQueryschema usingadditionalProperties: false, but the property list omittedfields— while the handler still reads it:So the param was rejected at validation before the handler could apply (or ignore) it.
This adds
fieldsback to thechannels.list.joinedquery schema, restoring the 8.7.x behavior until the param's scheduled removal in 9.0.0:fieldsignored;fieldsapplied as the projection, with the existing deprecation warning.additionalProperties: falseis kept, so genuinely unknown params still get a 400.No changeset: this is a regression against unreleased 8.8.0 code, not a change end users saw in a released version.
Issue(s)
https://rocketchat.atlassian.net/browse/CORE-2644
Regression from #41415.
Steps to test or reproduce
Before: HTTP 400
must NOT have additional properties. After: HTTP 200.Automated coverage added in
apps/meteor/tests/end-to-end/api/channels.tsunder[/channels.list.joined]:fieldsalone is accepted (200 instead of 400);fieldstogether with every other supported param (roomId,roomName,_id,query,sort,count,offset) is accepted, guarding the whole property list rather than the single key added;must NOT have additional properties, proving the schema was not simply loosened.The tests assert acceptance and response shape rather than the projection itself, since
parseJsonQueryonly honorsfieldswhenALLOW_UNSAFE_QUERY_AND_FIELDS_API_PARAMS=TRUE, which the e2e suite does not set.Further comments
channels.listhas the same shape — itsisChannelsListPropsschema also omitsfieldswhile the handler reads it viaparseJsonQuery. Left out of this PR to keep the regression fix scoped; worth a follow-up sweep over the other endpoints migrated in #41415.Summary by CodeRabbit
New Features
Bug Fixes
CORE-2644