Fix #298: move Sentinel credentials from browser storage to backend .env

Reported by @tg12. Pre-fix, the Settings panel stored real third-party
Copernicus CDSE client_id + client_secret in browser localStorage /
sessionStorage via the privacy storage helper, and the proxy routes
required those values to come back in every tile/token request body.
Any same-origin script (XSS, malicious browser extension, dev-tools
HAR export) had read access to the credentials.

This change moves them server-side, behind the same .env-backed admin
flow every other third-party API key (OpenSky, AIS Stream, Finnhub,
Shodan, …) already uses.

Backend
-------
backend/services/api_settings.py
  * Added SENTINEL_CLIENT_ID and SENTINEL_CLIENT_SECRET entries to
    API_REGISTRY. The existing GET/PUT /api/settings/api-keys flow
    (already require_local_operator-gated, .env-backed) now manages
    them — no new route surface.

backend/routers/tools.py
  * /api/sentinel/token and /api/sentinel/tile resolve credentials via
    a new _resolve_sentinel_credentials() helper: body fields win for
    back-compat with any legacy callers, otherwise the helper reads
    SENTINEL_CLIENT_ID / SENTINEL_CLIENT_SECRET from os.environ.
  * When neither source has a value, the route returns 400 with a
    friendly pointer ("Set SENTINEL_CLIENT_ID and SENTINEL_CLIENT_SECRET
    in the API Keys panel") instead of the curt "required" message.
    The user's standing rule against hostile errors applies.
  * Function bodies only — decorator lines untouched, so this PR does
    not conflict with #303 (which adds Depends(require_local_operator)
    to the same routes).

Frontend
--------
frontend/src/lib/sentinelHub.ts — rewritten
  * Removed: getSentinelCredentials / setSentinelCredentials /
    clearSentinelCredentials / getSentinelCredentialStorageMode.
    These were the browser-storage read/write helpers; their existence
    was the bug.
  * Added: checkBackendSentinelStatus(), refreshSentinelStatus(),
    getCachedSentinelStatus(), and a kept-for-back-compat
    hasSentinelCredentials() shim. Status is sourced from
    /api/settings/api-keys (the same endpoint the API Keys panel
    already uses), so we don't add a new route just for this read.
  * Added: migrateLegacySentinelBrowserKeys() — one-shot, idempotent
    helper that clears sb_sentinel_client_id / _secret / _instance_id
    from BOTH localStorage and sessionStorage. We deliberately do NOT
    auto-POST those legacy browser values to the backend; doing so
    would silently migrate a secret across a trust boundary without
    operator consent. Operators re-enter once in the API Keys panel
    and the legacy keys get wiped here.
  * fetchSentinelTile and getSentinelToken no longer send client_id /
    client_secret in the request body. The backend uses .env.

frontend/src/components/SettingsPanel.tsx
  * Dropped sb_sentinel_client_id / _secret / _instance_id from
    PRIVACY_SENSITIVE_BROWSER_KEYS — they're no longer written.
  * SentinelTab rewritten: removed the inline Client ID / Client Secret
    inputs + Save / Clear / Test buttons. Replaced with a status panel
    that calls checkBackendSentinelStatus() on mount, a one-click
    "Open API Keys Panel" button, and a migration banner that appears
    only when migrateLegacySentinelBrowserKeys() actually cleared
    something.
  * Setup guide STEP 3 now points to the API Keys panel instead of
    the local form.

frontend/src/app/page.tsx
  * Added a one-time useEffect that fires checkBackendSentinelStatus()
    on mount so the cached value (which the synchronous
    hasSentinelCredentials() shim reads) is populated before
    MaplibreViewer's tile-URL memo runs.

Tests
-----
backend/tests/test_sentinel_credentials_server_side.py (new)
  * API_REGISTRY surface — sentinel_client_id / sentinel_client_secret
    are registered with the right env_keys, ALLOWED_ENV_KEYS lets
    /api/settings/api-keys PUT them.
  * Resolution order — body wins, env is fallback, neither → 400 with
    the friendly pointer message, and NO upstream HTTP call when
    neither source has credentials (asserted via
    MagicMock(side_effect=AssertionError)).
  * /api/sentinel/tile same shape.

frontend/src/__tests__/utils/sentinelHub.test.ts (new)
  * migrateLegacySentinelBrowserKeys clears localStorage AND
    sessionStorage, reports what it cleared, idempotent.
  * fetchSentinelTile + getSentinelToken POST WITHOUT client_id /
    client_secret in the body (plants leaked credentials in browser
    storage first to prove they are NOT picked up).
  * checkBackendSentinelStatus parses /api/settings/api-keys correctly:
    true only when both keys is_set, false on partial config or
    network errors.

All 7 backend tests + 8 frontend tests pass locally. The
test_no_new_duplicate_routes guard and the api-settings test suite
still pass.

Credit: @tg12 for the audit report.
This commit is contained in:
BigBodyCobain
2026-05-22 10:44:50 -06:00
parent 19fb7f0b1e
commit b041b5e97c
7 changed files with 767 additions and 215 deletions
@@ -0,0 +1,169 @@
/**
* Issue #298 (tg12): Sentinel credentials must no longer live in browser
* storage, and the proxy calls must not forward them in request bodies.
* These tests pin both invariants on ``lib/sentinelHub``:
*
* 1. ``migrateLegacySentinelBrowserKeys()`` clears the legacy keys
* idempotently and reports what it cleared.
* 2. ``fetchSentinelTile()`` and ``getSentinelToken()`` POST WITHOUT
* ``client_id`` or ``client_secret`` in the body — the backend
* resolves credentials from its ``.env``. A future refactor that
* accidentally re-introduces browser-storage reads (e.g. by
* restoring ``getSentinelCredentials()`` and forwarding it) gets a
* loud test failure here rather than a silent privacy regression.
* 3. ``checkBackendSentinelStatus()`` queries ``/api/settings/api-keys``
* and returns true only when both Sentinel keys report ``is_set``.
*/
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest';
import {
migrateLegacySentinelBrowserKeys,
fetchSentinelTile,
getSentinelToken,
checkBackendSentinelStatus,
refreshSentinelStatus,
} from '@/lib/sentinelHub';
const originalFetch = globalThis.fetch;
describe('lib/sentinelHub — issue #298 server-side credentials', () => {
beforeEach(() => {
window.localStorage.clear();
window.sessionStorage.clear();
refreshSentinelStatus();
});
afterEach(() => {
globalThis.fetch = originalFetch;
window.localStorage.clear();
window.sessionStorage.clear();
refreshSentinelStatus();
});
describe('migrateLegacySentinelBrowserKeys', () => {
it('clears legacy localStorage keys and reports what it cleared', () => {
window.localStorage.setItem('sb_sentinel_client_id', 'sh-leaked-id');
window.localStorage.setItem('sb_sentinel_client_secret', 'leaked-secret');
window.localStorage.setItem('sb_sentinel_instance_id', 'leaked-instance');
const result = migrateLegacySentinelBrowserKeys();
expect(window.localStorage.getItem('sb_sentinel_client_id')).toBeNull();
expect(window.localStorage.getItem('sb_sentinel_client_secret')).toBeNull();
expect(window.localStorage.getItem('sb_sentinel_instance_id')).toBeNull();
expect(result.cleared.sort()).toEqual([
'sb_sentinel_client_id',
'sb_sentinel_client_secret',
'sb_sentinel_instance_id',
].sort());
});
it('clears sessionStorage too (privacy-strict mode used to put them there)', () => {
window.sessionStorage.setItem('sb_sentinel_client_id', 'sh-session-id');
window.sessionStorage.setItem('sb_sentinel_client_secret', 'session-secret');
const result = migrateLegacySentinelBrowserKeys();
expect(window.sessionStorage.getItem('sb_sentinel_client_id')).toBeNull();
expect(window.sessionStorage.getItem('sb_sentinel_client_secret')).toBeNull();
expect(result.cleared).toContain('sb_sentinel_client_id');
expect(result.cleared).toContain('sb_sentinel_client_secret');
});
it('is idempotent — calling it on a clean store reports nothing cleared', () => {
const result = migrateLegacySentinelBrowserKeys();
expect(result.cleared).toEqual([]);
});
});
describe('proxy requests no longer forward credentials', () => {
it('fetchSentinelTile POSTs without client_id/client_secret in the body', async () => {
// Plant credentials in browser storage to prove they would NOT be
// picked up even if present. Pre-#298, this would have been read
// from localStorage and posted in the body.
window.localStorage.setItem('sb_sentinel_client_id', 'sh-leaked-id');
window.localStorage.setItem('sb_sentinel_client_secret', 'leaked-secret');
const fetchMock = vi.fn(async () => new Response(new ArrayBuffer(0), { status: 200 }));
globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch;
await fetchSentinelTile(6, 30, 20, 'TRUE-COLOR', '2026-01-01');
expect(fetchMock).toHaveBeenCalledTimes(1);
const [, init] = fetchMock.mock.calls[0] as [unknown, RequestInit];
const body = JSON.parse(String(init.body));
expect(body).not.toHaveProperty('client_id');
expect(body).not.toHaveProperty('client_secret');
// Sanity: the legitimate fields are still there.
expect(body).toMatchObject({ preset: 'TRUE-COLOR', date: '2026-01-01', z: 6, x: 30, y: 20 });
});
it('getSentinelToken POSTs with an empty form body (backend uses env)', async () => {
window.localStorage.setItem('sb_sentinel_client_id', 'sh-leaked-id');
window.localStorage.setItem('sb_sentinel_client_secret', 'leaked-secret');
const fetchMock = vi.fn(async () =>
new Response(JSON.stringify({ access_token: 'stub', expires_in: 300 }), { status: 200 }),
);
globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch;
const token = await getSentinelToken();
expect(token).toBe('stub');
expect(fetchMock).toHaveBeenCalledTimes(1);
const [, init] = fetchMock.mock.calls[0] as [unknown, RequestInit];
const body = String(init.body);
// Body is a URLSearchParams stringification. We assert that the
// leaked credential never appears in it.
expect(body).not.toContain('sh-leaked-id');
expect(body).not.toContain('leaked-secret');
});
});
describe('checkBackendSentinelStatus', () => {
it('returns true when both Sentinel keys report is_set on /api/settings/api-keys', async () => {
const fetchMock = vi.fn(async (input: unknown) => {
const url = String(input);
if (url.endsWith('/api/settings/api-keys')) {
return new Response(
JSON.stringify([
{ id: 'sentinel_client_id', env_key: 'SENTINEL_CLIENT_ID', is_set: true },
{ id: 'sentinel_client_secret', env_key: 'SENTINEL_CLIENT_SECRET', is_set: true },
{ id: 'opensky_client_id', env_key: 'OPENSKY_CLIENT_ID', is_set: false },
]),
{ status: 200 },
);
}
return new Response('not found', { status: 404 });
});
globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch;
const configured = await checkBackendSentinelStatus();
expect(configured).toBe(true);
});
it('returns false when only one of the two keys is set', async () => {
const fetchMock = vi.fn(async () =>
new Response(
JSON.stringify([
{ id: 'sentinel_client_id', env_key: 'SENTINEL_CLIENT_ID', is_set: true },
{ id: 'sentinel_client_secret', env_key: 'SENTINEL_CLIENT_SECRET', is_set: false },
]),
{ status: 200 },
),
);
globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch;
const configured = await checkBackendSentinelStatus();
expect(configured).toBe(false);
});
it('fails safely (false) when the backend errors', async () => {
const fetchMock = vi.fn(async () => { throw new Error('network down'); });
globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch;
const configured = await checkBackendSentinelStatus();
expect(configured).toBe(false);
});
});
});