Skip to content

fix: fixed logout url issue#12

Open
jawad-khan wants to merge 2 commits into
foss-mainfrom
jawad/logout-url-fix
Open

fix: fixed logout url issue#12
jawad-khan wants to merge 2 commits into
foss-mainfrom
jawad/logout-url-fix

Conversation

@jawad-khan
Copy link
Copy Markdown

Description

Replace moneta value from SMB_NAME env var in logout urls.

Motivation and Context

FIX #

Screenshots

API Changes

  • This PR includes API changes

Change Type

  • Bug fix
  • New feature
  • Performance improvement
  • Refactoring
  • Documentation
  • Dependency/Build system
  • Breaking change
  • Other (specify):

Testing Performed

  • Tested locally
  • Manual/QA verification

Checklist

  • Follows project coding standards and conventions
  • Documentation updated as needed
  • Dependencies updated as needed
  • No lint/build errors or new warnings
  • All relevant tests are passing

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Fixes the post-logout redirect host rewriting so the redirect goes to the correct platform portal domain by making the first DNS label configurable via SMB_NAME/NEXT_PUBLIC_SMB_NAME (defaulting to moneta).

Changes:

  • Update logout redirect host rewriting to use NEXT_PUBLIC_SMB_NAME (default moneta).
  • Add runtime placeholder substitution support for NEXT_PUBLIC_SMB_NAME in the web Docker entrypoint and Dockerfile.
  • Wire SMB_NAME/NEXT_PUBLIC_SMB_NAME through Docker Compose and document it in .env.example files.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
surfsense_web/lib/auth-utils.ts Uses NEXT_PUBLIC_SMB_NAME to rewrite logout redirect host.
surfsense_web/docker-entrypoint.js Adds placeholder replacement for __NEXT_PUBLIC_SMB_NAME__ using SMB_NAME/NEXT_PUBLIC_SMB_NAME.
surfsense_web/Dockerfile Introduces build arg/env placeholder for NEXT_PUBLIC_SMB_NAME.
surfsense_web/.env.example Documents NEXT_PUBLIC_SMB_NAME for local development.
docker/docker-compose.yml Provides SMB_NAME to the web container in production compose.
docker/docker-compose.dev.yml Adds NEXT_PUBLIC_SMB_NAME build arg wiring for dev compose builds.
docker/.env.example Documents SMB_NAME for Docker-based deployments.

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

Comment thread surfsense_web/lib/auth-utils.ts Outdated
const portalHost = window.location.hostname.replace(/^[^.]*\./, "moneta.");
// Docker: set SMB_NAME on the container; docker-entrypoint substitutes NEXT_PUBLIC_SMB_NAME.
// Local dev: set NEXT_PUBLIC_SMB_NAME in .env (default moneta).
const smbLabel = process.env.NEXT_PUBLIC_SMB_NAME?.trim() || "moneta";
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

smbLabel is interpolated directly into the redirect host. If it contains dots, slashes, @, or other URL-special characters, this can produce an unexpected cross-domain redirect or an invalid URL. Consider validating it as a single DNS label (e.g., /^[a-z0-9-]+$/i) and falling back to the default when it doesn’t match.

Suggested change
const smbLabel = process.env.NEXT_PUBLIC_SMB_NAME?.trim() || "moneta";
const rawSmbLabel = process.env.NEXT_PUBLIC_SMB_NAME?.trim();
const smbLabel =
rawSmbLabel && /^(?!-)[a-z0-9-]{1,63}(?<!-)$/i.test(rawSmbLabel) ? rawSmbLabel : "moneta";

Copilot uses AI. Check for mistakes.
// Local dev: set NEXT_PUBLIC_SMB_NAME in .env (default moneta).
const smbLabel = process.env.NEXT_PUBLIC_SMB_NAME?.trim() || "moneta";
const portalHost = window.location.hostname.replace(/^[^.]*\./, `${smbLabel}.`);
window.location.href = `${window.location.protocol}//${portalHost}`;
Copy link

Copilot AI May 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The redirect uses window.location.hostname (no port) and builds protocol//host, which drops any non-default port the app is running on. This can break setups where SurfSense is accessed via an explicit port (e.g. localhost:3000 / mapped Docker ports). Consider preserving window.location.port when present (or using window.location.host and rewriting just the hostname portion).

Suggested change
window.location.href = `${window.location.protocol}//${portalHost}`;
const portSuffix = window.location.port ? `:${window.location.port}` : "";
window.location.href = `${window.location.protocol}//${portalHost}${portSuffix}`;

Copilot uses AI. Check for mistakes.
@awais786
Copy link
Copy Markdown

awais786 commented May 1, 2026

Reviewed against the sso-rules invariant we merged in awais786/sso-rules#1 (required env, no default, fail loudly) and the sibling PRs in Outline #10 / Penpot #11 / Twenty #2 (all fail-fast). Closed my parallel PR #11 in favor of this one — your compose coverage (docker/.env.example, docker/docker-compose.yml, docker/docker-compose.dev.yml) was the load-bearing piece I missed.

The headline blocker is the "moneta" default scattered across 3 files (auth-utils.ts, docker-entrypoint.js, both .env.example). It re-introduces the exact bug class we're trying to close:

  1. Same failure mode, one layer back. Original bug was "foss." baked in. Defaulting to "moneta" is the same literal, just deeper. Next domain move silently breaks the same way.
  2. Silent vs loud. Unset env → SPA renders wrong host, no error. Required env → container crashes at startup with a clear message. Auth-adjacent code should fail loud.
  3. False configurability. Future dev sees SMB_NAME= in compose, assumes it's wired, doesn't verify the value. Bug shape: works in dev (default kicks in), breaks in prod (default ≠ deployment's portal).
  4. Cross-app consistency. Outline/Penpot/Twenty all fail-fast under SSO. SurfSense with a default makes them look like overengineered outliers.

Concrete changes

surfsense_web/lib/auth-utils.ts:247 — drop the fallback:

const smbName = process.env.NEXT_PUBLIC_SMB_NAME!.trim();

surfsense_web/docker-entrypoint.js — add fail-fast before the replacements array, drop the double-fallback:

if (!process.env.SMB_NAME && process.env.NEXT_PUBLIC_FASTAPI_BACKEND_AUTH_TYPE === "SSO") {
  console.error("[entrypoint] ERROR: SMB_NAME env is required when AUTH_TYPE=SSO.");
  console.error("[entrypoint]        Set it to the portal hostname prefix (e.g. 'moneta').");
  process.exit(1);
}
// ...
["__NEXT_PUBLIC_SMB_NAME__", process.env.SMB_NAME || ""],

surfsense_web/.env.example + docker/.env.example — drop the =moneta default; document as required under SSO. Container env name is SMB_NAME (uniform across every devstack app — Outline, Penpot, SurfSense, Twenty all read the same name); NEXT_PUBLIC_SMB_NAME is the bundle-internal placeholder set automatically by the Dockerfile, not a user-facing knob.

docker/docker-compose.yml:201 + docker/docker-compose.dev.yml:211 — drop :-moneta defaults to match the required-env policy.

Nits

  • surfsense_web/lib/auth-utils.ts:247: rename smbLabelsmbName to match the variable name in every other app's logout file (keeps grep clean across the 4 forks).
  • The 3-line comment about the fallback at auth-utils.ts:243-245 becomes dead weight once the fallback is removed — replace with the canonical 2-liner from Plane/Outline/Twenty.

Suggested path

Either: (a) push the changes above and merge as-is, or (b) merge today to unblock the cutover and land a small follow-up PR with the fail-fast + drop-defaults changes. I'm happy to open the follow-up if (b) is the call.

Drops the SMB_NAME / NEXT_PUBLIC_SMB_NAME plumbing introduced for the
post-logout portal redirect. The previous approach required threading
the value through three files (.env.example → Dockerfile build-arg →
docker-entrypoint.js placeholder substitution → bundle); any broken
link silently routed logout to the wrong host.

Switching to a regex on window.location.hostname removes the env
dependency and works for any `<prefix>-<app>.<domain>` shape:
- foss-research.local.moneta.dev → foss.local.moneta.dev
- moneta-research.askii.ai       → moneta.askii.ai

Reverts: docker/.env.example, docker/docker-compose*.yml,
surfsense_web/{.env.example,Dockerfile,docker-entrypoint.js} to upstream.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants