Modified for portfolio by Vodki · Pull Request #22 · Nimpoo/ft_transcendence

Vodki · 2025-12-12T21:13:00Z

No description provided.

Copilot

Pull request overview

This PR transforms the ft_transcendence project from a 42 School OAuth-based authentication system into a portfolio-ready application with simplified username-only authentication. The changes remove dependencies on 42's API infrastructure and make the application self-contained and easier to deploy for demonstration purposes.

Key Changes

Replaced OAuth authentication with username-only login: Users can now log in with just a username, creating ephemeral accounts automatically, removing dependency on 42's OAuth API
Centralized API configuration: Created isomorphic URL configuration (frontend/src/lib/api.ts) to handle client/server rendering and deployment flexibility
Simplified deployment setup: Updated generate-env.sh to generate security tokens and deployment configuration instead of requiring 42 API credentials

Reviewed changes

Copilot reviewed 24 out of 24 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
`generate-env.sh`	Replaced 42 OAuth credential prompts with automatic secret generation and deployment configuration for ALLOWED_HOSTS and CORS
`backend/users/views.py`	Replaced 42 OAuth token validation with username-only login; creates ephemeral users automatically
`backend/users/models.py`	Removed `fortytwo_id` field; added `is_ephemeral` boolean to track temporary vs permanent users
`backend/backend/settings.py`	Made SECRET_KEY configurable via environment; added ALLOWED_HOSTS and CORS_ALLOWED_ORIGINS from env vars; integrated WhiteNoise middleware
`backend/backend/urls.py`	Wrapped all API endpoints under `/api/` prefix for cleaner reverse proxy routing
`backend/backend/routing.py`	Updated WebSocket routes to include `/api/` prefix
`backend/requirements.txt`	Removed `random-username` and `requests` dependencies; added `whitenoise` for static file serving
`backend/Dockerfile`	Updated to use specific Python version (3.12) instead of `latest` tag
`frontend/src/lib/api.ts`	New file providing isomorphic API URL configuration for client/server rendering with Kubernetes DNS support
`frontend/src/app/page.tsx`	Replaced 42 OAuth button with username input form and client-side login logic
`frontend/src/app/callback/page.tsx`	Removed OAuth callback handler (no longer needed)
`frontend/src/app/callback/api.ts`	Removed 42 OAuth token exchange logic (no longer needed)
`frontend/src/providers/Session.tsx`	Updated to use centralized API_URL configuration
`frontend/src/providers/Socket.tsx`	Updated to use centralized WS_URL configuration
`frontend/src/providers/Game.tsx`	Updated to use centralized WS_URL configuration
`frontend/src/components/*`	Updated all components to use `getAssetUrl()` helper for avatar/media URLs
`frontend/Dockerfile`	Removed CLIENT_SECRET build argument (OAuth no longer used)
`docker-compose.yml`	Removed OAuth-related build args; added new environment variables for Django and deployment config

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-12-12T21:21:04Z

backend/users/views.py

+            user = User.objects.get(login=login)
+            # If user exists and is ephemeral, generate a unique username
+            if user.is_ephemeral:
+                suffix = secrets.token_hex(3)
+                new_login = f"{login[:16]}_{suffix}"
+                user = User.objects.create(
+                    login=new_login,
+                    display_name=new_login,
+                    is_ephemeral=True
                )


The logic for handling ephemeral users contains a critical race condition. When multiple requests with the same username arrive simultaneously, they can all pass the User.objects.get(login=login) check and see user.is_ephemeral=True, then all attempt to create new users with suffixed names. This can lead to unpredictable behavior where the same user gets multiple accounts created.

Consider using select_for_update() to lock the user record during this check, or using atomic transactions with unique constraints to handle concurrent access safely.

Copilot · 2025-12-12T21:21:05Z

frontend/src/app/page.tsx

+									type="button"
+									className="btn btn-primary btn-lg"
+									onClick={handleLogin}
+									disabled={isLoading || username.length < 2}


The username validation logic is inconsistent between frontend and backend. The frontend validates username.length < 2 on the trimmed string (line 37), but then checks username.length < 2 again on the original string before trimming (line 194). This could allow usernames with leading/trailing spaces to bypass the length check in the button's disabled state.

Ensure consistent validation by checking the trimmed length in both places.

Suggested change

disabled={isLoading || username.length < 2}

disabled={isLoading || username.trim().length < 2}

Copilot · 2025-12-12T21:21:05Z

frontend/src/lib/api.ts

+export const API_URL = typeof window !== 'undefined'
+  ? '' // Client: relative paths work with Ingress
+  : 'http://pong-backend:8000'; // Server: K8s internal DNS


The hardcoded Kubernetes internal DNS name 'http://pong-backend:8000' creates tight coupling to a specific deployment infrastructure. This makes the application less portable and harder to deploy in different environments (development, staging, different orchestration platforms).

Consider making this configurable via an environment variable that can be set at build time or runtime for better flexibility across different deployment scenarios.

Copilot · 2025-12-12T21:21:05Z

frontend/src/lib/api.ts

+
+export const WS_URL = typeof window !== 'undefined'
+  ? `wss://${window.location.host}` // Client: same domain as page
+  : 'wss://pong-backend:8000'; // Server: rarely used for WS


The WebSocket URL configuration has a potential issue: WebSockets established server-side (during SSR) will fail because WebSocket connections cannot be established from Next.js server components. The fallback 'wss://pong-backend:8000' will never work in practice for SSR contexts.

Since WebSockets should only be used client-side, the server-side fallback is misleading. Consider adding a runtime check that throws an error if WebSocket initialization is attempted server-side, or simply remove the server-side branch.

Suggested change

: 'wss://pong-backend:8000'; // Server: rarely used for WS

: (() => { throw new Error('WebSocket connections cannot be established server-side.'); })();

Copilot · 2025-12-12T21:21:06Z

backend/backend/settings.py

 DEBUG = False

-ALLOWED_HOSTS = ["*"]
+ALLOWED_HOSTS = os.environ.get("ALLOWED_HOSTS", "localhost").split(",")


Setting DEBUG = False while having wildcard ALLOWED_HOSTS (through environment variable) is a good security practice, but the default value "localhost" in ALLOWED_HOSTS might not be appropriate for production deployments where the actual hostname should be explicitly configured.

Consider requiring ALLOWED_HOSTS to be explicitly set without a default, or at minimum document that the default is only suitable for local development.

Suggested change

ALLOWED_HOSTS = os.environ.get("ALLOWED_HOSTS", "localhost").split(",")

ALLOWED_HOSTS_ENV = os.environ.get("ALLOWED_HOSTS")

if ALLOWED_HOSTS_ENV is None:

raise RuntimeError("ALLOWED_HOSTS environment variable is required (comma-separated list of allowed hostnames)")

ALLOWED_HOSTS = ALLOWED_HOSTS_ENV.split(",")

Copilot · 2025-12-12T21:21:07Z

backend/backend/settings.py

 DEBUG = False

-ALLOWED_HOSTS = ["*"]
+ALLOWED_HOSTS = os.environ.get("ALLOWED_HOSTS", "localhost").split(",")


Splitting environment variables by comma without trimming whitespace can lead to issues. If users enter values like "localhost, example.com" (with spaces after commas), the resulting values will include leading spaces which could cause CORS and host validation failures.

Consider stripping whitespace from each value: os.environ.get("ALLOWED_HOSTS", "localhost").split(",") should be [h.strip() for h in os.environ.get("ALLOWED_HOSTS", "localhost").split(",")].

Copilot · 2025-12-12T21:21:08Z

backend/users/views.py

+            # If user exists and is NOT ephemeral (has 2FA), check 2FA
+            elif user.dfa_secret:
+                dfa = body_payload.get("dfa")
+                if dfa is None:
+                    return JsonResponse({"wait!": "yo I need dfa bro"})
+                if not pyotp.TOTP(user.dfa_secret).verify(dfa):
+                    return JsonResponse(
+                        {"error": "dfa", "message": "Digits not correct."}, status=401
+                    )


This authentication mechanism creates a significant security vulnerability. Any user can log in as any existing non-ephemeral user simply by knowing their username - there is no password or other authentication factor required. The code only checks for 2FA (dfa_secret) if it's enabled, but doesn't require any initial credential verification.

This means an attacker can enumerate usernames and gain access to any account that doesn't have 2FA enabled. Consider implementing proper password-based authentication or another secure authentication method.

Copilot · 2025-12-12T21:21:08Z

frontend/src/lib/api.ts

+  ? `wss://${window.location.host}` // Client: same domain as page
+  : 'wss://pong-backend:8000'; // Server: rarely used for WS


The new URL structure wraps all API endpoints under an "api/" prefix, but the WebSocket paths in routing.py also include this prefix. However, in the frontend lib/api.ts, the WS_URL doesn't include "/api" in the path - it's added when constructing the full WebSocket URL (e.g., ${WS_URL}/api/users/ in Socket.tsx line 28).

This creates a potential inconsistency: if someone uses WS_URL directly without appending "/api", they'll connect to the wrong endpoint. Consider either including "/api" in the WS_URL constant or documenting this requirement clearly.

Suggested change

? `wss://${window.location.host}` // Client: same domain as page

: 'wss://pong-backend:8000'; // Server: rarely used for WS

? `wss://${window.location.host}/api` // Client: same domain as page, with /api prefix

: 'wss://pong-backend:8000/api'; // Server: rarely used for WS, with /api prefix

Copilot · 2025-12-12T21:21:08Z

frontend/src/lib/api.ts

+export const API_URL = typeof window !== 'undefined'
+  ? '' // Client: relative paths work with Ingress
+  : 'http://pong-backend:8000'; // Server: K8s internal DNS


The comment "Client: Uses relative URLs (browser completes automatically)" is misleading. The code sets API_URL to an empty string (''), which means requests will use relative URLs. However, the comment should clarify that this only works because of the Ingress/reverse proxy configuration that routes /api/* to the backend service.

Consider updating the comment to: "Client: Uses relative paths (empty string) - relies on Ingress/reverse proxy to route /api/* to backend"

Copilot · 2025-12-12T21:21:08Z

frontend/src/app/page.tsx

 	const { session, status } = useSession()
+	const [username, setUsername] = useState("")
+	const [isLoading, setIsLoading] = useState(false)
+	const [cookies, setCookie] = useCookies(["session"])


Unused variable cookies.

Suggested change

const [cookies, setCookie] = useCookies(["session"])

const [, setCookie] = useCookies(["session"])

Nimpoo

Va te faire shampooiner Copilot

Modified for portfolio

36753e8

Vodki requested a review from Copilot December 12, 2025 21:13

Copilot started reviewing on behalf of Vodki December 12, 2025 21:13 View session

Copilot AI reviewed Dec 12, 2025

View reviewed changes

Nimpoo reviewed Dec 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modified for portfolio#22

Modified for portfolio#22
Vodki wants to merge 1 commit intoNimpoo:mainfrom
Vodki:portfolio

Vodki commented Dec 12, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Copilot AI Dec 12, 2025

Uh oh!

Nimpoo left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	disabled={isLoading \|\| username.length < 2}
	disabled={isLoading \|\| username.trim().length < 2}

	: 'wss://pong-backend:8000'; // Server: rarely used for WS
	: (() => { throw new Error('WebSocket connections cannot be established server-side.'); })();

-ALLOWED_HOSTS = os.environ.get("ALLOWED_HOSTS", "localhost").split(",")
+ALLOWED_HOSTS_ENV = os.environ.get("ALLOWED_HOSTS")
+if ALLOWED_HOSTS_ENV is None:
+    raise RuntimeError("ALLOWED_HOSTS environment variable is required (comma-separated list of allowed hostnames)")
+ALLOWED_HOSTS = ALLOWED_HOSTS_ENV.split(",")

		? `wss://${window.location.host}` // Client: same domain as page
		: 'wss://pong-backend:8000'; // Server: rarely used for WS

	const [cookies, setCookie] = useCookies(["session"])
	const [, setCookie] = useCookies(["session"])

Conversation

Vodki commented Dec 12, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Key Changes

Reviewed changes

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 12, 2025

Choose a reason for hiding this comment

Uh oh!

Nimpoo left a comment

Choose a reason for hiding this comment

Va te faire shampooiner Copilot

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants