feat: handle cookie based login #269
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces comprehensive changes to cookie management and login handling across multiple components of the application. The modifications span from the core interpreter to the frontend, adding support for tracking login states, managing cookies, and enhancing workflow execution. Key improvements include new interfaces for cookie storage, methods for fetching and setting cookies, and UI components to specify login requirements when creating robots. Changes
Sequence DiagramsequenceDiagram
participant User
participant Frontend
participant Backend
participant Interpreter
participant Browser
User->>Frontend: Create Robot
Frontend->>Backend: Save Workflow
Backend->>Backend: Store Robot with Login State
User->>Frontend: Start Robot Run
Frontend->>Backend: Initiate Run
Backend->>Interpreter: Execute Workflow
Interpreter->>Browser: Fetch Stored Cookies
Browser-->>Interpreter: Return Cookies
Interpreter->>Browser: Apply Cookies
Interpreter->>Browser: Execute Workflow
Browser-->>Interpreter: Workflow Results
Interpreter->>Backend: Store Cookies
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
🪧 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
🧹 Nitpick comments (15)
server/src/workflow-management/classes/Interpreter.ts (3)
7-8: Consider verifying Axios error handling options.
You are importing Axios as the default export; ensure that you configure timeouts and error interceptors if necessary for production robustness.
258-268: Add detailed error logging or rethrow.
Currently, the method swallows errors by pushing messages to debug logs. Consider rethrowing exceptions or returning them so the caller can decide how to handle them.
359-359: Log structure is correct, but watch out for PII in debug messages.
Ensure this.debugMessages doesn’t inadvertently log sensitive data like passwords.server/src/api/record.ts (1)
583-583: Avoid assignment within the parameter list for maintainability.
The expression “(newPage: Page) => currentPage = newPage” can be confusing. Extract the assignment to a separate statement to clarify intent.- (newPage: Page) => currentPage = newPage + (newPage: Page) => { + currentPage = newPage; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 583-583: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
maxun-core/src/interpret.ts (4)
151-161: filterValidCookies method is short and effective.
Be mindful of whether you want partial logging or difference logging in production.
163-174: applyStoredCookies error handling.
Returning false on failure is good. Consider additional logs or telemetry if a repeated failure scenario emerges.
188-193: isLoginUrl check is simplistic, but covers typical patterns.
For advanced detection, consider using domain-based or script-based checks.
Line range hint
583-583: Consider if you still need filterValidCookies.
It’s currently commented out, but you might want to re-enable it to remove expired cookies once the login is done.server/src/models/Robot.ts (3)
18-27: LGTM! Consider adding cookie validation.The
StoredCookieinterface correctly defines all standard cookie attributes. The security-related fields are appropriately marked as optional.Consider adding validation for the
sameSiteattribute when cookies are stored, as "None" requires thesecureflag to be true in modern browsers:export function validateCookie(cookie: StoredCookie): void { if (cookie.sameSite === "None" && !cookie.secure) { throw new Error("SameSite=None requires the Secure attribute"); } }
29-32: Consider using Date type for lastUpdated.While using number for timestamps is common, using Date type would provide better type safety and clarity.
interface CookieStorage { cookies: StoredCookie[]; - lastUpdated: number; + lastUpdated: Date; }
122-130: Consider adding an index for isLogin.The model initialization looks good. Since
isLoginmight be used for filtering robots, consider adding an index to improve query performance.isLogin: { type: DataTypes.BOOLEAN, allowNull: false, defaultValue: false, + index: true, // Add index for better query performance },server/src/workflow-management/scheduler/index.ts (1)
129-129: Refactor assignment in expression.The assignment within the callback makes the code harder to maintain. Consider extracting it to a separate statement.
- workflow, currentPage, (newPage: Page) => currentPage = newPage, plainRun.interpreterSettings, plainRun.robotMetaId + workflow, currentPage, (newPage: Page) => { + currentPage = newPage; + }, plainRun.interpreterSettings, plainRun.robotMetaId🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
src/components/molecules/RecordingsTable.tsx (1)
313-314: Enhance accessibility for the radio group.Consider adding a more descriptive
aria-labelfor better screen reader support.- aria-labelledby="login-requirement-radio-group" + aria-label="Select whether the site requires login credentials"server/src/routes/storage.ts (1)
95-122: LGTM! Consider adding JSDoc documentation.The GET endpoint for cookie retrieval is well-implemented with proper error handling and logging.
Add JSDoc documentation to describe the response structure:
/** * GET endpoint for retrieving cookies for a specific robot. * @route GET /recordings/:id/cookies * @param {string} id.path.required - Robot ID * @returns {Object} 200 - Cookie storage object * @returns {Object} 404 - Robot not found error * @returns {Object} 500 - Server error */server/src/workflow-management/classes/Generator.ts (1)
Line range hint
501-516: LGTM! Consider adding parameter documentation.The method correctly handles the isLogin parameter for robot creation.
Add parameter documentation:
/** * Creates a recording metadata and stores the current workflow * with the metadata to the file system. * @param fileName The name of the file * @param userId The ID of the user creating the workflow * @param isLogin Whether this workflow represents a login sequence * @returns {Promise<void>} */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
maxun-core/src/interpret.ts(9 hunks)maxun-core/src/preprocessor.ts(1 hunks)maxun-core/src/types/workflow.ts(1 hunks)server/src/api/record.ts(1 hunks)server/src/models/Robot.ts(4 hunks)server/src/routes/storage.ts(4 hunks)server/src/workflow-management/classes/Generator.ts(3 hunks)server/src/workflow-management/classes/Interpreter.ts(5 hunks)server/src/workflow-management/scheduler/index.ts(1 hunks)src/components/molecules/RecordingsTable.tsx(3 hunks)src/components/molecules/SaveRecording.tsx(2 hunks)src/context/globalInfo.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
server/src/workflow-management/scheduler/index.ts
[error] 129-129: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
server/src/routes/storage.ts
[error] 526-526: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
server/src/api/record.ts
[error] 583-583: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (27)
server/src/workflow-management/classes/Interpreter.ts (5)
9-23: Cookie interfaces look good.
The definitions are comprehensive, covering properties like secure, httpOnly, sameSite, etc.
270-279: Include success/failure in HTTP response checks.
Similar to fetchCookies, consider verifying the HTTP status code from the PUT operation to ensure cookies are successfully stored before logging success.
291-292: Good addition of optional robotId parameter.
This preserves backward compatibility while allowing cookie fetching for certain flows when a robotId is provided.
294-297: Check for nullish return in cookies.
If fetchCookies fails or returns undefined, you might encounter runtime issues when spreading or using the result. Consider handling a null/undefined return more defensively.
327-327: Passing cookies to Interpreter is a neat approach.
This injection allows consistent cookie handling throughout. Consider verifying that the same cookie data structure is used in all references.
maxun-core/src/interpret.ts (11)
18-19: RegexableString import is consistent with your usage.
Ensures type consistency across modules.
20-34: Cookie interfaces are consistent with the server implementation.
They match the structure in the server’s code, promoting type-safety across boundaries.
88-91: Initialize cookies to avoid null checks.
Good practice for robust default cookie data.
93-94: loginSuccessful flag is helpful.
This boolean is straightforward and eliminates the need for repeated checks of an external state.
95-97: Constructor usage of partial options is sound.
Ensures fallback defaults and integrates new cookies parameter gracefully.
137-149: Cookie expiry logic is correct.
Properly handles real timestamps and large values, ensuring cookies persist as intended.
194-208: getUrlString function.
Regex transformation is comprehensive. Confirm the removal of ^, $, and ? matches your intended patterns.
210-221: findFirstPostLoginAction logic.
Reversing from the end is a neat approach. This efficiently identifies the last non-login step.
326-346: Cookie extraction upon successful login is well-placed.
Storing cookies and resetting loginSuccessful ensures future runs can skip login if needed.
776-803: Applying stored cookies before workflow execution is beneficial.
Adding more robust fallback logic if the goto fails might improve user experience in partial connectivity scenarios.
Line range hint 915-950: Return of CookieData is consistent with server usage.
Streamlines the handshake with the server’s setCookies method.
maxun-core/src/types/workflow.ts (1)
14-14: Exporting RegexableString broadens reusability.
This aligns with your usage in interpret.ts and fosters type consistency across modules.
server/src/models/Robot.ts (1)
45-46: LGTM! Attributes align with cookie-based login feature.
The new attributes are well-structured:
isLoginflag clearly indicates if the robot requires authenticationcookie_storageis correctly marked as optional since not all robots require authentication
src/context/globalInfo.tsx (3)
25-26: LGTM! Interface changes follow existing patterns.
The addition of isLogin and its setter method follows the established pattern in the interface.
53-53: LGTM! Default value aligns with model.
The default value of false for isLogin matches the database model's default value, maintaining consistency.
76-76: LGTM! Context implementation follows React best practices.
The implementation of isLogin state and its inclusion in the context value follows React's best practices for state management.
Also applies to: 116-117
src/components/molecules/SaveRecording.tsx (1)
24-24: LGTM! Context usage is correct.
The addition of isLogin to the destructured values from useGlobalInfoStore follows the established pattern.
server/src/workflow-management/scheduler/index.ts (1)
129-129: Verify robotMetaId usage in interpreter.
The addition of robotMetaId to InterpretRecording appears to be part of cookie-based login implementation. Let's verify its usage in the interpreter.
✅ Verification successful
Based on the search results, I can see that robotMetaId is used as part of a cookie-based authentication system where:
- The interpreter class in
server/src/workflow-management/classes/Interpreter.tsusesrobotMetaIdto fetch and store cookies for robots. - The cookies are stored in the database through the Robot model (
server/src/models/Robot.ts). - The storage routes (
server/src/routes/storage.ts) handle cookie retrieval and updates for robots.
The addition of robotMetaId to InterpretRecording is a necessary change to support cookie persistence between workflow runs, allowing the system to maintain login states. This is verified by the codebase showing a complete implementation of cookie management for robots.
The robotMetaId parameter addition is correctly implemented and necessary for cookie-based authentication.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify robotMetaId usage in interpreter
# Search for robotMetaId usage in interpreter implementation
rg -A 5 'InterpretRecording.*robotMetaId'
# Search for cookie-related handling in interpreter
rg -A 5 'cookie|login' --type ts
Length of output: 28250
🧰 Tools
🪛 Biome (1.9.4)
[error] 129-129: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
src/components/molecules/RecordingsTable.tsx (1)
310-321: LGTM! Well-structured login requirement form.
The implementation uses appropriate Material-UI components and integrates well with the global state management.
server/src/routes/storage.ts (2)
318-318: LGTM! Correctly initializes login state for duplicated robot.
Setting isLogin: false for duplicated robots is the correct approach as login state should not be inherited.
23-23: 🛠️ Refactor suggestion
Remove redundant import.
The StoredCookie import appears to be redundant as the Robot model is already imported above. Additionally, the import path seems incorrect as it's importing from the Robot model itself.
-import StoredCookie from "../models/Robot";Likely invalid or redundant comment.
server/src/workflow-management/classes/Generator.ts (1)
132-134: LGTM! Correctly handles isLogin parameter.
The socket event handler properly extracts and passes the isLogin parameter to the saveNewWorkflow method.
| const cookie = await interpreter.run(page, params); | ||
|
|
||
| if (robotId && cookie) { | ||
| await this.setCookies(robotId, cookie); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle concurrency if multiple setCookies calls may occur.
If multiple runs for the same robot happen at once, you might want to synchronize or handle race conditions for storing cookies.
| const payload = { fileName: recordingName, userId: user.id, isLogin: isLogin }; | ||
| socket?.emit('save', payload); | ||
| setWaitingForSave(true); | ||
| console.log(`Saving the recording as ${recordingName} for userId ${user.id}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for socket communication.
While the addition of isLogin to the payload is correct, the socket communication lacks proper error handling.
Consider adding error handling:
const saveRecording = async () => {
if (user) {
const payload = { fileName: recordingName, userId: user.id, isLogin: isLogin };
- socket?.emit('save', payload);
+ if (!socket?.connected) {
+ notify('error', 'Connection lost. Please try again.');
+ return;
+ }
+ socket.emit('save', payload, (error: any) => {
+ if (error) {
+ setWaitingForSave(false);
+ notify('error', 'Failed to save recording. Please try again.');
+ }
+ });
setWaitingForSave(true);
console.log(`Saving the recording as ${recordingName} for userId ${user.id}`);
} else {
console.error('User not logged in. Cannot save recording.');
+ notify('error', 'Please log in to save recordings.');
}
};Committable suggestion skipped: line range outside the PR's diff.
| // workflowCopy = initSpecialRecurse( | ||
| // workflowCopy, | ||
| // '$regex', | ||
| // (regex) => new RegExp(regex), | ||
| // ); |
There was a problem hiding this comment.
💡 Codebase verification
Commenting out regex initialization could break URL pattern matching
The verification reveals that regex functionality is actively used in the codebase:
$regexis part of the workflow type definition (RegexableString)- URL patterns with
$regexare generated inGenerator.ts - Regex pattern matching is used in
interpret.tsfor URL validation with.test()
Commenting out the regex initialization in preprocessor.ts will prevent the conversion of string patterns to RegExp objects, which will cause runtime errors when the code attempts to use .test() on unconverted regex strings.
🔗 Analysis chain
Verify the impact of removing regex initialization.
The commented-out code block handles regex parameter initialization. This change could affect workflow processing if regex functionality is still expected elsewhere in the codebase.
Let me gather more context about the regex initialization and its usage.
Let me check one more aspect to understand how URLs with regex patterns are processed in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for regex usage in workflows and related code
# Search for regex patterns in workflow files
rg -A 3 '\$regex'
# Search for RegExp usage in workflow processing
ast-grep --pattern 'new RegExp($_)'
Length of output: 2501
Script:
#!/bin/bash
# Check the implementation of initSpecialRecurse and its usage
ast-grep --pattern 'function initSpecialRecurse($_) {
$$$
}'
# Look for where regex patterns are actually used in workflow processing
rg -A 5 'new RegExp' maxun-core/
Length of output: 376
Script:
#!/bin/bash
# Check how URLs with regex patterns are processed
rg -A 10 "if \('\$regex' in url\)" maxun-core/
# Look for the actual usage of RegExp in URL matching
rg -A 5 "\.test\(|\.match\(" maxun-core/
Length of output: 1016
| /** | ||
| * PUT endpoint for updating cookies for a specific robot. | ||
| * Expects a cookie storage object in the request body. | ||
| */ | ||
| router.put('/recordings/:id/cookies', async (req, res) => { | ||
| try { | ||
| const robot = await Robot.findOne({ | ||
| where: { 'recording_meta.id': req.params.id } | ||
| }); | ||
|
|
||
| if (!robot) { | ||
| return res.status(404).json({ | ||
| error: 'Robot not found' | ||
| }); | ||
| } | ||
|
|
||
| await robot.update({ | ||
| cookie_storage: req.body.cookies | ||
| }); | ||
|
|
||
| return res.json({ | ||
| message: 'Cookie storage updated successfully' | ||
| }); | ||
|
|
||
| } catch (error) { | ||
| logger.log('error', `Error updating cookies for robot ${req.params.id}: ${error}`); | ||
| return res.status(500).json({ | ||
| error: 'Failed to update cookies' | ||
| }); | ||
| } | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for cookies.
The PUT endpoint for updating cookies lacks input validation for the cookie structure.
Consider adding:
- Type safety for cookie structure
- Validation for required cookie fields
- Size limits for cookie storage
Example implementation:
interface Cookie {
name: string;
value: string;
domain?: string;
path?: string;
expires?: number;
httpOnly?: boolean;
secure?: boolean;
sameSite?: 'Strict' | 'Lax' | 'None';
}
// Add before the update
if (!Array.isArray(req.body.cookies)) {
return res.status(400).json({
error: 'Cookies must be an array'
});
}
const isValidCookie = (cookie: any): cookie is Cookie => {
return typeof cookie.name === 'string'
&& typeof cookie.value === 'string'
&& cookie.name.length <= 4096 // Reasonable size limit
&& cookie.value.length <= 4096;
};
if (!req.body.cookies.every(isValidCookie)) {
return res.status(400).json({
error: 'Invalid cookie format'
});
}| @@ -460,7 +523,7 @@ router.post('/runs/run/:id', requireSignIn, async (req: AuthenticatedRequest, re | |||
| if (browser && currentPage) { | |||
| const workflow = AddGeneratedFlags(recording.recording); | |||
| const interpretationInfo = await browser.interpreter.InterpretRecording( | |||
| workflow, currentPage, (newPage: Page) => currentPage = newPage, plainRun.interpreterSettings); | |||
| workflow, currentPage, (newPage: Page) => currentPage = newPage, plainRun.interpreterSettings, plainRun.robotMetaId); | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix assignment in expression.
The assignment of currentPage within the function call parameters can lead to confusion and is flagged by static analysis.
Refactor to make the code more explicit:
- workflow, currentPage, (newPage: Page) => currentPage = newPage, plainRun.interpreterSettings, plainRun.robotMetaId);
+ workflow,
+ currentPage,
+ (newPage: Page) => {
+ currentPage = newPage;
+ return currentPage;
+ },
+ plainRun.interpreterSettings,
+ plainRun.robotMetaId
+ );📝 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.
| workflow, currentPage, (newPage: Page) => currentPage = newPage, plainRun.interpreterSettings, plainRun.robotMetaId); | |
| workflow, | |
| currentPage, | |
| (newPage: Page) => { | |
| currentPage = newPage; | |
| return currentPage; | |
| }, | |
| plainRun.interpreterSettings, | |
| plainRun.robotMetaId | |
| ); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 526-526: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
amhsirak
added
Type: Feature
Handling cookie based login by storing cookies on successful login action. Helps bypass entering credentials for each login session.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores