[WEB-2729] chore: updated live server auth cookies handling#5913
[WEB-2729] chore: updated live server auth cookies handling#5913
Conversation
WalkthroughThe changes primarily involve updates to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (5)
live/src/core/types/common.d.ts (1)
6-8: Consider adding JSDoc comments for better documentation.
The type definition looks good, but adding JSDoc comments would improve code documentation and provide better context for other developers.
+/**
+ * Context type for HocusPocus server containing authentication cookie.
+ * Used for maintaining authentication state in WebSocket connections.
+ */
export type HocusPocusServerContext = {
+ /** Authentication cookie string used for maintaining user session */
cookie: string;
};live/src/core/lib/authentication.ts (1)
Line range hint 14-31: Consider adding cookie validation.
The function accepts a cookie string but doesn't validate its format or presence. Consider adding basic validation to ensure the cookie is properly formatted before making the API call.
export const handleAuthentication = async (props: Props) => {
const { cookie, userId } = props;
+ if (!cookie || typeof cookie !== 'string' || cookie.trim() === '') {
+ throw new Error('Invalid or missing authentication cookie');
+ }
// fetch current user info
let response;live/src/core/extensions/index.ts (2)
Line range hint 34-64: Refactor Promise constructor usage to avoid anti-pattern.
The current implementation has two issues:
- Using an async function in Promise constructor is an anti-pattern as it makes error handling unreliable.
- The TODO comment about the ESLint error should be addressed rather than disabled.
Consider refactoring to:
- // TODO: Fix this lint error.
- // eslint-disable-next-line no-async-promise-executor
- return new Promise(async (resolve) => {
- try {
- let fetchedData = null;
- if (documentType === "project_page") {
- fetchedData = await fetchPageDescriptionBinary(
- params,
- pageId,
- cookie,
- );
- } else {
- fetchedData = await fetchDocument({
- cookie,
- documentType,
- pageId,
- params,
- });
- }
- resolve(fetchedData);
- } catch (error) {
- manualLogger.error("Error in fetching document", error);
- }
- });
+ try {
+ if (documentType === "project_page") {
+ return await fetchPageDescriptionBinary(
+ params,
+ pageId,
+ cookie,
+ );
+ }
+ return await fetchDocument({
+ cookie,
+ documentType,
+ pageId,
+ params,
+ });
+ } catch (error) {
+ manualLogger.error("Error in fetching document", error);
+ return null;
+ }Line range hint 66-97: Fix Promise handling in store method.
The current implementation has several issues:
- Using an async function in Promise constructor (same anti-pattern as fetch).
- The Promise never resolves or rejects, which could cause memory leaks.
- Error handling doesn't propagate errors properly.
Consider refactoring to:
- // TODO: Fix this lint error.
- // eslint-disable-next-line no-async-promise-executor
- return new Promise(async () => {
- try {
- if (documentType === "project_page") {
- await updatePageDescription(params, pageId, state, cookie);
- } else {
- await updateDocument({
- cookie,
- documentType,
- pageId,
- params,
- updatedDescription: state,
- });
- }
- } catch (error) {
- manualLogger.error("Error in updating document:", error);
- }
- });
+ try {
+ if (documentType === "project_page") {
+ await updatePageDescription(params, pageId, state, cookie);
+ } else {
+ await updateDocument({
+ cookie,
+ documentType,
+ pageId,
+ params,
+ updatedDescription: state,
+ });
+ }
+ return true;
+ } catch (error) {
+ manualLogger.error("Error in updating document:", error);
+ throw error; // Propagate error to Hocuspocus
+ }live/src/core/hocuspocus-server.ts (1)
Line range hint 50-57: Handle errors more specifically in handleAuthentication
The catch block throws a generic error message "Authentication unsuccessful!". Providing more specific error messages can aid in debugging and improve user feedback. Consider handling different error types to give more informative messages.
Apply this diff to handle errors more specifically:
} catch (error) {
- throw Error("Authentication unsuccessful!");
+ if (error instanceof SomeSpecificError) {
+ throw new Error("Specific error message related to the issue.");
+ } else {
+ throw new Error("Authentication unsuccessful: " + error.message);
+ }
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- live/src/core/extensions/index.ts (6 hunks)
- live/src/core/hocuspocus-server.ts (2 hunks)
- live/src/core/lib/authentication.ts (2 hunks)
- live/src/core/types/common.d.ts (1 hunks)
- packages/editor/src/core/hooks/use-collaborative-editor.ts (1 hunks)
- packages/editor/src/core/types/editor.ts (1 hunks)
🔇 Additional comments (8)
live/src/core/types/common.d.ts (1)
Line range hint 1-4: LGTM!
The union type combining project_page with TAdditionalDocumentTypes is well-structured and provides good type safety.
live/src/core/lib/authentication.ts (2)
14-14: LGTM: Parameter destructuring updated correctly.
The destructuring syntax correctly matches the Props type definition.
10-10: Verify the security implications of using userId instead of token.
The change from token to userId might have security implications. While the cookie provides authentication, using a potentially predictable userId instead of an unpredictable token could make the system more vulnerable to brute force attacks.
packages/editor/src/core/types/editor.ts (1)
141-141: Consider security implications of cookie handling.
The addition of the optional cookie property to TUserDetails is well-structured and maintains backward compatibility. However, since cookies often contain sensitive authentication data:
- Ensure the cookie is only used for authentication purposes and not exposed unnecessarily
- Consider documenting the expected cookie format/content in a comment
- Verify that the cookie is properly sanitized before use
Let's verify the cookie usage across the codebase:
✅ Verification successful
Cookie handling is properly secured in the implementation
Based on the verification results, the cookie implementation is secure and well-handled:
- The cookie is only used in authentication context within
handleAuthenticationfunction - Cookie is properly passed through secure channels:
- Used in authenticated API requests with proper headers
- Stored in WebSocket context for maintaining connection state
- No instances of cookie exposure through logging were found
- Cookie is handled server-side in
hocuspocus-server.tswith proper fallback mechanisms
The implementation aligns with security best practices by:
- Keeping cookie handling on the server side
- Using cookies only for authentication purposes
- Not exposing cookies in client-side code or logs
- Properly validating cookie presence before authentication
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential security issues in cookie handling
# Expected: Cookie should only be used in authentication context
# Test 1: Look for cookie usage in authentication-related code
echo "Checking cookie usage in authentication flows..."
rg -l 'cookie.*authenticate|authenticate.*cookie'
# Test 2: Check for potential cookie exposure in logging or client-side code
echo "Checking for potential cookie exposure..."
rg -l 'console\.log.*cookie|cookie.*console\.log'
Length of output: 303
Script:
#!/bin/bash
# Let's try a broader search to understand cookie usage
# Search for cookie-related code
echo "Searching for cookie usage patterns..."
rg -A 3 -B 3 'cookie' --type ts
# Search for files importing or using TUserDetails
echo "Searching for TUserDetails usage..."
rg -A 3 -B 3 'TUserDetails'
# Look for WebSocket or authentication-related code that might use the cookie
echo "Searching for WebSocket and authentication patterns..."
rg -A 3 'WebSocket|authenticate|authorization' --type ts
Length of output: 20479
live/src/core/extensions/index.ts (2)
20-23: LGTM! Well-organized imports with clear type definitions.
The addition of HocusPocusServerContext and TDocumentTypes types enhances type safety for the cookie handling changes.
119-119: LGTM! Consistent error logging format.
The added commas improve consistency in the logging statements.
Also applies to: 133-133, 138-138
live/src/core/hocuspocus-server.ts (2)
47-47: Verify that context is properly initialized before assignment
When assigning cookie to context, ensure that context is properly initialized and not undefined. If context is undefined, attempting to assign a property to it may result in a runtime error.
52-54: Ensure handleAuthentication receives the correct parameters
Verify that the handleAuthentication function expects an object with cookie and userId. Passing unexpected parameters may lead to unexpected behavior or runtime errors.
| if (response.id !== userId) { | ||
| throw Error("Authentication failed: Token doesn't match the current user."); |
There was a problem hiding this comment.
Update error message to reflect userId instead of token.
The error message still mentions "Token" despite the parameter being changed to userId. This could be confusing for debugging.
- throw Error("Authentication failed: Token doesn't match the current user.");
+ throw Error("Authentication failed: User ID doesn't match the current user.");📝 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.
| if (response.id !== userId) { | |
| throw Error("Authentication failed: Token doesn't match the current user."); | |
| if (response.id !== userId) { | |
| throw Error("Authentication failed: User ID doesn't match the current user."); |
| parameters: realtimeConfig.queryParams, | ||
| // using user id as a token to verify the user on the server | ||
| token: user.id, | ||
| token: JSON.stringify(user), |
There was a problem hiding this comment.
Update useMemo dependencies to match usage.
The dependency array includes user.id, but the hook now depends on the entire user object due to JSON.stringify(user). This could lead to missed updates if other user properties change.
-[id, realtimeConfig, serverHandler, user.id]
+[id, realtimeConfig, serverHandler, user]Also applies to: 64-64
Reconsider sending the entire user object as token.
Sending the complete user object as a token raises several concerns:
- Security: Exposing more user data than necessary could lead to potential information leakage
- Performance: Increased payload size due to serializing the entire user object
- Type safety: No validation on the stringified user structure
Consider creating a minimal token object with only the required fields (e.g., id and cookie).
-token: JSON.stringify(user),
+token: JSON.stringify({ id: user.id, cookie: user.cookie }),📝 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.
| token: JSON.stringify(user), | |
| token: JSON.stringify({ id: user.id, cookie: user.cookie }), |
| try { | ||
| const parsedToken = JSON.parse(token) as TUserDetails; | ||
| userId = parsedToken.id; | ||
| cookie = parsedToken.cookie; | ||
| } catch (error) { | ||
| // If token parsing fails, fallback to request headers | ||
| console.error("Token parsing failed, using request headers:", error); | ||
| } finally { | ||
| // If cookie is still not found, fallback to request headers | ||
| if (!cookie) { | ||
| cookie = requestHeaders.cookie?.toString(); | ||
| } |
There was a problem hiding this comment.
Ensure userId is obtained when token parsing fails
When the parsing of token fails, userId remains undefined because it is only assigned within the try block. As a result, even if cookie is obtained from requestHeaders, the authentication will fail due to the missing userId.
Consider adding a fallback mechanism to obtain userId when token parsing fails. This ensures that authentication can proceed if userId can be retrieved from another source.
| cookie = parsedToken.cookie; | ||
| } catch (error) { | ||
| // If token parsing fails, fallback to request headers | ||
| console.error("Token parsing failed, using request headers:", error); |
There was a problem hiding this comment.
Avoid logging raw errors to prevent sensitive data exposure
Logging the raw error object may expose sensitive information if the error contains details about user input. To prevent potential leakage of sensitive data, consider logging only the error message or a generic message.
Apply this diff to modify the logging statement:
- console.error("Token parsing failed, using request headers:", error);
+ console.error("Token parsing failed, using request headers. Error:", error.message);📝 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.
| console.error("Token parsing failed, using request headers:", error); | |
| console.error("Token parsing failed, using request headers. Error:", error.message); |
1akhanBaheti
changed the title
4 participants
Summary by CodeRabbit
New Features
HocusPocusServerContextand updates toTUserDetails.Bug Fixes
Documentation
Chores
issue attached: WEB-2729