diff --git a/.github/workflows/backend-ci.yml b/.github/workflows/backend-ci.yml index 34b1142f..83b79e60 100644 --- a/.github/workflows/backend-ci.yml +++ b/.github/workflows/backend-ci.yml @@ -5,22 +5,28 @@ name: Backend CI # registry, pairing, and the panel<->agent command loop exercised end-to-end # by tests/test_agent_poll_e2e.py — instead of finding out at install time. +# main is deliberately absent from `push`: release.yml gates itself on this +# workflow via `ci-gate`, so a push to main already runs the suite once. Listing +# main here ran it a second time, standalone, on every merge. on: push: - branches: [dev, main] + branches: [dev] paths: - 'backend/**' - '.github/workflows/backend-ci.yml' pull_request: - branches: [dev, main] + branches: [main] paths: - 'backend/**' - '.github/workflows/backend-ci.yml' workflow_call: jobs: - pytest: - name: pytest + # The ratchet needs to see the WHOLE suite, so it cannot live inside a shard — + # each shard only collects its own quarter. Its own job costs no wall-clock + # time: it runs beside the shards and finishes in seconds. + ratchet: + name: test-count ratchet runs-on: ubuntu-latest steps: - uses: actions/checkout@v4 @@ -42,6 +48,49 @@ jobs: # Lowering the floor requires editing BASELINE_COUNT in the same commit. working-directory: backend run: python tests/check_test_count.py - - name: Run tests + + # Sharded because this job WAS the entire wait: 21m50s of a ~22m pipeline, + # while every other workflow finished inside a minute. Splitting the 3173 + # tests over 4 runners cuts the critical path to roughly a quarter. + # + # Sharding rather than pytest-xdist is deliberate: each shard is its own VM, + # so the process-shared state that makes in-process parallelism unsafe here + # (templates.json, APPS_DIR — see the per-PID DB dance in tests/conftest.py) + # simply isn't shared. No test code has to change. + # + # NOTE: with no .test_durations file, pytest-split balances by test COUNT, + # not by time — and these tests range from 0.1s to 10s+. If one shard lands + # most of the slow ones it becomes the new critical path. If that shows up, + # run once with `--store-durations` and commit backend/.test_durations to + # switch it to duration-based balancing. + pytest: + name: pytest (${{ matrix.group }}/4) + runs-on: ubuntu-latest + strategy: + # Report every failing shard in one pass instead of hiding shards 2-4 + # behind the first failure — one round of fixes instead of four. + fail-fast: false + matrix: + group: [1, 2, 3, 4] + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-python@v5 + with: + python-version: '3.11' + cache: pip + cache-dependency-path: backend/requirements.txt + - name: Install dependencies + working-directory: backend + run: | + python -m pip install --upgrade pip + pip install -r requirements.txt + pip install pytest pytest-split + - name: Run tests (shard ${{ matrix.group }} of 4) + # Scoped to `tests` rather than a bare `pytest`. Identical here (3173 + # either way, since backend/dev-data/ is gitignored and absent from a + # CI checkout), but it makes the command reproducible on a dev box: a + # bare pytest there tries to collect the locally deployed apps under + # backend/dev-data/ and dies during collection. Copy this line verbatim + # to debug a red shard locally. working-directory: backend - run: python -m pytest -v + run: python -m pytest tests -v --splits 4 --group ${{ matrix.group }} diff --git a/.github/workflows/extensions-ci.yml b/.github/workflows/extensions-ci.yml index 2b468270..1415c81f 100644 --- a/.github/workflows/extensions-ci.yml +++ b/.github/workflows/extensions-ci.yml @@ -8,14 +8,14 @@ name: Extensions CI on: push: - branches: [dev, main] + branches: [dev] paths: - 'builtin-extensions/**' - 'frontend/src/plugins/**' - 'scripts/sync-builtin-frontends.mjs' - '.github/workflows/extensions-ci.yml' pull_request: - branches: [dev, main] + branches: [main] paths: - 'builtin-extensions/**' - 'frontend/src/plugins/**' diff --git a/.github/workflows/frontend-ci.yml b/.github/workflows/frontend-ci.yml new file mode 100644 index 00000000..e1f325fe --- /dev/null +++ b/.github/workflows/frontend-ci.yml @@ -0,0 +1,58 @@ +name: Frontend CI + +# The frontend's lint gate ran nowhere until now — `npm run lint` was in +# package.json and in CLAUDE.md, but no workflow ever invoked it. That silently +# unenforced three project-specific checkers that exist precisely because a +# human review keeps missing what they catch: +# +# check-settings-index every Settings tab has a search-index entry +# check-theme-tokens the theme-token whitelist stays in 3-way sync +# check-html-sinks every raw-HTML sink is sanitized or annotated (XSS) +# +# Only `npm run lint` runs here. The frontend is already COMPILED in CI by +# Release Build Smoke Test, whose scripts/build-release.sh does `npm ci && +# npm run build` — adding a build job here would just duplicate that. +# +# backend/app/** is in the paths because check-html-sinks scans it too (for +# `|safe`, `Markup(`, `render_template_string`), so a backend-only commit can +# introduce a sink this must catch. + +on: + push: + branches: [dev] + paths: + - 'frontend/**' + - 'backend/app/**' + - 'scripts/check-html-sinks.mjs' + - '.github/workflows/frontend-ci.yml' + pull_request: + branches: [main] + paths: + - 'frontend/**' + - 'backend/app/**' + - 'scripts/check-html-sinks.mjs' + - '.github/workflows/frontend-ci.yml' + workflow_call: + +jobs: + lint: + name: lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: '22' + cache: npm + cache-dependency-path: frontend/package-lock.json + - name: Install dependencies + working-directory: frontend + run: npm ci + - name: Lint + # eslint + the three checkers, chained by the package.json script. + # Currently 926 warnings / 0 errors, and eslint exits 0 on warnings — + # so this gates on errors only. If you ever want the warning count + # ratcheted the way backend/tests/BASELINE_COUNT ratchets test count, + # add --max-warnings= here rather than mass-fixing in one commit. + working-directory: frontend + run: npm run lint diff --git a/.github/workflows/release-smoke.yml b/.github/workflows/release-smoke.yml index b2a35cfb..b47ef848 100644 --- a/.github/workflows/release-smoke.yml +++ b/.github/workflows/release-smoke.yml @@ -5,9 +5,12 @@ name: Release Build Smoke Test # frontend source files) before they reach main, instead of failing late in the # release workflow. +# main is deliberately absent from `push`: on a merge to main, release.yml's +# `build-release` job runs this very script for real moments later, so building +# the tarball here too was pure duplication. on: push: - branches: [dev, main] + branches: [dev] paths: - 'frontend/**' - 'backend/**' @@ -16,7 +19,7 @@ on: - '.github/workflows/release.yml' - 'VERSION' pull_request: - branches: [dev, main] + branches: [main] paths: - 'frontend/**' - 'backend/**' diff --git a/.github/workflows/scripts-ci.yml b/.github/workflows/scripts-ci.yml index 24df61c2..4fe0dcd1 100644 --- a/.github/workflows/scripts-ci.yml +++ b/.github/workflows/scripts-ci.yml @@ -14,7 +14,7 @@ name: Scripts CI on: push: - branches: [dev, main] + branches: [dev] paths: - 'scripts/**' - 'templates/**' @@ -23,7 +23,7 @@ on: - 'serverkit' - '.github/workflows/scripts-ci.yml' pull_request: - branches: [dev, main] + branches: [main] paths: - 'scripts/**' - 'templates/**' diff --git a/.github/workflows/security-scan.yml b/.github/workflows/security-scan.yml index f565c33d..1861401c 100644 --- a/.github/workflows/security-scan.yml +++ b/.github/workflows/security-scan.yml @@ -7,7 +7,7 @@ on: - 'backend/**' - '.github/workflows/security-scan.yml' pull_request: - branches: [main, dev] + branches: [main] paths: - 'backend/**' - '.github/workflows/security-scan.yml' diff --git a/.github/workflows/test-system-utils.yml b/.github/workflows/test-system-utils.yml index 8dae39e2..35e02ba9 100644 --- a/.github/workflows/test-system-utils.yml +++ b/.github/workflows/test-system-utils.yml @@ -9,36 +9,21 @@ on: - 'backend/tests/test_utils_system*.py' - '.github/workflows/test-system-utils.yml' pull_request: - branches: [main, dev] + branches: [main] paths: - 'backend/app/utils/system.py' - 'backend/app/services/**' - 'backend/tests/test_utils_system*.py' +# The mocked `unit-tests` job that used to lead this file was removed: it ran +# `pytest tests/test_utils_system.py` (44 tests), and Backend CI's bare +# `pytest -v` already collects that exact file — there is no pytest.ini, +# addopts, or collect_ignore narrowing it. What is left here is the part +# Backend CI genuinely cannot do: exercise the package-manager detection +# against real apt/dnf inside real distro images. jobs: # ────────────────────────────────────────────────────────────────── - # Job 1: Mocked unit tests — fast, validates all logic - # ────────────────────────────────────────────────────────────────── - unit-tests: - name: Unit Tests - runs-on: ubuntu-latest - - steps: - - uses: actions/checkout@v4 - - - uses: actions/setup-python@v5 - with: - python-version: '3.11' - - - name: Install test deps - run: pip install pytest - - - name: Run unit tests - working-directory: backend - run: python -m pytest tests/test_utils_system.py -v - - # ────────────────────────────────────────────────────────────────── - # Job 2: Integration tests on real distros (no mocks) + # Job 1: Integration tests on real distros (no mocks) # ────────────────────────────────────────────────────────────────── integration-tests: name: Integration (${{ matrix.distro }}) @@ -91,7 +76,7 @@ jobs: run: python3 -m pytest tests/test_utils_system_integration.py -v # ────────────────────────────────────────────────────────────────── - # Job 3: Audit — grep for raw subprocess patterns in services + # Job 2: Audit — grep for raw subprocess patterns in services # ────────────────────────────────────────────────────────────────── audit-raw-patterns: name: Audit Raw Subprocess Patterns diff --git a/VERSION b/VERSION index fba132c4..2fff9b2e 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -1.7.82 +1.7.83 diff --git a/backend/app/services/template_service.py b/backend/app/services/template_service.py index d046d06b..22cb9b50 100644 --- a/backend/app/services/template_service.py +++ b/backend/app/services/template_service.py @@ -36,15 +36,32 @@ class TemplateService: INSTALLED_DIR = paths.APPS_DIR TEMPLATE_CONFIG = os.path.join(CONFIG_DIR, 'templates.json') - # Default template repository + # Default template repository. + # + # serverkit.ai proxies the serverkit-templates registry and is built for + # exactly this consumer: it serves /index.json and the + # /templates/.yaml path this class derives below, behind a + # TTL cache with last-good fallback. Pointing at the product domain rather + # than raw.githubusercontent also means a branch rename upstream cannot + # silently empty every panel's catalog. DEFAULT_REPOS = [ { 'name': 'serverkit-official', - 'url': 'https://raw.githubusercontent.com/serverkit/templates/main', + 'url': 'https://serverkit.ai/templates', 'enabled': True } ] + # Repo URLs that never worked and should be healed on read rather than + # left to rot in an operator's templates.json. `serverkit/templates` was a + # guess at the org name -- the registry is `jhd3197/serverkit-templates` -- + # so this URL has 404'd for its entire existence and no panel has ever + # fetched a template through it. Nothing is lost by replacing it. + DEAD_REPO_URLS = { + 'https://raw.githubusercontent.com/serverkit/templates/main', + 'https://raw.githubusercontent.com/serverkit/templates', + } + # Provider-owned templates (plan 52 D4 hook inversion): these ids are # listed and installable ONLY while the owning extension has registered as # their provider (i.e. it is installed + active this boot — registration @@ -167,11 +184,18 @@ def _run_provider_validate(cls, template_id, variables): @classmethod def get_config(cls) -> Dict: - """Get template configuration.""" + """Get template configuration. + + A panel that has ever saved this file keeps whatever repos were in it, + so fixing DEFAULT_REPOS alone would only help fresh installs. Dead URLs + are therefore corrected on read (see DEAD_REPO_URLS). Not written back + here -- a getter should not have a disk side effect -- so the repair + re-applies each read until something saves the config normally.""" if os.path.exists(cls.TEMPLATE_CONFIG): try: with open(cls.TEMPLATE_CONFIG, 'r') as f: - return json.load(f) + config = json.load(f) + return cls._heal_dead_repos(config) except Exception: pass return { @@ -180,6 +204,18 @@ def get_config(cls) -> Dict: 'last_sync': None } + @classmethod + def _heal_dead_repos(cls, config: Dict) -> Dict: + """Point any known-dead repo URL at the current default.""" + repos = config.get('repos') + if not isinstance(repos, list): + return config + default_url = cls.DEFAULT_REPOS[0]['url'] + for repo in repos: + if isinstance(repo, dict) and repo.get('url', '').rstrip('/') in cls.DEAD_REPO_URLS: + repo['url'] = default_url + return config + @classmethod def save_config(cls, config: Dict) -> Dict: """Save template configuration.""" @@ -2306,6 +2342,7 @@ def sync_templates(cls) -> Dict: config = cls.get_config() synced = 0 + unverified = 0 # saved, but the index pinned no sha256 for them errors = [] for repo in config.get('repos', []): @@ -2331,10 +2368,34 @@ def sync_templates(cls) -> Dict: response = requests.get(template_url, timeout=30) response.raise_for_status() - # Save locally + # Verify against the checksum the index pinned for this + # entry. A template is a deploy definition -- images, + # ports, volumes, env -- so a swapped file is worth + # refusing outright, and the index already carries the + # hash for every official entry. + # + # Missing hash is allowed (third-party repos may not + # publish one) and counted, mirroring how extensions + # treat unsigned-vs-invalid: absent is a caveat, wrong + # is a hard stop. + expected = (template_info.get('sha256') or '').strip().lower() + if expected: + actual = hashlib.sha256(response.content).hexdigest() + if actual != expected: + errors.append( + f"Checksum mismatch for {template_id}: index pinned " + f"{expected[:12]}..., downloaded {actual[:12]}.... Not saved." + ) + continue + else: + unverified += 1 + + # Written as bytes so what lands on disk is exactly the + # content that was hashed (text mode would rewrite line + # endings on Windows and no longer match). filepath = os.path.join(cls.TEMPLATES_DIR, f"{template_id}.yaml") - with open(filepath, 'w') as f: - f.write(response.text) + with open(filepath, 'wb') as f: + f.write(response.content) synced += 1 except Exception as e: @@ -2349,6 +2410,7 @@ def sync_templates(cls) -> Dict: return { 'success': True, 'synced': synced, + 'unverified': unverified, 'errors': errors if errors else None } diff --git a/backend/tests/BASELINE_COUNT b/backend/tests/BASELINE_COUNT index a0d2c03e..26807db2 100644 --- a/backend/tests/BASELINE_COUNT +++ b/backend/tests/BASELINE_COUNT @@ -1 +1 @@ -3138 \ No newline at end of file +3173 \ No newline at end of file diff --git a/backend/tests/test_template_repo_sync.py b/backend/tests/test_template_repo_sync.py new file mode 100644 index 00000000..ed4a8118 --- /dev/null +++ b/backend/tests/test_template_repo_sync.py @@ -0,0 +1,212 @@ +"""Proving tests for the template repository default and sync verification. + +Two things land here together on purpose. The default repo URL pointed at +`serverkit/templates`, an org that does not exist, so it 404'd for its whole +life and no panel ever fetched a template through it. Correcting it turns on +a download path that has therefore never actually run in production -- and +that path wrote whatever came back straight to disk without checking the +sha256 the index pins for every entry. Fixing the URL without the checksum +would be switching on an unverified fetch. +""" +import hashlib +import json +import os +from unittest.mock import patch + +import pytest + +from app.services.template_service import TemplateService + + +TEMPLATE_BODY = b"name: Demo\nversion: '1.0'\nservices:\n web:\n image: demo:1\n" +TEMPLATE_SHA = hashlib.sha256(TEMPLATE_BODY).hexdigest() + + +class FakeResponse: + def __init__(self, content=b"", payload=None, status_code=200): + self.content = content + self._payload = payload + self.status_code = status_code + + @property + def text(self): + return self.content.decode("utf-8") + + def json(self): + return self._payload + + def raise_for_status(self): + if self.status_code >= 400: + raise RuntimeError(f"HTTP {self.status_code}") + + +# -------------------------------------------------------------------------- +# Default repo + healing +# -------------------------------------------------------------------------- + +def test_default_repo_points_at_a_reachable_host(): + """The org `serverkit` does not exist; the registry is under jhd3197 and is + proxied by serverkit.ai. Guard against the old value coming back.""" + url = TemplateService.DEFAULT_REPOS[0]['url'] + assert 'raw.githubusercontent.com/serverkit/' not in url + assert url == 'https://serverkit.ai/templates' + + +def test_derived_urls_match_what_the_proxy_serves(): + """This class builds /index.json and + /templates/.yaml. serverkit.ai exposes both shapes; if this + ever drifts, every sync 404s silently.""" + base = TemplateService.DEFAULT_REPOS[0]['url'] + assert f"{base}/index.json" == 'https://serverkit.ai/templates/index.json' + assert f"{base}/templates/n8n.yaml" == 'https://serverkit.ai/templates/templates/n8n.yaml' + + +def test_dead_repo_url_is_healed_on_read(tmp_path): + """A panel that already saved templates.json keeps its repos, so fixing the + default alone would strand every existing install on the dead URL.""" + config_path = tmp_path / 'templates.json' + config_path.write_text(json.dumps({ + 'repos': [{ + 'name': 'serverkit-official', + 'url': 'https://raw.githubusercontent.com/serverkit/templates/main', + 'enabled': True, + }], + 'installed': {}, + 'last_sync': None, + })) + + with patch.object(TemplateService, 'TEMPLATE_CONFIG', str(config_path)): + config = TemplateService.get_config() + + assert config['repos'][0]['url'] == 'https://serverkit.ai/templates' + assert config['repos'][0]['name'] == 'serverkit-official' # nothing else touched + + +def test_healing_leaves_operator_repos_alone(tmp_path): + """Only the known-dead URLs are rewritten -- a custom repo is untouched.""" + config_path = tmp_path / 'templates.json' + config_path.write_text(json.dumps({ + 'repos': [ + {'name': 'mine', 'url': 'https://templates.example.com/sk', 'enabled': True}, + {'name': 'dead', 'url': 'https://raw.githubusercontent.com/serverkit/templates/main', + 'enabled': False}, + ], + })) + + with patch.object(TemplateService, 'TEMPLATE_CONFIG', str(config_path)): + repos = TemplateService.get_config()['repos'] + + assert repos[0]['url'] == 'https://templates.example.com/sk' + assert repos[1]['url'] == 'https://serverkit.ai/templates' + assert repos[1]['enabled'] is False # healing must not re-enable anything + + +def test_healing_survives_a_malformed_config(tmp_path): + """Garbage in the repos key must not take the whole panel's config down.""" + config_path = tmp_path / 'templates.json' + config_path.write_text(json.dumps({'repos': 'not-a-list'})) + + with patch.object(TemplateService, 'TEMPLATE_CONFIG', str(config_path)): + config = TemplateService.get_config() + + assert config['repos'] == 'not-a-list' # returned as-is, no crash + + +# -------------------------------------------------------------------------- +# sync_templates checksum verification +# -------------------------------------------------------------------------- + +def _run_sync(tmp_path, index_entry, body): + """Drive sync_templates against a one-entry fake repo.""" + index = {'templates': [index_entry]} + + def fake_get(url, timeout=None): + if url.endswith('/index.json'): + return FakeResponse(payload=index) + return FakeResponse(content=body) + + templates_dir = tmp_path / 'templates' + config_path = tmp_path / 'templates.json' + config_path.write_text(json.dumps({ + 'repos': [{'name': 'test', 'url': 'https://example.test/repo', 'enabled': True}], + })) + + with patch.object(TemplateService, 'TEMPLATES_DIR', str(templates_dir)), \ + patch.object(TemplateService, 'TEMPLATE_CONFIG', str(config_path)), \ + patch.object(TemplateService, 'CONFIG_DIR', str(tmp_path)), \ + patch('app.services.template_service.requests.get', side_effect=fake_get): + result = TemplateService.sync_templates() + + return result, templates_dir / 'demo.yaml' + + +def test_matching_checksum_is_saved(tmp_path): + result, path = _run_sync( + tmp_path, {'id': 'demo', 'sha256': TEMPLATE_SHA}, TEMPLATE_BODY) + + assert result['synced'] == 1 + assert result['unverified'] == 0 + assert not result['errors'] + assert path.exists() + # Byte-identical to what was verified. + assert hashlib.sha256(path.read_bytes()).hexdigest() == TEMPLATE_SHA + + +def test_mismatched_checksum_is_refused_and_not_written(tmp_path): + """A template is a deploy definition -- images, ports, volumes, env. A + swapped file is refused outright rather than saved with a warning.""" + result, path = _run_sync( + tmp_path, {'id': 'demo', 'sha256': 'de' * 32}, TEMPLATE_BODY) + + assert result['synced'] == 0 + assert not path.exists(), 'refused content must never reach disk' + assert result['errors'] and 'Checksum mismatch' in result['errors'][0] + + +def test_missing_checksum_is_allowed_but_counted(tmp_path): + """Third-party repos may publish no hashes; absent is a caveat, not a + hard stop -- mirroring unsigned-vs-invalid for extensions.""" + result, path = _run_sync(tmp_path, {'id': 'demo'}, TEMPLATE_BODY) + + assert result['synced'] == 1 + assert result['unverified'] == 1 + assert path.exists() + + +def test_checksum_comparison_ignores_case_and_padding(tmp_path): + result, path = _run_sync( + tmp_path, {'id': 'demo', 'sha256': f' {TEMPLATE_SHA.upper()} '}, TEMPLATE_BODY) + + assert result['synced'] == 1 + assert path.exists() + + +def test_one_bad_template_does_not_abort_the_rest(tmp_path): + """A poisoned entry must not stop the good ones from syncing.""" + good = b"name: Good\n" + index = {'templates': [ + {'id': 'bad', 'sha256': 'de' * 32}, + {'id': 'good', 'sha256': hashlib.sha256(good).hexdigest()}, + ]} + + def fake_get(url, timeout=None): + if url.endswith('/index.json'): + return FakeResponse(payload=index) + return FakeResponse(content=TEMPLATE_BODY if '/bad.yaml' in url else good) + + templates_dir = tmp_path / 'templates' + config_path = tmp_path / 'templates.json' + config_path.write_text(json.dumps({ + 'repos': [{'name': 'test', 'url': 'https://example.test/repo', 'enabled': True}], + })) + + with patch.object(TemplateService, 'TEMPLATES_DIR', str(templates_dir)), \ + patch.object(TemplateService, 'TEMPLATE_CONFIG', str(config_path)), \ + patch.object(TemplateService, 'CONFIG_DIR', str(tmp_path)), \ + patch('app.services.template_service.requests.get', side_effect=fake_get): + result = TemplateService.sync_templates() + + assert result['synced'] == 1 + assert (templates_dir / 'good.yaml').exists() + assert not (templates_dir / 'bad.yaml').exists() + assert len(result['errors']) == 1 diff --git a/frontend/src/App.jsx b/frontend/src/App.jsx index af4f6469..66048e24 100644 --- a/frontend/src/App.jsx +++ b/frontend/src/App.jsx @@ -6,6 +6,7 @@ import { ThemeProvider } from './contexts/ThemeContext'; import { LayoutProvider } from './contexts/LayoutContext'; import { ResourceTierProvider } from './contexts/ResourceTierContext'; import { NotificationsProvider } from './contexts/NotificationsContext'; +import { rememberRedirect } from './utils/redirectAfterLogin'; import { Toaster } from './components/ui/sonner'; import ThemeSync from './components/ThemeSync'; import DashboardLayout from './layouts/DashboardLayout'; @@ -214,6 +215,7 @@ function DevOnlyRoute({ children }) { function PrivateRoute({ children }) { const { isAuthenticated, loading, needsSetup, needsMigration } = useAuth(); + const location = useLocation(); if (loading) { return ; @@ -228,7 +230,14 @@ function PrivateRoute({ children }) { return ; } - return isAuthenticated ? children : ; + if (isAuthenticated) return children; + + // Park the destination so login can return to it. Deep links into the + // panel (?install= from a serverkit.ai badge, a shared /servers/:id) are + // routinely opened without a session, and landing on the dashboard with + // no explanation is the worst version of that. + rememberRedirect(location); + return ; } function PublicRoute({ children }) { diff --git a/frontend/src/pages/Login.jsx b/frontend/src/pages/Login.jsx index 7d4b8034..76e81b66 100644 --- a/frontend/src/pages/Login.jsx +++ b/frontend/src/pages/Login.jsx @@ -5,6 +5,7 @@ import api from '../services/api'; import SSOProviderIcon from '../components/SSOProviderIcon'; import ServerKitLogo from '../components/ServerKitLogo'; import AuthLayout from './auth/AuthLayout'; +import { consumeRedirect } from '../utils/redirectAfterLogin'; import { Button } from '@/components/ui/button'; import { Input } from '@/components/ui/input'; import { Label } from '@/components/ui/label'; @@ -54,7 +55,7 @@ const Login = () => { api.redeemLoginLink(token) .then((response) => { setUser(response.user); - navigate('/', { replace: true }); + navigate(consumeRedirect(), { replace: true }); }) .catch((err) => { setError(err.message || 'Invalid or expired login link'); @@ -110,7 +111,7 @@ const Login = () => { // No 2FA - complete login setUser(response.user); - navigate('/'); + navigate(consumeRedirect()); } catch (err) { setError(err.message || 'Failed to login'); } finally { @@ -138,7 +139,7 @@ const Login = () => { console.warn(response.warning); } - navigate('/'); + navigate(consumeRedirect()); } catch (err) { setError(err.message || 'Invalid verification code'); // Clear the code inputs on error diff --git a/frontend/src/pages/Marketplace.jsx b/frontend/src/pages/Marketplace.jsx index 798570d1..c702e1c6 100644 --- a/frontend/src/pages/Marketplace.jsx +++ b/frontend/src/pages/Marketplace.jsx @@ -220,7 +220,7 @@ const Marketplace = () => { const [filters, setFilters] = useState({ ownership: '', category: [] }); const [filtersOpen, setFiltersOpen] = useState(false); const location = useLocation(); - const [searchParams] = useSearchParams(); + const [searchParams, setSearchParams] = useSearchParams(); const navigate = useNavigate(); // The active view is driven by the route (/marketplace = browse, // /marketplace/installed = installed). The legacy ?tab=installed deep link @@ -268,6 +268,40 @@ const Marketplace = () => { useEffect(() => { loadExtensions(); }, [loadExtensions]); + // Deep link: /extensions?install= opens that extension's detail + // modal. This is what a serverkit.ai install link lands on (and the + // counterpart to Templates.jsx's ?install=). + // + // It opens the modal rather than installing: a URL arriving from another + // site must never be able to install anything on its own, so the operator + // still presses Install and every trust gate behind it still fires. + // + // Note /marketplace?install=… cannot work — that path only redirects here + // via , which drops the query string. Links must target + // /extensions directly. + const installSlug = searchParams.get('install'); + useEffect(() => { + if (!installSlug || loading) return; + + // Lookup only, so the catalog's featured/sort ordering is irrelevant. + const entry = [ + ...builtins.map(getLocalCatalogEntry), + ...registryExtensions.map(getRegistryCatalogEntry), + ].find((candidate) => candidate.installKey === installSlug); + + if (entry) { + setDetailEntry(entry); + } else { + toast.error(`No extension named "${installSlug}" in this panel's catalog.`); + } + + // Drop the param either way so a refresh doesn't reopen it. + const next = new URLSearchParams(searchParams); + next.delete('install'); + setSearchParams(next, { replace: true }); + // eslint-disable-next-line react-hooks/exhaustive-deps + }, [installSlug, loading, builtins, registryExtensions]); + const handleBuiltinInstall = async (slug) => { setInstalling(true); try { diff --git a/frontend/src/pages/SSOCallback.jsx b/frontend/src/pages/SSOCallback.jsx index a1fd9c66..f2d6512d 100644 --- a/frontend/src/pages/SSOCallback.jsx +++ b/frontend/src/pages/SSOCallback.jsx @@ -2,6 +2,7 @@ import { useEffect, useState } from 'react'; import { useParams, useNavigate, useSearchParams, Link } from 'react-router-dom'; import { useAuth } from '../contexts/AuthContext'; import api from '../services/api'; +import { consumeRedirect } from '../utils/redirectAfterLogin'; import { Loader } from 'lucide-react'; import { Button } from '@/components/ui/button'; @@ -41,7 +42,9 @@ const SSOCallback = () => { } setUser(response.user); - navigate('/'); + // sessionStorage survived the round trip to the identity provider; + // react-router state would not have. + navigate(consumeRedirect()); } catch (err) { setError(err.message || 'SSO authentication failed'); } diff --git a/frontend/src/utils/__tests__/redirectAfterLogin.test.mjs b/frontend/src/utils/__tests__/redirectAfterLogin.test.mjs new file mode 100644 index 00000000..41e5e72c Binary files /dev/null and b/frontend/src/utils/__tests__/redirectAfterLogin.test.mjs differ diff --git a/frontend/src/utils/redirectAfterLogin.js b/frontend/src/utils/redirectAfterLogin.js new file mode 100644 index 00000000..79e7910a --- /dev/null +++ b/frontend/src/utils/redirectAfterLogin.js @@ -0,0 +1,116 @@ +// Remembers where someone was headed when auth bounced them to /login, so +// they land there instead of on the dashboard. +// +// This exists because deep links into the panel are now a real entry point: +// serverkit.ai install links (/extensions?install=, +// /templates?install=) arrive from README badges and are, by definition, +// clicked by people who may not have an open session. Dropping the query +// string on the way through login made every one of those links land on a +// bare dashboard with no explanation. +// +// sessionStorage rather than react-router's location.state: the SSO flow +// leaves the origin entirely for the identity provider and comes back through +// /login/callback/, and router state cannot survive that. One +// mechanism that covers every path beats two that each cover half. + +const STORAGE_KEY = 'serverkit.redirectAfterLogin'; + +// How long a parked destination stays good. Long enough to cover a password +// manager, a TOTP prompt, or an SSO round trip that includes signing up at the +// provider; short enough that a destination abandoned earlier in the tab +// session does not resurface on an unrelated login and read as a glitch. +const MAX_AGE_MS = 30 * 60 * 1000; + +// A path far longer than any real panel route is not a destination. +const MAX_PATH_LENGTH = 2048; + +// Landing back on one of these after login is either a loop or nonsense. +const AUTH_PATH_PREFIXES = ['/login', '/register', '/setup', '/migrate', '/logout']; + +/** + * Validate a stored destination before we navigate to it. + * + * Returns the path, or null when it is unusable. Same-origin is enforced by + * shape: a destination must be one absolute path and nothing else. `//evil.com` + * and `/\evil.com` are the two that matter — browsers read both as + * protocol-relative URLs, so either would turn login into an open redirect. + */ +export function sanitizeRedirect(path) { + if (typeof path !== 'string' || !path) return null; + if (path.length > MAX_PATH_LENGTH) return null; + if (path[0] !== '/') return null; + if (path[1] === '/' || path[1] === '\\') return null; + // Control characters (newline, tab, NUL) can smuggle a value past the + // shape checks above once something downstream re-parses it. + // eslint-disable-next-line no-control-regex + if (/[\u0000-\u001f\u007f]/.test(path)) return null; + + const pathname = path.split(/[?#]/)[0]; + + // Dot segments, plain or percent-encoded, in any position. They collapse: + // `/..//evil.com` normalizes to `//evil.com`, which is only same-origin + // while it is resolved against a base — hand that same string to + // `window.location.href` and it leaves the site. Nothing in this app needs + // a dot segment, so refuse the gadget rather than reason about every + // consumer of the value. + if (pathname.split('/').some((segment) => segment === '.' || segment === '..')) return null; + if (/%2e/i.test(pathname)) return null; + + // Prefix match with a "/" boundary, case-insensitive: catches /login/ and + // /login/callback/ (which would re-run the SSO callback with no + // code and show an error), while leaving /logins and /login-help valid. + const lowered = pathname.toLowerCase(); + if (AUTH_PATH_PREFIXES.some((p) => lowered === p || lowered.startsWith(`${p}/`))) { + return null; + } + + return path; +} + +/** Store where the visitor was going. Call this before redirecting to /login. */ +export function rememberRedirect(location) { + if (!location) return; + const path = sanitizeRedirect( + `${location.pathname || ''}${location.search || ''}${location.hash || ''}`, + ); + if (!path || path === '/') return; // the dashboard is already the default + try { + window.sessionStorage.setItem(STORAGE_KEY, JSON.stringify({ path, at: Date.now() })); + } catch { + // Storage unavailable — fall back to the dashboard, as before. + } +} + +/** + * Read and clear the stored destination, falling back to the dashboard. + * Re-validates on the way out: the value is same-origin sessionStorage, but a + * post-login navigation is not the place to trust that assumption. + */ +export function consumeRedirect() { + let raw = null; + try { + raw = window.sessionStorage.getItem(STORAGE_KEY); + window.sessionStorage.removeItem(STORAGE_KEY); + } catch { + return '/'; + } + if (!raw) return '/'; + + let stored; + try { + stored = JSON.parse(raw); + } catch { + return '/'; + } + if (!stored || typeof stored !== 'object') return '/'; + + // A missing, non-numeric, or future timestamp is treated as expired rather + // than trusted: rememberRedirect is the only writer, so anything else is + // not a value this module put there. + const at = Number(stored.at); + if (!Number.isFinite(at) || at > Date.now() || Date.now() - at > MAX_AGE_MS) { + return '/'; + } + + return sanitizeRedirect(stored.path) || '/'; +}