[FH-2] Figma Oauth Implementation (#201) #206
Conversation
WalkthroughThe changes introduce three new environment variables for Figma OAuth integration in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.env.example (1)
38-41: LGTM! Consider a minor formatting adjustment.The addition of FIGMA_CLIENT_ID and FIGMA_CLIENT_SECRET variables is appropriate for the Figma OAuth implementation. The comment provides helpful context, and leaving the values empty in the example file is the correct approach for security.
For consistency with the rest of the file, consider removing the empty line 38. This would make the new section blend more seamlessly with the existing content. Here's a suggested change:
BSCSCAN_API_KEY= - # Figma App for OAuth FIGMA_CLIENT_ID= FIGMA_CLIENT_SECRET=
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (2)
templates/figma/components/PropertiesTab.tsx (1)
58-61: LGTM: Session management for Figma access token.The implementation of session management to retrieve the Figma access token is correct and aligns with the OAuth implementation. The type assertion ensures type safety.
Consider removing the empty line 61 for better code compactness.
templates/figma/Inspector.tsx (1)
130-130: Typographical error in commentThere's a typo in the comment on line 130. "rulese" should be "rules".
Apply the following diff to correct the typo:
- // biome-ignore lint/correctness/useExhaustiveDependencies: circular rulese + // biome-ignore lint/correctness/useExhaustiveDependencies: circular rules
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- .env.example (2 hunks)
- auth.ts (2 hunks)
- templates/figma/Inspector.tsx (4 hunks)
- templates/figma/components/FigmaTokenEditor.tsx (1 hunks)
- templates/figma/components/PropertiesTab.tsx (3 hunks)
- templates/figma/components/SlideEditor.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .env.example
🧰 Additional context used
🔇 Additional comments (12)
templates/figma/components/SlideEditor.tsx (1)
22-22: LGTM: Removal offigmaPATpropThe removal of the
figmaPATprop from theSlideDesignercomponent aligns with the PR objectives of implementing Figma OAuth. This change appears to be part of the transition from using a personal access token to an OAuth access token.To ensure this change doesn't introduce any regressions, please verify that all usages of
figmaPAThave been properly updated throughout the codebase. Run the following script to check for any remaining occurrences:If any occurrences are found, please ensure they are updated to use the new OAuth implementation.
templates/figma/components/PropertiesTab.tsx (4)
16-17: LGTM: New imports for session management.The addition of
useSessionandFrameTrainSessiontype is appropriate for integrating Figma OAuth. This change aligns with the PR objective of implementing Figma OAuth.
218-222: LGTM: Improved UX for Figma URL input.The changes to the Figma URL input field improve the user experience by providing clear instructions and disabling the input when the access token is not present. This is consistent with the OAuth implementation.
227-227: LGTM: Proper button state management.The update button is correctly disabled when
figmaAccessTokenis not present, preventing unauthorized update attempts. This change aligns well with the OAuth implementation.
Line range hint
1-292: Summary: Successful implementation of Figma OAuth with minor suggestions.The changes in this file successfully implement Figma OAuth, replacing the previous PAT-based authentication. The modifications are consistent throughout the component and align well with the PR objectives. Key points:
- Proper integration of session management for Figma access token.
- Consistent updates to UI elements based on the presence of the access token.
- Improved user experience with clear instructions and proper input/button states.
Consider implementing the suggested null check for
figmaAccessTokenin theupdateUrlfunction for improved robustness. Additionally, removing the unnecessary empty line would enhance code compactness.Overall, the implementation is solid and achieves the intended goal of integrating Figma OAuth.
templates/figma/components/FigmaTokenEditor.tsx (3)
2-5: Imports are correctThe necessary imports for
FrameTrainSession,signIn, anduseSessionhave been added appropriately.
8-8: Function signature updated appropriatelyThe
FigmaTokenEditorcomponent no longer accepts props and now relies on session data, streamlining its functionality.
13-23: Component rendering logic is appropriateThe rendering correctly toggles between prompting the user to connect their Figma account and indicating a successful connection based on
figmaAccessToken.auth.ts (2)
12-14:⚠️ Potential issueRemove duplicate interface definition of
FrameTrainSession.The
FrameTrainSessioninterface is defined twice in the code, at lines 8-10 and again at lines 12-14. This duplication is unnecessary and could lead to confusion or errors.Apply this diff to remove the duplicate definition:
-export interface FrameTrainSession extends Session { - figmaAccessToken?: string; -}Likely invalid or redundant comment.
43-50: Verify the profile properties returned by Figma's API.Ensure that the properties
id,handle,img_urlused in theprofilefunction match the actual response from Figma's user info API. Using incorrect property names may result in undefined values or errors.Consider checking Figma's API documentation to confirm the correct property names or run the following script to inspect the actual response structure:
Note: Ensure you handle access tokens securely and avoid exposing them.
templates/figma/Inspector.tsx (2)
39-39: Confirming the explicit reset ofpostUrlSetting
postUrl: undefinedensures that any previous value is cleared when previewing a slide, preventing unintended behavior.
186-198: Button components are correctly implementedThe buttons for slide navigation and deletion are appropriately configured with correct handlers and disabled states.
| const updateUrl = async () => { | ||
| console.debug(`updateFigmaUrl(${slideConfigId})`) | ||
|
|
||
| setIsUpdating(true) | ||
|
|
||
| // Fetch the Figma design | ||
| const figmaDesignResult = await getFigmaDesign(figmaPAT, newUrl) | ||
| const figmaDesignResult = await getFigmaDesign(figmaAccessToken!, newUrl) |
There was a problem hiding this comment.
Suggestion: Add null check for figmaAccessToken.
The use of figmaAccessToken instead of figmaPAT is correct. However, the non-null assertion (!) assumes the token will always be present, which might not be true in all cases.
Consider adding a null check before calling getFigmaDesign:
if (!figmaAccessToken) {
toast.error("Figma access token is missing. Please log in again.");
setIsUpdating(false);
return;
}
const figmaDesignResult = await getFigmaDesign(figmaAccessToken, newUrl);This change will provide a better user experience by showing an error message if the token is missing.
| const { data: session } = useSession(); | ||
|
|
||
| export default function FigmaTokenEditor({ figmaPAT, onChange, onCancel }: FigmaTokenEditorProps) { | ||
| const [newFigmaPAT, setNewFigmaPAT] = useState('') | ||
| const figmaAccessToken = (session as FrameTrainSession)?.figmaAccessToken; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify type handling by specifying generic type in useSession
By providing a generic type parameter to useSession, you can eliminate the need for type assertion, enhancing type safety and code clarity.
Apply this change:
- const { data: session } = useSession();
- const figmaAccessToken = (session as FrameTrainSession)?.figmaAccessToken;
+ const { data: session } = useSession<FrameTrainSession>();
+ const figmaAccessToken = session?.figmaAccessToken;📝 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.
| const { data: session } = useSession(); | |
| export default function FigmaTokenEditor({ figmaPAT, onChange, onCancel }: FigmaTokenEditorProps) { | |
| const [newFigmaPAT, setNewFigmaPAT] = useState('') | |
| const figmaAccessToken = (session as FrameTrainSession)?.figmaAccessToken; | |
| const { data: session } = useSession<FrameTrainSession>(); | |
| const figmaAccessToken = session?.figmaAccessToken; |
auth.ts
Outdated
| if (account?.provider === 'figma') { | ||
| // If the user is connecting Figma, store the access token for Figma | ||
| token.figmaAccessToken = account.access_token; | ||
| } |
There was a problem hiding this comment.
Avoid storing the Figma access token in the JWT token to enhance security.
Storing the figmaAccessToken in the JWT token can expose sensitive information to the client if the token is accessible on the client side. Access tokens should be kept securely on the server to prevent unauthorized access.
Consider storing the access token in a secure server-side session or managing it within server-side functions without exposing it to the client.
auth.ts
Outdated
| const frameTrainSession = session as FrameTrainSession; | ||
| if (token.user) frameTrainSession.user = { ...frameTrainSession.user, id: (token.user as any).id }; | ||
| frameTrainSession.figmaAccessToken = token.figmaAccessToken as string | undefined; | ||
| return frameTrainSession; |
There was a problem hiding this comment.
Do not include the Figma access token in the session object sent to the client.
By adding figmaAccessToken to the session object, you risk exposing the access token to the client side, which can lead to security vulnerabilities.
Modify the code to prevent the figmaAccessToken from being included in the session sent to the client. Handle Figma API interactions on the server side where the access token remains secure.
auth.ts
Outdated
| id: profile.id, | ||
| name: `${profile.handle}`, | ||
| email: profile.email, | ||
| image: profile.img_url,nre |
There was a problem hiding this comment.
Fix the typo in the 'image' property of the profile.
There is an extraneous nre appended to profile.img_url which will cause a syntax error.
Apply this diff to fix the typo:
image: profile.img_url,nre
+ image: profile.img_url,Committable suggestion was skipped due to low confidence.
auth.ts
Outdated
| `https://api.figma.com/v1/oauth/token?client_id=${provider.clientId}&client_secret=${provider.clientSecret}&redirect_uri=${provider.CallbackUrl}&code=${context.params.code}&grant_type=authorization_code`, | ||
| { method: "POST" } | ||
| ); | ||
| const json = await res.json(); | ||
|
|
||
| return { tokens: json }; | ||
| }, |
There was a problem hiding this comment.
Correct the OAuth token request to adhere to Figma's API specifications and handle errors properly.
The current implementation of the token request includes query parameters in the URL while using the POST method, which is unconventional. OAuth token requests typically send parameters in the request body with the application/x-www-form-urlencoded content type. Additionally, provider.CallbackUrl may not be the correct property for the redirect URI, and there is no error handling for the fetch response.
Apply this diff to correct the token request and add error handling:
const res = await fetch(
- `https://api.figma.com/v1/oauth/token?client_id=${provider.clientId}&client_secret=${provider.clientSecret}&redirect_uri=${provider.CallbackUrl}&code=${context.params.code}&grant_type=authorization_code`,
- { method: "POST" }
+ 'https://api.figma.com/v1/oauth/token',
+ {
+ method: "POST",
+ headers: {
+ "Content-Type": "application/x-www-form-urlencoded",
+ },
+ body: new URLSearchParams({
+ client_id: provider.clientId || '',
+ client_secret: provider.clientSecret || '',
+ redirect_uri: process.env.NEXTAUTH_URL + '/api/auth/callback/figma',
+ code: context.params.code || '',
+ grant_type: 'authorization_code',
+ }),
+ }
);
+if (!res.ok) {
+ throw new Error(`Token request failed with status ${res.status}`);
+}
const json = await res.json();📝 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.
| `https://api.figma.com/v1/oauth/token?client_id=${provider.clientId}&client_secret=${provider.clientSecret}&redirect_uri=${provider.CallbackUrl}&code=${context.params.code}&grant_type=authorization_code`, | |
| { method: "POST" } | |
| ); | |
| const json = await res.json(); | |
| return { tokens: json }; | |
| }, | |
| 'https://api.figma.com/v1/oauth/token', | |
| { | |
| method: "POST", | |
| headers: { | |
| "Content-Type": "application/x-www-form-urlencoded", | |
| }, | |
| body: new URLSearchParams({ | |
| client_id: provider.clientId || '', | |
| client_secret: provider.clientSecret || '', | |
| redirect_uri: process.env.NEXTAUTH_URL + '/api/auth/callback/figma', | |
| code: context.params.code || '', | |
| grant_type: 'authorization_code', | |
| }), | |
| } | |
| ); | |
| if (!res.ok) { | |
| throw new Error(`Token request failed with status ${res.status}`); | |
| } | |
| const json = await res.json(); | |
| return { tokens: json }; | |
| }, |
templates/figma/Inspector.tsx
Outdated
| <Checkbox | ||
| id="figmaUnderstood" | ||
| onCheckedChange={(e) => | ||
| (figmaUnderstoodRef.current = e === true) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use useState instead of useRef for checkbox state management
Using useState for figmaUnderstoodChecked aligns better with React's state management practices and ensures the component re-renders when the state changes.
Modify the code as follows:
Declare the state variable:
const [figmaUnderstoodChecked, setFigmaUnderstoodChecked] = useState(false)Update the checkbox handler:
<Checkbox
id="figmaUnderstood"
- onCheckedChange={(e) => (figmaUnderstoodRef.current = e === true)}
+ onCheckedChange={(e) => setFigmaUnderstoodChecked(e === true)}
/>Adjust the onClick handler:
onClick={() => {
addSlide()
- if (figmaUnderstoodRef.current) {
+ if (figmaUnderstoodChecked) {
setFigmaUnderstood(true)
}
}}
templates/figma/Inspector.tsx
Outdated
| style={{ | ||
| 'transform': | ||
| slideConfig.aspectRatio == '1:1' | ||
| ? 'scale(0.245)' | ||
| : 'scale(0.130)', | ||
| // Handle the case where no image has been configured but we need a min-width | ||
| ...(!slideConfig.baseImagePaths | ||
| ? { | ||
| 'width': | ||
| slideConfig.aspectRatio == '1:1' | ||
| ? dimensionsForRatio['1/1'].width | ||
| : dimensionsForRatio['1.91/1'].height, | ||
| } | ||
| : {}), | ||
| 'overflow': 'clip', | ||
| }} | ||
| className={`w-40 h-40 flex items-center justify-center mr-1 border-[1px] rounded-md cursor-pointer select-none ${ | ||
| selectedSlideIndex === index | ||
| ? 'border-highlight' | ||
| : 'border-input' | ||
| }`} | ||
| > | ||
| <div | ||
| style={{ | ||
| 'transform': | ||
| slideConfig.aspectRatio == '1:1' | ||
| ? 'scale(0.245)' | ||
| : 'scale(0.130)', | ||
| // Handle the case where no image has been configured but we need a min-width | ||
| ...(!slideConfig.baseImagePaths | ||
| ? { | ||
| 'width': | ||
| slideConfig.aspectRatio == '1:1' | ||
| ? dimensionsForRatio['1/1'].width | ||
| : dimensionsForRatio['1.91/1'].height, | ||
| } | ||
| : {}), | ||
| 'overflow': 'clip', | ||
| }} | ||
| > | ||
| <FigmaView slideConfig={slideConfig} /> | ||
| </div> | ||
| </div> | ||
| ))} | ||
|
|
||
| {figmaUnderstood ? ( | ||
| <div | ||
| className="w-40 h-40 flex items-center justify-center mr-1 border-input border-[1px] rounded-md cursor-pointer" | ||
| onClick={addSlide} | ||
| > | ||
| <span className="text-4xl">+</span> | ||
| <FigmaView slideConfig={slideConfig} /> | ||
| </div> | ||
| ) : ( | ||
| <AlertDialog.Root> | ||
| <AlertDialog.Trigger> | ||
| <div className="w-40 h-40 flex items-center justify-center mr-1 border-input border-[1px] rounded-md cursor-pointer"> | ||
| <span className="text-4xl">+</span> | ||
| </div> | ||
| </AlertDialog.Trigger> | ||
| <AlertDialog.Content> | ||
| <AlertDialog.Header> | ||
| <AlertDialog.Title>Resolution Notice</AlertDialog.Title> | ||
| <AlertDialog.Description className="flex flex-col gap-4"> | ||
| The Figma URL entered must lead to an artboard/section | ||
| that is either 630x630 or 1200x630 pixels in size. | ||
| <div className="flex flex-row items-center gap-2"> | ||
| <Checkbox | ||
| id="figmaUnderstood" | ||
| onCheckedChange={(e) => | ||
| (figmaUnderstoodRef.current = e === true) | ||
| } | ||
| /> | ||
| <Label htmlFor="figmaUnderstood"> | ||
| Don't show again. | ||
| </Label> | ||
| </div> | ||
| </AlertDialog.Description> | ||
| </AlertDialog.Header> | ||
| <AlertDialog.Footer> | ||
| <AlertDialog.Cancel>Back</AlertDialog.Cancel> | ||
| <AlertDialog.Action | ||
| onClick={() => { | ||
| addSlide() | ||
|
|
||
| if (figmaUnderstoodRef.current) { | ||
| setFigmaUnderstood(true) | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract complex style computations into a helper function
The inline style prop contains complex logic that affects readability. Consider extracting the style calculations into a helper function or variable.
Example:
function getSlideStyle(slideConfig) {
return {
transform: slideConfig.aspectRatio == '1:1' ? 'scale(0.245)' : 'scale(0.130)',
overflow: 'clip',
...(!slideConfig.baseImagePaths
? {
width:
slideConfig.aspectRatio == '1:1'
? dimensionsForRatio['1/1'].width
: dimensionsForRatio['1.91/1'].height,
}
: {}),
}
}Use it in the component:
<div style={getSlideStyle(slideConfig)}>
<FigmaView slideConfig={slideConfig} />
</div>
templates/figma/Inspector.tsx
Outdated
| <SlideEditor | ||
| key={config.slides[selectedSlideIndex].id} | ||
| slideConfig={config.slides[selectedSlideIndex]} | ||
| figmaPAT={config.figmaPAT} |
There was a problem hiding this comment.
Update prop name to figmaAccessToken in SlideEditor
The prop figmaPAT should be updated to figmaAccessToken to maintain consistency with the new naming convention.
Apply the following diff:
<SlideEditor
key={config.slides[selectedSlideIndex].id}
slideConfig={config.slides[selectedSlideIndex]}
- figmaPAT={config.figmaPAT}
+ figmaAccessToken={config.figmaAccessToken}
buttonTargets={buttonTargets}
onUpdate={(updatedSlideConfig) => updateSlide(updatedSlideConfig)}
/>Committable suggestion was skipped due to low confidence.
templates/figma/Inspector.tsx
Outdated
| <Configuration.Section title="Figma Login"> | ||
| <FigmaTokenEditor | ||
| figmaAccessToken={config.figmaAccessToken} | ||
| onChange={updateFigmaPAT} | ||
| onCancel={() => setEditingFigmaPAT(false)} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Inconsistent naming: rename updateFigmaPAT to updateFigmaAccessToken
The function updateFigmaPAT should be renamed to reflect the updated terminology of figmaAccessToken for clarity and consistency.
Apply the following diff:
-function updateFigmaPAT(updatedPAT: string) {
+function updateFigmaAccessToken(updatedAccessToken: string) {
console.debug('Inspector::updateFigmaPAT()')
setEditingFigmaPAT(false)
updateConfig({
- figmaPAT: updatedPAT,
+ figmaAccessToken: updatedAccessToken,
})
}Update the onChange prop accordingly:
<FigmaTokenEditor
figmaAccessToken={config.figmaAccessToken}
- onChange={updateFigmaPAT}
+ onChange={updateFigmaAccessToken}
onCancel={() => setEditingFigmaPAT(false)}
/>Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Outside diff range and nitpick comments (12)
app/api/auth/figma/status/route.ts (2)
3-7: Enhance the function comment for clarity.The comment provides good information about the function's purpose and security considerations. However, it could be more specific about the exact security threats it mitigates.
Consider expanding the comment to explicitly mention the security threats prevented:
/** * Returns the current Figma access token for the user. * - * We use httpOnly cookies to prevent XSS, token leaks and request forgery. + * We use httpOnly cookies to enhance security by: + * - Preventing XSS attacks from accessing the token via JavaScript + * - Mitigating token leaks through browser vulnerabilities + * - Reducing the risk of CSRF attacks */
8-16: Approve implementation with a suggestion for error handling.The implementation is correct and follows best practices:
- Proper use of NextRequest and NextResponse APIs
- Concise logic following the single responsibility principle
- Good use of optional chaining when accessing the cookie value
However, consider adding error handling to make the function more robust.
Consider wrapping the implementation in a try-catch block to handle potential errors:
export async function GET(request: NextRequest) { + try { const figmaAccessToken = request.cookies.get('figma_access_token')?.value; if (figmaAccessToken) { return NextResponse.json({ accessToken: figmaAccessToken }); } return NextResponse.json({ accessToken: null }); + } catch (error) { + console.error('Error retrieving Figma access token:', error); + return NextResponse.json({ error: 'Internal Server Error' }, { status: 500 }); + } }templates/figma/components/FigmaConnector.tsx (1)
6-8: Consider renaming the component for clarity.The current name
FigmaTokenEditordoesn't accurately reflect the component's functionality. It's more of a connector or manager for the Figma account connection rather than an editor for the token itself.Consider renaming the component to something like
FigmaConnectororFigmaAccountManagerto better represent its purpose. For example:-export default function FigmaTokenEditor() { +export default function FigmaConnector() {app/api/auth/figma/route.ts (1)
19-21: LGTM: Correct OAuth URL construction with a minor suggestionThe Figma OAuth URL is correctly constructed with all necessary parameters, and proper encoding is used for the
redirectUriandstate. The scope 'files:read' aligns with the PR objectives.Minor suggestion: Consider extracting the Figma OAuth base URL to a constant or environment variable for easier maintenance:
const FIGMA_OAUTH_URL = 'https://www.figma.com/oauth'; // ... const response = NextResponse.redirect( `${FIGMA_OAUTH_URL}?client_id=${FIGMA_CLIENT_ID}&redirect_uri=${encodeURIComponent(redirectUri)}&scope=files:read&response_type=code&state=${encodeURIComponent(state)}` );This change would make it easier to update the URL if needed in the future.
templates/figma/utils/FigmaApi.ts (3)
230-234: Approve parameter renaming and suggest error message update.The renaming of
figmaPATtofigmaAccessTokenaligns with the PR objectives for implementing Figma OAuth. However, the error message on line 235 still refers to "Personal Access Token (PAT)".Consider updating the error message to reflect the new parameter name:
- return { success: false, error: 'Personal Access Token (PAT) is missing or empty' } + return { success: false, error: 'Figma Access Token is missing or empty' }
Line range hint
278-297: Consistent changes, but error message and headers need attention.The renaming of
figmaPATtofigmaAccessTokenis consistent with the changes in thegetFigmaFilefunction. However, there are two issues that need to be addressed:
- The error message on line 283 still refers to "Personal Access Token (PAT)".
- The headers on lines 296-297 have the same redundancy issue as in the
getFigmaFilefunction.Please make the following changes:
- Update the error message:
- return { success: false, error: 'Personal Access Token (PAT) is missing or empty' } + return { success: false, error: 'Figma Access Token is missing or empty' }
- Remove the redundant header:
headers: { 'Authorization': `Bearer ${figmaAccessToken}`, - 'X-FIGMA-TOKEN': figmaAccessToken, },These changes will ensure consistency and correctness across the file.
Figma OAuth Implementation Requires Environment Configuration
The OAuth implementation in
templates/figma/utils/FigmaApi.tsshows correct usage offigmaAccessTokenandBearertokens. Additionally,FigmaProvideris properly integrated within the authentication module.However, the verification revealed critical issues:
- Missing
.envFile: The.envfile is not present in the repository.- Environment Variables Not Found:
FIGMA_CLIENT_IDFIGMA_CLIENT_SECRETNEXTAUTH_URLThese environment variables are essential for the OAuth functionality to operate correctly. Please ensure that the
.envfile is added with the necessary configurations.🔗 Analysis chain
Line range hint
1-438: Summary: Figma OAuth implementation changes look good with minor adjustments needed.The changes in this file align well with the PR objectives for implementing Figma OAuth. The renaming of
figmaPATtofigmaAccessTokenand the use of Bearer tokens in the authorization header are correct steps towards OAuth implementation.However, there are a few minor issues that should be addressed:
- Update error messages to reflect the new "Figma Access Token" terminology.
- Remove the potentially redundant 'X-FIGMA-TOKEN' header.
Once these adjustments are made, the implementation will be consistent and correctly aligned with OAuth standards.
To ensure that the OAuth implementation is complete, please run the following verification script:
This script will help verify that all necessary components for the Figma OAuth implementation are in place.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify Figma OAuth implementation # Check for environment variables echo "Checking for required environment variables..." grep -q "FIGMA_CLIENT_ID" .env && echo "FIGMA_CLIENT_ID found" || echo "FIGMA_CLIENT_ID not found" grep -q "FIGMA_CLIENT_SECRET" .env && echo "FIGMA_CLIENT_SECRET found" || echo "FIGMA_CLIENT_SECRET not found" grep -q "NEXTAUTH_URL" .env && echo "NEXTAUTH_URL found" || echo "NEXTAUTH_URL not found" # Check for FigmaProvider in authentication module echo "Checking for FigmaProvider in authentication module..." grep -r "FigmaProvider" . || echo "FigmaProvider not found" # Check for figmaAccessToken usage echo "Checking for figmaAccessToken usage..." grep -r "figmaAccessToken" . || echo "figmaAccessToken not found" # Check for Bearer token usage echo "Checking for Bearer token usage..." grep -r "Bearer" . || echo "Bearer token usage not found" echo "Verification complete. Please review the output to ensure all components of the Figma OAuth implementation are present."Length of output: 3766
templates/figma/Inspector.tsx (2)
163-167: LGTM: Figma OAuth integration improvementsThe changes align well with the PR objectives:
- Wrapping the component in
FigmaTokenProvidersuggests a context-based approach for managing Figma tokens.- Replacing "PAT" with "Figma Login" improves user understanding.
- Using
FigmaConnectorinstead ofFigmaTokenEditorlikely implements the OAuth flow.These changes should enhance the user experience by simplifying the login process.
Consider adding a brief comment above the
FigmaTokenProviderto explain its purpose and scope.
Line range hint
195-213: LGTM: Improved slide preview stylingThe changes to the slide preview styling are good improvements:
- Using template literals for
classNameenhances readability.- The restructured style object handles different aspect ratios more explicitly.
- Overall, the code is more maintainable.
Consider extracting the complex style object into a separate function for better readability and maintainability. For example:
const getSlidePreviewStyle = (slideConfig) => ({ transform: slideConfig.aspectRatio === '1:1' ? 'scale(0.245)' : 'scale(0.130)', width: !slideConfig.baseImagePaths ? slideConfig.aspectRatio === '1:1' ? dimensionsForRatio['1/1'].width : dimensionsForRatio['1.91/1'].height : undefined, overflow: 'clip', });Then use it in the JSX:
<div style={getSlidePreviewStyle(slideConfig)}> <FigmaView slideConfig={slideConfig} /> </div>app/api/auth/figma/callback/route.ts (2)
52-76: Consider setting 'sameSite' to 'strict' for cookies.To enhance security against CSRF attacks, consider setting the
sameSiteattribute of cookies to'strict'.Apply this diff to update the cookie settings:
response.cookies.set('figma_access_token', tokenData.access_token, { httpOnly: true, secure: true, - sameSite: 'lax', + sameSite: 'strict', path: '/', }); response.cookies.set('figma_refresh_token', tokenData.refresh_token, { httpOnly: true, secure: true, - sameSite: 'lax', + sameSite: 'strict', path: '/', }); response.cookies.set('figma_user_id', tokenData.user_id.toString(), { httpOnly: true, secure: true, - sameSite: 'lax', + sameSite: 'strict', path: '/', }); const expiresAt = Date.now() + tokenData.expires_in * 1000; response.cookies.set('figma_token_expires_at', expiresAt.toString(), { httpOnly: true, secure: true, - sameSite: 'lax', + sameSite: 'strict', path: '/', });
52-76: Re-evaluate token storage strategy for enhanced security.Storing access tokens and refresh tokens in client-side cookies may have security implications. Consider handling tokens on the server side or using HTTP-only cookies with additional security measures to mitigate risks.
templates/figma/components/PropertiesTab.tsx (1)
191-193: Consider improving the loading indicatorCurrently, when
loadingis true, the component displays a simple "Loading..." text. Enhancing this with a spinner or loading animation can provide better visual feedback to the user.For example:
if (loading) { return ( <div className="flex items-center justify-center"> <Loader2Icon className="mr-2 h-4 w-4 animate-spin" /> Loading... </div> ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- app/api/auth/figma/callback/route.ts (1 hunks)
- app/api/auth/figma/route.ts (1 hunks)
- app/api/auth/figma/status/route.ts (1 hunks)
- templates/figma/Inspector.tsx (8 hunks)
- templates/figma/components/FigmaConnector.tsx (1 hunks)
- templates/figma/components/FigmaTokenContext.tsx (1 hunks)
- templates/figma/components/FigmaTokenEditor.tsx (0 hunks)
- templates/figma/components/PropertiesTab.tsx (4 hunks)
- templates/figma/utils/FigmaApi.ts (4 hunks)
💤 Files with no reviewable changes (1)
- templates/figma/components/FigmaTokenEditor.tsx
🧰 Additional context used
🔇 Additional comments (18)
app/api/auth/figma/status/route.ts (2)
1-1: LGTM: Imports are correct and necessary.The imports of
NextRequestandNextResponsefrom 'next/server' are appropriate for implementing a Next.js API route.
1-16: Overall, good implementation with room for minor improvements.This new route effectively retrieves the Figma access token status, aligning well with the PR objectives for Figma OAuth implementation. The code is concise, follows good practices, and prioritizes security.
Key strengths:
- Use of httpOnly cookies for token storage
- Proper use of Next.js API route conventions
- Concise and focused implementation
Suggested improvements:
- Enhanced error handling
- More detailed security comment
- Additional token validation
These changes will further improve the robustness and security of the implementation.
templates/figma/components/FigmaConnector.tsx (2)
1-4: LGTM: Import statements are appropriate.The import statements are well-structured and include all necessary dependencies for the component's functionality. The 'use client' directive is correctly placed at the top, indicating this is a Client Component in Next.js.
1-28: Overall assessment: Component is functional with room for improvement.The
FigmaTokenEditorcomponent (which we suggested renaming toFigmaConnector) successfully implements the basic functionality for connecting a Figma account. It handles different states appropriately and uses Next.js features correctly.Key points from the review:
- Consider renaming the component to better reflect its purpose.
- Make the API route configurable for better flexibility.
- Enhance the UI for the connected state and add a disconnect option.
These improvements will make the component more robust, flexible, and user-friendly. Despite these suggestions, the current implementation is functional and aligns with the PR objectives of implementing Figma OAuth login.
app/api/auth/figma/route.ts (3)
1-7: LGTM: Proper imports and function signature for Next.js API routeThe imports and function signature are correctly implemented for a Next.js API route using the App Router. This follows the recommended approach for new Next.js applications.
23-27: LGTM: Secure cookie setting with a suggestion to verify sameSite optionThe 'oauth_state' cookie is set with appropriate security options (httpOnly and secure). However, please verify if
sameSite: 'none'is necessary for your setup:
- If your application is served from the same site as the API, you might want to use
sameSite: 'lax'orsameSite: 'strict'for better security.- If cross-origin requests are required (e.g., your frontend is on a different domain), then
sameSite: 'none'is correct.Ensure that this aligns with your application's architecture and security requirements.
1-27: Overall implementation aligns well with PR objectivesThis implementation of the Figma OAuth sign-in redirect aligns well with the PR objectives of implementing Figma OAuth login. It correctly handles the initial step of the OAuth flow, redirecting users to Figma's authorization page with the necessary parameters.
Key points:
- The implementation follows Next.js best practices for API routes.
- It includes CSRF protection, which is a good security measure.
- The scope is set to 'files:read', which will allow the application to access the user's Figma files as required.
The suggested improvements in the previous comments will further enhance the security and robustness of this implementation. Once these are addressed, this file will provide a solid foundation for the Figma OAuth integration.
templates/figma/Inspector.tsx (5)
18-19: LGTM: New imports for Figma OAuth implementationThe addition of
FigmaTokenProviderandFigmaConnectoraligns with the PR objectives for implementing Figma OAuth. These components will likely manage the OAuth token and handle the connection process, respectively.
37-37: Clarify the purpose ofpostUrlinpreviewSlidefunctionA new
postUrlproperty has been added to thesetPreviewDatacall, but it's set toundefined. Could you please clarify the intended use of this property and whether it's fully implemented?
Line range hint
120-132: ReviewuseEffectdependency array and lint ignore comment
The removal of
identifyFontsUsedfrom the dependency array could potentially lead to stale closures. Consider adding it back or explaining why it's not necessary.The lint ignore comment suggests a circular dependency issue. Could you provide more context on this circular rule and whether there's a way to resolve it without ignoring the lint rule?
284-285: LGTM: Consistent use of FigmaTokenProviderWrapping the entire component with
FigmaTokenProviderensures that the Figma token context is available throughout the component tree. This is consistent with the earlier changes and supports the Figma OAuth implementation.
Line range hint
1-320: Summary of Inspector.tsx reviewOverall, the changes in this file align well with the PR objectives for implementing Figma OAuth. Key points:
- The introduction of
FigmaTokenProviderandFigmaConnectorcomponents supports the OAuth implementation.- The UI has been updated to reflect the new login process, improving user experience.
- Some styling improvements have been made for better maintainability.
However, there are a few areas that need attention:
- Clarify the purpose of the new
postUrlproperty in thepreviewSlidefunction.- Review the
useEffecthook's dependency array and the lint ignore comment.- Update the
SlideEditorcomponent to usefigmaAccessTokeninstead offigmaPAT.Please address these points to ensure a smooth integration of the Figma OAuth functionality.
templates/figma/components/FigmaTokenContext.tsx (1)
48-54: Custom hookuseFigmaTokenis properly implementedThe custom hook
useFigmaTokencorrectly retrieves the context and includes a safeguard to ensure it's used within aFigmaTokenProvider. Throwing an error when the context isundefinedprevents accidental misuse and promotes better maintainability.templates/figma/components/PropertiesTab.tsx (5)
59-60: LGTMThe use of
useFigmaTokento retrievefigmaAccessTokenandloadingstate is appropriate for managing the Figma authentication state within the component.
67-67: Duplicate of previous comment
188-189: LGTMThe aspect ratio formatting logic correctly handles both integer and non-integer aspect ratios, ensuring the aspect ratio is displayed in a user-friendly format.
220-224: LGTMThe placeholder text for the Figma URL input dynamically reflects the authentication state, guiding the user appropriately based on whether they are connected to a Figma account.
229-229: LGTMThe button's disabled state correctly prevents the user from initiating the update when the necessary conditions are not met, enhancing the robustness of the UI interactions.
| export async function GET(request: NextRequest) { | ||
| const figmaAccessToken = request.cookies.get('figma_access_token')?.value; | ||
|
|
||
| if (figmaAccessToken) { | ||
| return NextResponse.json({ accessToken: figmaAccessToken }); | ||
| } | ||
|
|
||
| return NextResponse.json({ accessToken: null }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance security with token validation.
The use of httpOnly cookies and avoiding token exposure in URLs are good security practices. However, consider adding token validation before returning it to the client.
Add a simple validation check for the token's format:
export async function GET(request: NextRequest) {
const figmaAccessToken = request.cookies.get('figma_access_token')?.value;
- if (figmaAccessToken) {
+ if (figmaAccessToken && typeof figmaAccessToken === 'string' && figmaAccessToken.trim() !== '') {
return NextResponse.json({ accessToken: figmaAccessToken });
}
return NextResponse.json({ accessToken: null });
}This change ensures that only non-empty string tokens are returned, providing an additional layer of security.
📝 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 async function GET(request: NextRequest) { | |
| const figmaAccessToken = request.cookies.get('figma_access_token')?.value; | |
| if (figmaAccessToken) { | |
| return NextResponse.json({ accessToken: figmaAccessToken }); | |
| } | |
| return NextResponse.json({ accessToken: null }); | |
| } | |
| export async function GET(request: NextRequest) { | |
| const figmaAccessToken = request.cookies.get('figma_access_token')?.value; | |
| if (figmaAccessToken && typeof figmaAccessToken === 'string' && figmaAccessToken.trim() !== '') { | |
| return NextResponse.json({ accessToken: figmaAccessToken }); | |
| } | |
| return NextResponse.json({ accessToken: null }); | |
| } |
| const handleConnectFigma = () => { | ||
| const currentPage = window.location.href; | ||
| router.push(`/api/auth/figma?original_url=${encodeURIComponent(currentPage)}`); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the API route configurable.
The handleConnectFigma function correctly constructs the URL for Figma authentication. However, the API route is hardcoded, which might make the component less flexible if the route changes in the future.
Consider making the API route configurable, either through props or environment variables. For example:
+const FIGMA_AUTH_API_ROUTE = process.env.NEXT_PUBLIC_FIGMA_AUTH_API_ROUTE || '/api/auth/figma';
const handleConnectFigma = () => {
const currentPage = window.location.href;
- router.push(`/api/auth/figma?original_url=${encodeURIComponent(currentPage)}`);
+ router.push(`${FIGMA_AUTH_API_ROUTE}?original_url=${encodeURIComponent(currentPage)}`);
};This change would make the component more flexible and easier to maintain if the API route changes.
📝 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.
| const handleConnectFigma = () => { | |
| const currentPage = window.location.href; | |
| router.push(`/api/auth/figma?original_url=${encodeURIComponent(currentPage)}`); | |
| }; | |
| const FIGMA_AUTH_API_ROUTE = process.env.NEXT_PUBLIC_FIGMA_AUTH_API_ROUTE || '/api/auth/figma'; | |
| const handleConnectFigma = () => { | |
| const currentPage = window.location.href; | |
| router.push(`${FIGMA_AUTH_API_ROUTE}?original_url=${encodeURIComponent(currentPage)}`); | |
| }; |
| return ( | ||
| <div> | ||
| {loading ? ( | ||
| <p>Loading...</p> | ||
| ) : !figmaAccessToken ? ( | ||
| <Button onClick={handleConnectFigma} variant="primary"> | ||
| Connect Figma Account | ||
| </Button> | ||
| ) : ( | ||
| <p>Figma Account Connected</p> | ||
| )} | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the connected state UI and add disconnect functionality.
The current render logic correctly handles different states (loading, not connected, connected). However, there are opportunities to improve the user experience:
- The confirmation message when the account is connected is very basic.
- There's no way for the user to disconnect their Figma account if they want to.
Consider the following improvements:
- Enhance the connected state message to provide more information or actions.
- Add a disconnect option for users who want to change or remove their Figma account connection.
Here's an example of how you might implement these changes:
) : !figmaAccessToken ? (
<Button onClick={handleConnectFigma} variant="primary">
Connect Figma Account
</Button>
) : (
- <p>Figma Account Connected</p>
+ <div>
+ <p>Figma Account Connected</p>
+ <Button onClick={handleDisconnectFigma} variant="secondary">
+ Disconnect Figma Account
+ </Button>
+ </div>
)}You'll need to implement the handleDisconnectFigma function to handle the disconnection logic. This might involve clearing the token from your authentication context and potentially making an API call to revoke the token on the server side.
Committable suggestion was skipped due to low confidence.
app/api/auth/figma/route.ts
Outdated
| const { FIGMA_CLIENT_ID, NEXT_PUBLIC_HOST } = process.env; | ||
|
|
||
| // Get the original page URL from query params or referrer header | ||
| const originalUrl = request.nextUrl.searchParams.get('original_url'); |
There was a problem hiding this comment.
Enhance error handling and input validation
While the environment variable usage and URL parameter extraction are correct, consider the following improvements:
-
Add error handling for undefined environment variables:
const { FIGMA_CLIENT_ID, NEXT_PUBLIC_HOST } = process.env; if (!FIGMA_CLIENT_ID || !NEXT_PUBLIC_HOST) { throw new Error('Missing required environment variables'); }
-
Validate and sanitize the
originalUrl:const originalUrl = request.nextUrl.searchParams.get('original_url'); if (originalUrl && !originalUrl.startsWith(NEXT_PUBLIC_HOST)) { throw new Error('Invalid original_url'); }
These changes will improve the robustness and security of the function.
app/api/auth/figma/route.ts
Outdated
| const redirectUri = `${NEXT_PUBLIC_HOST}/api/auth/figma/callback`; | ||
| const state = JSON.stringify({ | ||
| csrfToken: Math.random().toString(36).substring(2), | ||
| originalUrl: originalUrl, | ||
| }); |
There was a problem hiding this comment.
Improve CSRF token generation security
The current implementation of the CSRF token using Math.random() is not cryptographically secure. Consider using a more robust method for generating the token:
import crypto from 'crypto';
// ...
const state = JSON.stringify({
csrfToken: crypto.randomBytes(32).toString('hex'),
originalUrl: originalUrl,
});This change will significantly enhance the security of your CSRF protection.
| const code = searchParams.get('code'); | ||
| const tokenUrl = 'https://api.figma.com/v1/oauth/token'; |
There was a problem hiding this comment.
Validate the presence of the authorization code.
Ensure that the code parameter is present before attempting to exchange it for a token.
Apply this diff to add validation:
const code = searchParams.get('code');
+ if (!code) {
+ return NextResponse.json({ error: 'Missing authorization code' }, { status: 400 });
+ }
const tokenUrl = 'https://api.figma.com/v1/oauth/token';📝 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.
| const code = searchParams.get('code'); | |
| const tokenUrl = 'https://api.figma.com/v1/oauth/token'; | |
| const code = searchParams.get('code'); | |
| if (!code) { | |
| return NextResponse.json({ error: 'Missing authorization code' }, { status: 400 }); | |
| } | |
| const tokenUrl = 'https://api.figma.com/v1/oauth/token'; |
| state = JSON.parse(decodeURIComponent(stateParam!)); | ||
| } catch { | ||
| return NextResponse.json({ error: 'Invalid state parameter' }, { status: 400 }); | ||
| } |
There was a problem hiding this comment.
Handle potential JSON parsing errors.
Wrap the JSON parsing in a try-catch block to handle any potential errors during decoding or parsing of the stateParam.
Apply this diff to enhance error handling:
let state;
try {
state = JSON.parse(decodeURIComponent(stateParam!));
+ } catch (error) {
+ return NextResponse.json({ error: 'Invalid state parameter' }, { status: 400 });
}Committable suggestion was skipped due to low confidence.
| const tokenData = await tokenResponse.json(); | ||
|
|
||
| if (!tokenResponse.ok) { | ||
| return NextResponse.json({ error: 'Failed to exchange authorization code for token' }, { status: 400 }); | ||
| } | ||
|
|
There was a problem hiding this comment.
Validate token data before usage.
Ensure that the tokenData contains all required properties before proceeding to set cookies.
Apply this diff to add validation:
const tokenData = await tokenResponse.json();
if (!tokenResponse.ok) {
return NextResponse.json({ error: 'Failed to exchange authorization code for token' }, { status: 400 });
}
+ if (!tokenData.access_token || !tokenData.refresh_token || !tokenData.expires_in || !tokenData.user_id) {
+ return NextResponse.json({ error: 'Incomplete token data received from Figma' }, { status: 500 });
+ }📝 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.
| const tokenData = await tokenResponse.json(); | |
| if (!tokenResponse.ok) { | |
| return NextResponse.json({ error: 'Failed to exchange authorization code for token' }, { status: 400 }); | |
| } | |
| const tokenData = await tokenResponse.json(); | |
| if (!tokenResponse.ok) { | |
| return NextResponse.json({ error: 'Failed to exchange authorization code for token' }, { status: 400 }); | |
| } | |
| if (!tokenData.access_token || !tokenData.refresh_token || !tokenData.expires_in || !tokenData.user_id) { | |
| return NextResponse.json({ error: 'Incomplete token data received from Figma' }, { status: 500 }); | |
| } |
| return NextResponse.json({ error: 'Failed to exchange authorization code for token' }, { status: 400 }); | ||
| } | ||
|
|
||
| const response = NextResponse.redirect(state.originalUrl); |
There was a problem hiding this comment.
Validate redirect URL to prevent open redirects.
Ensure that state.originalUrl is a trusted URL within your application's domain to prevent open redirect vulnerabilities.
Apply this diff to validate the redirect URL:
const response = NextResponse.redirect(state.originalUrl);
+ const allowedHosts = [NEXT_PUBLIC_HOST];
+ const redirectUrl = new URL(state.originalUrl);
+ if (!allowedHosts.includes(redirectUrl.origin)) {
+ return NextResponse.json({ error: 'Invalid redirect URL' }, { status: 400 });
+ }Committable suggestion was skipped due to low confidence.
| response.cookies.set('figma_access_token', tokenData.access_token, { | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: 'lax', | ||
| path: '/', | ||
| }); | ||
| response.cookies.set('figma_refresh_token', tokenData.refresh_token, { | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: 'lax', | ||
| path: '/', | ||
| }); | ||
| response.cookies.set('figma_user_id', tokenData.user_id.toString(), { | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: 'lax', | ||
| path: '/', | ||
| }); | ||
| const expiresAt = Date.now() + tokenData.expires_in * 1000; | ||
| response.cookies.set('figma_token_expires_at', expiresAt.toString(), { | ||
| httpOnly: true, | ||
| secure: true, | ||
| sameSite: 'lax', | ||
| path: '/', | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set cookie expiration using 'maxAge' for better control.
Instead of storing the token's expiration time in a separate cookie (figma_token_expires_at), consider using the maxAge option to set the expiration directly on the cookies.
Apply this diff to utilize maxAge:
response.cookies.set('figma_access_token', tokenData.access_token, {
httpOnly: true,
secure: true,
sameSite: 'lax',
path: '/',
+ maxAge: tokenData.expires_in,
});
response.cookies.set('figma_refresh_token', tokenData.refresh_token, {
httpOnly: true,
secure: true,
sameSite: 'lax',
path: '/',
+ maxAge: /* appropriate max age for refresh token */,
});
- response.cookies.set('figma_token_expires_at', expiresAt.toString(), {
- httpOnly: true,
- secure: true,
- sameSite: 'lax',
- path: '/',
- });Ensure that maxAge is set appropriately for each token type, and remove the unnecessary figma_token_expires_at cookie.
Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/api/auth/figma/signout/route.ts (2)
3-16: LGTM: Function implementation is correct and follows best practices.The POST function is well-implemented:
- It follows Next.js API route conventions.
- It creates an appropriate JSON response.
- It clears all relevant Figma-related cookies.
- The function is concise and focused on its single responsibility.
Consider adding basic error handling to make the function more robust:
export async function POST() { + try { const response = NextResponse.json({ message: 'Signout successful' }); // Clear Figma-related cookies response.cookies.delete('figma_access_token'); response.cookies.delete('figma_refresh_token'); response.cookies.delete('figma_user_id'); response.cookies.delete('figma_token_expires_at'); return response; + } catch (error) { + console.error('Error during Figma signout:', error); + return NextResponse.json({ message: 'Signout failed' }, { status: 500 }); + } }
6-16: Enhance security by adding authentication middleware.While the function correctly clears Figma-related cookies, it's crucial to ensure that this route is only accessible to authenticated users. This prevents potential abuse of the signout functionality.
Consider adding authentication middleware to this route. For example, using Next.js middleware:
- Create a new file
middleware.tsin the root of your project if it doesn't exist already.- Add the following code to protect the signout route:
import { NextResponse } from 'next/server' import type { NextRequest } from 'next/server' export function middleware(request: NextRequest) { const session = request.cookies.get('session')?.value if (!session && request.nextUrl.pathname.startsWith('/api/auth/figma/signout')) { return NextResponse.redirect(new URL('/api/auth/unauthorized', request.url)) } } export const config = { matcher: '/api/auth/figma/signout', }This middleware will redirect unauthenticated requests to an unauthorized endpoint, enhancing the security of your signout route.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- .env.example (1 hunks)
- app/api/auth/figma/signout/route.ts (1 hunks)
- templates/figma/components/FigmaConnector.tsx (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.example
- templates/figma/components/FigmaConnector.tsx
🧰 Additional context used
🔇 Additional comments (1)
app/api/auth/figma/signout/route.ts (1)
1-1: LGTM: Import statement is correct.The import of
NextResponsefrom 'next/server' is appropriate for creating the JSON response in this route.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- .env.example (1 hunks)
- app/api/auth/figma/callback/route.ts (1 hunks)
- app/api/auth/figma/signin/route.ts (1 hunks)
- templates/figma/Config.ts (0 hunks)
- templates/figma/components/FigmaConnector.tsx (1 hunks)
- templates/figma/components/PropertiesTab.tsx (4 hunks)
- templates/figma/utils/FigmaApi.ts (5 hunks)
- templates/figma/utils/FigmaFrameBuilder.ts (1 hunks)
💤 Files with no reviewable changes (1)
- templates/figma/Config.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- .env.example
- app/api/auth/figma/callback/route.ts
- templates/figma/components/FigmaConnector.tsx
- templates/figma/components/PropertiesTab.tsx
- templates/figma/utils/FigmaApi.ts
🧰 Additional context used
🔇 Additional comments (2)
templates/figma/utils/FigmaFrameBuilder.ts (1)
54-55: Approve the simplified condition for NoFigmaViewThe simplified condition aligns well with the PR objectives of implementing Figma OAuth. By removing the check for
config.figmaPAT, we've eliminated the dependency on personal access tokens, which is a positive security improvement. The new condition correctly focuses on the presence ofslideConfigandslideConfig.figmaUrl, which are the essential elements for rendering a Figma frame.To ensure the OAuth implementation is working as expected, please verify:
- The OAuth flow correctly sets
slideConfig.figmaUrl.- There are no remaining references to
config.figmaPATin related code.You can use the following script to check for any remaining references to
figmaPAT:If this search returns any results, those areas may need to be updated to work with the new OAuth implementation.
✅ Verification successful
No remaining references to
figmaPATfoundAll references to
figmaPAThave been successfully removed from the codebase, ensuring that the OAuth implementation is properly in place without dependencies on personal access tokens.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to figmaPAT rg 'figmaPAT' --type typescript --type javascriptLength of output: 84
Script:
#!/bin/bash # Search for any remaining references to figmaPAT in TypeScript and JavaScript files rg 'figmaPAT' -g '*.ts' -g '*.js'Length of output: 33
app/api/auth/figma/signin/route.ts (1)
34-34: ReviewsameSiteattribute value in cookie settingsThe
oauth_statecookie is set withsameSite: 'none'.response.cookies.set('oauth_state', state, { httpOnly: true, secure: true, sameSite: 'none', path: '/' });If cross-site requests are not necessary for your OAuth flow, consider using
sameSite: 'lax'orsameSite: 'strict'to enhance security against CSRF attacks.
| // biome-ignore lint/complexity/useSimplifiedLogicExpression: <explanation> | ||
| if (!FIGMA_CLIENT_ID || !NEXT_PUBLIC_HOST) { |
There was a problem hiding this comment.
Provide an explanation for ignoring lint rule
The comment on line 10 is missing an explanation for why the linter rule is being ignored.
// biome-ignore lint/complexity/useSimplifiedLogicExpression: <explanation>Please provide a meaningful explanation to help future maintainers understand the reasoning.
Apply this diff to include the explanation:
-// biome-ignore lint/complexity/useSimplifiedLogicExpression: <explanation>
+// biome-ignore lint/complexity/useSimplifiedLogicExpression: Both environment variables are required, and the current logic enhances readability.|
|
||
| const redirectUri = `${NEXT_PUBLIC_HOST}/api/auth/figma/callback`; | ||
| const state = JSON.stringify({ | ||
| csrfToken: Math.random().toString(36).substring(2), |
There was a problem hiding this comment.
Use a cryptographically secure method for generating CSRF tokens
Using Math.random() is not suitable for generating CSRF tokens because it is not cryptographically secure. This could lead to predictable tokens and potential security vulnerabilities.
Apply this diff:
+import { randomBytes } from 'crypto';
...
- csrfToken: Math.random().toString(36).substring(2),
+ csrfToken: randomBytes(32).toString('hex'),Committable suggestion was skipped due to low confidence.
Closes #201
Figma App
Frame Editor
OAuth2 flow
Summary by CodeRabbit
Summary by CodeRabbit
FIGMA_CLIENT_ID,FIGMA_CLIENT_SECRET, andNEXTAUTH_URL.Inspectorcomponent for improved management of Figma access tokens and slide configurations.FigmaTokenEditorwith theFigmaConnectorcomponent for better user interface and connection management.PropertiesTabcomponent to utilize session management for Figma access tokens.These updates significantly enhance the integration with Figma, making the authentication process more efficient and user-friendly.