Conversation
mm-cloud-bot
added
the
release-note
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughHTML endpoint handlers now detect requests expecting JSON and return a localized 406 JSON error. Magic-link, OAuth, and SAML routes are wired through HTML-specific handler constructors, with coverage for header parsing and rejection behavior. ChangesHTML endpoint negotiation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Handler.ServeHTTP
participant shouldRejectJSONRequest
participant WriteHTMLEndpointJSONRequestError
Client->>Handler.ServeHTTP: Request HTML endpoint with Accept application/json
Handler.ServeHTTP->>shouldRejectJSONRequest: Evaluate handler and client conditions
shouldRejectJSONRequest-->>Handler.ServeHTTP: Reject request
Handler.ServeHTTP->>WriteHTMLEndpointJSONRequestError: Write localized 406 JSON response
WriteHTMLEndpointJSONRequestError-->>Client: Return application/json error payload
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" 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: 1
🤖 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/channels/web/handlers_test.go`:
- Around line 64-90: Add table-driven coverage in
TestHandlerRejectsJSONRequestOnHTMLEndpoint for both shouldRejectJSONRequest
bypasses: an OAuth API request and a request carrying the X-Mobile-App header.
Assert each request reaches the wrapped HTML handler and returns HTTP 200, while
retaining the existing baseline rejection assertions.
🪄 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: bdd71a3d-20ce-4bcc-95a2-d7dbe2edab94
📒 Files selected for processing (9)
server/channels/utils/accept.goserver/channels/utils/accept_test.goserver/channels/utils/api.goserver/channels/web/handlers.goserver/channels/web/handlers_test.goserver/channels/web/magic_link.goserver/channels/web/oauth.goserver/channels/web/saml.goserver/i18n/en.json
|
@devinbinnie Asking your review specially because this might be also useful for desktop. |
devinbinnie
left a comment
devinbinnie
left a comment
There was a problem hiding this comment.
No issues here, I think this should be tested with Desktop App before merge to make sure it isn't rejecting its requests, or we'd have to do some backwards compatibility stuff for older versions.
As far as I can tell, Electron.Net library doesn't prescribe a Content-Type so we should be fine, but we should verify before merging.
devinbinnie
added
4: Reviews Complete
|
@devinbinnie Just did a quick test, and seems to be working just fine. |
|
/update-branch |
|
/update-branch |
Documentation Impact Analysis — updates neededDocumentation Impact AnalysisOverall Assessment: Documentation Updates Recommended Changes SummaryThis PR adds a new request-rejection mechanism so that HTML-returning endpoints ( Documentation Impact Details
Recommended Actions
ConfidenceMedium — The new HTTP 406 response contract and the |
lieut-data
left a comment
lieut-data
left a comment
There was a problem hiding this comment.
A couple of comments below.
Should we consider applying a feature flag to this functionality before shipping?
| method: http.MethodGet, | ||
| url: "http://localhost:8065/login/sso/saml", | ||
| headers: map[string]string{ | ||
| "Accept": "application/json", |
There was a problem hiding this comment.
Does mobile actually send application/json for these HTML-only endpoints?
There was a problem hiding this comment.
No, but we have to test the logic for coverage and make sure these don't reject.
There was a problem hiding this comment.
Ah, so this is about testing /login/sso/saml forcing HTML. Can we clarify the name to emphasize this?
| // Some OAuth routes live outside /api/v4/ but are JSON API endpoints (token exchange, | ||
| // authorize POST, etc.), not browser pages. Skip rejection for those callers. | ||
| if IsOAuthAPICall(c.App, r) { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm guessing we can't use the HTMLHandler because of the mobile comment below?
There was a problem hiding this comment.
That is my guess also.
There was a problem hiding this comment.
Sorry, looking for clarity -- can you elaborate on why your code can't simply rely on the HTMLHandler always doing the right thing, but we need to test X-Mobile-App explicitly?
There was a problem hiding this comment.
The code is prepared so certain HTML based endpoints (mostly oAuth) are able to return json like errors. handleContextError handles this by looking at certain things:
- webhook
- API calls
- Oauth endpoints
- Mobile header
Webhook and API calls return by default json, so we don't have to do anything extra. They can be handled by isHTML === false.
OAuth Endpoints (which is also where the Mobile header should be used) are HTML endpoints (isHTML === true), but they may return a JSON object if they fail.
So this code is being extremely cautious. Even if it returns a json body with the error, the request is probably not expecting a json. But still, in the hopes of trying not to change existing behavior, we do these checks.
Now, what is the Mobile header? The mobile header was added 8 years ago in this PR: #8943 . I don't see this header in today's mobile code, therefore my guess this is related to the legacy mobile app, V1, or just very old versions, rendering this dead code.
| // (redirects, session tokens). This matches existing error-handling behavior. | ||
| if r.Header.Get("X-Mobile-App") != "" { | ||
| return false | ||
| } |
There was a problem hiding this comment.
To clarify, are mobile clients getting different responses than non-mobile clients?
There was a problem hiding this comment.
I wouldn't be surprised if this is some kind of legacy code for the old mobile app (not even v1). I am being careful just to keep the current behavior, based on Cursor recommendations.
There was a problem hiding this comment.
Ah, I'm looking for more than Cursor's recommendations here. Can you investigate the actual behaviour and update the code to reflect the requirements?
| BaseURL string `json:"base_url"` | ||
| }{ | ||
| Id: "api.context.html_endpoint_json_request.app_error", | ||
| Message: translateFunc("api.context.html_endpoint_json_request.app_error", map[string]any{"BaseURL": baseURL}), |
There was a problem hiding this comment.
Do we need to embed the base URL twice, or should we have a more generic error message?
There was a problem hiding this comment.
More generic message work too. Cursor added it and I thought it was a nice to have, but 0/5.
| method: http.MethodGet, | ||
| url: "http://localhost:8065/login/sso/saml", | ||
| headers: map[string]string{ | ||
| "Accept": "application/json", |
There was a problem hiding this comment.
Ah, so this is about testing /login/sso/saml forcing HTML. Can we clarify the name to emphasize this?
| // Some OAuth routes live outside /api/v4/ but are JSON API endpoints (token exchange, | ||
| // authorize POST, etc.), not browser pages. Skip rejection for those callers. | ||
| if IsOAuthAPICall(c.App, r) { | ||
| return false | ||
| } | ||
|
|
There was a problem hiding this comment.
Sorry, looking for clarity -- can you elaborate on why your code can't simply rely on the HTMLHandler always doing the right thing, but we need to test X-Mobile-App explicitly?
| // (redirects, session tokens). This matches existing error-handling behavior. | ||
| if r.Header.Get("X-Mobile-App") != "" { | ||
| return false | ||
| } |
There was a problem hiding this comment.
Ah, I'm looking for more than Cursor's recommendations here. Can you investigate the actual behaviour and update the code to reflect the requirements?
5 participants
Summary
When a request with an accept header JSON, we still serve html if it hits an html only endpoint. We want to avoid that, among other things, to better handle from the clients when adding a new server.
We also add to the error what is the base url of the server, so we can improve how we handle adding a server in the clients.
Related PR
Mobile mattermost/mattermost-mobile#9922
Ticket Link
Fix https://mattermost.atlassian.net/browse/MM-67557
Release Note
Change Impact: 🟠 Medium
Regression Risk: This changes the HTTP response contract for multiple authentication-related HTML endpoints when
Accept: application/jsonis present (switching from HTML to a406JSON error), which may affect clients that assumed HTML was returned. Risk is mitigated by explicit exclusions for OAuth and mobile requests and by added handler-focused tests, but coverage for every impacted route/client variation may be incomplete.QA Recommendation: Targeted manual QA for SAML, magic-link, OAuth completion/login/signup flows, and at least one representative HTML login endpoint using both browser-style
AcceptandAccept: application/json, including mobile client behavior.