Feat/new app frontend #2714
Feat/new app frontend #2714
Conversation
WalkthroughThe pull request introduces several new files and configurations for the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (32)
app-front/public/.well-known/security-policy.txt (4)
1-4: Fix capitalization in header sectionAccording to French language conventions:
- "Décembre" should be "décembre"
- "Ministère" should be lowercase when not used as a proper noun
-Registre de preuve de covoiturage (RPC) Décembre 2023 -Startup d'État du Ministère +Registre de preuve de covoiturage (RPC) décembre 2023 +Startup d'État du ministère🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: Les mois de l’année s’écrivent sans majuscule.
Context: ...de covoiturage (RPC) Décembre 2023 Startup d'État du Ministère de la ...(MOIS)
[uncategorized] ~2-~2: Ce nom commence par une minuscule.
Context: ... Décembre 2023 Startup d'État du Ministère de la Transition Écologique et Solidair...(MINISTERE_MAJ)
37-41: Fix typography in commitment statementThe word "coeur" should use the proper French ligature "œ".
-L'équipe technique prend très à coeur la sécurité de ses utilisateurs +L'équipe technique prend très à cœur la sécurité de ses utilisateurs🧰 Tools
🪛 LanguageTool
[typographical] ~37-~37: Variante typographique déconseillée de « cœur ».
Context: ...etc L'équipe technique prend très à coeur la sécurité de ses utilisateurs et d...(OE)
48-51: Enhance security contact information formattingThe fingerprint line should be preceded by a non-breaking space before the colon, following French typography rules.
-Key fingerprint: 4810 3904 BA33 6554 3B27 BF28 F92C CE05 85A4 AE8F +Key fingerprint : 4810 3904 BA33 6554 3B27 BF28 F92C CE05 85A4 AE8F🧰 Tools
🪛 LanguageTool
[uncategorized] ~51-~51: Les deux-points sont précédés d’une espace insécable.
Context: ...gouv.fr/.well-known/security.asc Key fingerprint: 4810 3904 BA33 6554 3B27 BF28 F92C CE0...(FRENCH_WHITESPACE)
53-56: Fix gender agreement in response commitmentThe word "informé" should agree with the possibility of multiple recipients.
-elle vous tiendra informé de la +elle vous tiendra informé(e) de la🧰 Tools
🪛 LanguageTool
[uncategorized] ~54-~54: Vérifiez l’accord avec le sujet.
Context: ...iale à votre rapport, elle vous tiendra informé de la progression vers une correctio...(ACCORD_V_PPA)
api/justfile (1)
29-31: Review service dependency changesThe replacement of "kc" with "proxy" in both
start_e2eandci_test_integrationcommands suggests a significant change in the service architecture. This change aligns with the frontend updates mentioned in the PR objectives.Consider documenting:
- The reason for replacing "kc" with "proxy"
- Any new requirements for local development setup
- Impact on existing integration tests
Also applies to: 98-98
app-front/src/config/auth.ts (1)
2-5: Remove unnecessary template literalsThe template literals are unnecessary when there's no string interpolation.
export const auth = { - domain: `${process.env.NEXT_PUBLIC_PC_DOMAIN}`, - redirect_uri: `${process.env.NEXT_PUBLIC_PC_REDIRECT_URI}`, - client_id: `${process.env.NEXT_PUBLIC_PC_CLIENT_ID}`, - scopes: `${process.env.NEXT_PUBLIC_PC_SCOPES}`, + domain: process.env.NEXT_PUBLIC_PC_DOMAIN, + redirect_uri: process.env.NEXT_PUBLIC_PC_REDIRECT_URI, + client_id: process.env.NEXT_PUBLIC_PC_CLIENT_ID, + scopes: process.env.NEXT_PUBLIC_PC_SCOPES, };app-front/src/interfaces/componentsInterface.ts (2)
1-7: Add JSDoc documentation for BlockProps interfaceConsider adding documentation to improve code maintainability and IDE support.
/** * Props for the Block component * @property {string} title - The title of the block * @property {string} content - The main content of the block * @property {string} [img] - Optional URL for the block's image * @property {string} [alt] - Alternative text for the image * @property {Button[]} [buttons] - Optional array of action buttons */ export interface BlockProps { // ... existing properties }
9-14: Consider improvements to Button interfaceA few suggestions to enhance the Button interface:
- Add URL validation using a more specific type
- Consider using a theme-based approach for colors
- Add documentation for properties
type ThemeColor = 'primary' | 'secondary' | 'accent' | 'neutral'; type ValidUrl = `https://${string}` | `/${string}`; /** * Represents a clickable button with optional styling * @property {string} title - Button text * @property {ValidUrl} url - Target URL (must be HTTPS or relative path) * @property {string} [icon] - Optional icon identifier * @property {ThemeColor} [color] - Theme color for the button */ export interface Button { title: string; url: ValidUrl; icon?: string; color?: ThemeColor; }app-front/src/components/common/PageTitle.tsx (1)
3-11: Add prop validation and enhance accessibilityThe component implementation is clean and follows design system conventions. However, consider these improvements:
-export default function PageTitle({ title }: { title: string }) { +interface PageTitleProps { + title: string; +} + +export default function PageTitle({ title }: PageTitleProps) { + if (!title?.trim()) { + throw new Error('PageTitle: title prop cannot be empty'); + } + return ( - <div id='page-title' className={fr.cx('fr-grid-row', 'fr-mt-5v')}> + <div + id='page-title' + className={fr.cx('fr-grid-row', 'fr-mt-5v')} + role="banner" + > <div className={fr.cx('fr-col')}> <h1>{title}</h1> </div> </div> ); }app-front/src/components/layout/StartDsfr.tsx (2)
13-18: Add error handling and improve documentationThe initialization looks good, but consider these improvements:
-startReactDsfr({ defaultColorScheme, Link }); +try { + startReactDsfr({ defaultColorScheme, Link }); +} catch (error) { + console.error('Failed to initialize DSFR:', error); +} +/** + * StartDsfr initializes the French government design system (DSFR). + * Returns null as it's meant to be used for initialization only. + * @returns null + */ export function StartDsfr() { - //Yes, leave null here. + // This component only handles DSFR initialization + // and intentionally doesn't render anything return null; }
7-11: Consider moving type augmentation to a separate fileThe module augmentation is correct but could be better organized:
Consider moving the type augmentation to a separate
types/dsfr.d.tsfile to keep the component file focused on implementation and make types more discoverable.app-front/src/components/layout/Analytics.tsx (2)
10-12: Add cleanup function to useEffectThe Matomo initialization might need cleanup when the component unmounts.
useEffect(() => { init({ url: matomoUrl, siteId: matomoSiteId }); + return () => { + // Add cleanup if Matomo provides an API for this + }; }, []);
1-1: Move "use client" directive to a higher levelConsider moving the "use client" directive to a layout component if analytics should be available throughout the app.
app-front/src/components/layout/AppHeader.tsx (2)
20-24: Consider extracting repeated text to constantsThe title text is duplicated in both title and aria-label properties.
+const HOME_TITLE = 'Accueil | Espace partenaire Covoiturage.beta.gouv.fr'; + homeLinkProps={{ href: '/', - title: 'Accueil | Espace partenaire Covoiturage.beta.gouv.fr', - "aria-label": 'Accueil | Espace partenaire Covoiturage.beta.gouv.fr', + title: HOME_TITLE, + "aria-label": HOME_TITLE, }}
7-19: Consider extracting ministry name for internationalizationThe ministry name should be extracted to support multiple languages if needed.
Consider moving the text to a translation file or constants file for better maintainability and future internationalization support.
app-front/src/components/layout/Skiplinks.tsx (3)
5-5: Consider documenting the tabIndex usageThe
tabIndex={-1}on the container div makes it programmatically focusable but removes it from the natural tab order. While this is likely intentional for skip links, consider adding a comment explaining this accessibility implementation detail.
9-15: Optimize accessibility attributesThe
titleandaria-labelattributes contain duplicate information. According to ARIA best practices, when the visible text matches the intended accessible name, you can omit thearia-label.<a className={fr.cx('fr-link')} href="#content" - title="contenu" - aria-label="contenu" + title="Aller au contenu" > Contenu </a>Also applies to: 18-24, 27-33
1-1: Add type safety for design system utilitiesConsider adding type safety for the
fr.cx()calls to prevent potential runtime errors.import { fr } from '@codegouvfr/react-dsfr'; import type { ClassNameFromFrCx } from '@codegouvfr/react-dsfr'; const skiplinksClass: ClassNameFromFrCx = 'fr-skiplinks';app-front/src/components/auth/AuthButton.tsx (2)
23-23: Translate French comments to EnglishFor consistency, translate the French comment "Ajouter chaque paramètre encodé" to English.
- // Ajouter chaque paramètre encodé + // Add each encoded parameter
31-35: Remove unnecessary fragmentThe empty fragment wrapper is unnecessary since there's only one child element.
- return ( - <> - <ProConnectButton onClick={() => {router.push(convertParamsToQueryString(baseUrl, params))}} /> - </> - ); + return <ProConnectButton onClick={() => router.push(convertParamsToQueryString(baseUrl, params))} />;app-front/src/app/administration/page.tsx (1)
20-20: Replace placeholder tab contentThe tabs contain placeholder content (
Content of tab1, etc.). This should be replaced with actual implementation.Would you like me to help create the content components for each tab section?
Also applies to: 24-24, 28-28, 32-32
app-front/src/providers/AuthProvider.tsx (1)
5-13: Enhance AuthContextProps interfaceThe interface should include additional properties for better auth state management.
interface AuthContextProps { isAuth: boolean; state?: string; nonce?: string; token?: string; user: { name: string; role: string; permissions?: string[]; // Add role-based access control }; isLoading: boolean; // Add loading state error?: Error; // Add error state login: () => Promise<void>; logout: () => Promise<void>; refreshToken: () => Promise<void>; // Add token refresh }app-front/src/components/layout/Navigation.tsx (1)
6-9: Handle loading state during authenticationThe navigation should handle the loading state while authentication is being determined.
export default function Navigation() { const pathname = usePathname(); const {isAuth, isLoading, user} = useAuth(); if (isLoading) { return <MainNavigation id='header-navigation' items={[]} />; // Show empty navigation while loading } // ... rest of the component }app-front/src/app/activite/page.tsx (1)
44-44: Optimize image loading and improve accessibilityThe image loading can be optimized and accessibility improved.
-start={<Image src='https://static.covoiturage.beta.gouv.fr/Obs_021e3f4a41.svg' alt='' width={70} height={70} />} +start={<Image + src='https://static.covoiturage.beta.gouv.fr/Obs_021e3f4a41.svg' + alt='Observatoire icon' + width={70} + height={70} + priority + loading="eager" +/>}app-front/src/config/index.ts (1)
21-30: Consider removing or implementing the commented codeThe commented
nextEnvironmentVariablesfunction appears to be unused. Either implement it if needed or remove it to maintain code cleanliness.app-front/src/app/layout.tsx (2)
33-44: Clean up font preloading configurationThe commented-out font preloading configurations create unnecessary code noise and may impact maintainability.
Either remove the commented fonts or document why they're kept for future reference:
<DsfrHead Link={Link} preloadFonts={[ - //"Marianne-Light", - //"Marianne-Light_Italic", 'Marianne-Regular', - //"Marianne-Regular_Italic", 'Marianne-Medium', - //"Marianne-Medium_Italic", 'Marianne-Bold', - //"Marianne-Bold_Italic", - //"Spectral-Regular", - //"Spectral-ExtraBold" ]} />
53-57: Consider adding aria-label to main contentThe main content area could benefit from an accessible label for better screen reader navigation.
- <main tabIndex={-1}> + <main tabIndex={-1} aria-label="Contenu principal"> {children} <ScrollToTop /> <Follow /> </main>app-front/src/components/layout/Follow.tsx (1)
4-81: Consider extracting social media links configurationThe social media links configuration could be extracted into a separate constant to improve maintainability and reusability.
const SOCIAL_MEDIA_LINKS = [ { platform: 'twitter-x', href: 'https://twitter.com/Covoitbetagouv', text: 'Twitter' }, // ... other social media configs ] as const; // Then in the component: {SOCIAL_MEDIA_LINKS.map(({ platform, href, text }) => ( <li key={platform}> <a className={fr.cx(`fr-btn--${platform}`, 'fr-btn')} target="_blank" rel="noopener noreferrer" href={href} title={`${text} - nouvelle fenêtre`} aria-label={`${text} - nouvelle fenêtre`} > {text} <span className={fr.cx('fr-sr-only')}> - nouvelle fenêtre</span> </a> </li> ))}app-front/src/components/layout/AppFooter.tsx (3)
8-64: Consider extracting link configurationsThe link list configuration could be extracted into a separate constant to improve maintainability.
const FOOTER_LINKS = [ { categoryName: 'Startup d\'Etat Covoiturage.beta.gouv', links: [ { href: 'https://doc.covoiturage.beta.gouv.fr/bienvenue/qui-sommes-nous', text: 'Qui sommes-nous ?' }, // ... other links ] }, // ... other categories ] as const; // Then transform this in the component to add common properties const linkList = FOOTER_LINKS.map(category => ({ categoryName: category.categoryName, links: category.links.map(link => ({ text: link.text, linkProps: { href: link.href, target: '_blank', rel: 'noopener noreferrer', title: `${link.text} | nouvelle fenêtre`, 'aria-label': `${link.text} | nouvelle fenêtre` } })) }));
77-81: Consider handling image loading failuresThe operator logo should handle loading failures gracefully.
Consider adding an
onErrorhandler or using Next.jsImagecomponent for better image handling:import Image from 'next/image'; // ... in the component operatorLogo={{ alt: 'Registre de Preuve de Covoiturage', imgUrl: 'https://static.covoiturage.beta.gouv.fr/logo_rpc_d82e4b3a4a.png', orientation: 'horizontal', // Add error handling if supported by the Footer component onError: (e) => { console.error('Failed to load operator logo:', e); // Optionally set a fallback } }}
93-94: Fix typo in license textThere's a grammatical error in the license text.
- {' '}Toutes les illustrations sont réalisés par + {' '}Toutes les illustrations sont réalisées parpublic/src/context/DashboardProvider.tsx (1)
1-12: Add a custom hook for consuming the contextTo improve reusability and type safety, consider adding a custom hook for consuming this context.
Add this implementation to the file:
export const useDashboardContext = () => { const context = useContext(DashboardContext); if (context === undefined) { throw new Error('useDashboardContext must be used within a DashboardProvider'); } return context; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app-front/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
api/justfile(1 hunks)app-front/.env.example(1 hunks)app-front/.eslintrc.json(1 hunks)app-front/.gitignore(1 hunks)app-front/README.md(1 hunks)app-front/next.config.js(1 hunks)app-front/package.json(1 hunks)app-front/public/.well-known/security-policy.txt(1 hunks)app-front/public/.well-known/security.asc(1 hunks)app-front/public/.well-known/security.txt(1 hunks)app-front/src/app/activite/page.tsx(1 hunks)app-front/src/app/administration/page.tsx(1 hunks)app-front/src/app/layout.tsx(1 hunks)app-front/src/app/page.tsx(1 hunks)app-front/src/components/auth/AuthButton.tsx(1 hunks)app-front/src/components/common/PageTitle.tsx(1 hunks)app-front/src/components/layout/Analytics.tsx(1 hunks)app-front/src/components/layout/AppFooter.tsx(1 hunks)app-front/src/components/layout/AppHeader.tsx(1 hunks)app-front/src/components/layout/Follow.tsx(1 hunks)app-front/src/components/layout/Navigation.tsx(1 hunks)app-front/src/components/layout/ScrollToTop.tsx(1 hunks)app-front/src/components/layout/Skiplinks.tsx(1 hunks)app-front/src/components/layout/StartDsfr.tsx(1 hunks)app-front/src/components/layout/defaultColorScheme.tsx(1 hunks)app-front/src/config/analytics.ts(1 hunks)app-front/src/config/auth.ts(1 hunks)app-front/src/config/index.ts(1 hunks)app-front/src/config/search.ts(1 hunks)app-front/src/interfaces/componentsInterface.ts(1 hunks)app-front/src/providers/AuthProvider.tsx(1 hunks)app-front/src/styles/global.scss(1 hunks)app-front/tsconfig.json(1 hunks)public/next.config.js(2 hunks)public/src/context/DashboardProvider.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (12)
- app-front/src/components/layout/defaultColorScheme.tsx
- app-front/public/.well-known/security.asc
- app-front/src/config/analytics.ts
- app-front/.eslintrc.json
- app-front/src/styles/global.scss
- app-front/tsconfig.json
- app-front/.gitignore
- app-front/.env.example
- app-front/public/.well-known/security.txt
- public/next.config.js
- app-front/README.md
- app-front/package.json
🧰 Additional context used
🪛 LanguageTool
app-front/public/.well-known/security-policy.txt
[uncategorized] ~1-~1: Les mois de l’année s’écrivent sans majuscule.
Context: ...de covoiturage (RPC) Décembre 2023 Startup d'État du Ministère de la ...
(MOIS)
[uncategorized] ~2-~2: Ce nom commence par une minuscule.
Context: ... Décembre 2023 Startup d'État du Ministère de la Transition Écologique et Solidair...
(MINISTERE_MAJ)
[typographical] ~37-~37: Variante typographique déconseillée de « cœur ».
Context: ...etc L'équipe technique prend très à coeur la sécurité de ses utilisateurs et d...
(OE)
[uncategorized] ~51-~51: Les deux-points sont précédés d’une espace insécable.
Context: ...gouv.fr/.well-known/security.asc Key fingerprint: 4810 3904 BA33 6554 3B27 BF28 F92C CE0...
(FRENCH_WHITESPACE)
[uncategorized] ~54-~54: Vérifiez l’accord avec le sujet.
Context: ...iale à votre rapport, elle vous tiendra informé de la progression vers une correctio...
(ACCORD_V_PPA)
[style] ~69-~69: Dans un contexte formel des synonymes peuvent enrichir votre style.
Context: ...es qui nous intéressent Nous sommes intéressés par tout problème qui pourrait comprome...
(REP_INTERESSER)
[style] ~74-~74: Dans un contexte formel des synonymes peuvent enrichir votre style.
Context: ...i quelques exemples de failles qui nous intéressent : - Cross-site scripting (XSS) ...
(REP_INTERESSER)
[style] ~85-~85: Un autre verbe peut sembler plus formel et professionnel.
Context: ... d'État 2.4. Les failles qui ne nous intéressent pas Nous pensons que les failles suivan...
(INTERESSER_PEU)
[style] ~87-~87: Le point de vue peut être modifié afin d’alléger cette phrase.
Context: ...failles qui ne nous intéressent pas Nous pensons que les failles suivantes sont hors périmètre s...
(FOCALISATION_NOUS_VOUS)
🪛 Biome (1.9.4)
app-front/src/config/index.ts
[error] 36-36: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
🔇 Additional comments (6)
app-front/public/.well-known/security-policy.txt (2)
64-65: Verify the GitHub repository link
Please ensure that the GitHub repository link is accessible and points to the correct repository.
✅ Verification successful
GitHub repository link is valid and accessible
The GitHub repository link https://github.com/betagouv/preuve-covoiturage/releases points to a valid and accessible repository, confirming that users will be able to access the latest release information as intended in the security policy.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the GitHub repository link
gh repo view betagouv/preuve-covoiturage > /dev/null
if [ $? -eq 0 ]; then
echo "Repository exists and is accessible"
else
echo "Repository not found or not accessible"
fi
Length of output: 141
1-138: Ensure security.txt compliance with RFC 9116
The security policy appears well-structured but should be verified against RFC 9116 requirements.
Consider adding an "Expires" field in the security.txt file to comply with RFC 9116 requirements.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: Les mois de l’année s’écrivent sans majuscule.
Context: ...de covoiturage (RPC) Décembre 2023 Startup d'État du Ministère de la ...
(MOIS)
[uncategorized] ~2-~2: Ce nom commence par une minuscule.
Context: ... Décembre 2023 Startup d'État du Ministère de la Transition Écologique et Solidair...
(MINISTERE_MAJ)
[typographical] ~37-~37: Variante typographique déconseillée de « cœur ».
Context: ...etc L'équipe technique prend très à coeur la sécurité de ses utilisateurs et d...
(OE)
[uncategorized] ~51-~51: Les deux-points sont précédés d’une espace insécable.
Context: ...gouv.fr/.well-known/security.asc Key fingerprint: 4810 3904 BA33 6554 3B27 BF28 F92C CE0...
(FRENCH_WHITESPACE)
[uncategorized] ~54-~54: Vérifiez l’accord avec le sujet.
Context: ...iale à votre rapport, elle vous tiendra informé de la progression vers une correctio...
(ACCORD_V_PPA)
[style] ~69-~69: Dans un contexte formel des synonymes peuvent enrichir votre style.
Context: ...es qui nous intéressent Nous sommes intéressés par tout problème qui pourrait comprome...
(REP_INTERESSER)
[style] ~74-~74: Dans un contexte formel des synonymes peuvent enrichir votre style.
Context: ...i quelques exemples de failles qui nous intéressent : - Cross-site scripting (XSS) ...
(REP_INTERESSER)
[style] ~85-~85: Un autre verbe peut sembler plus formel et professionnel.
Context: ... d'État 2.4. Les failles qui ne nous intéressent pas Nous pensons que les failles suivan...
(INTERESSER_PEU)
[style] ~87-~87: Le point de vue peut être modifié afin d’alléger cette phrase.
Context: ...failles qui ne nous intéressent pas Nous pensons que les failles suivantes sont hors périmètre s...
(FOCALISATION_NOUS_VOUS)
api/justfile (1)
29-31: Verify proxy service configuration
The start_e2e command now includes "proxy" in its default services. Please ensure that:
- The proxy service is properly configured in the docker-compose files
- The service dependencies are correctly ordered (proxy should start before dependent services)
Let's verify the proxy service configuration:
✅ Verification successful
Proxy service configuration is properly set up
The Traefik proxy service is correctly configured in docker-compose.e2e.yml with:
- Proper port mappings (80, 443, 8080)
- TLS/HTTPS support via websecure entrypoint
- Service discovery enabled through Docker provider
- All required services (api, cms, s3, mailer) have proper Traefik labels for routing
- Network configuration allowing communication between services via 'back' and 'front' networks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check proxy service configuration in docker-compose files
# Check if proxy service is defined in docker-compose files
echo "Checking proxy service configuration..."
rg -A 10 'proxy:' docker-compose.*.yml
# Check for any proxy-related environment variables
echo "Checking proxy-related environment variables..."
rg 'PROXY_|proxy' .env* docker-compose.*.yml
Length of output: 887
Script:
#!/bin/bash
# Let's check for service dependencies and startup order
# Check for service dependencies in docker-compose files
echo "Checking service dependencies..."
rg -A 5 'depends_on:' docker-compose.e2e.yml
# Check if any services are configured to wait for proxy
echo "Checking services configuration for proxy dependencies..."
rg -B 5 -A 5 'traefik' docker-compose.e2e.yml
# Check for any labels or routing configurations
echo "Checking traefik routing configurations..."
rg 'traefik\..*=' docker-compose.e2e.yml
Length of output: 6671
app-front/src/components/auth/AuthButton.tsx (1)
15-15: Document or implement the commented ACR values
There's a commented out acr_values:'eidas1' line. If this is intended for future implementation or represents a security requirement, it should be documented or tracked in an issue.
Would you like me to create an issue to track the implementation of eidas1 authentication level?
public/src/context/DashboardProvider.tsx (2)
1-2: Verify type compatibility between useDashboard and DashboardContextType
Ensure that the return type of useDashboard hook matches the dashboard property in DashboardContextType.
Let's verify the type definitions:
✅ Verification successful
Based on the gathered information, I can now generate the final response:
Type compatibility between useDashboard and DashboardContextType is verified
The types are compatible:
useDashboardhook returns an object withparams,lastPeriod,loading, andgetParamswhich matches thedashboardproperty requirements inDashboardContextType- The
Paramstype used ingetParamsmatches the parameter type defined in the interface - The hook's implementation aligns with the context's type definition
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check type definitions for potential mismatches
# Check the interface definition
echo "Checking DashboardContextType interface:"
rg -A 5 "interface DashboardContextType"
# Check the hook implementation
echo "Checking useDashboard hook implementation:"
rg -A 5 "export const useDashboard"
Length of output: 688
Script:
#!/bin/bash
# Let's search for the interface definition in a different way and also check the hook's return type
# Search for DashboardContextType in all files
echo "Searching for DashboardContextType definition:"
fd -e ts -e tsx | xargs rg "DashboardContextType" -B 2 -A 5
# Search for the complete useDashboard implementation
echo -e "\nSearching for complete useDashboard implementation:"
ast-grep --pattern 'export const useDashboard = () => {
$$$
}'
Length of output: 10305
Script:
#!/bin/bash
# Let's check for the DashboardParams type and other related types
echo "Searching for DashboardParams type definition:"
rg "type DashboardParams" -B 2 -A 10
echo -e "\nSearching for PerimeterType and PeriodType:"
rg "type (PerimeterType|PeriodType)" -B 2 -A 5
echo -e "\nSearching for Params type:"
rg "type Params" -B 2 -A 5
Length of output: 26709
1-12: Verify access control for dashboard data
Ensure that the useDashboard hook implements proper access control and that sensitive dashboard data is only available to authenticated users.
Let's check the authentication integration:
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
app-front/public/.well-known/security-policy.txt (5)
1-2: Fix capitalization in the header dateAccording to French language rules, months should not be capitalized.
-Registre de preuve de covoiturage (RPC) Décembre 2023 +Registre de preuve de covoiturage (RPC) décembre 2023🧰 Tools
🪛 LanguageTool
[uncategorized] ~1-~1: Les mois de l’année s’écrivent sans majuscule.
Context: ...de covoiturage (RPC) Décembre 2023 Startup d'État du Ministère de la ...(MOIS)
[uncategorized] ~2-~2: Ce nom commence par une minuscule.
Context: ... Décembre 2023 Startup d'État du Ministère de la Transition Écologique et Solidair...(MINISTERE_MAJ)
48-51: Enhance security contact information formattingThe PGP key information should be more clearly formatted with proper French typography (space before colon).
[email protected] -https://observatoire.covoiturage.gouv.fr/.well-known/security.asc -Key fingerprint: 4810 3904 BA33 6554 3B27 BF28 F92C CE05 85A4 AE8F +https://observatoire.covoiturage.gouv.fr/.well-known/security.asc +Empreinte de la clé : 4810 3904 BA33 6554 3B27 BF28 F92C CE05 85A4 AE8F🧰 Tools
🪛 LanguageTool
[uncategorized] ~51-~51: Les deux-points sont précédés d’une espace insécable.
Context: ...gouv.fr/.well-known/security.asc Key fingerprint: 4810 3904 BA33 6554 3B27 BF28 F92C CE0...(FRENCH_WHITESPACE)
64-65: Add version information clarityThe version support statement should be more specific about the release channel.
-La version supportée est celle de la dernière release. -https://github.com/betagouv/preuve-covoiturage/releases +La version supportée est celle de la dernière release stable publiée sur : +https://github.com/betagouv/preuve-covoiturage/releases
133-136: Add security standards referencesConsider adding references to additional security standards and best practices.
[RFC9116] https://www.rfc-editor.org/rfc/rfc9116 https://securitytxt.org/ + + [CVSS] https://www.first.org/cvss/ + [CWE] https://cwe.mitre.org/ + [OWASP] https://owasp.org/
137-137: Add structure to Hall of Fame sectionThe "Hall of Fame" section is empty and needs structure for recognizing security researchers.
Would you like me to provide a template for the Hall of Fame section that includes:
- Recognition categories
- Submission date
- Impact level
- Researcher attribution preferences
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app-front/public/.well-known/security-policy.txt(1 hunks)app-front/src/app/proconnect/page.tsx(1 hunks)app-front/src/components/auth/AuthButton.tsx(1 hunks)app-front/src/providers/AuthProvider.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app-front/src/components/auth/AuthButton.tsx
🧰 Additional context used
🪛 LanguageTool
app-front/public/.well-known/security-policy.txt
[uncategorized] ~1-~1: Les mois de l’année s’écrivent sans majuscule.
Context: ...de covoiturage (RPC) Décembre 2023 Startup d'État du Ministère de la ...
(MOIS)
[uncategorized] ~2-~2: Ce nom commence par une minuscule.
Context: ... Décembre 2023 Startup d'État du Ministère de la Transition Écologique et Solidair...
(MINISTERE_MAJ)
[typographical] ~37-~37: Variante typographique déconseillée de « cœur ».
Context: ...etc L'équipe technique prend très à coeur la sécurité de ses utilisateurs et d...
(OE)
[uncategorized] ~51-~51: Les deux-points sont précédés d’une espace insécable.
Context: ...gouv.fr/.well-known/security.asc Key fingerprint: 4810 3904 BA33 6554 3B27 BF28 F92C CE0...
(FRENCH_WHITESPACE)
[uncategorized] ~54-~54: Vérifiez l’accord avec le sujet.
Context: ...iale à votre rapport, elle vous tiendra informé de la progression vers une correctio...
(ACCORD_V_PPA)
[style] ~69-~69: Dans un contexte formel des synonymes peuvent enrichir votre style.
Context: ...es qui nous intéressent Nous sommes intéressés par tout problème qui pourrait comprome...
(REP_INTERESSER)
[style] ~74-~74: Dans un contexte formel des synonymes peuvent enrichir votre style.
Context: ...i quelques exemples de failles qui nous intéressent : - Cross-site scripting (XSS) ...
(REP_INTERESSER)
[style] ~85-~85: Un autre verbe peut sembler plus formel et professionnel.
Context: ... d'État 2.4. Les failles qui ne nous intéressent pas Nous pensons que les failles suivan...
(INTERESSER_PEU)
[style] ~87-~87: Le point de vue peut être modifié afin d’alléger cette phrase.
Context: ...failles qui ne nous intéressent pas Nous pensons que les failles suivantes sont hors périmètre s...
(FOCALISATION_NOUS_VOUS)
🔇 Additional comments (3)
app-front/public/.well-known/security-policy.txt (1)
97-102: Remove duplicate entry in out-of-scope vulnerabilities
"Attaques physiques" appears in the list of out-of-scope vulnerabilities.
- Social Engineering
- Attaques physiques
- Attaques par phishing
- Attaques par clickjacking
- Attaques par sniffing
- Attaques par MITM
- - Attaques physiques
- DNSSECapp-front/src/providers/AuthProvider.tsx (2)
43-48:
Implement the login and logout functions
The login and logout functions are currently placeholders that return empty strings. These functions should be implemented to manage the authentication process, including redirecting users, handling tokens, and updating the authentication state.
Consider implementing the login function to initiate the authentication flow and the logout function to clear authentication data and redirect the user as necessary. Here's an example structure:
const login = () => {
// Redirect to the authentication provider
window.location.href = `${Config.get('auth.domain')}/authorize?response_type=code&client_id=${Config.get('auth.client_id')}&redirect_uri=${Config.get('auth.redirect_uri')}&scope=${Config.get('auth.scopes')}&state=${state}&nonce=${nonce}`;
};
const logout = () => {
// Clear authentication state
SetIsAuth(false);
SetToken(undefined);
SetUser({ name: '', role: '' });
// Redirect to a logout confirmation page or home
window.location.href = '/';
};30-42:
Improve error handling in getCode function
Currently, the getCode function logs the error and rethrows it, which may expose sensitive information and disrupt the application flow. Instead, handle the error gracefully without throwing it back to the caller.
Apply the following changes to modify the error handling:
} catch (e) {
console.error("Code validation failed", e);
- throw e;
+ // Handle the error without throwing
+ SetIsAuth(false);
+ // Optionally, display an error message to the user or redirect
}Likely invalid or redundant comment.
| - Confirmer le problème et déterminer les versions affectées. | ||
| - Vérifier le code pour trouver tout problème similaire potentiel. | ||
| - Informer les instances concernées qu'une faille est en cours de | ||
| résolution | ||
| - Préparer les correctifs et les déployer en production | ||
| - Communiquer aux différentes instances concernées que le | ||
| correctif est déployé |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance the disclosure process description
The vulnerability handling process should include timeframes and severity assessment.
- - Confirmer le problème et déterminer les versions affectées.
- - Vérifier le code pour trouver tout problème similaire potentiel.
- - Informer les instances concernées qu'une faille est en cours de
- résolution
- - Préparer les correctifs et les déployer en production
- - Communiquer aux différentes instances concernées que le
- correctif est déployé
+ - Confirmer le problème et évaluer sa gravité dans les 72 heures
+ - Déterminer les versions affectées et l'impact sur les utilisateurs
+ - Vérifier le code pour identifier tout problème similaire potentiel
+ - Informer les instances concernées du plan de résolution
+ - Préparer et tester les correctifs
+ - Déployer les correctifs en production selon la gravité :
+ * Critique : dans les 24 heures
+ * Haute : dans les 72 heures
+ * Moyenne : dans la semaine
+ - Communiquer la résolution aux parties concernées📝 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.
| - Confirmer le problème et déterminer les versions affectées. | |
| - Vérifier le code pour trouver tout problème similaire potentiel. | |
| - Informer les instances concernées qu'une faille est en cours de | |
| résolution | |
| - Préparer les correctifs et les déployer en production | |
| - Communiquer aux différentes instances concernées que le | |
| correctif est déployé | |
| - Confirmer le problème et évaluer sa gravité dans les 72 heures | |
| - Déterminer les versions affectées et l'impact sur les utilisateurs | |
| - Vérifier le code pour identifier tout problème similaire potentiel | |
| - Informer les instances concernées du plan de résolution | |
| - Préparer et tester les correctifs | |
| - Déployer les correctifs en production selon la gravité : | |
| * Critique : dans les 24 heures | |
| * Haute : dans les 72 heures | |
| * Moyenne : dans la semaine | |
| - Communiquer la résolution aux parties concernées |
| const [state, SetState] = useState<string>(); | ||
| const [nonce, SetNonce] = useState<string>(); | ||
| const [code, SetCode] = useState<string>(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Initialize state and nonce upon component mount
The state and nonce variables are essential for the authentication process, but they are currently uninitialized until set elsewhere. Initialize them when the component mounts to ensure they are available when needed.
Apply the following changes:
const [state, SetState] = useState<string>();
const [nonce, SetNonce] = useState<string>();
+ // Initialize state and nonce on mount
+ useEffect(() => {
+ SetState(generateString());
+ SetNonce(generateString());
+ }, []);Committable suggestion skipped: line range outside the PR's diff.
| @@ -0,0 +1,67 @@ | |||
| 'use client'; | |||
| import crypto from 'crypto'; | |||
There was a problem hiding this comment.
Replace Node.js crypto module with Web Crypto API
The crypto module imported from 'crypto' is a Node.js module and is not available in the browser environment. For client-side applications, use the Web Crypto API instead.
Apply the following changes to use the Web Crypto API:
- import crypto from 'crypto';
...
const generateString = () => {
- return crypto.randomBytes(16).toString('hex');
+ const array = new Uint8Array(16);
+ window.crypto.getRandomValues(array);
+ return Array.from(array, byte => byte.toString(16).padStart(2, '0')).join('');
};Committable suggestion skipped: line range outside the PR's diff.
| <PageTitle title={`En cours de connexion...`} /> | ||
| {state} | ||
| </div> |
There was a problem hiding this comment.
Avoid exposing sensitive information in the UI
Displaying the state value directly in the UI may expose sensitive information used in the authentication process. It's important to ensure that such details are not rendered on the client side.
Remove the direct display of state:
<PageTitle title={`En cours de connexion...`} />
- {state}📝 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.
| <PageTitle title={`En cours de connexion...`} /> | |
| {state} | |
| </div> | |
| <PageTitle title={`En cours de connexion...`} /> | |
| </div> |
| const { state, getCode } =useAuth(); | ||
| useEffect(() => { | ||
| getCode(params.get('state'), params.get('code'),params.get('iss')); | ||
| }, [params, getCode]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize the dependency array in useEffect
Including params in the dependency array can cause unnecessary re-renders because useSearchParams may return a new object on each render. Instead, depend on the specific parameters you are watching.
Apply the following change:
}, [
- params,
+ params.get('state'),
+ params.get('code'),
+ params.get('iss'),
getCode
]);📝 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.
| }, [params, getCode]); | |
| }, [ | |
| params.get('state'), | |
| params.get('code'), | |
| params.get('iss'), | |
| getCode | |
| ]); |
| getCode(params.get('state'), params.get('code'),params.get('iss')); | ||
| }, [params, getCode]); |
There was a problem hiding this comment.
Add checks for query parameters before calling getCode
Calling getCode without verifying that state, code, and iss are present may lead to errors if any of these parameters are missing from the URL.
Apply the following changes to ensure the parameters are valid:
useEffect(() => {
- getCode(params.get('state'), params.get('code'), params.get('iss'));
+ const stateValue = params.get('state');
+ const codeValue = params.get('code');
+ const issValue = params.get('iss');
+ if (stateValue && codeValue && issValue) {
+ getCode(stateValue, codeValue, issValue);
+ } else {
+ // Handle missing parameters
+ console.error('Missing authentication parameters');
+ // Optionally redirect or display an error message
+ }
}, [params, getCode]);📝 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.
| getCode(params.get('state'), params.get('code'),params.get('iss')); | |
| }, [params, getCode]); | |
| useEffect(() => { | |
| const stateValue = params.get('state'); | |
| const codeValue = params.get('code'); | |
| const issValue = params.get('iss'); | |
| if (stateValue && codeValue && issValue) { | |
| getCode(stateValue, codeValue, issValue); | |
| } else { | |
| // Handle missing parameters | |
| console.error('Missing authentication parameters'); | |
| // Optionally redirect or display an error message | |
| } | |
| }, [params, getCode]); |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
app-front/src/components/auth/AuthButton.tsx (1)
32-32: Usewindow.location.assigninstead ofrouter.pushfor external navigationSince the authorization URL is an external link, using
router.pushis not appropriate. The Next.js router is intended for client-side navigation within the application. For external URLs, usewindow.location.assignto navigate to the external authorization endpoint.Update the navigation method:
- router.push(addParamsToUrl(baseUrl, params)); + window.location.assign(addParamsToUrl(baseUrl, params));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app-front/src/components/auth/AuthButton.tsx(1 hunks)app-front/src/helpers/auth.ts(1 hunks)app-front/src/providers/AuthProvider.tsx(1 hunks)
🔇 Additional comments (3)
app-front/src/providers/AuthProvider.tsx (1)
33-34:
Improve error handling to prevent sensitive information leakage
Logging the error object e directly during authentication can potentially expose sensitive information. Instead, log a generic error message and handle the error appropriately without exposing internal details.
Apply this change:
try {
// ...
} catch (e) {
- console.error("Code validation failed", e);
+ console.error("Code validation failed");
// Optionally handle the error without throwing
}app-front/src/helpers/auth.ts (1)
1-5:
Replace Node.js crypto module with Web Crypto API
The crypto module imported from 'crypto' is a Node.js module and is not available in the browser environment. For client-side applications, use the Web Crypto API instead.
Replace the generateNonce function to use the Web Crypto API:
- import crypto from "crypto";
export const generateNonce = () => {
- return crypto.randomBytes(16).toString("hex");
+ const array = new Uint8Array(16);
+ window.crypto.getRandomValues(array);
+ return Array.from(array, byte => byte.toString(16).padStart(2, '0')).join('');
};app-front/src/components/auth/AuthButton.tsx (1)
24-31: 🛠️ Refactor suggestion
Add type safety and error handling for authentication parameters
The authentication parameters are retrieved from the configuration without checks for undefined values. If any of these configuration values are missing, it could lead to runtime errors. Consider adding type safety and error handling to ensure all required configuration values are present.
Update the code to validate configuration values:
const clientId = Config.get('auth.client_id');
+ if (!clientId) {
+ throw new Error('Missing auth.client_id configuration');
+ }
const redirectUri = Config.get('auth.redirect_uri');
+ if (!redirectUri) {
+ throw new Error('Missing auth.redirect_uri configuration');
+ }
const scopes = Config.get('auth.scopes');
+ if (!scopes) {
+ throw new Error('Missing auth.scopes configuration');
+ }
const params = {
response_type: 'code',
client_id: clientId,
redirect_uri: redirectUri,
scope: scopes,
state: stateToken,
nonce: nonceToken,
};| iss?: string, | ||
| user?: {name: string, role: string}, | ||
| getCode: (stateValue: string | null, codeValue: string | null, issValue: string | null) => void, |
There was a problem hiding this comment.
Define state variables for user and iss as per the AuthContextProps interface
The user and iss properties are defined in the AuthContextProps interface but are not associated with any state variables in the AuthProvider. This means these values cannot be provided or updated within the context, potentially leading to issues when components expect these values.
Add state variables for user and iss, and include them in the provider's value:
+ const [user, setUser] = useState<{ name: string, role: string }>();
+ const [iss, setIss] = useState<string>();
...
return (
<AuthContext.Provider value={{
isAuth,
state,
setState,
nonce,
setNonce,
code,
getCode,
+ user,
+ iss
}}>
{children}
</AuthContext.Provider>
);Also applies to: 20-24
| if(stateValue === state && issValue === `${Config.get('auth.domain')}/api/v2`) { | ||
| setCode(codeValue ?? undefined); | ||
| setIsAuth(true); | ||
| } |
There was a problem hiding this comment.
Ensure state and stateValue are defined before comparison
In the getCode function, comparing stateValue === state without checking if both values are defined may lead to unintended behavior when either value is undefined. Consider validating that both stateValue and state are not undefined before performing the comparison.
Modify the condition to include nullish checks:
- if (stateValue === state && issValue === `${Config.get('auth.domain')}/api/v2`) {
+ if (stateValue && state && stateValue === state && issValue === `${Config.get('auth.domain')}/api/v2`) {📝 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.
| if(stateValue === state && issValue === `${Config.get('auth.domain')}/api/v2`) { | |
| setCode(codeValue ?? undefined); | |
| setIsAuth(true); | |
| } | |
| if (stateValue && state && stateValue === state && issValue === `${Config.get('auth.domain')}/api/v2`) { | |
| setCode(codeValue ?? undefined); | |
| setIsAuth(true); | |
| } |
app-front/package.json
Outdated
| "@mui/material": "^6.1.6", | ||
| "@socialgouv/matomo-next": "^1.9.2", | ||
| "keycloak-js": "^26.0.5", | ||
| "next": "14.2.17", |
There was a problem hiding this comment.
On peut peut-être passer en version 15 ?
app-front/package.json
Outdated
| "@emotion/styled": "^11.13.0", | ||
| "@mui/material": "^6.1.6", | ||
| "@socialgouv/matomo-next": "^1.9.2", | ||
| "keycloak-js": "^26.0.5", |
There was a problem hiding this comment.
Keycloak-js à spprimer du coup ?
There was a problem hiding this comment.
oui c'est à supprimer
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app-front/tsconfig.json (2)
29-29: Consider using a more modern JavaScript targetThe current target of ES2017 might be unnecessarily conservative for a modern web application. Consider upgrading to at least ES2020 or ES2021 for better performance and newer language features, as most modern browsers support these versions.
- "target": "ES2017" + "target": "ES2021"
37-39: Enhance exclude patterns for better build performanceWhile excluding
node_modulesis standard, consider adding other common build and cache directories to optimize TypeScript compilation."exclude": [ - "node_modules" + "node_modules", + "**/.next", + "**/dist", + "**/build", + "**/.coverage" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app-front/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
app-front/next.config.js(1 hunks)app-front/package.json(1 hunks)app-front/tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app-front/next.config.js
- app-front/package.json
🔇 Additional comments (1)
app-front/tsconfig.json (1)
2-28: LGTM! Configuration follows Next.js best practices
The TypeScript configuration is well-structured and includes all necessary settings for a Next.js project:
- Strict type checking is enabled for better type safety
- Proper module resolution is configured for the Next.js bundler
- Path aliases are correctly set up for clean imports
- All necessary TypeScript file patterns are included
Also applies to: 30-36
Summary by CodeRabbit
Release Notes
New Features
Documentation
Chores
Style
Enhancements