# Fix OAuth callback crash on malformed state parameter #1285
# Fix OAuth callback crash on malformed state parameter #12857vignesh wants to merge 1 commit intoRocketChat:developfrom
state parameter #1285Conversation
Fixes RocketChat#1284 The OAuth callback endpoint crashes when receiving a malformed query parameter because decodeURIComponent throws an unhandled URIError. This breaks the entire OAuth callback flow and login for affected users. Changes: - Add type validation for state parameter (must be string) - Wrap decodeURIComponent in try/catch to catch URIError - Return controlled 400 response with user-friendly error message instead of crashing endpoint - Extract CSP header to constant to reduce duplication The endpoint now gracefully handles both missing and malformed state values by returning a safe callback HTML response with appropriate error messaging.
There was a problem hiding this comment.
Pull request overview
Fixes a crash in the rc-app OAuth callback endpoint when the state query param is missing or malformed by validating the type and guarding decodeURIComponent, returning a controlled 400 HTML response instead of throwing.
Changes:
- Validate
stateis a string before decoding. - Wrap
decodeURIComponent(state)intry/catchand return a 400 error page on failure. - Deduplicate the CSP header value via a local constant.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const contentSecurityPolicy = | ||
| "default-src 'self'; script-src 'self' 'unsafe-inline'; connect-src 'self'; img-src 'self' data:; style-src 'self' 'unsafe-inline'; frame-src 'self'; font-src 'self'; object-src 'none'"; | ||
|
|
There was a problem hiding this comment.
contentSecurityPolicy is created on every request even though it’s a true constant. Consider moving it to a module-level const (e.g., CONTENT_SECURITY_POLICY) to avoid repeated allocation and make the intent clearer.
| if (typeof state !== "string") { | ||
| return { | ||
| status: 400, | ||
| content: await getCallbackContent( | ||
| read, | ||
| null, | ||
| "", | ||
| "Invalid state parameter" | ||
| ), | ||
| headers: { | ||
| "Content-Security-Policy": contentSecurityPolicy, | ||
| }, | ||
| }; | ||
| } | ||
|
|
||
| let origin; | ||
| try { | ||
| origin = decodeURIComponent(state); | ||
| } catch (e) { | ||
| return { | ||
| status: 400, | ||
| content: await getCallbackContent( | ||
| read, | ||
| null, | ||
| "", | ||
| "Invalid state parameter" | ||
| ), | ||
| headers: { | ||
| "Content-Security-Policy": contentSecurityPolicy, | ||
| }, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The 400-response blocks for invalid/malformed state are duplicated. Consider extracting a small helper (e.g., buildInvalidStateResponse()) to return the common { status, content, headers } shape so future changes (message, headers, etc.) only need to be made once.
| let origin; | ||
| try { | ||
| origin = decodeURIComponent(state); | ||
| } catch (e) { |
There was a problem hiding this comment.
let origin; introduces an implicit any and catch (e) declares an unused variable. Prefer typing origin as string and using an optional catch binding (catch { ... }) or _e to avoid unused-variable lint warnings.
| let origin; | |
| try { | |
| origin = decodeURIComponent(state); | |
| } catch (e) { | |
| let origin: string; | |
| try { | |
| origin = decodeURIComponent(state); | |
| } catch { |
Fixes #1284
The OAuth callback endpoint crashes when receiving a malformed
statequery parameter becausedecodeURIComponentthrows an unhandledURIError. This breaks the entire OAuth callback flow and login for affected users.Changes
decodeURIComponentin try/catch to catchURIErrorThe endpoint now gracefully handles both missing and malformed state values by returning a safe callback HTML response with appropriate error messaging.
Testing
Manual Testing:
/api/apps/public/{appId}/callback?code=valid_code&state=%E0%A4%ARegression Testing: