[FE-Feat] 논의 id 난수화 #365
[FE-Feat] 논의 id 난수화 #365
Conversation
WalkthroughThis update introduces a system-wide migration from numeric to string-based discussion IDs across all client and server modules. It also adds environment configuration, encryption utilities, and middleware for secure discussion ID handling in the server. New dependencies for environment management and HTTP proxying are included, with supporting utilities and middleware for proxying and transforming discussion IDs. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server (Express)
participant ProxyMiddleware
participant Backend API
Client->>Server (Express): HTTP request with encrypted discussionId
Server (Express)->>ProxyMiddleware: Passes request to /api route
ProxyMiddleware->>ProxyMiddleware: Decrypts discussionId in path/body
ProxyMiddleware->>Backend API: Forwards request with decrypted discussionId
Backend API-->>ProxyMiddleware: Responds with JSON (plain discussionId)
ProxyMiddleware->>ProxyMiddleware: Encrypts discussionId in response
ProxyMiddleware-->>Server (Express): Returns modified response
Server (Express)-->>Client: Sends response with encrypted discussionId
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (14)
frontend/server/middlewares/discussionIdTransformer/index.js (1)
1-14: Verify target URL for different environmentsThe middleware is well-structured, but the API target URL is hardcoded to
http://localhost:8080/api. This might cause issues in different environments (staging, production).Consider making the target URL configurable via environment variables to support deployment across different environments.
- target: 'http://localhost:8080/api', + target: process.env.API_URL || 'http://localhost:8080/api',frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (1)
35-35: Redundant toString() conversionSince
discussionIdis now a string, thetoString()conversion is redundant and can be removed.- navigate({ to: '/discussion/$id', params: { id: discussionId.toString() } }); + navigate({ to: '/discussion/$id', params: { id: discussionId } });frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)
32-32: Consider removing redundant toString() callsNow that
discussionIdis typed as a string, the.toString()calls may be unnecessary. While they don't cause functional issues (they're safe to keep as a defensive measure), they could be removed for code cleanliness.- const { discussion } = useDiscussionQuery(discussionId.toString()); + const { discussion } = useDiscussionQuery(discussionId); - <Link params={{ id: discussionId.toString() }} to='/discussion/$id'> + <Link params={{ id: discussionId }} to='/discussion/$id'>Also applies to: 59-59
frontend/server/utils/object.js (1)
13-13: Consider handling non-string values more explicitlyWhile
String()will convert most values safely, you might want to add more explicit handling for certain edge cases likeundefinedor complex objects that don't convert well to strings.- newData[key] = manipulateFunc(String(object[key])); + newData[key] = object[key] !== undefined ? manipulateFunc(String(object[key])) : object[key];Alternatively, add a comment explaining that the conversion is expected and handled appropriately by the
manipulateFunc.frontend/server/utils/collectResponse.js (1)
3-8: Consider removing commented codeWhile keeping the old implementation as a reference can be helpful during development, it's generally better to remove commented-out code before merging to production. The git history will preserve the old implementation if needed for reference.
-// export const collectResponse = (proxyRes) => new Promise((resolve, reject) => { -// const chunks = []; -// proxyRes.on('data', chunk => chunks.push(chunk)); -// proxyRes.on('end', () => resolve(Buffer.concat(chunks).toString('utf-8'))); -// proxyRes.on('error', reject); -// });frontend/server/utils/stringTransformer.js (2)
3-3: Unused constant KEYThis constant is defined but not used anywhere in the file. Consider removing it if it's not needed or document its intended purpose if it will be used in future implementations.
-const KEY = 'uniform-length-transformer-key';
5-9: Remove or complete commented codeThere's commented code that doesn't appear to be used. Either complete the implementation if it's needed or remove it to keep the codebase clean.
-// export const uniformLengthTransformer = (outputLength) => { -// (text) => { - -// }; -// };frontend/server/index.js (1)
44-44: Error handling could be improvedThe error handling has been changed to directly end the response instead of passing to the next middleware. Consider adding proper error logging and sending a structured error response with an appropriate status code.
- res.end(e); + console.error('Server error:', e); + res.status(500).json({ error: 'Internal Server Error' });frontend/server/utils/cipher.js (2)
14-34: Well-implemented AES-GCM encryption functionThe encryption function correctly uses crypto.randomBytes for IV generation, properly configures the cipher, and returns all necessary components (IV, content, tag) with appropriate encoding. Good job documenting the function with JSDoc comments.
However, consider adding validation for the key length to ensure it's 32 bytes as required for AES-256-GCM.
encrypt: (text, key) => { try { + // Validate key length + if (key.length !== 32) { + throw new Error('Invalid key length. AES-256-GCM requires a 32-byte key.'); + } // 12바이트 IV 생성 (AES-GCM의 권장 길이) const iv = crypto.randomBytes(12);
46-63: Add input validation to decrypt functionThe decrypt function should validate that all required properties (iv, content, tag) exist in the encryptedData object before attempting to use them.
decrypt: (encryptedData, key) => { try { + // Validate input + if (!encryptedData || !encryptedData.iv || !encryptedData.content || !encryptedData.tag) { + throw new Error('Invalid encrypted data: missing required properties'); + } + // Validate key length + if (key.length !== 32) { + throw new Error('Invalid key length. AES-256-GCM requires a 32-byte key.'); + } const iv = Buffer.from(encryptedData.iv, 'base64');frontend/server/middlewares/discussionIdTransformer/discussionCipher.js (1)
7-14: Consider making the target index configurableThe hardcoded
targetIdx = 3assumes a fixed URL structure. This could be brittle if the URL structure changes in the future.- export const decodeDiscussionIdOfUrl = (url) => { - const parts = url.split('/'); // ex: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] - const targetIdx = 3; // discussionId가 위치하는 index + export const decodeDiscussionIdOfUrl = (url, targetIdx = 3) => { + const parts = url.split('/'); // ex: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] + // targetIdx: index where discussionId is located (default: 3)frontend/server/middlewares/discussionIdTransformer/proxyHandlers.js (3)
11-12: Add more descriptive comments for the endpointsThe regex pattern is defined but not well documented. Adding a comment explaining what URL patterns it's intended to match would improve code maintainability.
const DISCUSSION_CREATE_ENDPOINT = '/v1/discussion'; -const DISCUSSION_GET_ENDPOINT_REGEX = /^\/v1\/discussion\/([^/]+)\/?$/; +// Matches URLs like '/v1/discussion/{discussionId}' or '/v1/discussion/{discussionId}/' +const DISCUSSION_GET_ENDPOINT_REGEX = /^\/v1\/discussion\/([^/]+)\/?$/;
21-29: Improve req.body validationThe current check handles objects but might not properly handle edge cases. Consider adding explicit null checks.
export const handleProxyReq = (proxyReq, req, res) => { - if (req.body && typeof req.body === 'object') { + if (req.body && typeof req.body === 'object' && req.body !== null) {
35-39: Consider standardizing API response field namesThe comment mentions that some APIs use 'id' instead of 'discussionId', requiring conditional logic. Consider standardizing these field names in your API responses for better maintainability.
While this solution works, having inconsistent field names ('id' vs 'discussionId') across APIs increases complexity and potential for bugs. If possible, standardize your API responses to use a consistent field name for discussion identifiers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between d33db15 and a189181ad5fb1e318f2b3a8218dd436edb099b92.
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (36)
frontend/package.json(2 hunks)frontend/server/envconfig.js(1 hunks)frontend/server/index.js(3 hunks)frontend/server/middlewares/discussionIdTransformer/discussionCipher.js(1 hunks)frontend/server/middlewares/discussionIdTransformer/index.js(1 hunks)frontend/server/middlewares/discussionIdTransformer/proxyHandlers.js(1 hunks)frontend/server/utils/cipher.js(1 hunks)frontend/server/utils/collectResponse.js(1 hunks)frontend/server/utils/object.js(1 hunks)frontend/server/utils/stringTransformer.js(1 hunks)frontend/src/features/discussion/api/invitationApi.ts(1 hunks)frontend/src/features/discussion/api/keys.ts(1 hunks)frontend/src/features/discussion/api/mutations.ts(1 hunks)frontend/src/features/discussion/api/queries.ts(1 hunks)frontend/src/features/discussion/api/queryOptions.ts(1 hunks)frontend/src/features/discussion/model/index.ts(2 hunks)frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx(1 hunks)frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx(2 hunks)frontend/src/features/shared-schedule/model/finishedSchedules.ts(1 hunks)frontend/src/features/shared-schedule/model/ongoingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/model/upcomingSchedules.ts(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(1 hunks)frontend/src/features/timeline-schedule/api/index.ts(1 hunks)frontend/src/features/timeline-schedule/api/keys.ts(1 hunks)frontend/src/features/timeline-schedule/api/queries.ts(1 hunks)frontend/src/features/timeline-schedule/api/queryOptions.ts(1 hunks)frontend/src/features/timeline-schedule/model/index.ts(1 hunks)frontend/src/features/timeline-schedule/ui/index.tsx(1 hunks)frontend/src/pages/CandidateSchedulePage/Header.tsx(1 hunks)frontend/src/pages/CandidateSchedulePage/index.tsx(1 hunks)frontend/src/pages/DiscussionPage/DiscussionInvitePage/index.tsx(1 hunks)frontend/src/pages/MyCalendarPage/DiscussionContext.ts(1 hunks)frontend/src/pages/MyCalendarPage/index.tsx(1 hunks)frontend/src/pages/UpcomingScheduleDetailPage/index.tsx(1 hunks)frontend/src/routes/_main/discussion/invite/$id.tsx(1 hunks)
🧰 Additional context used
🧬 Code Definitions (6)
frontend/server/middlewares/discussionIdTransformer/index.js (1)
frontend/server/middlewares/discussionIdTransformer/proxyHandlers.js (6)
rewriteDiscussionPath(14-19)rewriteDiscussionPath(14-19)handleProxyReq(21-29)handleProxyReq(21-29)handleProxyRes(31-51)handleProxyRes(31-51)
frontend/src/features/discussion/api/invitationApi.ts (1)
frontend/src/features/discussion/model/invitation.ts (1)
InvitationResponseSchema(5-16)
frontend/server/envconfig.js (1)
frontend/server/index.js (1)
dirname(10-10)
frontend/server/utils/cipher.js (1)
frontend/server/middlewares/discussionIdTransformer/discussionCipher.js (5)
aesGcm(18-18)key(17-17)key(25-25)iv(27-27)encryptedData(28-28)
frontend/src/routes/_main/discussion/invite/$id.tsx (1)
frontend/src/features/discussion/api/queryOptions.ts (1)
invitationQueryOption(6-9)
frontend/server/middlewares/discussionIdTransformer/proxyHandlers.js (2)
frontend/server/middlewares/discussionIdTransformer/discussionCipher.js (6)
decodeDiscussionIdOfUrl(7-14)decodeDiscussionIdOfUrl(7-14)decryptDiscussionId(24-30)decryptDiscussionId(24-30)encryptDiscussionId(16-22)encryptDiscussionId(16-22)frontend/server/utils/collectResponse.js (2)
collectResponse(24-34)collectResponse(24-34)
🔇 Additional comments (41)
frontend/src/features/shared-schedule/model/ongoingSchedules.ts (1)
6-6: Type change to support encoded discussion IDsThis change from
z.number()toz.string()aligns with the PR objective of encoding discussion IDs. Since the encoded IDs will be string values produced by the AES-GCM encryption and base64 encoding, this type change is necessary to properly validate the new format.frontend/src/features/shared-schedule/model/upcomingSchedules.ts (1)
6-6: Type change to support encoded discussion IDsChanging from
z.number()toz.string()ensures that the schema can validate encoded discussion IDs correctly. This change is consistent with the similar modifications made in other schema files, maintaining type consistency across the application.frontend/src/features/shared-schedule/model/finishedSchedules.ts (1)
6-6: Type change to support encoded discussion IDsThe modification from
z.number()toz.string()completes the pattern of changes to discussion ID types across all schedule-related schema files. This ensures that the finished schedules data structure properly handles the new encoded ID format.frontend/src/pages/UpcomingScheduleDetailPage/index.tsx (1)
27-27: Removed number conversion for discussion IDThis change correctly removes the
Number()conversion previously applied to the discussion ID, allowing the encoded string ID to be passed directly to theTimelineScheduleModalcomponent. This aligns with the changes made to the component's type definitions.frontend/src/features/timeline-schedule/model/index.ts (1)
35-35: Type change looks appropriate.Changing
discussionIdfromz.number()toz.string()aligns with the PR's goal of encoding discussion IDs. Since encoded IDs will be handled as strings rather than numbers, this change ensures proper type checking and validation.frontend/src/routes/_main/discussion/invite/$id.tsx (1)
22-22: Correctly removed ID conversion.The change appropriately removes the numeric conversion of the discussion ID parameter, which aligns with the ID encoding implementation described in the PR. The ID is now correctly passed as a string to the query option.
frontend/src/pages/DiscussionPage/DiscussionInvitePage/index.tsx (1)
25-25: Correctly removed type coercion.The removal of the unary plus operator (
+) is appropriate as the discussion ID is now being handled as a string rather than a number, which aligns with the PR's implementation of ID encoding.frontend/src/features/timeline-schedule/api/queryOptions.ts (1)
6-6: Parameter type change is consistent.Changing the
discussionIdparameter type fromnumbertostringis consistent with the other changes in this PR. This ensures that encoded discussion IDs are correctly handled throughout the application.frontend/package.json (1)
55-55: New dependencies added for discussion ID encryption and middleware.These new dependencies align with the PR objectives of implementing a proxy middleware for encoding/decoding discussion IDs using AES-GCM encryption:
dotenvwill handle the environment variables for encryption keyshttp-proxy-middlewarewill intercept API requests/responses to transform discussion IDsAlso applies to: 64-64
frontend/src/features/timeline-schedule/api/keys.ts (1)
6-6:Details
✅ Verification successful
Type change from number to string for discussionId parameter.
This change is consistent with the PR objectives to encode discussion IDs, transforming them from numbers to encrypted strings using AES-GCM and base64 encoding.
🏁 Script executed:
#!/bin/bash # Verify that all other instances of discussionId in timeline-schedule feature have been updated to string type rg "discussionId: number" -t ts --context 3 src/features/timeline-schedule/Length of output: 149
Action Required: Verify 'discussionId' Type Consistency
The change in
frontend/src/features/timeline-schedule/api/keys.tsfrom a numeric to a string type for thediscussionIdparameter aligns with the PR objectives of encoding discussion IDs. However, the initial verification command pointed to an incorrect directory. Please re-run the above script (or a similar check) to ensure that no instances ofdiscussionId: numberremain within the timeline-schedule feature.
🏁 Script executed:
#!/bin/bash # Ensure the keys file exists in the updated path and search for any numeric discussionId usage FILE="frontend/src/features/timeline-schedule/api/keys.ts" if [ ! -f "$FILE" ]; then echo "File not found: $FILE" exit 1 fi # Search for any instance of discussionId typed as number in the timeline-schedule feature rg "discussionId: number" -t ts --context 3 frontend/src/features/timeline-schedule/Length of output: 336
Discussion ID Type Update Verified
The refactored type change in
frontend/src/features/timeline-schedule/api/keys.ts(i.e., changingdiscussionIdfrom number to string) is consistent with the encryption and encoding objectives of the PR. A search fordiscussionId: numberwithin the timeline-schedule feature did not return any results, confirming that other instances have been updated accordingly.frontend/src/features/discussion/api/keys.ts (1)
27-27:Details
✅ Verification successful
Type change from number to string for invitationQueryKey's discussionId parameter.
This change aligns with the PR objectives to encode all discussion IDs using AES-GCM encryption and base64 encoding. This transformation requires the type to be updated from number to string.
🏁 Script executed:
#!/bin/bash # Check if there are any remaining instances where discussionId is still typed as number in the discussion feature rg "discussionId: number" -t ts --context 3 src/features/discussion/Length of output: 135
Could you please run the above command to verify that no outdated type declarations remain in the discussion feature? The previous search targeted a non-existent directory, so a manual check using this corrected path is needed. Once confirmed, we can fully approve the change to using a string for the invitationQueryKey's discussionId parameter.
Action Required:
- Re-run the provided script with the correct path.
- Manually verify the output to ensure there are no instances of
discussionId: numberremaining infrontend/src/features/discussion/.
🏁 Script executed:
#!/bin/bash # Re-run the search for any instances of discussionId typed as number using the corrected path. rg "discussionId: number" -t ts --context 3 frontend/src/features/discussion/Length of output: 201
Approved: Discussion ID Type Updated
- Verified that
invitationQueryKeynow accepts astringfor thediscussionIdparameter.- A re-run of the search in
frontend/src/features/discussion/did not return any outdated instances ofdiscussionId: number, ensuring the change aligns with the encryption and encoding requirements.This update meets the PR objectives.
frontend/src/features/timeline-schedule/ui/index.tsx (2)
24-24: Type change from number to string for discussionId prop.This change is consistent with the PR objectives to encode discussion IDs as strings using AES-GCM encryption. This type update ensures the component correctly handles the encrypted string format of discussion IDs.
40-40: Function call using the updated string discussionId type.The
useCandidateDetailQueryis correctly using the string-typed discussionId with no changes needed to the function call itself, showing good compatibility with the type change.frontend/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (2)
68-68: Type change for discussionId is consistent with the application-wide changesThis change from
numbertostringfordiscussionIdis aligned with the PR objective to encode discussion IDs. The change ensures type consistency when using the encrypted string IDs.
79-81:Details
✅ Verification successful
Verify that toString() call is still needed
Since
discussionIdis now a string, thetoString()call in the Link params might be redundant. However, this is harmless as it ensures the value is always a string even if the type system is bypassed.
🏁 Script executed:
#!/bin/bash # Check if there are other places in the codebase where discussionId.toString() is used rg "discussionId\.toString\(\)" --type ts,tsxLength of output: 78
Action Required: Verify discussionId Type Consistency
The original search command failed due to unrecognized file types. Please re-run the updated command above to search for instances of
discussionId.toString()within.tsand.tsxfiles. If no matches are found, it confirms thatdiscussionIdis consistently a string in the codebase, which makes the call totoString()redundant (although harmless).
🏁 Script executed:
#!/bin/bash rg "discussionId\.toString\(\)" -g "*.ts" -g "*.tsx"Length of output: 1043
DiscussionId.toString() Usage Consistency Verified
We've confirmed via search that the
.toString()call is used consistently across multiple components (e.g., in UpcomingScheduleListItem.tsx, ScheduleCard.tsx, ScheduleDetails.tsx, etc.). SincediscussionIdis now a string throughout the codebase, these calls are redundant. However, they also serve as a defensive safeguard in case type guarantees are bypassed. If you're confident in the type system's integrity, you can safely remove these conversions for clarity, or leave them in place without adverse effects.frontend/src/features/discussion/api/queries.ts (1)
111-111: Type change for discussionId parameter is consistent with other functionsThe parameter type change from
numbertostringforuseInviteInfoQuerymaintains consistency with other functions in this file that also use string-typed discussion IDs.frontend/src/features/timeline-schedule/api/queries.ts (1)
7-7: Type change for discussionId parameter aligns with the PR objectiveChanging the
discussionIdparameter type fromnumbertostringis consistent with the PR's goal of using encoded discussion IDs across the application.frontend/src/pages/CandidateSchedulePage/index.tsx (2)
20-20: Removed Number conversion to match the new typeCorrectly removed the
Number(id)conversion since thediscussionIdis now expected to be a string. This change is consistent with the updated component props.
28-28: Removed Number conversion to match the new typeCorrectly removed the
Number(id)conversion since thediscussionIdis now expected to be a string. This change is consistent with the updated component props.frontend/server/envconfig.js (1)
1-15: Good implementation of environment variable managementThis file correctly sets up environment variable loading for the AES-GCM key used in discussion ID encryption. The implementation follows best practices:
- Uses ES modules syntax
- Properly resolves the path to the
.env.localfile- Exports only the needed environment variable
The environment configuration is well-structured and aligns with the PR objective of implementing discussion ID encryption.
frontend/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (1)
14-19: Type change to support encoded discussion IDsThe change from
numbertostringfordiscussionIdproperly supports the new encoded ID format.frontend/src/features/discussion/api/mutations.ts (1)
33-33: Type change to support encoded discussion IDsChanging the
discussionIdtype fromnumbertostringaligns with the PR objective of encoding discussion IDs. This ensures type safety throughout the API call chain.frontend/src/features/discussion/model/index.ts (2)
39-39: Type change aligns with ID encryptionChanging the ID type from
numbertostringis consistent with the PR objective of encoding discussion IDs. This type change is necessary since the encoded IDs will be string-based after the AES-GCM encryption and base64 encoding.
80-80: Type change for discussionId is consistentChanging
discussionIdto a string type follows the same pattern applied to theidfield earlier in this file. This ensures consistency in how discussion identifiers are handled throughout the application.frontend/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)
27-27: Type change aligns with encrypted IDs implementationThe change from
numbertostringfor the discussionId type is consistent with the overall project changes for discussion ID encoding.frontend/server/utils/object.js (1)
1-20: Well-designed recursive object manipulation utilityThis utility function is well-structured for recursively processing objects and transforming specific key values. It properly handles various data types (arrays, objects, primitives) and maintains immutability by creating new objects rather than modifying the original.
The implementation correctly:
- Processes arrays using map
- Checks for null/non-object values
- Creates a new object to avoid mutation
- Applies the manipulation function only to the specified keys
- Recursively processes nested structures
This seems to be a core function for the ID transformation middleware described in the PR objectives.
frontend/src/features/discussion/api/queryOptions.ts (1)
6-6: Type change consistent with ID encryption strategyChanging the
discussionIdparameter type fromnumbertostringaligns with the overall approach of using string-based encrypted IDs. This ensures type consistency between the API calls, query keys, and data models.frontend/src/features/discussion/api/invitationApi.ts (2)
6-6: Type change aligns with discussion ID encryptionChanging
discussionIdtype fromnumbertostringis consistent with the PR objective of encoding discussion IDs. This ensures that encrypted IDs (which will be strings) can be properly handled by the API functions.
11-11: Type change consistent with the encryption schemeThe parameter type change from
numbertostringfordiscussionIdis appropriate and matches the changes made in thegetInvitationInfomethod, maintaining consistency across the API interface.frontend/src/pages/MyCalendarPage/index.tsx (1)
20-20: State type updated for encoded discussion IDChanging the
selectedIdstate fromnumber | nulltostring | nullis consistent with the PR's approach of encoding discussion IDs as strings. This change ensures that theDiscussionProvidercontext will correctly handle the encrypted ID format.frontend/server/utils/collectResponse.js (2)
12-23: Well-documented utility functionThe JSDoc comments are comprehensive and explain the purpose, parameters, return values, and potential errors of the function. This is excellent practice for maintainability.
24-34: Modern and efficient implementation for response collectionThe implementation using async/await with proper error handling is clean and efficient. This utility function will be essential for the middleware to process and potentially transform responses containing discussion IDs.
Some observations:
- Modern async iteration is used instead of event listeners
- Proper error handling with informative messages
- Efficiently collects and processes chunks of data
frontend/src/features/timeline-schedule/api/index.ts (1)
8-8: Type change looks good: number → string for discussionId parameterThe change from number to string for the discussionId parameter aligns with the PR objective of encoding discussion IDs. This ensures compatibility with the new encoded format being used throughout the application.
frontend/server/utils/stringTransformer.js (2)
19-33: Base64URL implementation looks goodThe base64Url implementation correctly handles URL-safe encoding and decoding, including proper padding management. The code is well-commented and follows best practices.
11-17:Details
❓ Verification inconclusive
Non-cryptographic hash function implementation
The implemented hash function is a simple non-cryptographic hash that may not be suitable for security-sensitive operations. If this is used for any security purpose, consider using a more robust cryptographic hash function from the Node.js crypto module.
🏁 Script executed:
#!/bin/bash # Check if this hash function is used in security contexts rg -A 3 -B 3 "hash\(" --glob="!*/node_modules/*" --glob="!*/frontend/server/utils/stringTransformer.js"Length of output: 105
Attention: Verify Non-Cryptographic Hash Usage
In
frontend/server/utils/stringTransformer.js(lines 11–17), thehashfunction is implemented as a simple non-cryptographic hash. This implementation is typically acceptable for operations like caching or general text transformation. However, it is important to ensure that this function is not inadvertently used in any security-sensitive context.
- The initial grep check (excluding this file) produced no results, which suggests that the function is not invoked in contexts that imply cryptographic responsibility.
- Action Required: Please manually verify all usages of the
hashfunction within the codebase to confirm it isn’t being used for any security-critical operations. If it turns out that security-sensitive hashing is required, consider replacing this implementation with a robust cryptographic hash function from Node.js’scryptomodule.frontend/server/index.js (2)
20-23: Middleware setup for discussion ID transformation looks goodThe addition of express.json() middleware and the discussionIdTransformer for API routes is well-structured. The JSON parsing middleware is correctly placed before the transformer to ensure request bodies are properly parsed.
29-33: API route bypass logic properly implementedThe conditional check to bypass the HTML rendering logic for API routes is correct and necessary for the middleware to handle API requests appropriately.
frontend/server/utils/cipher.js (1)
1-64:Details
✅ Verification successful
Consider the randomness impact on user experience
As mentioned in the PR objectives, using a random nonce (IV) in AES-GCM means the same input will produce different outputs each time. While this enhances security, it may impact user experience if users expect consistent output for the same ID.
If consistent URLs are a priority, you might want to implement an alternative approach such as using a deterministic IV derived from the discussion ID itself, though this would reduce security. The current implementation prioritizes security, which is generally a good choice.
🌐 Web query:
What are the security implications of using a deterministic IV in AES-GCM encryption?💡 Result:
Using a deterministic Initialization Vector (IV) in AES-GCM encryption introduces critical security considerations. AES-GCM requires IV uniqueness for each encryption under the same key to prevent catastrophic security failures. Here’s a breakdown of the implications:
Key Security Risks
IV Reuse Vulnerability:
- Reusing an IV with the same key allows attackers to recover the authentication key ( H ), compromising both confidentiality and integrity[2][9]. For example, if two ciphertexts share an IV and key, XORing them reveals the XOR of their plaintexts[10].
- IV collisions also reduce security bounds, enabling practical attacks like polynomial root-solving to decrypt data without knowing the key[3][4].
Predictability Concerns:
- Deterministic IVs (e.g., counters) risk predictability if not securely implemented. Predictable IVs simplify algebraic cryptanalysis, especially in large-scale systems[1][6].
- Example: AWS encountered operational risks when systems encrypted ( 2^{32} ) messages in under two seconds, overwhelming counter-based IVs and forcing frequent rekeying[4].
Multi-User Weaknesses:
- Distributed systems sharing keys face challenges partitioning IV counter spaces. Predictable IVs across users/applications exacerbate risks of cross-context attacks[4][7].
Operational Challenges
- State Management: Deterministic IVs require non-volatile storage to track counters securely, complicating FIPS compliance and disaster recovery[4].
- Scalability Limits: Systems with high throughput (e.g., IoT networks) risk counter exhaustion, leading to forced key rotations or IV reuse[4][6].
- Complexity in Uniqueness Guarantees: Unique IV generation demands coordination in distributed environments, increasing design complexity[1][6].
Best Practices for Mitigation
- Use Random IVs: A 96-bit random IV reduces collision risk (birthday bound of ( 2^{-32} )) and simplifies implementation[5][6].
- Hybrid Deterministic-Random IVs: Combine a fixed secret XORed with a counter to add unpredictability while maintaining uniqueness[1][6].
- Key Rotation: Limit key usage to ( 2^{32} ) encryptions to avoid counter overflow[4][6][10].
Comparative Security
Aspect Deterministic IV Random IV Uniqueness Required via counters/fixed fields Probabilistic (birthday bound) Predictability Risk if implementation is flawed High entropy by design Operational Overhead Complex state management Simpler, stateless In summary, deterministic IVs in AES-GCM introduce risks of IV reuse and predictability, demanding rigorous implementation safeguards. Random IVs or hybrid approaches with secret salts are preferred for most applications[1][4][6]. Systems requiring determinism (e.g., file encryption) must enforce strict IV uniqueness and key rotation policies.
Citations:
- 1: https://www.ietf.org/archive/id/draft-mcgrew-iv-gen-00.html
- 2: https://csrc.nist.gov/csrc/media/projects/block-cipher-techniques/documents/bcm/comments/cwc-gcm/ferguson2.pdf
- 3: https://rules.sonarsource.com/java/type/vulnerability/rspec-6432/?search=equals
- 4: https://csrc.nist.gov/csrc/media/Events/2023/third-workshop-on-block-cipher-modes-of-operation/documents/accepted-papers/Practical%20Challenges%20with%20AES-GCM.pdf
- 5: https://www.haikel-fazzani.eu.org/blog/post/aes-encryption-modes-gcm-cbc-ctr
- 6: https://blog.secure-monkey.com/considerations-when-using-aes-gcm-for-encrypting-files/
- 7: https://eprint.iacr.org/2016/564.pdf
- 8: https://connect2id.com/blog/deterministic-encryption-with-aes-siv
- 9: https://www.elttam.com/blog/key-recovery-attacks-on-gcm/
- 10: https://nvlpubs.nist.gov/nistpubs/legacy/sp/nistspecialpublication800-38d.pdf
Security Versus Consistency in IV Generation
The current implementation correctly uses a random 12-byte IV to ensure unique nonces for each encryption, a critical requirement for AES-GCM's security. While this randomness means that encrypting the same input will produce different outputs—potentially impacting scenarios where consistent output (e.g., consistent URLs) is expected—switching to a deterministic IV (such as one derived from a discussion ID) would expose the system to significant vulnerabilities. Deterministic IVs can lead to IV reuse and make the encryption predictable, thereby weakening both confidentiality and integrity.
If consistent output is a business requirement, any alternative approach must incorporate additional safeguards (such as incorporating secret salts or designing a hybrid method) to mitigate these risks. However, such changes should be weighed very carefully against the potential for catastrophic security failures that deterministic IVs can introduce.
frontend/src/features/discussion/ui/DiscussionInviteCard/index.tsx (1)
15-15: Type change from number to string looks goodThe change from
numbertostringfordiscussionIdproperly supports the new ID encoding approach. This change aligns with the PR objective to implement encoded discussion IDs.Also applies to: 64-64
frontend/src/pages/MyCalendarPage/DiscussionContext.ts (1)
7-8: Type change from number to string is consistentThe change from
numbertostringfor bothselectedIdandsetSelectedIdmaintains consistency with the discussion ID encoding approach implemented across the codebase.frontend/server/middlewares/discussionIdTransformer/discussionCipher.js (1)
16-22:Details
❓ Verification inconclusive
AES-GCM produces different outputs for the same input
As noted in the PR objectives, using AES-GCM with a random nonce (IV) will produce different ciphertexts for the same input. This is secure but may create UX issues if users expect consistent URLs.
If consistent output is required, consider using a deterministic encryption approach or a fixed IV (though this reduces security). Please verify which approach better suits your requirements:
🌐 Web query:
What are the security implications of using a fixed IV with AES-GCM versus using a random IV?💡 Result:
Using a fixed IV with AES-GCM creates catastrophic security risks, while random IVs introduce probabilistic risks that require careful management. Here's a breakdown of the implications:
Fixed IV Security Risks
- Authentication breakdown: Reusing an IV (nonce) with the same key in AES-GCM allows attackers to forge valid authentication tags, compromising data integrity. This enables tampering with ciphertext undetected[3][6].
- Confidentiality compromise: A reused IV causes reuse of the CTR keystream, allowing attackers to:
- Recover plaintext via XOR analysis of ciphertexts[3][6]
- Derive the authentication key (H) if two messages use the same IV[6]
- Practical attacks: Demonstrated exploits include web application vulnerabilities where static nonces enabled forged ciphertext injection and data exfiltration[6].
Random IV Considerations
Collision probability: With 96-bit IVs, the birthday paradox implies a ≈50% collision chance after encrypting 2^48 messages[1][3]. In multi-key systems (e.g., cloud services), this risk grows exponentially:
Keys Messages/Key Total Collision Risk 1 2^32 2^-32 128 2^29 2^-26[1] Operational challenges:
- Requires strict key rotation policies to maintain security bounds[1][5]
- High-throughput systems (>2^32 messages/sec) can't practically rotate keys fast enough[1]
Comparative Risks
Aspect Fixed IV Random IV Authentication Completely broken on reuse Secure if IVs are unique Confidentiality Keystream reuse allows plaintext recovery Secure until IV collision occurs Practicality Simple implementation Requires robust IV management NIST Compliance Prohibited Allowed with usage limits[1][4] Mitigation Strategies
- Use deterministic IV constructions like AES-GCM-SIV (though not FIPS-approved)[1]
- Implement key derivation schemes to reduce IV reuse probability[1]
- Combine 12-byte fixed prefix with 4-byte counter for high-volume systems[4][5]
- Avoid AES-GCM for extremely high-throughput use cases (>1 billion encryptions/key)[1]
The choice between fixed and random IVs isn't binary – deterministic approaches with counters often provide better security than purely random IVs in large-scale systems[1][5]. However, even deterministic implementations must ensure global uniqueness across distributed environments to prevent counter reuse[1][4].
Citations:
- 1: https://csrc.nist.gov/csrc/media/Events/2023/third-workshop-on-block-cipher-modes-of-operation/documents/accepted-papers/Practical%20Challenges%20with%20AES-GCM.pdf
- 2: https://en.wikipedia.org/wiki/Galois/Counter_Mode
- 3: https://stackered.com/blog/iv-mishandling/
- 4: https://www.techtarget.com/whatis/definition/initialization-vector-IV
- 5: https://www.seald.io/blog/3-common-mistakes-when-implementing-encryption
- 6: https://www.elttam.com/blog/key-recovery-attacks-on-gcm/
Action Required: Confirm Encryption Consistency Requirements vs. Security Tradeoffs
The current implementation correctly generates a random IV with AES-GCM, ensuring robust security by producing non-deterministic ciphertexts even for identical inputs. Note that this non-determinism may lead to UX challenges if consistent URLs or identifiers are expected. However, switching to a fixed IV to achieve determinism is strongly discouraged due to severe security risks (e.g., keystream reuse, authentication tag forgery, and potential plaintext recovery).
- Security Impact: A fixed IV undermines AES-GCM's guarantees, as repeated nonces allow attackers to compromise both confidentiality and integrity.
- Alternatives: For consistent outputs without sacrificing security, consider:
- A deterministic encryption mode (such as AES-GCM-SIV), or
- A nonce construction strategy (e.g., combining a fixed prefix with a counter) that ensures uniqueness.
- Next Steps: Verify if your application's requirements favor deterministic outputs over the inherent randomness of secure AES-GCM. If consistent output is mandatory, adopt a deterministic encryption scheme rather than resorting to a fixed IV.
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Proper type update for encoded discussion ID
The change from number to string for discussionId in the HeaderProps interface aligns with the encryption implementation. However, I notice that while discussionId is defined in the interface, it's not actually used in the component rendering.
🏁 Script executed:
#!/bin/bash
# Check if discussionId is used elsewhere in the Header component or its children
rg -A 2 -B 2 "discussionId" --type tsx frontend/src/pages/CandidateSchedulePage/Header.tsx
rg -A 2 -B 2 "discussionId" --type tsx frontend/src/features/discussion/ui/DiscussionConfirmButtonLength of output: 245
Action Required: Unused Discussion ID Prop Detected
The type update for discussionId (changed from number to string) properly aligns it with the encryption implementation. However, a revised search using a broader file pattern indicates that discussionId is defined in the HeaderProps interface but isn’t referenced in the component’s rendering or its immediate children. Please verify whether this prop is intended for future use; if not, consider removing it to keep the interface clean.
- Confirm if
discussionIdshould be integrated into the rendering logic. - If the prop is redundant, remove it from the interface.
frontend/server/utils/cipher.js
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error message construction issue in decrypt function
Similar to the encrypt function, the Error constructor doesn't concatenate arguments in the decrypt function.
- throw new Error('aesGcm: decrypt failed with error:', err);
+ throw new Error(`aesGcm: decrypt failed with error: ${err.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.
| throw new Error('aesGcm: decrypt failed with error:', err); | |
| throw new Error(`aesGcm: decrypt failed with error: ${err.message}`); |
frontend/server/utils/cipher.js
Outdated
There was a problem hiding this comment.
🛠️ Refactor suggestion
Error message construction issue
The Error constructor doesn't concatenate arguments. The error details will be lost with the current implementation.
- throw new Error('aesGcm: encrypt failed with error:', err);
+ throw new Error(`aesGcm: encrypt failed with error: ${err.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.
| throw new Error('aesGcm: encrypt failed with error:', err); | |
| throw new Error(`aesGcm: encrypt failed with error: ${err.message}`); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for invalid cipherText
The decryptDiscussionId function will throw an error if cipherText is invalid or malformed. Consider adding try-catch blocks to handle potential decryption failures gracefully.
export const decryptDiscussionId = (cipherText) => {
- const key = Buffer.from(serverEnv.AES_GCM_KEY_OF_DISCUSSION_ID, 'base64');
- const concat = base64Url.decode(cipherText);
- const [iv, content, tag] = concat.split('.');
- const encryptedData = { iv, content, tag };
- return aesGcm.decrypt(encryptedData, key);
+ try {
+ const key = Buffer.from(serverEnv.AES_GCM_KEY_OF_DISCUSSION_ID, 'base64');
+ const concat = base64Url.decode(cipherText);
+ const [iv, content, tag] = concat.split('.');
+ // Validate that all components are present
+ if (!iv || !content || !tag) {
+ throw new Error('Invalid cipher format');
+ }
+ const encryptedData = { iv, content, tag };
+ return aesGcm.decrypt(encryptedData, key);
+ } catch (error) {
+ console.error('Failed to decrypt discussion ID:', error);
+ return cipherText; // Return original or a fallback value
+ }📝 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.
| export const decryptDiscussionId = (cipherText) => { | |
| const key = Buffer.from(serverEnv.AES_GCM_KEY_OF_DISCUSSION_ID, 'base64'); | |
| const concat = base64Url.decode(cipherText); | |
| const [iv, content, tag] = concat.split('.'); | |
| const encryptedData = { iv, content, tag }; | |
| return aesGcm.decrypt(encryptedData, key); | |
| }; | |
| export const decryptDiscussionId = (cipherText) => { | |
| try { | |
| const key = Buffer.from(serverEnv.AES_GCM_KEY_OF_DISCUSSION_ID, 'base64'); | |
| const concat = base64Url.decode(cipherText); | |
| const [iv, content, tag] = concat.split('.'); | |
| // Validate that all components are present | |
| if (!iv || !content || !tag) { | |
| throw new Error('Invalid cipher format'); | |
| } | |
| const encryptedData = { iv, content, tag }; | |
| return aesGcm.decrypt(encryptedData, key); | |
| } catch (error) { | |
| console.error('Failed to decrypt discussion ID:', error); | |
| return cipherText; // Return original or a fallback value | |
| } | |
| }; |
There was a problem hiding this comment.
@coderabbitai
aesGcm 모듈 내부에서 encrypt, decrypt 시 에러 핸들링을 수행하고 있습니다. decryptDiscussionId에서도 에러 핸들링을 해주어야 할 필요가 있을까요?
There was a problem hiding this comment.
@dioo1461 aesGcm 모듈 내부에 에러 핸들링이 있지만, 해당 모듈은 에러를 잡아서 처리하기보다는 다시 throw하는 방식으로 구현되어 있습니다:
// aesGcm.decrypt 내부
try {
// 복호화 로직...
} catch (err) {
throw new Error(`Decryption failed: ${err.message}`);
}decryptDiscussionId 함수에서 추가 에러 핸들링이 필요한 이유는:
- base64Url.decode나 split 등의 작업에서 발생하는 에러는 aesGcm에서 처리하지 않습니다
- URL 처리 과정에서 사용되는 함수이므로 실패 시 전체 요청 처리가 중단될 수 있습니다
- 잘못된 형식의 cipherText가 들어올 경우 애플리케이션 흐름이 중단되지 않도록 하는 것이 안전합니다
에러 핸들링을 추가하면 비정상적인 입력에도 애플리케이션이 계속 작동할 수 있어 사용자 경험이 향상됩니다.
✏️ Learnings added
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#365
File: frontend/server/middlewares/discussionIdTransformer/discussionCipher.js:24-30
Timestamp: 2025-03-28T06:06:04.491Z
Learning: The decryptDiscussionId function can benefit from additional error handling despite the existing error handling in the aesGcm module, since aesGcm rethrows errors rather than handling them with fallbacks. Error handling for the URL processing-related functions improves application resilience.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling with specific error types
The current try-catch block catches all errors and falls back to returning the original data. Consider adding more granular error handling for better debugging.
try {
const result = manipulateValueOfObject(
targetKey,
JSON.parse(stringData),
encryptDiscussionId,
);
res.end(JSON.stringify(result));
} catch (err) {
- // 변환 실패 시 원본 데이터를 그대로 반환
+ // Log the specific error type for better debugging
+ console.error('Error processing response:', err.message);
+ if (err instanceof SyntaxError) {
+ console.warn('Failed to parse JSON response');
+ } else {
+ console.warn('Failed to manipulate or encrypt discussion ID');
+ }
+ // Return original data if transformation fails
res.end(stringData);
};📝 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.
| export const handleProxyRes = async (proxyRes, req, res) => { | |
| const stringData = await collectResponse(proxyRes); | |
| res.statusCode = proxyRes.statusCode; | |
| res.setHeader('Content-Type', proxyRes.headers['Content-type'] || 'application/json'); | |
| // discussion id 필드명을 'discussionId'가 아닌 'id'로 반환하는 API가 존재하여, 이에 대해 분기처리 | |
| const isDiscussionApi = ( | |
| req.url === DISCUSSION_CREATE_ENDPOINT || DISCUSSION_GET_ENDPOINT_REGEX.test(req.url) | |
| ); | |
| const targetKey = isDiscussionApi ? 'id' : 'discussionId'; | |
| try { | |
| const result = manipulateValueOfObject( | |
| targetKey, | |
| JSON.parse(stringData), | |
| encryptDiscussionId, | |
| ); | |
| res.end(JSON.stringify(result)); | |
| } catch (err) { | |
| // 변환 실패 시 원본 데이터를 그대로 반환 | |
| res.end(stringData); | |
| }; | |
| }; | |
| export const handleProxyRes = async (proxyRes, req, res) => { | |
| const stringData = await collectResponse(proxyRes); | |
| res.statusCode = proxyRes.statusCode; | |
| res.setHeader('Content-Type', proxyRes.headers['Content-type'] || 'application/json'); | |
| // discussion id 필드명을 'discussionId'가 아닌 'id'로 반환하는 API가 존재하여, 이에 대해 분기처리 | |
| const isDiscussionApi = ( | |
| req.url === DISCUSSION_CREATE_ENDPOINT || DISCUSSION_GET_ENDPOINT_REGEX.test(req.url) | |
| ); | |
| const targetKey = isDiscussionApi ? 'id' : 'discussionId'; | |
| try { | |
| const result = manipulateValueOfObject( | |
| targetKey, | |
| JSON.parse(stringData), | |
| encryptDiscussionId, | |
| ); | |
| res.end(JSON.stringify(result)); | |
| } catch (err) { | |
| // Log the specific error type for better debugging | |
| console.error('Error processing response:', err.message); | |
| if (err instanceof SyntaxError) { | |
| console.warn('Failed to parse JSON response'); | |
| } else { | |
| console.warn('Failed to manipulate or encrypt discussion ID'); | |
| } | |
| // Return original data if transformation fails | |
| res.end(stringData); | |
| }; | |
| }; |
frontend/server/envconfig.js
Outdated
There was a problem hiding this comment.
p3: .env 로 변경해야 할 거 같아요. 암호화 키 값은 FE_ENV에 저장하면 좋을 거 같아요
login 요청에 대해 path를 'api/v1/login' 이 아닌 'v1/login'으로 식별하여 was에 전송하던 것이 원인이었음
dioo1461
force-pushed
the
feature/fe/encode-discussion-id
branch
from
a189181 to
bd63602
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (3)
frontend/apps/client/src/features/shared-schedule/model/upcomingSchedules.ts (1)
21-22: 🛠️ Refactor suggestionRemove the lingering
numberunion before it leaks back inThe rest of the PR migrates
discussionIdtostring; keepingz.number()here defeats the type-safety you just gained (callers will continue to send numbers).- // TODO: discussionId 인코딩 적용 후 z.string()으로 변경 - discussionId: z.union([z.string(), z.number()]), + discussionId: z.string(), // encoded discussion idEven if the BE still returns numbers in niche paths, it is better to coerce at the boundary than relax the contract here.
frontend/apps/client/src/features/discussion/api/index.ts (1)
61-71:⚠️ Potential issue
response.parsewill crash – wrong object invoked
request.*returns the server payload, which has noparsemethod.
We still need the Zod schema (or equivalent) to perform validation.- const response = await request.post(`/api/v1/discussion/${id}/confirm`, { body }); - return response.parse(response); + const raw = await request.post(`/api/v1/discussion/${id}/confirm`, { body }); + return DiscussionConfirmResponse.parse(raw);Same fix applies to
getDiscussionConfirm.Remember to import the value (not
type) ofDiscussionConfirmResponse:-import type { ... , DiscussionConfirmResponse, ... } from '../model'; +import { DiscussionConfirmResponse } from '../model'; +import type { + DiscussionCalendarRequest, + DiscussionCalendarResponse, + DiscussionConfirmRequest, + /* rest unchanged */ +} from '../model';Without this adjustment, the code will not compile.
frontend/apps/client/src/features/discussion/api/invitationApi.ts (1)
6-17: 💡 Verification agent🧩 Analysis chain
API signature updated – verify call-sites & consider spelling fix
- All callers must now pass a
string. A quick ripgrep search can confirm there are no lingering numeric arguments.- The method name
postInviatationJoincontains a typo (“Inviatation”). Consider renaming topostInvitationJoinbefore the typo spreads.-export const invitationApi = { - getInvitationInfo: async (discussionId: string) => { +export const invitationApi = { + getInvitationInfo: async (discussionId: string) => { ... - postInviatationJoin: async (discussionId: string, password?: string) => { + postInvitationJoin: async (discussionId: string, password?: string) => {
🏁 Script executed:
#!/bin/bash set -e echo "Searching for getInvitationInfo calls" rg -n -C2 "getInvitationInfo\(" echo "Searching for postInviatationJoin calls" rg -n -C2 "postInviatationJoin\(" echo "Searching for postInvitationJoin calls" rg -n -C2 "postInvitationJoin\(" echo "Searching for any invitationApi usage" rg -n -C2 "invitationApi"Length of output: 3613
Spelling fix & call‐site update for invitationApi
- Rename the method in
frontend/apps/client/src/features/discussion/api/invitationApi.ts- postInviatationJoin: async (discussionId: string, password?: string) => { + postInvitationJoin: async (discussionId: string, password?: string) => {- Update its invocation in
frontend/apps/client/src/features/discussion/api/mutations.ts (around line 34)- invitationApi.postInviatationJoin(body.discussionId, body.password) + invitationApi.postInvitationJoin(body.discussionId, body.password)- Verified all existing callers (e.g. in
queryOptions.tsandmutations.ts) passdiscussionIdas a string.- No numeric‐literal invocations of
getInvitationInfowere found.
🧹 Nitpick comments (8)
frontend/apps/client/src/features/shared-schedule/model/upcomingSchedules.ts (1)
8-14: Consider introducing a sharedEncodedDiscussionIdSchemafor consistency
discussionIdis now astringin several schemas. Copy-pasting the literalz.string()everywhere will become error-prone once you need tighter validation (e.g. non-empty, base64 check).
A small helper such as:// @utils/zodSchemas.ts export const EncodedDiscussionIdSchema = z.string().min(1);would let you replace every occurrence with
EncodedDiscussionIdSchema, guaranteeing that a future change is one-liner.frontend/apps/client/src/features/shared-schedule/model/ongoingSchedules.ts (1)
5-5: Reuse a central schema instead of hard-codingSame comment as before – replacing
z.string()with a sharedEncodedDiscussionIdSchemakeeps all schedule models in sync and prevents accidental divergence.frontend/apps/client/src/features/timeline-schedule/model/index.ts (1)
34-34: Validate the encoded formatIf the encrypted ID is always URL-safe base64, consider tightening the schema with a regex:
discussionId: z.string().regex(/^[A-Za-z0-9_-]+$/),so malformed IDs are caught early.
frontend/apps/client/src/features/shared-schedule/model/finishedSchedules.ts (1)
6-6: Nit: keep the comment trail consistentFor parity with the other models, drop a short comment explaining that the ID is an AES-GCM + base64 token. Future readers will not have to dig through the PR.
frontend/apps/client/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx (1)
34-34: Avoid redundanttoString()on an already‐string ID
discussionIdis now typed asstring, so the extra conversion is unnecessary:- navigate({ to: '/discussion/$id', params: { id: discussionId.toString() } }); + navigate({ to: '/discussion/$id', params: { id: discussionId } });Small win: one less allocation and slightly clearer intent.
frontend/apps/client/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (1)
28-28: Redundant.toString()calls can be dropped
discussionIdis already astring; calling.toString()twice is unnecessary extra work and a small readability hit.-const { discussion } = useDiscussionQuery(discussionId.toString()); +const { discussion } = useDiscussionQuery(discussionId); -<Link params={{ id: discussionId.toString() }} to='/discussion/$id'> +<Link params={{ id: discussionId }} to='/discussion/$id'>Also applies to: 55-55
frontend/apps/client/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx (2)
24-27:.toString()is no longer required here either
discussion.idis alreadystring; drop the conversion for consistency.-const { calendar: candidates = [] } = useDiscussionCalendarQuery( - discussion.id.toString(), { size: 3 }, ONGOING_SCHEDULE_DETAIL_GC_TIME, -); +const { calendar: candidates = [] } = useDiscussionCalendarQuery( + discussion.id, { size: 3 }, ONGOING_SCHEDULE_DETAIL_GC_TIME, +);
58-71: Prop & router params: remove redundant conversions
After changingdiscussionIdtostring, these conversions can be simplified.-Link - params={{ id: discussionId.toString() }} +Link + params={{ id: discussionId }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between a189181ad5fb1e318f2b3a8218dd436edb099b92 and bd63602.
📒 Files selected for processing (16)
frontend/.gitignore(1 hunks)frontend/apps/client/src/features/discussion/api/index.ts(2 hunks)frontend/apps/client/src/features/discussion/api/invitationApi.ts(1 hunks)frontend/apps/client/src/features/discussion/api/keys.ts(1 hunks)frontend/apps/client/src/features/discussion/api/mutations.ts(1 hunks)frontend/apps/client/src/features/discussion/api/queries.ts(1 hunks)frontend/apps/client/src/features/discussion/api/queryOptions.ts(1 hunks)frontend/apps/client/src/features/discussion/model/index.ts(2 hunks)frontend/apps/client/src/features/discussion/ui/DiscussionInviteCard/SubmitForm.tsx(1 hunks)frontend/apps/client/src/features/discussion/ui/DiscussionInviteCard/index.tsx(2 hunks)frontend/apps/client/src/features/shared-schedule/model/finishedSchedules.ts(1 hunks)frontend/apps/client/src/features/shared-schedule/model/ongoingSchedules.ts(1 hunks)frontend/apps/client/src/features/shared-schedule/model/upcomingSchedules.ts(1 hunks)frontend/apps/client/src/features/shared-schedule/ui/OngoingSchedules/RecommendedSchedules.tsx(1 hunks)frontend/apps/client/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx(1 hunks)frontend/apps/client/src/features/timeline-schedule/model/index.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/.gitignore
- frontend/apps/client/src/features/discussion/ui/DiscussionInviteCard/index.tsx
🔇 Additional comments (8)
frontend/apps/client/src/features/discussion/api/queries.ts (1)
111-112:Details
✅ Verification successful
Double-check downstream typing of
discussionId
useInviteInfoQuerynow expects astring, but the underlyinginvitationQueryOption→invitationApi.getInvitationInfochain might still declare the parameter asnumber.
A quick grep will spot any mismatches:If anything shows up, bump them to
stringto avoid silentanywidening.
🏁 Script executed:
#!/bin/bash # Find TypeScript declarations that still accept number for getInvitationInfo / discussionId rg -n $'(getInvitationInfo|getInvitationJoin|postInviatationJoin).*number'Length of output: 75
Let’s dig into the actual declarations for
invitationQueryOptionand the API methods (the original grep may have missed typos/spelling). We’ll pull a few lines around each definition:
🏁 Script executed:
#!/bin/bash # Show the signature & usage of invitationQueryOption rg -n "invitationQueryOption" -C 3 # Find and inspect the API methods that take discussionId rg -n "(getInvitationInfo|getInvitationJoin|postInvitationJoin)" -C 5Length of output: 11972
Type consistency confirmed for
discussionIdAll downstream functions—including
invitationQueryOption,invitationApi.getInvitationInfo, andpostInviatationJoin—are already declared withdiscussionId: string. A code-wide search found no remainingnumberparameters for these methods, so no further changes are needed.frontend/apps/client/src/features/discussion/api/queryOptions.ts (1)
8-11: Confirm that all call-sites now pass astringThe signature switch to
stringlooks good, but any lingeringnumberusages will now type-error or forceas unknown as. Please run a quick grep/TS compile to ensure nothing still expectsnumber.frontend/apps/client/src/features/discussion/api/keys.ts (1)
27-27: LGTM – consistentstringkeyKey generator correctly mirrors the new ID type.
frontend/apps/client/src/features/discussion/api/mutations.ts (1)
30-35: Mutation type updated – ensure backend accepts string
discussionIdis now forwarded as astring; verify thatinvitationApi.postInviatationJoinand the backend route also expect a stringified ID.frontend/apps/client/src/features/shared-schedule/ui/OngoingSchedules/ScheduleDetails.tsx (2)
23-23: Prop type update is consistent with the global refactor
discussionIdis now declared asstring, matching the new model definition. 👍
47-51: Consider normalisingdiscussion.idto avoid mixed-type URIs
discussion.idis now typed asstring; confirm that no.toString()is hiding an underlying number elsewhere to keep URL generation predictable.frontend/apps/client/src/features/discussion/model/index.ts (2)
40-41: Conversion ofidtostringmatches API encryption output
Good alignment with the encrypted / randomised ID feature.
80-90:Details
❌ Incorrect review comment
DiscussionConfirmResponseschema is no longer exported – potential breaking change
Other modules that previously imported this schema for runtime validation will now fail to compile.-const DiscussionConfirmResponse = z.object({ ... }); +export const DiscussionConfirmResponse = z.object({ ... });
🏁 Script executed:
#!/bin/bash set -e echo "=== Export statements in model/index.ts ===" rg "^export" -n frontend/apps/client/src/features/discussion/model/index.ts || true echo echo "=== File content lines 60-120 in model/index.ts ===" sed -n '60,120p' frontend/apps/client/src/features/discussion/model/index.ts || true echo echo "=== Occurrences of DiscussionConfirmResponse in codebase ===" rg "DiscussionConfirmResponse" -n .Length of output: 4662
Ignore: No value-level imports of the schema exist
- Only the type alias (
export type DiscussionConfirmResponse) is imported elsewhere viaimport type- The Zod schema (
const DiscussionConfirmResponse = …) isn’t used or imported in any other module- No runtime imports are broken, so no change is needed here
Likely an incorrect or invalid review comment.
|
변경사항 있습니다!!
/+ 현재 client 패키지에서 사용하는 .env 파일 위치가 frontend 루트로 지정돼 있는데, client 워크스페이스 루트에서 .env를 관리하면 어떨까 싶습니다! |
hamo-o
left a comment
hamo-o
left a comment
There was a problem hiding this comment.
수고하셨습니다!!
ㅜㅜ 서버는 진짜 코드 읽으면서 배우네요. 코드 스타일 관련해서만 피드백 남겼습니다..!
+ discussionIdTransformer 폴더 대신, middlewares/discussionId/ 만들어서 도메인별로 미들웨어를 관리하면 어떨까요?
There was a problem hiding this comment.
p3;
/ 이나 . 등을 상수로 관리해도 좋을 것 같습니다.
There was a problem hiding this comment.
/는 URL 문법에 따른 분리자이므로 상수화하는 게 오히려 가독성을 해칠 수 있을 것 같습니다..! 다만 암호문의 분리자로 사용하는 .는 유지보수 측면에서 효과가 있을 것 같아요. 반영하겠습니다!
There was a problem hiding this comment.
넵 좋습니다!
그런데 생각해 보니 함수명과 매개변수명의 url 이라는 워딩이 조금 어색한 것 같습니다. path로 대체하면 어떨까요?
There was a problem hiding this comment.
아 좋습니다! path로 명명하는 게 더 명확하겠네요
| if (!targetObject || typeof targetObject !== 'object') { | ||
| return targetObject; | ||
| } |
There was a problem hiding this comment.
p3; 요 조건을 최상단으로 올리는게 가독성이 더 좋을 것 같아요.
| const newData: Record<string, unknown> = {}; for (const key in targetObject) { | ||
| // 만약 target key이면 해당 값에 대해 변환 함수 적용 | ||
| if (key === targetKey) { | ||
| newData[key] = manipulateFunc(String(targetObject[key])); | ||
| } else { | ||
| // 그 외의 값은 재귀적으로 처리 | ||
| newData[key] = manipulateValueOfObject(targetKey, targetObject[key], manipulateFunc); | ||
| } | ||
| } | ||
| return newData as T; |
There was a problem hiding this comment.
p3;
reduce 고차함수 활용이 더 깔끔해 보입니다!
| const newData: Record<string, unknown> = {}; for (const key in targetObject) { | |
| // 만약 target key이면 해당 값에 대해 변환 함수 적용 | |
| if (key === targetKey) { | |
| newData[key] = manipulateFunc(String(targetObject[key])); | |
| } else { | |
| // 그 외의 값은 재귀적으로 처리 | |
| newData[key] = manipulateValueOfObject(targetKey, targetObject[key], manipulateFunc); | |
| } | |
| } | |
| return newData as T; | |
| return Object.entries(targetObject).reduce((newData, [key, value]) => { | |
| if (key === targetKey) newData[key] = manipulateFunc(String(value)); | |
| else newData[key] = manipulateValueOfObject(targetKey, value, manipulateFunc); | |
| return newData; | |
| }, {} as Record<string, unknown>) as T; |
| while (encodedText.length % 4) { | ||
| encodedText += '='; | ||
| } |
There was a problem hiding this comment.
p3; 4의 배수 만들어주기. 요런 유틸로 따로 뺄 수 있을 것 같습니다.
| while (encodedText.length % 4) { | |
| encodedText += '='; | |
| } | |
| const padBase64 = (str: string): string => { | |
| const padLength = (4 - (str.length % 4)) % 4; | |
| return str + '='.repeat(padLength); | |
| }; |
| dts: false, | ||
| bundle: false, | ||
| bundle: true, | ||
| external: ['vite', 'dotenv'], |
There was a problem hiding this comment.
p3;
서버에서는 개발환경에서만 vite를 쓰고 있는데 넣으신 이유가 있을까요?
There was a problem hiding this comment.
if (!isProduction) {
const { createServer } = await import('vite');
...서버 코드에 dev일 때에만 vite를 동적 import하긴 하지만, tsup이 번들링할 때 런타임에서의 분기는 고려하지 않기 때문에 해당 구문(await import('vite'))까지 함께 처리하려고 시도합니다.
때문에 tsup은 vite 코드를 함께 번들링하려고 하는데, server 워크스페이스에는 vite가 설치되어있지 않기 때문에 빌드에 실패합니다.
tsup.config.ts에서 bundle: true로 정의하고 있었는데, 이 옵션을 키면 프로젝트에 포함된 모든 외부 의존성을 번들에 함께 묶는다고 합니다. 이를 false로 바꿔주어 해결했습니다.
저희 배포할 때 node_modules 설치해주는 과정이 있으니까 괜찮겠죠..?
+ 정정합니다.. bundle: false 설정하니 외부 의존성뿐만 아니라 index.js가 참조하는 모든 모듈을 번들에서 제외해버리네요..
결국 다시 원래대로 돌렸습니다. 더 좋은 방법을 알고 계시다면 알려주세요,,
There was a problem hiding this comment.
아 말씀하신게 맞습니다! 지금 코드 유지하면 될 것 같습니다.
| return Buffer.concat(chunks).toString('utf-8'); | ||
| } catch (err) { | ||
| if (err instanceof Error) { | ||
| throw new Error(`Failed to collect response: ${err.message}`); |
There was a problem hiding this comment.
추후에 서버에서 던져지는 에러들은 따로 클래스로 만들어도 좋을 것 같네요.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
frontend/apps/server/src/utils/stringTransformer.ts (1)
17-20: 패딩 함수 구현이 올바릅니다.Base64 패딩 계산 로직이 정확하게 구현되었습니다.
frontend/apps/server/src/utils/object.ts (1)
1-18: 구현이 깔끔하고 잘 작성되었습니다.이전 리뷰 의견이 잘 반영되어
reduce고차함수를 활용한 깔끔한 구현이 되었습니다. 제네릭 타입 보존과 재귀적 처리도 올바르게 구현되었습니다.frontend/apps/server/src/discussionIdTransformer/discussionCipher.ts (1)
30-36: 복호화 함수의 입력 검증 및 오류 처리 강화 필요학습된 피드백에 따르면,
aesGcm모듈이 오류를 다시 던지므로 추가적인 오류 처리가 필요합니다. 또한split('.')결과가 정확히 3개 요소를 포함하는지 검증해야 합니다.다음과 같이 개선해보세요:
export const decryptDiscussionId = (cipherText: string) => { + if (!cipherText || typeof cipherText !== 'string') { + throw new Error('암호문이 유효하지 않습니다.'); + } + const key = Buffer.from(aesGcmKey, 'base64'); - const concat = base64Url.decode(cipherText); - const [iv, content, tag] = concat.split('.'); + + try { + const concat = base64Url.decode(cipherText); + const parts = concat.split('.'); + + if (parts.length !== 3) { + throw new Error('암호문 형식이 올바르지 않습니다.'); + } + + const [iv, content, tag] = parts; + if (!iv || !content || !tag) { + throw new Error('암호문 구성 요소가 누락되었습니다.'); + } + const encryptedData = { iv, content, tag }; return aesGcm.decrypt(encryptedData, key); + } catch (error) { + throw new Error(`Discussion ID 복호화 실패: ${error instanceof Error ? error.message : '알 수 없는 오류'}`); + } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
frontend/apps/server/package.json(1 hunks)frontend/apps/server/src/discussionIdTransformer/discussionCipher.ts(1 hunks)frontend/apps/server/src/utils/object.ts(1 hunks)frontend/apps/server/src/utils/stringTransformer.ts(1 hunks)frontend/apps/server/vitest.config.ts(0 hunks)
💤 Files with no reviewable changes (1)
- frontend/apps/server/vitest.config.ts
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#365
File: frontend/server/middlewares/discussionIdTransformer/discussionCipher.js:24-30
Timestamp: 2025-03-28T06:06:04.683Z
Learning: The decryptDiscussionId function can benefit from additional error handling despite the existing error handling in the aesGcm module, since aesGcm rethrows errors rather than handling them with fallbacks. Error handling for the URL processing-related functions improves application resilience.
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#53
File: frontend/src/components/Input/MultiInput.tsx:50-59
Timestamp: 2025-02-04T03:10:35.809Z
Learning: User dioo1461 prefers to receive code reviews and responses in Korean language.
frontend/apps/server/package.json (1)
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#411
File: frontend/apps/client/package.json:7-7
Timestamp: 2025-05-27T17:33:56.448Z
Learning: In the Team4-enDolphin project, the server uses Vite as middleware in development mode, so the `dev` script in frontend/apps/client/package.json only needs to start the server (`pnpm --filter server start`) rather than running separate Vite dev server and server processes.
frontend/apps/server/src/discussionIdTransformer/discussionCipher.ts (1)
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#365
File: frontend/server/middlewares/discussionIdTransformer/discussionCipher.js:24-30
Timestamp: 2025-03-28T06:06:04.683Z
Learning: The decryptDiscussionId function can benefit from additional error handling despite the existing error handling in the aesGcm module, since aesGcm rethrows errors rather than handling them with fallbacks. Error handling for the URL processing-related functions improves application resilience.
🧬 Code Graph Analysis (1)
frontend/apps/server/src/discussionIdTransformer/discussionCipher.ts (3)
frontend/apps/server/src/envconfig.ts (1)
serverEnv(11-13)frontend/apps/server/src/utils/cipher.ts (1)
aesGcm(4-73)frontend/apps/server/src/utils/stringTransformer.ts (1)
base64Url(3-15)
🔇 Additional comments (4)
frontend/apps/server/package.json (2)
12-14:http-proxy-middleware런타임 의존성 추가 👍프록시 미들웨어 구현을 위해
http-proxy-middleware ^3.0.3을dependencies에 추가한 결정이 적절합니다. 해당 패키지는 자체적으로 타입 선언을 포함하므로 별도@types패키지가 필요 없습니다.
15-19: Vitest 설정 제거 → 테스트 공백 여부 확인 필요
@endolphin/vitest-config가 빠지면서 서버 애플리케이션의 테스트 러너 설정이 사라졌습니다.
테스트 커버리지가 완전히 소실되지 않았는지, 혹은 다른 위치(루트 등)로 통합했는지 확인을 권장합니다.frontend/apps/server/src/discussionIdTransformer/discussionCipher.ts (2)
7-10: 환경 변수 검증이 잘 구현되었습니다.모듈 로드 시점에 필수 환경 변수를 검증하는 것은 좋은 방식입니다.
22-28: 암호화 로직이 올바르게 구현되었습니다.AES-GCM 암호화와 Base64 URL-safe 인코딩을 조합한 구현이 보안 요구사항을 잘 충족합니다.
| export const decodeDiscussionIdOfUrl = (path: string) => { | ||
| // (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] | ||
| const parts = path.split('/'); | ||
| const targetIdx = 3; // discussionId가 위치하는 index | ||
| if (parts.length > targetIdx) { | ||
| parts[targetIdx] = decryptDiscussionId(parts[targetIdx]); | ||
| } | ||
| return parts.join('/'); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
URL 경로 파싱의 견고성 개선 필요
하드코딩된 인덱스 3을 사용하는 것은 다양한 URL 패턴에 대해 취약할 수 있습니다. 경로 구조가 예상과 다를 경우 잘못된 세그먼트가 복호화될 수 있습니다.
더 견고한 구현을 위해 다음과 같이 개선해보세요:
export const decodeDiscussionIdOfUrl = (path: string) => {
+ if (!path || typeof path !== 'string') {
+ return path;
+ }
const parts = path.split('/');
const targetIdx = 3; // discussionId가 위치하는 index
- if (parts.length > targetIdx) {
+ if (parts.length > targetIdx && parts[targetIdx]) {
+ try {
parts[targetIdx] = decryptDiscussionId(parts[targetIdx]);
+ } catch (error) {
+ console.warn('Discussion ID 복호화 실패:', error);
+ // 복호화 실패 시 원본 경로 반환
+ return path;
+ }
}
return parts.join('/');
};🤖 Prompt for AI Agents
In frontend/apps/server/src/discussionIdTransformer/discussionCipher.ts around
lines 12 to 20, the current code uses a hardcoded index 3 to locate the
discussionId in the URL path, which is fragile for varying URL structures. To
fix this, identify the 'discussion' segment dynamically in the path array, then
decrypt the segment immediately following it. This approach ensures the correct
segment is decrypted regardless of URL pattern changes, improving robustness.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/apps/server/src/middlewares/discussionIdTransformer/index.ts (1)
7-8: TODO 주석을 제거하세요이미
serverEnv.BASE_URL을 사용하여 환경변수로 관리하고 있으므로 TODO 주석이 더 이상 유효하지 않습니다.export const discussionIdTransformer = createProxyMiddleware({ - // TODO: env로 분리 target: `${serverEnv.BASE_URL}/api`,frontend/apps/server/src/middlewares/discussionIdTransformer/proxyHandlers.ts (1)
8-12: import 경로에서 .js 확장자를 제거하세요TypeScript 프로젝트에서 .js 확장자는 불필요하며 잠재적인 문제를 일으킬 수 있습니다.
import { decodeDiscussionIdOfUrl, decryptDiscussionId, encryptDiscussionId, -} from './discussionCipher.js'; +} from './discussionCipher';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
frontend/apps/server/src/envconfig.ts(1 hunks)frontend/apps/server/src/index.ts(2 hunks)frontend/apps/server/src/middlewares/discussionIdTransformer/discussionCipher.ts(1 hunks)frontend/apps/server/src/middlewares/discussionIdTransformer/index.ts(1 hunks)frontend/apps/server/src/middlewares/discussionIdTransformer/proxyHandlers.ts(1 hunks)frontend/apps/server/src/utils/isProduction.ts(1 hunks)frontend/apps/server/src/utils/stringTransformer.ts(1 hunks)frontend/apps/server/tsup.config.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/apps/server/src/utils/isProduction.ts
- frontend/apps/server/src/envconfig.ts
- frontend/apps/server/src/utils/stringTransformer.ts
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#365
File: frontend/server/middlewares/discussionIdTransformer/discussionCipher.js:24-30
Timestamp: 2025-03-28T06:06:04.683Z
Learning: The decryptDiscussionId function can benefit from additional error handling despite the existing error handling in the aesGcm module, since aesGcm rethrows errors rather than handling them with fallbacks. Error handling for the URL processing-related functions improves application resilience.
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#53
File: frontend/src/components/Input/MultiInput.tsx:50-59
Timestamp: 2025-02-04T03:10:35.809Z
Learning: User dioo1461 prefers to receive code reviews and responses in Korean language.
frontend/apps/server/tsup.config.ts (3)
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#426
File: frontend/packages/ui/vite.config.ts:6-0
Timestamp: 2025-06-13T07:39:57.840Z
Learning: `frontend/packages/ui/vite.config.ts` 에서 `defineConfig` 를 `vitest/config` 로부터 가져오는 것은 정상적인 패턴이다. `vitest/config` 는 Vite의 `defineConfig` 를 re-export 하며, `test` 옵션 타입을 추가로 제공한다.
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#411
File: frontend/apps/client/package.json:7-7
Timestamp: 2025-05-27T17:33:56.448Z
Learning: In the Team4-enDolphin project, the server uses Vite as middleware in development mode, so the `dev` script in frontend/apps/client/package.json only needs to start the server (`pnpm --filter server start`) rather than running separate Vite dev server and server processes.
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#89
File: frontend/src/routes/__root.tsx:18-20
Timestamp: 2025-02-04T09:08:12.489Z
Learning: In the Team4-enDolphin project, environment-specific features should be lazy-loaded and conditionally imported based on `defaultENV.MODE` to optimize bundle size and enable tree-shaking in production builds.
frontend/apps/server/src/index.ts (4)
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#89
File: frontend/src/routes/__root.tsx:18-20
Timestamp: 2025-02-04T09:08:12.489Z
Learning: In the Team4-enDolphin project, environment-specific features should be lazy-loaded and conditionally imported based on `defaultENV.MODE` to optimize bundle size and enable tree-shaking in production builds.
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#365
File: frontend/server/middlewares/discussionIdTransformer/discussionCipher.js:24-30
Timestamp: 2025-03-28T06:06:04.683Z
Learning: The decryptDiscussionId function can benefit from additional error handling despite the existing error handling in the aesGcm module, since aesGcm rethrows errors rather than handling them with fallbacks. Error handling for the URL processing-related functions improves application resilience.
Learnt from: hamo-o
PR: softeer5th/Team4-enDolphin#411
File: frontend/apps/client/package.json:7-7
Timestamp: 2025-05-27T17:33:56.448Z
Learning: In the Team4-enDolphin project, the server uses Vite as middleware in development mode, so the `dev` script in frontend/apps/client/package.json only needs to start the server (`pnpm --filter server start`) rather than running separate Vite dev server and server processes.
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#426
File: frontend/packages/ui/vite.config.ts:6-0
Timestamp: 2025-06-13T07:39:57.840Z
Learning: `frontend/packages/ui/vite.config.ts` 에서 `defineConfig` 를 `vitest/config` 로부터 가져오는 것은 정상적인 패턴이다. `vitest/config` 는 Vite의 `defineConfig` 를 re-export 하며, `test` 옵션 타입을 추가로 제공한다.
frontend/apps/server/src/middlewares/discussionIdTransformer/index.ts (1)
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#365
File: frontend/server/middlewares/discussionIdTransformer/discussionCipher.js:24-30
Timestamp: 2025-03-28T06:06:04.683Z
Learning: The decryptDiscussionId function can benefit from additional error handling despite the existing error handling in the aesGcm module, since aesGcm rethrows errors rather than handling them with fallbacks. Error handling for the URL processing-related functions improves application resilience.
frontend/apps/server/src/middlewares/discussionIdTransformer/discussionCipher.ts (1)
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#365
File: frontend/server/middlewares/discussionIdTransformer/discussionCipher.js:24-30
Timestamp: 2025-03-28T06:06:04.683Z
Learning: The decryptDiscussionId function can benefit from additional error handling despite the existing error handling in the aesGcm module, since aesGcm rethrows errors rather than handling them with fallbacks. Error handling for the URL processing-related functions improves application resilience.
frontend/apps/server/src/middlewares/discussionIdTransformer/proxyHandlers.ts (1)
Learnt from: dioo1461
PR: softeer5th/Team4-enDolphin#365
File: frontend/server/middlewares/discussionIdTransformer/discussionCipher.js:24-30
Timestamp: 2025-03-28T06:06:04.683Z
Learning: The decryptDiscussionId function can benefit from additional error handling despite the existing error handling in the aesGcm module, since aesGcm rethrows errors rather than handling them with fallbacks. Error handling for the URL processing-related functions improves application resilience.
🧬 Code Graph Analysis (3)
frontend/apps/server/src/index.ts (1)
frontend/apps/server/src/middlewares/discussionIdTransformer/index.ts (1)
discussionIdTransformer(6-16)
frontend/apps/server/src/middlewares/discussionIdTransformer/discussionCipher.ts (3)
frontend/apps/server/src/envconfig.ts (1)
serverEnv(15-18)frontend/apps/server/src/utils/cipher.ts (1)
aesGcm(4-73)frontend/apps/server/src/utils/stringTransformer.ts (1)
base64Url(3-15)
frontend/apps/server/src/middlewares/discussionIdTransformer/proxyHandlers.ts (3)
frontend/apps/server/src/middlewares/discussionIdTransformer/discussionCipher.ts (3)
decodeDiscussionIdOfUrl(12-20)decryptDiscussionId(30-36)encryptDiscussionId(22-28)frontend/apps/server/src/utils/object.ts (1)
manipulateValueOfObject(1-18)frontend/apps/server/src/utils/collectResponse.ts (1)
collectResponse(15-30)
🔇 Additional comments (5)
frontend/apps/server/tsup.config.ts (1)
11-12: 번들링 설정이 적절합니다!
bundle: true로 설정하고 동적으로 import되는vite와 환경변수 로딩을 위한dotenv를 external로 지정한 것은 올바른 접근입니다.frontend/apps/server/src/index.ts (1)
74-77: 미들웨어 설정이 올바르게 구성되었습니다!
express.json()을 프록시 미들웨어보다 먼저 적용한 것이 적절하며, 주석으로 각 미들웨어의 목적을 명확히 설명한 점이 좋습니다.frontend/apps/server/src/middlewares/discussionIdTransformer/discussionCipher.ts (1)
22-28: 암호화 구현이 적절합니다!AES-GCM 암호화 컴포넌트들을 점(.)으로 연결하고 URL-safe base64로 인코딩하는 방식이 안전합니다.
frontend/apps/server/src/middlewares/discussionIdTransformer/proxyHandlers.ts (2)
53-69: API 응답 처리가 실용적으로 구현되었습니다!서로 다른 API 엔드포인트의 응답 형식 차이를 적절히 처리하고 있으며, 변환 실패 시 원본 데이터를 반환하는 방어적 프로그래밍이 좋습니다.
17-22: rewriteDiscussionPath은 생성·조회 경로 모두 처리합니다
rewriteDiscussionPath가/v1/discussion접두사로 시작하는 모든 요청(/v1/discussion및/v1/discussion/{id})에 대해decodeDiscussionIdOfUrl을 호출하므로,DISCUSSION_GET_ENDPOINT_REGEX에 매칭되는 조회 경로도 정상적으로 처리됩니다. 별도 수정은 필요 없습니다.Likely an incorrect or invalid review comment.
| export const decodeDiscussionIdOfUrl = (path: string) => { | ||
| // (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] | ||
| const parts = path.split('/'); | ||
| const targetIdx = 3; // discussionId가 위치하는 index | ||
| if (parts.length > targetIdx) { | ||
| parts[targetIdx] = decryptDiscussionId(parts[targetIdx]); | ||
| } | ||
| return parts.join('/'); | ||
| }; |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
decryptDiscussionId 오류 처리를 추가하세요
decryptDiscussionId가 실패할 경우를 대비한 오류 처리가 필요합니다. 잘못된 형식의 ID나 복호화 실패 시 적절한 처리가 없으면 전체 요청이 실패할 수 있습니다.
export const decodeDiscussionIdOfUrl = (path: string) => {
// (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite']
const parts = path.split('/');
const targetIdx = 3; // discussionId가 위치하는 index
if (parts.length > targetIdx) {
- parts[targetIdx] = decryptDiscussionId(parts[targetIdx]);
+ try {
+ parts[targetIdx] = decryptDiscussionId(parts[targetIdx]);
+ } catch (error) {
+ // 복호화 실패 시 원본 경로 반환 또는 에러 전파 결정 필요
+ console.error('Discussion ID 복호화 실패:', error);
+ // Option 1: 원본 경로 반환
+ // return path;
+ // Option 2: 에러 재발생
+ throw error;
+ }
}
return parts.join('/');
};Add error handling around decryptDiscussionId
decryptDiscussionId can throw (e.g., on invalid or malformed IDs), which would cause the entire request to fail. Wrap the call in a try/catch to handle decryption errors gracefully—either by returning the original path segment or by propagating a controlled error.
Locations to update:
- frontend/apps/server/src/middlewares/discussionIdTransformer/discussionCipher.ts: around line 16 (inside
decodeDiscussionIdOfUrl)
Suggested diff:
export const decodeDiscussionIdOfUrl = (path: string) => {
// (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite']
const parts = path.split('/');
const targetIdx = 3; // discussionId가 위치하는 index
if (parts.length > targetIdx) {
- parts[targetIdx] = decryptDiscussionId(parts[targetIdx]);
+ try {
+ parts[targetIdx] = decryptDiscussionId(parts[targetIdx]);
+ } catch (error) {
+ console.error('Discussion ID 복호화 실패:', error);
+ // 복호화 실패 시 원본 경로 그대로 반환하거나, 필요에 따라 에러 재전파:
+ // return path;
+ throw error;
+ }
}
return parts.join('/');
};📝 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.
| export const decodeDiscussionIdOfUrl = (path: string) => { | |
| // (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] | |
| const parts = path.split('/'); | |
| const targetIdx = 3; // discussionId가 위치하는 index | |
| if (parts.length > targetIdx) { | |
| parts[targetIdx] = decryptDiscussionId(parts[targetIdx]); | |
| } | |
| return parts.join('/'); | |
| }; | |
| export const decodeDiscussionIdOfUrl = (path: string) => { | |
| // (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] | |
| const parts = path.split('/'); | |
| const targetIdx = 3; // discussionId가 위치하는 index | |
| if (parts.length > targetIdx) { | |
| try { | |
| parts[targetIdx] = decryptDiscussionId(parts[targetIdx]); | |
| } catch (error) { | |
| console.error('Discussion ID 복호화 실패:', error); | |
| // 복호화 실패 시 원본 경로 그대로 반환하거나, 필요에 따라 에러 재전파: | |
| // return path; | |
| throw error; | |
| } | |
| } | |
| return parts.join('/'); | |
| }; |
🤖 Prompt for AI Agents
In
frontend/apps/server/src/middlewares/discussionIdTransformer/discussionCipher.ts
around lines 12 to 20, the call to decryptDiscussionId can throw errors on
invalid input, causing request failures. Wrap the decryptDiscussionId call
inside a try/catch block; if an error occurs, either keep the original path
segment unchanged or throw a controlled error with a clear message. This will
ensure graceful error handling without breaking the entire request.
bff에서 해당 요청이 백엔드에 보내지지 않도록 가로채어, 클라이언트에 404를 상태코드로 가지는 response를 반환하도록 처리함
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
frontend/apps/server/src/middlewares/discussionId/index.ts (1)
24-35: Improve error handling and consider path validation.The verifier middleware has good basic error handling, but consider these improvements:
- The error message "Invalid discussion id" might be too generic - consider logging the actual error for debugging
- The hard-coded 404 status might not always be appropriate for decryption failures
- Consider validating the path structure before attempting decryption
export const discussionIdVerifier: RequestHandler = (req, res, next) => { if (req.path.startsWith(DISCUSSION_CREATE_ENDPOINT)) { try { decodeDiscussionIdOfUrl(req.path); return next(); - } catch { + } catch (error) { + // Log the actual error for debugging while keeping user-facing message generic + console.error('Discussion ID verification failed:', error); res.status(404).json({ code: 'NOT_FOUND', message: 'Invalid discussion id' }); return; } } return next(); };frontend/apps/server/src/middlewares/discussionId/discussionCipher.ts (1)
22-28: Consider adding input validation for encryption function.The encryption function should validate its input to prevent potential issues with empty or invalid strings.
export const encryptDiscussionId = (text: string) => { + if (!text || text.trim() === '') { + throw new Error('Discussion ID cannot be empty'); + } const key = Buffer.from(aesGcmKey, 'base64'); const { iv, content, tag } = aesGcm.encrypt(text, key); const concat = `${iv}.${content}.${tag}`; const cipherText = base64Url.encode(concat); return cipherText; };frontend/apps/server/src/middlewares/discussionId/proxyHandlers.ts (4)
14-16: Consider making endpoint patterns more maintainable.The hard-coded endpoint constants are good, but consider centralizing all API endpoint patterns in a single configuration file for better maintainability.
17-22: Path rewriting logic is correct but could be more explicit.The path rewriting logic is sound, but the function name and logic could be more explicit about what it does.
export const rewriteDiscussionPath = (path: string): string => { - if (path.startsWith(DISCUSSION_CREATE_ENDPOINT)) { + // Only rewrite paths that contain discussion IDs that need decryption + if (path.startsWith(DISCUSSION_CREATE_ENDPOINT) && path !== DISCUSSION_CREATE_ENDPOINT) { return decodeDiscussionIdOfUrl(path); } return path; };
24-41: Good request handling but missing content-type validation.The request handling logic is well-implemented with proper fallback for non-object bodies. However, consider adding content-type validation for security.
export const handleProxyReq = ( proxyReq: ClientRequest, req: Request, ): void => { + // Validate content-type for JSON requests + const contentType = req.headers['content-type']; + if (contentType && !contentType.includes('application/json')) { + req.pipe(proxyReq); + return; + } + if (!req.body || typeof req.body !== 'object') { req.pipe(proxyReq); return; } const data = manipulateValueOfObject('discussionId', req.body, decryptDiscussionId); const bodyString = JSON.stringify(data); const { 'content-length': _, ...headers } = req.headers; + // Set correct content-length for modified body + proxyReq.setHeader('content-length', Buffer.byteLength(bodyString)); for (const[key, value] of Object.entries(headers)) { if (value) proxyReq.setHeader(key, value); } proxyReq.write(bodyString); proxyReq.end(); };
43-69: Response handling is robust but consider logging transformation failures.The response handling logic is well-implemented with proper error fallback. The manual field name handling for different APIs is a practical solution to API inconsistencies.
export const handleProxyRes = async ( proxyRes: IncomingMessage & { headers: IncomingHttpHeaders }, req: Request, res: ServerResponse, ) => { const statusCode = proxyRes.statusCode ?? 502; const { 'content-length': _, ...headers } = proxyRes.headers; res.writeHead(statusCode, headers); const stringData = await collectResponse(proxyRes); // discussion id의 필드명을 'discussionId'가 아닌 'id'로 반환하는 API가 존재하여, 이에 대해 manual하게 분기처리 const isDiscussionApi = ( req.url === DISCUSSION_CREATE_ENDPOINT || DISCUSSION_GET_ENDPOINT_REGEX.test(req.url) ); const targetKey = isDiscussionApi ? 'id' : 'discussionId'; try { const result = manipulateValueOfObject( targetKey, JSON.parse(stringData), encryptDiscussionId, ); res.end(JSON.stringify(result)); } catch (err) { + // Log transformation failures for debugging + console.warn('Failed to transform discussion IDs in response:', err); // 변환 실패 시 원본 데이터를 그대로 반환 res.end(stringData); }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
frontend/apps/client/src/utils/error/HTTPError.ts(1 hunks)frontend/apps/client/src/utils/error/handleError.ts(1 hunks)frontend/apps/server/src/index.ts(2 hunks)frontend/apps/server/src/middlewares/discussionId/discussionCipher.ts(1 hunks)frontend/apps/server/src/middlewares/discussionId/index.ts(1 hunks)frontend/apps/server/src/middlewares/discussionId/proxyHandlers.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/apps/client/src/utils/error/HTTPError.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/apps/server/src/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (3)
frontend/apps/server/src/middlewares/discussionId/index.ts (3)
frontend/apps/server/src/envconfig.ts (1)
serverEnv(15-18)frontend/apps/server/src/middlewares/discussionId/proxyHandlers.ts (4)
rewriteDiscussionPath(17-22)handleProxyReq(24-41)handleProxyRes(43-69)DISCUSSION_CREATE_ENDPOINT(14-14)frontend/apps/server/src/middlewares/discussionId/discussionCipher.ts (1)
decodeDiscussionIdOfUrl(12-20)
frontend/apps/server/src/middlewares/discussionId/discussionCipher.ts (3)
frontend/apps/server/src/envconfig.ts (1)
serverEnv(15-18)frontend/apps/server/src/utils/cipher.ts (1)
aesGcm(4-73)frontend/apps/server/src/utils/stringTransformer.ts (1)
base64Url(3-15)
frontend/apps/server/src/middlewares/discussionId/proxyHandlers.ts (3)
frontend/apps/server/src/middlewares/discussionId/discussionCipher.ts (3)
decodeDiscussionIdOfUrl(12-20)decryptDiscussionId(30-36)encryptDiscussionId(22-28)frontend/apps/server/src/utils/object.ts (1)
manipulateValueOfObject(1-18)frontend/apps/server/src/utils/collectResponse.ts (1)
collectResponse(15-30)
🔇 Additional comments (4)
frontend/apps/client/src/utils/error/handleError.ts (2)
27-27: Consider the UX implications of global 404 redirects.The implementation follows the established pattern for error handling, which is good for consistency. However, redirecting to
/not-foundfor all 404 errors might be too aggressive - not every 404 response should trigger a full page redirect (e.g., failed API calls for optional data, background requests, etc.).Consider whether this global redirect behavior aligns with your UX requirements, or if it should be more selective based on the context of the request.
32-32: Good formatting improvement.The added blank line improves readability by providing visual separation between the error type check and the notification logic.
frontend/apps/server/src/middlewares/discussionId/discussionCipher.ts (1)
7-10: Good practice: Early key validation prevents runtime failures.The early validation of the AES-GCM key at module load time is excellent - it ensures the application fails fast if misconfigured rather than failing silently during runtime.
frontend/apps/server/src/middlewares/discussionId/proxyHandlers.ts (1)
54-57: URL Matching Logic Verified
The current logic correctly distinguishes the create (POST /v1/discussion) and get (GET /v1/discussion/:id) endpoints—usingid—and defaults todiscussionIdfor all other routes under/v1/discussion/.... We discovered these discussion-related endpoints in the codebase:
/v1/discussion/v1/discussion/${discussionId}/v1/discussion/${discussionId}/candidate-event/(calendar|rank|details)/v1/discussion/${discussionId}/participants/v1/discussion/${discussionId}/invite/v1/discussion/${discussionId}/join/v1/discussion/${id}/(confirm|restart|role|shared-event)The
DISCUSSION_GET_ENDPOINT_REGEXonly matches the exact/v1/discussion/:idpath (with optional trailing slash), so all deeper paths fall into thediscussionIdbranch as intended. No gaps found.
| export const decodeDiscussionIdOfUrl = (path: string) => { | ||
| // (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] | ||
| const parts = path.split('/'); | ||
| const targetIdx = 3; // discussionId가 위치하는 index | ||
| if (parts.length > targetIdx) { | ||
| parts[targetIdx] = decryptDiscussionId(parts[targetIdx]); | ||
| } | ||
| return parts.join('/'); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the URL structure more flexible and add validation.
The current implementation assumes a fixed URL structure with the discussion ID always at index 3. This could be fragile if the API structure changes.
export const decodeDiscussionIdOfUrl = (path: string) => {
const parts = path.split('/');
- const targetIdx = 3; // discussionId가 위치하는 index
- if (parts.length > targetIdx) {
- parts[targetIdx] = decryptDiscussionId(parts[targetIdx]);
- }
+ const targetIdx = 3; // discussionId가 위치하는 index
+ if (parts.length <= targetIdx) {
+ throw new Error('Invalid URL structure for discussion ID extraction');
+ }
+ if (!parts[targetIdx] || parts[targetIdx].trim() === '') {
+ throw new Error('Discussion ID not found in expected URL position');
+ }
+ parts[targetIdx] = decryptDiscussionId(parts[targetIdx]);
return parts.join('/');
};📝 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.
| export const decodeDiscussionIdOfUrl = (path: string) => { | |
| // (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] | |
| const parts = path.split('/'); | |
| const targetIdx = 3; // discussionId가 위치하는 index | |
| if (parts.length > targetIdx) { | |
| parts[targetIdx] = decryptDiscussionId(parts[targetIdx]); | |
| } | |
| return parts.join('/'); | |
| }; | |
| export const decodeDiscussionIdOfUrl = (path: string) => { | |
| // (출력 예시) parts: ['', 'v1', 'discussion', '{encoded_discussion_id}', 'invite'] | |
| const parts = path.split('/'); | |
| const targetIdx = 3; // discussionId가 위치하는 index | |
| if (parts.length <= targetIdx) { | |
| throw new Error('Invalid URL structure for discussion ID extraction'); | |
| } | |
| if (!parts[targetIdx] || parts[targetIdx].trim() === '') { | |
| throw new Error('Discussion ID not found in expected URL position'); | |
| } | |
| parts[targetIdx] = decryptDiscussionId(parts[targetIdx]); | |
| return parts.join('/'); | |
| }; |
🤖 Prompt for AI Agents
In frontend/apps/server/src/middlewares/discussionId/discussionCipher.ts around
lines 12 to 20, the code assumes the discussion ID is always at index 3 in the
URL path, which is fragile. Refactor the function to dynamically locate the
discussion ID segment by searching for the 'discussion' keyword in the path
parts array, then decrypt the following segment if it exists. Also, add
validation to ensure the expected segments exist before attempting decryption to
avoid errors with unexpected URL structures.
| export const decryptDiscussionId = (cipherText: string) => { | ||
| const key = Buffer.from(aesGcmKey, 'base64'); | ||
| const concat = base64Url.decode(cipherText); | ||
| const [iv, content, tag] = concat.split('.'); | ||
| const encryptedData = { iv, content, tag }; | ||
| return aesGcm.decrypt(encryptedData, key); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper error handling and input validation for decryption.
The decryption function needs better error handling and input validation to prevent crashes and provide meaningful error messages.
export const decryptDiscussionId = (cipherText: string) => {
+ if (!cipherText || cipherText.trim() === '') {
+ throw new Error('Cipher text cannot be empty');
+ }
+
+ try {
const key = Buffer.from(aesGcmKey, 'base64');
const concat = base64Url.decode(cipherText);
const [iv, content, tag] = concat.split('.');
+
+ if (!iv || !content || !tag) {
+ throw new Error('Invalid cipher text format');
+ }
+
const encryptedData = { iv, content, tag };
return aesGcm.decrypt(encryptedData, key);
+ } catch (error) {
+ if (error instanceof Error) {
+ throw new Error(`Failed to decrypt discussion ID: ${error.message}`);
+ }
+ throw new Error('Failed to decrypt discussion ID: Unknown error');
+ }
};🤖 Prompt for AI Agents
In frontend/apps/server/src/middlewares/discussionId/discussionCipher.ts around
lines 30 to 36, the decryptDiscussionId function lacks input validation and
error handling. Add checks to ensure the cipherText is a valid string and
properly formatted before processing. Wrap the decryption logic in a try-catch
block to catch and handle any errors, returning a clear error message or
handling the failure gracefully instead of allowing the function to crash.
| export const discussionIdTransformer = createProxyMiddleware({ | ||
| target: `${serverEnv.BASE_URL}/api`, | ||
| changeOrigin: true, | ||
| selfHandleResponse: true, | ||
| pathRewrite: rewriteDiscussionPath, | ||
| on: { | ||
| proxyReq: handleProxyReq, | ||
| proxyRes: handleProxyRes, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify BASE_URL configuration and consider security implications.
The proxy middleware configuration looks correct, but there are a few considerations:
- The
selfHandleResponse: truesetting is appropriate since you're manually handling responses for encryption - Consider adding request/response size limits to prevent potential DoS attacks
- Verify that
serverEnv.BASE_URLis properly validated and doesn't allow malicious redirects
🏁 Script executed:
#!/bin/bash
# Check if BASE_URL validation exists in envconfig
rg -A 5 -B 5 "BASE_URL.*validation|validate.*BASE_URL" frontend/apps/server/src/Length of output: 82
🏁 Script executed:
#!/bin/bash
# Locate and inspect the serverEnv definition to verify BASE_URL validation
file=$(rg -l "export const serverEnv" -R frontend/apps/server/src | head -n 1)
echo "Found serverEnv in: $file"
sed -n '1,200p' "$file"Length of output: 601
🏁 Script executed:
#!/bin/bash
# Inspect imports in discussionIdTransformer to locate serverEnv source
sed -n '1,50p' frontend/apps/server/src/middlewares/discussionId/index.tsLength of output: 1034
🏁 Script executed:
#!/bin/bash
# Locate envconfig file defining serverEnv
env_file=$(fd 'envconfig\.(ts|js)$' frontend/apps/server/src)
echo "Found envconfig file(s):"
echo "$env_file"
# Display the definition of serverEnv including any validation logic
for file in $env_file; do
echo "==== $file ===="
sed -n '1,200p' "$file"
doneLength of output: 912
Ensure BASE_URL is validated and enforce size limits
There is currently no validation on process.env.BASE_URL in envconfig.ts, and the proxy middleware has no request/response size limits. Both gaps can expose the service to misconfiguration or DoS vectors.
Recommended actions:
- In
frontend/apps/server/src/envconfig.ts: validate thatBASE_URLis present and a well-formed HTTP(S) URL. For example, use Zod or a simple regex:import { z } from 'zod'; const EnvSchema = z.object({ BASE_URL: z.string().url(), AES_GCM_KEY_OF_DISCUSSION_ID: z.string().min(16), }); export const serverEnv = EnvSchema.parse({ BASE_URL: process.env.BASE_URL, AES_GCM_KEY_OF_DISCUSSION_ID: process.env.AES_GCM_KEY_OF_DISCUSSION_ID, });
- In your Express setup (before the proxy), limit payload size:
import express from 'express'; const app = express(); app.use(express.json({ limit: '1mb' })); app.use(express.urlencoded({ limit: '1mb', extended: true })); // then mount discussionIdTransformer…
- Optionally, if you need per-route limits, wrap the proxy in a size-checking middleware or configure your upstream API gateway to cap request/response bodies.
These changes will prevent invalid BASE_URL values and mitigate large-payload DoS risks.
🤖 Prompt for AI Agents
In frontend/apps/server/src/envconfig.ts, add validation for BASE_URL to ensure
it is present and a valid HTTP(S) URL using a schema validation library like
Zod. Then, in the Express server setup before mounting the
discussionIdTransformer proxy middleware in
frontend/apps/server/src/middlewares/discussionId/index.ts, add middleware to
limit JSON and URL-encoded payload sizes to 1mb to prevent large-payload DoS
risks. Optionally, consider adding per-route size limits or configuring the API
gateway for further protection.
#️⃣ 연관된 이슈>
📝 작업 내용> 이번 PR에서 작업한 내용을 간략히 설명해주세요(이미지 첨부 가능)
🙏 여기는 꼭 봐주세요! > 리뷰어가 특별히 봐주었으면 하는 부분이 있다면 작성해주세요
main.tsx에서 StrictMode 해제 후 테스트해보시는 것을 권장드립니다.Summary by CodeRabbit
New Features
Bug Fixes
Chores
.gitignoreto exclude environment files from version control.Documentation