feat: pro-connect (step 1)

[rap2hpoutre]

No description provided.

[revu-bot]

This PR implements ProConnect (French government authentication service) integration, replacing a simple password-based auth system with OAuth2/OIDC. The implementation is generally solid but has several critical security and production-readiness issues that must be addressed:

Critical Issues:

1. Debug mode enabled in production (exposes sensitive auth data in logs)
2. Missing environment variable validation (app will crash if credentials not set)
3. Excessive console logging throughout authentication flow
4. No error handling for authentication failures in UI

Architecture Concerns:

• The custom ProConnect provider implementation is well-structured
• Session management properly integrates with Sentry
• Type definitions are comprehensive

Security Notes:

• OAuth2 PKCE flow correctly implemented
• Proper token handling in callbacks
• Session data appropriately scoped

[revu-bot]

CRITICAL: Missing environment variable validation. If PROCONNECT_CLIENT_ID or PROCONNECT_CLIENT_SECRET are not set, the app will crash with a cryptic error. This should fail fast with a clear error message during startup.

The non-null assertion operator (!) bypasses TypeScript's safety checks without runtime validation.

Suggested change

	export const authOptions: NextAuthOptions = {
	providers: [
	ProConnectProvider({
	clientId: process.env.PROCONNECT_CLIENT_ID!,
	clientSecret: process.env.PROCONNECT_CLIENT_SECRET!,
	}),
	],
	// Validate required environment variables
	if (!process.env.PROCONNECT_CLIENT_ID) {
	throw new Error('PROCONNECT_CLIENT_ID environment variable is required');
	}
	if (!process.env.PROCONNECT_CLIENT_SECRET) {
	throw new Error('PROCONNECT_CLIENT_SECRET environment variable is required');
	}
	if (!process.env.NEXTAUTH_SECRET) {
	throw new Error('NEXTAUTH_SECRET environment variable is required');
	}
	export const authOptions: NextAuthOptions = {
	providers: [
	ProConnectProvider({
	clientId: process.env.PROCONNECT_CLIENT_ID,
	clientSecret: process.env.PROCONNECT_CLIENT_SECRET,
	}),
	],

[revu-bot]

ProConnect Authentication Implementation Review

This PR implements ProConnect (AgentConnect) OAuth authentication to replace the simple access code system. The implementation is generally solid with proper JWT verification, session management, and authorization checks. However, there are several critical security and architectural issues that must be addressed:

Critical Issues

1. Missing environment variable validation - Server will crash if required ProConnect credentials are not set
2. Insecure session secret handling - Default "todo" value in .env.example is dangerous
3. Authorization bypass vulnerability - Race condition in AuthorizationCheck component
4. Incomplete logout flow - Missing server-side session cleanup

Important Issues

5. Hardcoded domain list - Authorization domains duplicated across multiple files
6. Missing error boundaries - Client components lack error handling
7. Type safety gaps - Session type extensions not fully enforced

Summary

The OAuth flow implementation is well-structured with proper PKCE, JWT verification, and multi-algorithm support. The authorization layer correctly separates authentication from authorization. However, the critical security issues around environment validation, session management, and race conditions must be fixed before merging.

Recommendation: Request changes to address critical security issues before approval.

[revu-bot]

ProConnect Authentication Implementation Review

This PR implements ProConnect (AgentConnect) OAuth authentication to replace the simple access code system. The implementation is generally solid with proper JWT verification, session management, and authorization checks. However, there are several critical security and reliability issues that must be addressed:

Critical Issues

1. Missing environment variable validation - Application will crash at runtime if ProConnect credentials are not configured
2. Unsafe client-side environment variable exposure - NEXT_PUBLIC_PROCONNECT_ENV creates security risks
3. Incomplete error handling - Several authentication flows lack proper error boundaries

Important Improvements Needed

1. Authorization logic needs hardening - Race conditions in redirect flow
2. Type safety gaps - Missing runtime validation for session data
3. Logout flow has edge cases - Fallback behavior needs improvement

Summary

• Files Changed: 19 files (13 added, 3 modified, 3 deleted)
• High-Impact Issues: 3 critical, 4 important
• Overall Quality: Good foundation, needs security hardening before production deployment

[revu-bot]

[CRITICAL] Missing environment variable validation

Why it matters:

• Application will crash at runtime if PROCONNECT_CLIENT_ID or PROCONNECT_CLIENT_SECRET are undefined
• Error will occur during NextAuth initialization, making the entire app unusable
• No clear error message for developers during setup

Proposed change:
Add validation at the top of the file with clear error messages:

if (!process.env.PROCONNECT_CLIENT_ID || !process.env.PROCONNECT_CLIENT_SECRET) {
  throw new Error(
    'Missing required ProConnect environment variables. Please set PROCONNECT_CLIENT_ID and PROCONNECT_CLIENT_SECRET in your .env file.'
  )
}

Suggested change

	export const authOptions: NextAuthOptions = {
	providers: [
	ProConnectProvider({
	clientId: process.env.PROCONNECT_CLIENT_ID!,
	clientSecret: process.env.PROCONNECT_CLIENT_SECRET!,
	}),
	],
	// Validate required environment variables
	if (!process.env.PROCONNECT_CLIENT_ID || !process.env.PROCONNECT_CLIENT_SECRET) {
	throw new Error(
	'Missing required ProConnect environment variables. Please set PROCONNECT_CLIENT_ID and PROCONNECT_CLIENT_SECRET in your .env file.'
	)
	}
	export const authOptions: NextAuthOptions = {
	providers: [
	ProConnectProvider({
	clientId: process.env.PROCONNECT_CLIENT_ID,
	clientSecret: process.env.PROCONNECT_CLIENT_SECRET,
	}),
	],

[revu-bot]

[CRITICAL] Avoid exposing environment selection to client-side

Why it matters:

• NEXT_PUBLIC_* variables are embedded in client-side JavaScript bundles
• Exposing environment configuration creates potential security risks
• Attackers could identify integration vs production endpoints
• This value should only be used server-side

Proposed change:
Remove NEXT_PUBLIC_PROCONNECT_ENV and use only the server-side PROCONNECT_ENV. Update proconnect-logout.ts to get the environment from an API endpoint or server-side prop instead of reading it directly from process.env.NEXT_PUBLIC_PROCONNECT_ENV.

[revu-bot]

[IMPORTANT] Add error handling for userinfo request failures

Why it matters:

• Network failures or ProConnect API issues will cause unhandled promise rejections
• Users will see generic error pages instead of helpful authentication error messages
• No retry logic or graceful degradation

Proposed change:
Wrap the userinfo request in try-catch and provide better error context:

async request({ tokens }) {
  try {
    const response = await fetch(`${PROCONNECT_DOMAIN}/userinfo`, {
      headers: {
        Authorization: `Bearer ${tokens.access_token}`,
      },
    });

    if (!response.ok) {
      throw new Error(`Userinfo request failed: ${response.status} ${response.statusText}`);
    }
    // ... rest of the code
  } catch (error) {
    console.error('ProConnect userinfo request failed:', error);
    throw new Error('Failed to fetch user information from ProConnect. Please try again.');
  }
}

[revu-bot]

[IMPORTANT] JWT verification error handling is incomplete

Why it matters:

• If JWT verification fails, the error message is unclear
• Multiple JWKS URLs are tried but errors are silently swallowed
• Debugging authentication issues will be difficult

Proposed change:
Collect and report all verification errors:

const errors: Error[] = [];
for (const jwksUrl of jwksUrls) {
  try {
    const JWKS = jose.createRemoteJWKSet(new URL(jwksUrl));
    const { payload } = await jose.jwtVerify(jwt, JWKS, {
      algorithms: ["RS256", "ES256"],
    });
    return payload as unknown as P;
  } catch (error) {
    errors.push(error as Error);
    continue;
  }
}

throw new Error(
  `Could not verify ${header.alg} JWT with available JWKS endpoints. Errors: ${errors.map(e => e.message).join(', ')}`
);

Suggested change

	} else if (header.alg === "RS256" || header.alg === "ES256") {
	// For RS256/ES256, verify with JWKS
	const jwksUrls = [
	`${PROCONNECT_DOMAIN}/jwks`,
	`${PROCONNECT_DOMAIN}/.well-known/jwks.json`,
	];
	for (const jwksUrl of jwksUrls) {
	try {
	const JWKS = jose.createRemoteJWKSet(new URL(jwksUrl));
	const { payload } = await jose.jwtVerify(jwt, JWKS, {
	algorithms: ["RS256", "ES256"],
	});
	return payload as unknown as P;
	} catch {
	// Try next URL
	continue;
	}
	}
	throw new Error(
	`Could not verify ${header.alg} JWT with available JWKS endpoints`
	);
	} else if (header.alg === "RS256" || header.alg === "ES256") {
	// For RS256/ES256, verify with JWKS
	const jwksUrls = [
	`${PROCONNECT_DOMAIN}/jwks`,
	`${PROCONNECT_DOMAIN}/.well-known/jwks.json`,
	];
	const errors: Error[] = [];
	for (const jwksUrl of jwksUrls) {
	try {
	const JWKS = jose.createRemoteJWKSet(new URL(jwksUrl));
	const { payload } = await jose.jwtVerify(jwt, JWKS, {
	algorithms: ["RS256", "ES256"],
	});
	return payload as unknown as P;
	} catch (error) {
	errors.push(error as Error);
	continue;
	}
	}
	throw new Error(
	`Could not verify ${header.alg} JWT with available JWKS endpoints. Errors: ${errors.map(e => e.message).join(', ')}`
	);

[revu-bot]

[IMPORTANT] Race condition in authorization redirect logic

Why it matters:

• Setting shouldRedirect and isChecking to true, then calling router.push() creates a race condition
• Component may re-render before navigation completes, causing flicker or duplicate redirects
• The return statement after router.push() doesn't prevent the rest of the effect from running in subsequent renders

Proposed change:
Simplify the logic and ensure single redirect:

if (status === "authenticated" && session?.unauthorized) {
  router.push("/access-denied");
  return; // Exit early, keep loading state
}

// Only clear loading state if we're not redirecting
setIsChecking(false);
setShouldRedirect(false);

[revu-bot]

[IMPORTANT] Logout error handling needs improvement

Why it matters:

• If ProConnect logout URL construction fails, user still gets signed out locally but remains logged in at ProConnect
• This creates a confusing state where re-authentication happens immediately
• The fallback signOut({ callbackUrl: "/" }) doesn't clear ProConnect session

Proposed change:
Add better error handling and user feedback:

try {
  const idToken = session?.idToken;
  
  if (!idToken) {
    console.warn('No ID token available for ProConnect logout');
    // Still attempt local signout
    await signOut({ callbackUrl: "/" });
    return;
  }

  const postLogoutRedirectUri = window.location.origin;
  const proconnectLogoutUrl = buildProConnectLogoutUrl(
    idToken,
    postLogoutRedirectUri
  );

  await signOut({ redirect: false });
  window.location.href = proconnectLogoutUrl;
} catch (error) {
  console.error("Logout error:", error);
  // Show user-friendly error message
  alert('Une erreur est survenue lors de la déconnexion. Vous allez être déconnecté localement.');
  await signOut({ callbackUrl: "/" });
}

[revu-bot]

[MINOR] Email domain comparison should be case-insensitive throughout

Why it matters:

• Email addresses are case-insensitive per RFC 5321
• Current code converts emailDomain to lowercase but compares against mixed-case ALLOWED_EMAIL_DOMAINS
• Could cause false negatives if domains in the array have uppercase characters

Note: The comparison at line 26 does use .toLowerCase() on both sides, so this is already handled correctly. However, for clarity and to prevent future bugs, consider normalizing the ALLOWED_EMAIL_DOMAINS array itself to lowercase at definition time.

[revu-bot]

[MINOR] Consider validating environment variable

Why it matters:

• If NEXT_PUBLIC_PROCONNECT_ENV is undefined or has an unexpected value, the function silently defaults to integration
• This could cause production apps to use integration endpoints

Proposed change:
Add validation:

export function getProConnectLogoutUrl(): string {
  const env = process.env.NEXT_PUBLIC_PROCONNECT_ENV;
  
  if (env && env !== 'production' && env !== 'integration') {
    console.warn(`Invalid NEXT_PUBLIC_PROCONNECT_ENV: ${env}, defaulting to integration`);
  }
  
  const PROCONNECT_DOMAIN =
    env === "production"
      ? "https://auth.agentconnect.gouv.fr/api/v2"
      : "https://fca.integ01.dev-agentconnect.fr/api/v2";

  return `${PROCONNECT_DOMAIN}/session/end`;
}

[tokenbureau]

🎉 Deployment for commit 96cdfee :

Ingresses

• 🚀 https://api-srdt-proconnect.ovh.fabrique.social.gouv.fr/
• 🚀 https://web-srdt-proconnect.ovh.fabrique.social.gouv.fr/

Docker images

• 📦 docker pull harbor.fabrique.social.gouv.fr/srdt/srdt/api:sha-96cdfeebaef01e2b5f658b438848acc4c3e261fb
• 📦 docker pull harbor.fabrique.social.gouv.fr/srdt/srdt/web:sha-96cdfeebaef01e2b5f658b438848acc4c3e261fb

Debug

• 📕 Loki logs for namespace srdt-proconnect
• 📈 Pods monitoring for namespace srdt-proconnect
• 📈 Workloads monitoring for namespace srdt-proconnect