Update yorkie-js-sdk to v0.5.5
#407
Conversation
WalkthroughThe changes in this pull request primarily involve enhancements to error handling and token management within the backend and frontend services. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
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: 3
🧹 Outside diff range and nitpick comments (5)
backend/src/files/files.controller.ts (1)
File endpoints should not be public - authentication required
Based on the evidence found in the codebase:
- The
createUploadPresignedUrlendpoint accepts aworkspaceIdparameter and validates workspace existence, indicating workspace-specific access control is intended- Other controllers in the system (workspaces, workspace-documents) use
@ApiBearerAuth()and require authenticated requests- File operations are sensitive operations that could be misused if left public (unlimited uploads, unauthorized downloads)
- The application has a proper authentication system in place with JWT tokens and guards
Recommendations:
- Remove the
@Public()decorator from all three endpoints infiles.controller.ts- Add
@ApiBearerAuth()decorator at the controller level- Consider adding workspace-level authorization checks in the service layer
🔗 Analysis chain
Line range hint
23-75: Verify if public access to file endpoints is intended.Given that this PR involves changes to authorization and token validation, it's worth verifying if all endpoints in this controller should remain public (using
@Public()decorator). Currently, all file operations (upload, download, and export) bypass authentication.Let's verify the authentication requirements:
Let me gather more specific information about the file endpoints and their security context:
Let me check one more aspect regarding workspace validation and file access patterns:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for similar file-related endpoints in the codebase and their authentication patterns # Check for other controllers handling file operations echo "Searching for other file-related controllers..." rg -l "Controller.*[\"']file" --type ts # Check for usage patterns of @Public decorator echo "Checking @Public decorator usage patterns..." rg "@Public\(\)" -B 2 -A 2 --type ts # Look for any security-related comments or documentation echo "Checking for security-related comments..." rg -i "auth.*|security.*|public.*access" --type tsLength of output: 17591
Script:
#!/bin/bash # Check for workspace validation in the files service echo "Checking files.service.ts implementation..." rg -A 10 "createUploadPresignedUrl|createDownloadPresignedUrl|exportMarkdown" backend/src/files/files.service.ts # Check if there's any workspace-level authorization echo "Checking workspace authorization patterns..." rg "workspaceId.*auth|auth.*workspace" --type ts # Check for any security-related configuration echo "Checking security configuration..." cat backend/src/files/files.module.tsLength of output: 1774
Script:
#!/bin/bash # Check for workspace validation in files service echo "Checking workspace validation in files service..." ast-grep --pattern 'async createUploadPresignedUrl($_) { $$$ }' # Check for any middleware or guards related to files echo "Checking for file-related guards..." rg -l "Guard.*file|file.*Guard" --type ts # Check if there are any workspace role checks echo "Checking workspace role validations..." rg "WorkspaceRole|workspace.*role" backend/src/files/files.service.tsLength of output: 460
frontend/src/hooks/api/user.ts (1)
40-40: Consider improving error handling and interceptor management.The current implementation has potential issues:
- The error retry logic could create infinite loops if the new token is also invalid
- The interceptor setup is complex and could be extracted for better maintainability
Consider adding a retry limit and extracting the interceptor logic:
+const MAX_RETRY_ATTEMPTS = 3; + +const createAuthInterceptor = ( + mutateRefreshToken: () => Promise<string>, + dispatch: AppDispatch +) => { + return axios.interceptors.response.use( + (response) => response, + async (error) => { + const retryCount = error.config._retryCount || 0; + if ( + error.response?.status === 401 && + retryCount < MAX_RETRY_ATTEMPTS && + !error.config._retry + ) { + if (error.config.url === "/auth/refresh") { + dispatch(logout()); + dispatch(setUserData(null)); + return Promise.reject(error); + } + error.config._retry = true; + error.config._retryCount = retryCount + 1; + const newAccessToken = await mutateRefreshToken(); + error.config.headers["Authorization"] = `Bearer ${newAccessToken}`; + return axios(error.config); + } + return Promise.reject(error); + } + ); +};Then update the useEffect to use this:
useEffect(() => { - const interceptor = axios.interceptors.response.use(...) + const interceptor = createAuthInterceptor(mutateRefreshToken, dispatch); setAxiosInterceptorAdded(true); return () => { setAxiosInterceptorAdded(false); axios.interceptors.response.eject(interceptor); }; }, [authStore, dispatch, mutateRefreshToken]);Also applies to: 54-56, 69-69
backend/src/check/check.service.ts (1)
51-54: Use strict equality operators (===) instead of loose equality (==)In TypeScript, it's best practice to use strict equality operators to avoid unexpected type coercion.
Apply this diff to update the equality checks:
- if (type == "default") { + if (type === "default") { - } else if (type == "share") { + } else if (type === "share") {frontend/src/hooks/useYorkieDocument.ts (2)
27-47: Clarify the usage of thereasonparameter ingetYorkieTokenThe
reasonparameter is used as a flag to decide whether to refresh the token. Consider the following improvements:
- Rename the parameter: Changing
reasonto a more descriptive name likeshouldRefreshorforceRefreshenhances clarity.- Add documentation: If
reasoncan have specific values that influence behavior, documenting these values and their effects would be beneficial.
39-41: Include original error details when token refresh failsTo aid in debugging, include the original error message when throwing a new error. This provides more context about the failure.
Apply this diff to include the error details:
} catch (error) { - throw new Error("Failed to refresh token"); + throw new Error(`Failed to refresh token: ${error.message}`); }Ensure that the error message does not expose sensitive information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (6)
backend/src/check/check.service.ts(2 hunks)backend/src/files/files.controller.ts(1 hunks)frontend/package.json(1 hunks)frontend/src/hooks/api/types/user.d.ts(1 hunks)frontend/src/hooks/api/user.ts(3 hunks)frontend/src/hooks/useYorkieDocument.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/package.json
🧰 Additional context used
🪛 Biome
backend/src/check/check.service.ts
[error] 50-50: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
🔇 Additional comments (8)
frontend/src/hooks/api/types/user.d.ts (1)
15-21: LGTM! Well-structured type definitions for token refresh mechanism.
The new type definitions are clear, focused, and follow TypeScript best practices. They appropriately model the request/response structure for token refresh operations.
Let's verify the consistency of token-related types across the codebase:
✅ Verification successful
The token refresh types are properly integrated and used consistently across the codebase
The verification shows that:
- The
RefreshTokenRequestandRefreshTokenResponsetypes are correctly used in the token refresh implementation infrontend/src/hooks/api/user.ts - The token refresh mechanism is properly integrated with:
- Redux store management (
authSlice.ts) - Axios interceptors for automatic token refresh on 401 errors
- Yorkie document authentication (
useYorkieDocument.ts)
- Redux store management (
- The implementation follows a standard token refresh pattern with proper error handling and state management
The types are used as intended and are part of a well-structured authentication system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent token type usage across the frontend
# Expected: Types should be used in authentication-related components and API calls
# Search for token-related types usage
echo "Checking RefreshToken types usage:"
rg -t typescript "RefreshToken(Request|Response)" frontend/
# Search for other token-related types to ensure consistency
echo "\nChecking other token-related types:"
rg -t typescript "Token.*Type|TokenRequest|TokenResponse" frontend/
Length of output: 365
Script:
#!/bin/bash
# Let's try without typescript file type flag since it's not recognized
# First, check RefreshToken types usage
echo "Checking RefreshToken types usage:"
rg "RefreshToken(Request|Response)" frontend/
# Search for token-related implementations
echo -e "\nChecking token-related implementations:"
rg "(refresh.*token|token.*refresh)" frontend/ -i
# Look for files that might use these types
echo -e "\nChecking .ts and .tsx files in frontend/src/hooks/api:"
fd "\.(ts|tsx)$" frontend/src/hooks/api --exec cat {}
Length of output: 23878
backend/src/files/files.controller.ts (1)
11-11: LGTM! Documentation improvements look good.
The addition of @ApiTags("Files") and its import improves the Swagger API documentation organization.
Also applies to: 18-18
frontend/src/hooks/api/user.ts (2)
7-12: LGTM: Import statements are properly structured.
The new type imports for token refresh functionality are correctly added and organized.
24-26: Verify secure token handling across the application.
Since this code handles sensitive authentication tokens, let's verify the security implementation:
- Check for proper token storage
- Verify token transmission security
- Ensure proper token cleanup on logout
Also applies to: 54-56
frontend/src/hooks/useYorkieDocument.ts (4)
10-10: Importing useRefreshTokenMutation for token refreshing
The import of useRefreshTokenMutation correctly integrates the token refresh functionality into the hook.
25-25: Initializing mutateRefreshToken
Properly initializing mutateRefreshToken from useRefreshTokenMutation ensures that the token refresh mechanism is ready for use.
50-60: Integrate getYorkieToken as authTokenInjector in createYorkieClient
Using getYorkieToken as the authTokenInjector streamlines token management within the Yorkie client, aligning with the updated yorkie-js-sdk version.
96-96: Update client initialization without passing a token
The initializeYorkie function now calls createYorkieClient() without a token parameter, which matches the updated client creation logic.
What this PR does / why we need it:
yorkie-js-sdktov0.5.5Which issue(s) this PR fixes:
Fixes #395
Special notes for your reviewer:
Does this PR introduce a user-facing change?:
Additional documentation:
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores
yorkie-js-sdkto improve stability.