From fcf346333818dd51418237148a9d75b5d9a64944 Mon Sep 17 00:00:00 2001
From: "tokamak-pm[bot]" <264983013+tokamak-pm[bot]@users.noreply.github.com>
Date: Tue, 7 Apr 2026 08:38:16 +0000
Subject: [PATCH 1/4] fix(llm-api): add warnings for insecure default connector
secrets
Authored-By: tokamak-pm[bot] <264983013+tokamak-pm[bot]@users.noreply.github.com>
---
.../llm-api/internal/infrastructure/connector/provider.go | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/services/llm-api/internal/infrastructure/connector/provider.go b/services/llm-api/internal/infrastructure/connector/provider.go
index 0bbc4ba1..fe5a6f04 100644
--- a/services/llm-api/internal/infrastructure/connector/provider.go
+++ b/services/llm-api/internal/infrastructure/connector/provider.go
@@ -6,6 +6,7 @@ import (
"github.com/google/wire"
"github.com/rs/zerolog"
+ "github.com/rs/zerolog/log"
"gorm.io/gorm"
"jan-server/services/llm-api/internal/config"
@@ -23,7 +24,7 @@ var ProviderSet = wire.NewSet(
// ProvideTokenEncryptor creates a token encryptor from config.
func ProvideTokenEncryptor(cfg *config.Config) (*TokenEncryptor, error) {
if cfg.ConnectorTokenEncryptionKey == "" {
- // Generate a default key for development (not for production!)
+ log.Warn().Msg("ConnectorTokenEncryptionKey is not set, using insecure default key. Do NOT use this in production.")
cfg.ConnectorTokenEncryptionKey = "0000000000000000000000000000000000000000000000000000000000000000"
}
@@ -64,7 +65,7 @@ func ProvideConnectorService(
}
}
if len(stateHMAC) == 0 {
- // Use a default for development
+ logger.Warn().Msg("OAuthStateSecret is not set, using insecure default secret. Do NOT use this in production.")
stateHMAC = []byte("development-oauth-state-secret-key-32b")
}
From a9e577783466346fe41e7320b2e6f05487d2a10b Mon Sep 17 00:00:00 2001
From: "tokamak-pm[bot]" <264983013+tokamak-pm[bot]@users.noreply.github.com>
Date: Tue, 7 Apr 2026 08:38:34 +0000
Subject: [PATCH 2/4] fix(web): prevent open redirect bypass in
isAllowedExternalRedirect
Replace regex-based URL validation with proper URL parsing to prevent
userinfo-based open redirect bypasses (e.g., http://localhost:8080@evil.com).
Extract shared utility to apps/web/src/lib/redirect-utils.ts and add
missing accessToken null check in __root.tsx handleCloseModal.
Authored-By: tokamak-pm[bot] <264983013+tokamak-pm[bot]@users.noreply.github.com>
---
apps/web/src/components/form/login.tsx | 6 +-----
apps/web/src/lib/redirect-utils.ts | 12 ++++++++++++
apps/web/src/routes/__root.tsx | 7 ++-----
apps/web/src/routes/auth/callback.tsx | 4 +---
apps/web/src/routes/login.tsx | 6 +-----
5 files changed, 17 insertions(+), 18 deletions(-)
create mode 100644 apps/web/src/lib/redirect-utils.ts
diff --git a/apps/web/src/components/form/login.tsx b/apps/web/src/components/form/login.tsx
index e24a5482..3739217a 100644
--- a/apps/web/src/components/form/login.tsx
+++ b/apps/web/src/components/form/login.tsx
@@ -8,6 +8,7 @@ import {
import { Input } from "@janhq/interfaces/input";
import { Google } from "@janhq/interfaces/svgs/google";
import { buildGoogleAuthUrl } from "@/lib/oauth";
+import { isAllowedExternalRedirect } from "@/lib/redirect-utils";
import { useState } from "react";
import { useAuth } from "@/stores/auth-store";
import { useRouter } from "@tanstack/react-router";
@@ -30,11 +31,6 @@ export function LoginForm({
const { loginWithOAuth } = useAuth();
const router = useRouter();
- const isAllowedExternalRedirect = (value: string) => {
- // Allow localhost with any port for development
- return /^http:\/\/localhost:\d+/.test(value);
- };
-
const getRedirectUrl = () => {
const url = new URL(window.location.href);
const redirectParam = url.searchParams.get(URL_PARAM.REDIRECT);
diff --git a/apps/web/src/lib/redirect-utils.ts b/apps/web/src/lib/redirect-utils.ts
new file mode 100644
index 00000000..7c668d7a
--- /dev/null
+++ b/apps/web/src/lib/redirect-utils.ts
@@ -0,0 +1,12 @@
+/**
+ * Validates whether a URL is an allowed external redirect target.
+ * Uses proper URL parsing to prevent userinfo-based open redirect bypasses.
+ */
+export const isAllowedExternalRedirect = (value: string): boolean => {
+ try {
+ const url = new URL(value);
+ return url.protocol === 'http:' && url.hostname === 'localhost';
+ } catch {
+ return false;
+ }
+};
diff --git a/apps/web/src/routes/__root.tsx b/apps/web/src/routes/__root.tsx
index 6b8ac2e5..8e25130a 100644
--- a/apps/web/src/routes/__root.tsx
+++ b/apps/web/src/routes/__root.tsx
@@ -12,6 +12,7 @@ import { SettingsDialog } from "@/components/settings/settings-dialog";
import { CreateProject } from "@/components/projects/create-project";
import { SearchDialog } from "@/components/search/search-dialog";
import { useAuth } from "@/stores/auth-store";
+import { isAllowedExternalRedirect } from "@/lib/redirect-utils";
import { useRightSidebarStore } from "@/stores/right-sidebar-store";
import { ThemeProvider } from "@/components/themes/theme-provider";
import { Toaster } from "@janhq/interfaces/sonner";
@@ -136,14 +137,10 @@ function RootLayout() {
const searchSection = searchParams.get(URL_PARAM.SEARCH);
const isSearchOpen = !!searchSection;
- const isAllowedExternalRedirect = (value: string) => {
- // Allow localhost with any port for development
- return /^http:\/\/localhost:\d+/.test(value);
- };
-
const handleCloseModal = (redirectUrl?: string) => {
// Case 1: External redirect (e.g., http://localhost:29999 from install script)
if (redirectUrl && isAllowedExternalRedirect(redirectUrl)) {
+ if (!accessToken) return;
const bearerToken = `Bearer ${accessToken}`;
const encodedToken = btoa(bearerToken);
const target = new URL(redirectUrl);
diff --git a/apps/web/src/routes/auth/callback.tsx b/apps/web/src/routes/auth/callback.tsx
index 06efa9ec..0c8687ff 100644
--- a/apps/web/src/routes/auth/callback.tsx
+++ b/apps/web/src/routes/auth/callback.tsx
@@ -2,6 +2,7 @@ import { createFileRoute, useNavigate } from "@tanstack/react-router";
import { useEffect, useRef, useState } from "react";
import { useAuth } from "@/stores/auth-store";
import { retrieveOAuthState, exchangeCodeForTokens } from "@/lib/oauth";
+import { isAllowedExternalRedirect } from "@/lib/redirect-utils";
export const Route = createFileRoute("/auth/callback")({
component: OAuthCallbackPage,
@@ -55,9 +56,6 @@ function OAuthCallbackPage() {
loginWithOAuth(tokens);
console.log("OAuth login successful");
// Navigate to the original URL or home
- const isAllowedExternalRedirect = (value: string) =>
- /^http:\/\/localhost:\d+/.test(value);
-
if (oauthData.redirectUrl && isAllowedExternalRedirect(oauthData.redirectUrl)) {
const bearerToken = `Bearer ${tokens.access_token}`;
const encodedToken = btoa(bearerToken);
diff --git a/apps/web/src/routes/login.tsx b/apps/web/src/routes/login.tsx
index cf0ad203..9c1782c6 100644
--- a/apps/web/src/routes/login.tsx
+++ b/apps/web/src/routes/login.tsx
@@ -1,6 +1,7 @@
import { createFileRoute, useNavigate } from "@tanstack/react-router";
import { useEffect } from "react";
import { useAuth } from "@/stores/auth-store";
+import { isAllowedExternalRedirect } from "@/lib/redirect-utils";
import { URL_PARAM } from "@/constants";
export const Route = createFileRoute("/login" as "/")({
@@ -13,11 +14,6 @@ function LoginRoute() {
const isGuest = useAuth((state) => state.isGuest);
const accessToken = useAuth((state) => state.accessToken);
- const isAllowedExternalRedirect = (value: string) => {
- // Allow localhost with any port for development
- return /^http:\/\/localhost:\d+/.test(value);
- };
-
useEffect(() => {
if (!isAuthenticated || isGuest) {
return;
From 8aad6b9d96b84cf2595ebac52aa1815e2641362c Mon Sep 17 00:00:00 2001
From: "tokamak-pm[bot]" <264983013+tokamak-pm[bot]@users.noreply.github.com>
Date: Tue, 7 Apr 2026 08:42:45 +0000
Subject: [PATCH 3/4] docs: add recent commits review report for PRs #484-#486
Authored-By: tokamak-pm[bot] <264983013+tokamak-pm[bot]@users.noreply.github.com>
---
docs/reviews/recent-commits-review.md | 263 ++++++++++++++++++++++++++
1 file changed, 263 insertions(+)
create mode 100644 docs/reviews/recent-commits-review.md
diff --git a/docs/reviews/recent-commits-review.md b/docs/reviews/recent-commits-review.md
new file mode 100644
index 00000000..addea3a4
--- /dev/null
+++ b/docs/reviews/recent-commits-review.md
@@ -0,0 +1,263 @@
+# Recent Commits Review — 2026-04-07
+
+> Automated review of commits merged to main (PRs #484–#486).
+> Phase 1 fixes have already been applied for critical security issues.
+> This report documents remaining findings that require team discussion or larger refactors.
+
+---
+
+## Finding 1: API Key Access to Inactive Models Lacks Per-Model Authorization
+
+| Field | Value |
+|---|---|
+| **Severity** | CRITICAL (design concern) |
+| **Source PR** | #485 |
+| **Priority** | P1 — requires team decision before fix |
+
+### Affected Files
+- `services/llm-api/internal/interfaces/httpserver/handlers/chathandler/chat_handler.go` (lines 195–202)
+- `services/llm-api/internal/interfaces/httpserver/handlers/messageshandler/messages_handler.go` (lines 78–86, 462–469)
+- `services/llm-api/internal/interfaces/httpserver/handlers/modelhandler/provider_handler.go` (lines 196–220)
+
+### Description
+
+When a request arrives with API key authentication (`X-Auth-Method: apikey`), the chat and messages handlers bypass the active-model filter and call `SelectProviderModelForModelPublicIDIncludingInactive`, which uses `FindByModelKey` (no active filter) instead of `FindActiveByModelKey`. This allows any valid API key to access any model — including inactive/disabled models — regardless of which project, organization, or scope the API key belongs to.
+
+The regular (non-API-key) code path correctly restricts access to active models only via `SelectProviderModelForModelPublicID`, which calls `FindActiveByModelKey`. However, the "including inactive" variant performs no additional authorization: it does not verify that the API key has permission to use the specific model or that the model belongs to the same project/org as the key.
+
+This pattern is duplicated identically in three locations: the OpenAI-compatible chat handler, the Anthropic messages handler's `CreateMessage`, and its `CountTokens` method.
+
+### Evidence
+
+**`chathandler/chat_handler.go` (lines 195–202):**
+```go
+isAPIKeyAuth := strings.EqualFold(reqCtx.GetHeader("X-Auth-Method"), "apikey")
+var selectedProviderModel *domainmodel.ProviderModel
+var selectedProvider *domainmodel.Provider
+if isAPIKeyAuth {
+ selectedProviderModel, selectedProvider, err = h.providerHandler.SelectProviderModelForModelPublicIDIncludingInactive(ctx, request.Model)
+} else {
+ selectedProviderModel, selectedProvider, err = h.providerHandler.SelectProviderModelForModelPublicID(ctx, request.Model)
+}
+```
+
+**`messageshandler/messages_handler.go` (lines 78–86) — identical pattern in `CreateMessage`:**
+```go
+isAPIKeyAuth := strings.EqualFold(reqCtx.GetHeader("X-Auth-Method"), "apikey")
+var selectedProviderModel *domainmodel.ProviderModel
+var selectedProvider *domainmodel.Provider
+var err error
+if isAPIKeyAuth {
+ selectedProviderModel, selectedProvider, err = h.providerHandler.SelectProviderModelForModelPublicIDIncludingInactive(ctx, request.Model)
+} else {
+ selectedProviderModel, selectedProvider, err = h.providerHandler.SelectProviderModelForModelPublicID(ctx, request.Model)
+}
+```
+
+**`messageshandler/messages_handler.go` (lines 462–469) — identical pattern in `CountTokens`:**
+```go
+isAPIKeyAuth := strings.EqualFold(reqCtx.GetHeader("X-Auth-Method"), "apikey")
+var selectedProviderModel *domainmodel.ProviderModel
+var err error
+if isAPIKeyAuth {
+ selectedProviderModel, _, err = h.providerHandler.SelectProviderModelForModelPublicIDIncludingInactive(ctx, request.Model)
+} else {
+ selectedProviderModel, _, err = h.providerHandler.SelectProviderModelForModelPublicID(ctx, request.Model)
+}
+```
+
+**`modelhandler/provider_handler.go` (lines 196–220) — the "including inactive" method has no authorization check:**
+```go
+func (providerHandler *ProviderHandler) SelectProviderModelForModelPublicIDIncludingInactive(ctx context.Context, modelPublicID string) (*domainmodel.ProviderModel, *domainmodel.Provider, error) {
+ if strings.TrimSpace(modelPublicID) == "" {
+ return nil, nil, platformerrors.NewError(...)
+ }
+
+ providerModels, err := providerHandler.providerModelService.FindByModelKey(ctx, modelPublicID)
+ // ... no ownership/scope check — returns any matching model regardless of API key identity
+}
+```
+
+Compare with the active-only variant (line 149) which calls `FindActiveByModelKey` — but neither variant checks API key scope.
+
+### Recommended Fix
+
+This requires a team decision on the authorization model before implementation. Options include:
+
+1. **Project-scoped API keys:** Verify that the API key belongs to the same project as the requested model. This requires associating API keys with projects and filtering models by project ownership.
+2. **Explicit model allowlists per API key:** Each API key has a list of model IDs it can access, checked at selection time.
+3. **Remove inactive model access for API keys:** If there is no business requirement for API keys to access inactive models, use the same `FindActiveByModelKey` path for all auth methods.
+
+Until a decision is made, consider adding an audit log entry whenever an API key accesses an inactive model to measure the scope of this gap.
+
+---
+
+## Finding 2: Duplicate Redirect Logic Across Frontend Files
+
+| Field | Value |
+|---|---|
+| **Severity** | MEDIUM |
+| **Source PR** | #486 |
+| **Priority** | P3 — cleanup, no security risk after Phase 1 fix |
+
+### Affected Files
+- `apps/web/src/components/form/login.tsx`
+- `apps/web/src/routes/__root.tsx`
+- `apps/web/src/routes/auth/callback.tsx`
+- `apps/web/src/routes/login.tsx`
+- `apps/web/src/lib/redirect-utils.ts` (shared utility, added in Phase 1)
+
+### Description
+
+Phase 1 successfully extracted `isAllowedExternalRedirect` into `apps/web/src/lib/redirect-utils.ts`, and all four files now import from that shared location. The security-sensitive URL validation is no longer duplicated.
+
+However, **redirect handling logic** is still duplicated across the files. The same three-step redirect pattern — (1) check for external localhost redirect and append encoded token, (2) check for internal path redirect starting with `/`, (3) fall back to home — is implemented independently in three places with slight variations. Additionally, `getRedirectUrl` in `login.tsx` (form component) and `getRedirectTarget` in `__root.tsx` perform similar URL-parameter extraction.
+
+### Evidence
+
+**External redirect + token pattern — duplicated in 3 files:**
+
+`apps/web/src/routes/login.tsx` (lines 26–36):
+```tsx
+if (redirectUrl && isAllowedExternalRedirect(redirectUrl)) {
+ if (!accessToken) {
+ return;
+ }
+ const bearerToken = `Bearer ${accessToken}`;
+ const encodedToken = btoa(bearerToken);
+ const target = new URL(redirectUrl);
+ target.searchParams.set("token", encodedToken);
+ window.location.href = target.toString();
+ return;
+}
+```
+
+`apps/web/src/routes/__root.tsx` (lines 142–149, inside `handleCloseModal`):
+```tsx
+if (redirectUrl && isAllowedExternalRedirect(redirectUrl)) {
+ if (!accessToken) return;
+ const bearerToken = `Bearer ${accessToken}`;
+ const encodedToken = btoa(bearerToken);
+ const target = new URL(redirectUrl);
+ target.searchParams.set("token", encodedToken);
+ window.location.href = target.toString();
+ return;
+}
+```
+
+`apps/web/src/routes/auth/callback.tsx` (lines 59–66):
+```tsx
+if (oauthData.redirectUrl && isAllowedExternalRedirect(oauthData.redirectUrl)) {
+ const bearerToken = `Bearer ${tokens.access_token}`;
+ const encodedToken = btoa(bearerToken);
+ const target = new URL(oauthData.redirectUrl);
+ target.searchParams.set("token", encodedToken);
+ window.location.href = target.toString();
+ return;
+}
+```
+
+**Redirect URL extraction — duplicated in 2 files:**
+
+`apps/web/src/components/form/login.tsx` (lines 34–57, `getRedirectUrl`):
+```tsx
+const getRedirectUrl = () => {
+ const url = new URL(window.location.href);
+ const redirectParam = url.searchParams.get(URL_PARAM.REDIRECT);
+ if (redirectParam && (redirectParam.startsWith("/") || isAllowedExternalRedirect(redirectParam))) {
+ return redirectParam;
+ }
+ // ... additional cases
+};
+```
+
+`apps/web/src/routes/__root.tsx` (lines 83–90, `getRedirectTarget`):
+```tsx
+const getRedirectTarget = () => {
+ const url = new URL(window.location.href);
+ const redirectParam = url.searchParams.get(URL_PARAM.REDIRECT);
+ if (redirectParam && redirectParam.startsWith("/")) {
+ return redirectParam;
+ }
+ return url.pathname + url.search;
+};
+```
+
+### Recommended Fix
+
+Consolidate the duplicated logic into `apps/web/src/lib/redirect-utils.ts`:
+
+1. Add a `performExternalRedirect(redirectUrl: string, accessToken: string)` function that handles the token-encoding-and-redirect pattern.
+2. Add a `getRedirectUrl()` function that extracts and validates the redirect parameter from the current URL.
+3. Update all four files to use the shared functions, eliminating the duplicated code.
+
+This is a cleanup task with no security risk (Phase 1 already secured the validation). Priority is P3.
+
+---
+
+## Finding 3: LoginRoute Renders Blank Page During Redirect
+
+| Field | Value |
+|---|---|
+| **Severity** | LOW |
+| **Source PR** | #486 |
+| **Priority** | P4 — UX polish |
+
+### Affected Files
+- `apps/web/src/routes/login.tsx` (line 47)
+
+### Description
+
+The `LoginRoute` component returns `null` when the user is already authenticated and a redirect is being processed. During the brief period between rendering and the `useEffect` firing to perform the redirect, users see a completely blank page. This is particularly noticeable on slower connections or when the redirect involves an external URL (which requires a full page navigation).
+
+The OAuth callback page (`auth/callback.tsx`) already handles this correctly by showing a spinner with "Completing authentication..." text.
+
+### Evidence
+
+`apps/web/src/routes/login.tsx` (lines 11–48):
+```tsx
+function LoginRoute() {
+ const navigate = useNavigate();
+ const isAuthenticated = useAuth((state) => state.isAuthenticated);
+ const isGuest = useAuth((state) => state.isGuest);
+ const accessToken = useAuth((state) => state.accessToken);
+
+ useEffect(() => {
+ if (!isAuthenticated || isGuest) {
+ return;
+ }
+ // ... redirect logic
+ }, [accessToken, isAuthenticated, isGuest, navigate]);
+
+ return null; // <-- blank page during redirect
+}
+```
+
+For comparison, `apps/web/src/routes/auth/callback.tsx` (lines 102–108) shows the correct pattern:
+```tsx
+return (
+
+
+
+
Completing authentication...
+
+
+);
+```
+
+### Recommended Fix
+
+Replace `return null` with a loading indicator. The spinner pattern from `auth/callback.tsx` can be reused directly:
+
+```tsx
+return (
+