-
Notifications
You must be signed in to change notification settings - Fork 3.6k
[WEB-2729] chore: updated live server auth cookies handling #5913
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
f777631
f5b0fa0
dcafd06
29cea39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,10 @@ import { v4 as uuidv4 } from "uuid"; | |
| import { handleAuthentication } from "@/core/lib/authentication.js"; | ||
| // extensions | ||
| import { getExtensions } from "@/core/extensions/index.js"; | ||
| // editor types | ||
| import { TUserDetails } from "@plane/editor"; | ||
| // types | ||
| import { type HocusPocusServerContext } from "@/core/types/common.js"; | ||
|
|
||
| export const getHocusPocusServer = async () => { | ||
| const extensions = await getExtensions(); | ||
|
|
@@ -12,20 +16,40 @@ export const getHocusPocusServer = async () => { | |
| name: serverName, | ||
| onAuthenticate: async ({ | ||
| requestHeaders, | ||
| context, | ||
| // user id used as token for authentication | ||
| token, | ||
| }) => { | ||
| // request headers | ||
| const cookie = requestHeaders.cookie?.toString(); | ||
| let cookie: string | undefined = undefined; | ||
| let userId: string | undefined = undefined; | ||
|
|
||
| if (!cookie) { | ||
| throw Error("Credentials not provided"); | ||
| // Extract cookie (fallback to request headers) and userId from token (for scenarios where | ||
| // the cookies are not passed in the request headers) | ||
| 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(); | ||
| } | ||
|
Comment on lines
+28
to
+39
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure When the parsing of Consider adding a fallback mechanism to obtain |
||
| } | ||
|
|
||
| if (!cookie || !userId) { | ||
| throw new Error("Credentials not provided"); | ||
| } | ||
|
|
||
| // set cookie in context, so it can be used throughout the ws connection | ||
| (context as HocusPocusServerContext).cookie = cookie; | ||
|
|
||
| try { | ||
| await handleAuthentication({ | ||
| cookie, | ||
| token, | ||
| userId, | ||
| }); | ||
| } catch (error) { | ||
| throw Error("Authentication unsuccessful!"); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||
|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,11 +7,11 @@ const userService = new UserService(); | |||||||||
|
|
||||||||||
| type Props = { | ||||||||||
| cookie: string; | ||||||||||
| token: string; | ||||||||||
| userId: string; | ||||||||||
| }; | ||||||||||
|
|
||||||||||
| export const handleAuthentication = async (props: Props) => { | ||||||||||
| const { cookie, token } = props; | ||||||||||
| const { cookie, userId } = props; | ||||||||||
| // fetch current user info | ||||||||||
| let response; | ||||||||||
| try { | ||||||||||
|
|
@@ -20,7 +20,7 @@ export const handleAuthentication = async (props: Props) => { | |||||||||
| manualLogger.error("Failed to fetch current user:", error); | ||||||||||
| throw error; | ||||||||||
| } | ||||||||||
| if (response.id !== token) { | ||||||||||
| if (response.id !== userId) { | ||||||||||
| throw Error("Authentication failed: Token doesn't match the current user."); | ||||||||||
|
Comment on lines
+23
to
24
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update error message to reflect userId instead of token. The error message still mentions "Token" despite the parameter being changed to - 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
Suggested change
|
||||||||||
| } | ||||||||||
|
|
||||||||||
|
|
||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -39,7 +39,7 @@ export const useCollaborativeEditor = (props: TCollaborativeEditorProps) => { | |||||
| name: id, | ||||||
| parameters: realtimeConfig.queryParams, | ||||||
| // using user id as a token to verify the user on the server | ||||||
| token: user.id, | ||||||
| token: JSON.stringify(user), | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Update useMemo dependencies to match usage. The dependency array includes -[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:
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
Suggested change
|
||||||
| url: realtimeConfig.url, | ||||||
| onAuthenticationFailed: () => { | ||||||
| serverHandler?.onServerError?.(); | ||||||
|
|
||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid logging raw errors to prevent sensitive data exposure
Logging the raw
errorobject 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:
📝 Committable suggestion