diff --git a/api/caddy_manager.py b/api/caddy_manager.py index 9fcdcd0..9113e90 100644 --- a/api/caddy_manager.py +++ b/api/caddy_manager.py @@ -634,7 +634,16 @@ class CaddyManager(BaseServiceManager): else: caddy_host = os.environ.get('CADDY_CERT_HOST', 'cell-caddy') caddy_port = int(os.environ.get('CADDY_HTTPS_PORT', '443')) - result = self._check_cert_via_ssl(caddy_host, caddy_port) + # Use the effective domain as TLS SNI so Caddy serves the right + # certificate. Without this, Caddy receives SNI='cell-caddy' which + # matches no cert and the handshake returns nothing. + sni = None + if self.config_manager: + try: + sni = self.config_manager.get_effective_domain() + except Exception: + pass + result = self._check_cert_via_ssl(caddy_host, caddy_port, sni=sni) status = result if result is not None else { 'status': 'unknown', 'expiry': None, 'days_remaining': None } @@ -649,14 +658,18 @@ class CaddyManager(BaseServiceManager): return status @staticmethod - def _check_cert_via_ssl(hostname: str, port: int = 443) -> Optional[Dict[str, Any]]: - """Open an SSL connection and return cert expiry info, or None on failure.""" + def _check_cert_via_ssl(hostname: str, port: int = 443, sni: str = None) -> Optional[Dict[str, Any]]: + """Open an SSL connection and return cert expiry info, or None on failure. + + Connect to hostname:port but present sni (if given) as the TLS server + name so Caddy returns the right certificate for the public domain. + """ ctx = _ssl.create_default_context() ctx.check_hostname = False ctx.verify_mode = _ssl.CERT_NONE try: with _socket.create_connection((hostname, port), timeout=5) as raw: - with ctx.wrap_socket(raw, server_hostname=hostname) as tls: + with ctx.wrap_socket(raw, server_hostname=sni or hostname) as tls: der = tls.getpeercert(binary_form=True) if not der: return None diff --git a/api/routes/config.py b/api/routes/config.py index b52fd17..976db66 100644 --- a/api/routes/config.py +++ b/api/routes/config.py @@ -792,6 +792,12 @@ def apply_pending_config(): + (' (network_recreate)' if needs_network_recreate else '') ) else: + # Clear needs_restart immediately so frontend polls don't see stale + # state while the container restart runs in the background thread. + config_manager.configs['_pending_restart']['needs_restart'] = False + config_manager.configs['_pending_restart']['applying'] = True + config_manager._save_all_configs() + def _do_apply(): import time as _time import subprocess as _subprocess @@ -808,7 +814,7 @@ def apply_pending_config(): logger.error(f"docker compose up failed: {result.stderr.strip()}") else: logger.info(f'docker compose up completed for: {containers}') - _clear_pending_restart() + _clear_pending_restart() threading.Thread(target=_do_apply, daemon=False).start() return jsonify({ diff --git a/tests/test_caddy_manager.py b/tests/test_caddy_manager.py index ca0e0fd..6f439d9 100644 --- a/tests/test_caddy_manager.py +++ b/tests/test_caddy_manager.py @@ -412,6 +412,45 @@ class TestRefreshCertStatus(unittest.TestCase): # Should have been persisted to identity mgr.config_manager.set_identity_field.assert_called_with('tls', expected) + def test_refresh_cert_status_uses_effective_domain_as_sni(self): + """refresh_cert_status passes the effective domain as SNI, not the container hostname. + + Without this, Caddy receives SNI='cell-caddy' which matches no certificate + and the SSL handshake returns nothing, leaving cert status as 'unknown'. + """ + mgr = _mgr(identity={'cell_name': 'pic1', 'domain_mode': 'pic_ngo'}) + mgr.config_manager.get_effective_domain.return_value = 'pic1.pic.ngo' + expected = {'status': 'valid', 'expiry': '2026-12-01T00:00:00+00:00', 'days_remaining': 179} + with patch.object(CaddyManager, '_check_cert_via_ssl', return_value=expected) as mock_ssl: + mgr.refresh_cert_status() + # The SNI keyword argument must be the effective domain, not the container name. + call_kwargs = mock_ssl.call_args + sni_passed = call_kwargs.kwargs.get('sni') or ( + call_kwargs.args[2] if len(call_kwargs.args) > 2 else None + ) + self.assertEqual(sni_passed, 'pic1.pic.ngo', + f'Expected SNI=pic1.pic.ngo but got {sni_passed!r}') + + def test_check_cert_via_ssl_passes_sni_to_wrap_socket(self): + """_check_cert_via_ssl uses sni parameter as server_hostname in SSL handshake.""" + der = self._make_der_cert(60) + mock_tls = MagicMock() + mock_tls.__enter__ = MagicMock(return_value=mock_tls) + mock_tls.__exit__ = MagicMock(return_value=False) + mock_tls.getpeercert.return_value = der + mock_raw = MagicMock() + mock_raw.__enter__ = MagicMock(return_value=mock_raw) + mock_raw.__exit__ = MagicMock(return_value=False) + with patch('caddy_manager._socket.create_connection', return_value=mock_raw) as mock_conn: + with patch('caddy_manager._ssl.create_default_context') as mock_ctx: + mock_ctx.return_value.wrap_socket.return_value = mock_tls + CaddyManager._check_cert_via_ssl('cell-caddy', 443, sni='pic1.pic.ngo') + # TCP connects to container hostname, SSL handshake uses the public domain + mock_conn.assert_called_with(('cell-caddy', 443), timeout=5) + mock_ctx.return_value.wrap_socket.assert_called_with( + mock_raw, server_hostname='pic1.pic.ngo' + ) + def test_refresh_cert_status_ssl_failure_returns_unknown(self): """When SSL check returns None, status is 'unknown'.""" mgr = _mgr(identity={'cell_name': 'alpha', 'domain_mode': 'pic_ngo'}) diff --git a/tests/test_config_apply.py b/tests/test_config_apply.py index b551cd6..5e18902 100644 --- a/tests/test_config_apply.py +++ b/tests/test_config_apply.py @@ -179,6 +179,45 @@ class TestConfigApplyRoute(unittest.TestCase): self.assertIn('-d', cmd) self.assertIn('dns', cmd) + # ── Race-condition fix: needs_restart cleared synchronously ──────────── + # For non-'*' container restarts the background thread takes ~300 ms. + # The frontend polls /api/config/pending every 5 s; if needs_restart is + # still True when that poll fires, the banner re-appears after Apply. + # Fix: set needs_restart=False and applying=True before spawning the thread. + + @patch('threading.Thread') + @patch('docker.from_env') + def test_specific_containers_clears_needs_restart_synchronously( + self, mock_docker, mock_thread): + """needs_restart must be False as soon as apply returns, not after thread.""" + mock_docker.side_effect = Exception('no docker in test') + mock_thread.return_value = MagicMock() # thread is mocked — never runs + _set_pending_restart(['cell_name changed to pic2'], ['dns']) + + self.client.post('/api/config/apply') + + pending = config_manager.configs.get('_pending_restart', {}) + self.assertFalse(pending.get('needs_restart', True), + 'needs_restart must be False immediately after apply for non-* restarts') + self.assertTrue(pending.get('applying', False), + 'applying must be True while the background thread runs') + + @patch('threading.Thread') + @patch('docker.from_env') + def test_wildcard_containers_sets_applying_but_not_clears_needs_restart( + self, mock_docker, mock_thread): + """For '*' restarts the helper container clears the flag; API must not.""" + mock_docker.side_effect = Exception('no docker in test') + mock_thread.return_value = MagicMock() + _set_pending_restart(['ip_range changed'], ['*']) + + self.client.post('/api/config/apply') + + pending = config_manager.configs.get('_pending_restart', {}) + # Wildcard restart: API sets applying=True but leaves needs_restart=True + # so the helper container can clear it on success. + self.assertTrue(pending.get('applying', False)) + # ── Exception in route body returns 500 ─────────────────────────────── @patch('app.config_manager') diff --git a/webui/src/App.jsx b/webui/src/App.jsx index be5dffe..1bcc22a 100644 --- a/webui/src/App.jsx +++ b/webui/src/App.jsx @@ -188,18 +188,17 @@ function AppCore() { const [applyStatus, setApplyStatus] = useState(null); // null | 'saving' | 'restarting' | 'done' | 'timeout' | 'error' const [applyError, setApplyError] = useState(''); - const { flushAll, hasDirty, clearAllDirty } = useDraftConfig(); + const { flushAll, hasDirty, isDirty, clearAllDirty } = useDraftConfig(); const handleApply = useCallback(async () => { setApplyError(''); - if (hasDirty()) { - setApplyStatus('saving'); - try { - await flushAll(); - } catch { - // flush errors are shown via Settings toasts; continue with apply - } + setApplyStatus('saving'); + try { + if (hasDirty()) await flushAll(); + } catch { + // flush errors are shown via Settings toasts; continue with apply } + clearAllDirty(); try { await cellAPI.applyPending(); } catch (err) { @@ -228,7 +227,7 @@ function AppCore() { setApplyStatus('timeout'); setApplyError('Containers may still be starting — check docker logs if services are unavailable'); setTimeout(() => setApplyStatus(null), 8000); - }, [flushAll, hasDirty]); + }, [flushAll, hasDirty, clearAllDirty]); const handleCancel = useCallback(async () => { clearAllDirty(); @@ -328,7 +327,7 @@ function AppCore() { )} - {isOnline && (pending.needs_restart || hasDirty()) && !applyStatus && ( + {isOnline && (pending.needs_restart || isDirty) && !applyStatus && ( )} diff --git a/webui/src/__tests__/Settings.IdentitySave.test.jsx b/webui/src/__tests__/Settings.IdentitySave.test.jsx index 34dea07..176fad7 100644 --- a/webui/src/__tests__/Settings.IdentitySave.test.jsx +++ b/webui/src/__tests__/Settings.IdentitySave.test.jsx @@ -51,7 +51,9 @@ vi.mock('../contexts/DraftConfigContext', () => ({ registerFlusher: vi.fn(() => vi.fn()), setDirty: (...a) => mockSetDirty(...a), hasDirty: vi.fn(() => false), + isDirty: false, flushAll: vi.fn(), + clearAllDirty: vi.fn(), }), })); @@ -230,3 +232,53 @@ describe('Cell Identity — Accept path (saveIdentity called by flusher)', () => expect(mockUpdateConfig.mock.calls[0][0]).toMatchObject({ cell_name: 'pic2' }); }); }); + +describe('Cell Identity — DraftConfig dirty state is set synchronously on change', () => { + /** + * Verifies that draftConfig.setDirty is called in the same synchronous + * event handler as the identity state change, not in a deferred effect. + * This is what allows the banner to appear immediately (the isDirty reactive + * state in DraftConfigContext re-renders App.jsx without waiting for a poll). + */ + beforeEach(() => { vi.useFakeTimers({ shouldAdvanceTime: true }); vi.clearAllMocks(); defaultMocks(); }); + afterEach(async () => { await act(async () => { vi.runAllTimers(); }); vi.useRealTimers(); vi.resetModules(); }); + + it('setDirty("identity", true) is called before any timer fires after cell_name change', async () => { + await renderSettings(); + mockSetDirty.mockClear(); + + fireEvent.change(screen.getByDisplayValue('pic1'), { target: { value: 'pic2' } }); + + // setDirty must be called synchronously within the event, not in a timer + expect(mockSetDirty).toHaveBeenCalledWith('identity', true); + expect(mockSetDirty).toHaveBeenCalledTimes(1); + }); + + it('setDirty("identity", true) is called before any timer fires after ip_range change', async () => { + await renderSettings(); + mockSetDirty.mockClear(); + + fireEvent.change(screen.getByDisplayValue('172.20.0.0/16'), { target: { value: '10.0.0.0/8' } }); + + expect(mockSetDirty).toHaveBeenCalledWith('identity', true); + expect(mockSetDirty).toHaveBeenCalledTimes(1); + }); + + it('setDirty("identity", false) is called after saveIdentity completes', async () => { + mockGetConfig + .mockResolvedValueOnce({ data: makeCfg() }) + .mockResolvedValue({ data: makeCfg({ ip_range: '10.0.0.0/8' }) }); + await renderSettings(); + mockSetDirty.mockClear(); + + fireEvent.change(screen.getByDisplayValue('172.20.0.0/16'), { target: { value: '10.0.0.0/8' } }); + + // auto-save fires after 800 ms + await act(async () => { vi.advanceTimersByTime(900); }); + await act(async () => { await Promise.resolve(); }); + + const calls = mockSetDirty.mock.calls; + expect(calls.some(([k, v]) => k === 'identity' && v === true)).toBe(true); + expect(calls.some(([k, v]) => k === 'identity' && v === false)).toBe(true); + }); +}); diff --git a/webui/src/contexts/DraftConfigContext.jsx b/webui/src/contexts/DraftConfigContext.jsx index e2f1e10..459cb1f 100644 --- a/webui/src/contexts/DraftConfigContext.jsx +++ b/webui/src/contexts/DraftConfigContext.jsx @@ -1,4 +1,4 @@ -import { createContext, useContext, useRef, useCallback } from 'react'; +import { createContext, useContext, useRef, useCallback, useState } from 'react'; const DraftConfigContext = createContext(null); @@ -11,9 +11,12 @@ export function DraftConfigProvider({ children }) { }, []); const hasDirtyRef = useRef({}); // key → boolean + // isDirty is a reactive mirror of hasDirtyRef so consumers re-render immediately. + const [isDirty, setIsDirty] = useState(false); - const setDirty = useCallback((key, isDirty) => { - hasDirtyRef.current[key] = isDirty; + const setDirty = useCallback((key, dirty) => { + hasDirtyRef.current[key] = dirty; + setIsDirty(Object.values(hasDirtyRef.current).some(Boolean)); }, []); const hasDirty = useCallback(() => { @@ -27,10 +30,11 @@ export function DraftConfigProvider({ children }) { const clearAllDirty = useCallback(() => { hasDirtyRef.current = {}; + setIsDirty(false); }, []); return ( - + {children} );