mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-19 05:26:47 +00:00
fix(settings): make the self-hosted routing engine an instance setting
The field was rendered in every non-managed user's own Map settings and written to their personal settings row. The CSP is built from something else: bootstrap reads the ADMIN default and puts that origin into `connect-src`. So filling the field in was the one path the product offered, and it could not work — the route calculator happily built its URLs against the typed origin and the browser refused every one of them with a bare TypeError, not a status, so no fallback and no retry could see it. Day-plan routes, transport legs, the mobile day route, Studio's road route and the whole road-trip rail stopped routing, app-wide, with a generic "route could not be calculated". Nothing in the UI could repair it except emptying the field again, and there was no admin field to set the value that would have made the origin allowed. The repo has decided this shape once already: `llm_base_url` is admin-only because it names where the server sends its own requests and only whoever runs the instance can judge what is reachable. This is the same class for a different reason, so it joins the same guard, which is now named for what it actually covers. Clearing stays open to everyone, so a row written before the rule cleans itself up rather than blocking the save. The field moves to the admin's default-user settings, next to the tile URL and the Carto key, where a value takes effect.
This commit is contained in:
@@ -43,6 +43,7 @@ type Defaults = {
|
||||
blur_booking_codes?: boolean
|
||||
map_tile_url?: string
|
||||
carto_api_key?: string
|
||||
routing_base_url?: string
|
||||
map_provider?: string
|
||||
mapbox_access_token?: string
|
||||
mapbox_style?: string
|
||||
@@ -119,6 +120,7 @@ export default function DefaultUserSettingsTab(): React.ReactElement {
|
||||
const managed = useAuthStore((s) => s.managed)
|
||||
const [mapboxToken, setMapboxToken] = useState('')
|
||||
const [cartoKey, setCartoKey] = useState('')
|
||||
const [routingBase, setRoutingBase] = useState('')
|
||||
const [mapboxStyle, setMapboxStyle] = useState('')
|
||||
|
||||
useEffect(() => {
|
||||
@@ -128,6 +130,7 @@ export default function DefaultUserSettingsTab(): React.ReactElement {
|
||||
setMapTileUrl(normalizeTileUrl(data.map_tile_url || ''))
|
||||
setMapboxToken(data.mapbox_access_token || '')
|
||||
setCartoKey(data.carto_api_key || '')
|
||||
setRoutingBase(data.routing_base_url || '')
|
||||
setMapboxStyle(provider === 'leaflet' ? (data.mapbox_style || '') : styleForProvider(provider, provider === 'maplibre-gl' ? data.maplibre_style : data.mapbox_style))
|
||||
setLoaded(true)
|
||||
}).catch(() => setLoaded(true))
|
||||
@@ -150,6 +153,7 @@ export default function DefaultUserSettingsTab(): React.ReactElement {
|
||||
if (key === 'map_tile_url') setMapTileUrl('')
|
||||
if (key === 'mapbox_access_token') setMapboxToken('')
|
||||
if (key === 'carto_api_key') setCartoKey('')
|
||||
if (key === 'routing_base_url') setRoutingBase('')
|
||||
if (key === 'mapbox_style' || key === 'maplibre_style') {
|
||||
const provider = normalizeProvider(defaults.map_provider)
|
||||
setMapboxStyle(provider === 'leaflet' ? '' : defaultStyleForProvider(provider))
|
||||
@@ -365,6 +369,32 @@ export default function DefaultUserSettingsTab(): React.ReactElement {
|
||||
<p className="text-xs mt-1 text-content-faint">{t('admin.defaultSettings.cartoKeyHint')}</p>
|
||||
</div>
|
||||
)}
|
||||
{/* Instance configuration, not a taste, which is why it sits here rather
|
||||
than in a user's own Map settings. The origin has to appear in the CSP
|
||||
`connect-src` the server emits at boot, and that list is built from
|
||||
this default — a value on a personal settings row is read by the route
|
||||
calculator and then refused by the browser, so the whole app stops
|
||||
routing with no error it could report. The write route refuses one
|
||||
from a non-admin for the same reason. */}
|
||||
{!managed && (
|
||||
<div style={{ marginTop: 14 }}>
|
||||
<label className="block text-sm font-medium mb-1.5 text-content-secondary">
|
||||
{t('settings.routingBase')}
|
||||
<ResetButton field="routing_base_url" />
|
||||
</label>
|
||||
<input
|
||||
type="text"
|
||||
value={routingBase}
|
||||
onChange={(e: React.ChangeEvent<HTMLInputElement>) => setRoutingBase(e.target.value)}
|
||||
onBlur={() => save({ routing_base_url: routingBase.trim() })}
|
||||
placeholder="https://osrm.example.org"
|
||||
spellCheck={false}
|
||||
autoComplete="off"
|
||||
className="w-full px-3 py-2 border border-slate-300 rounded-lg text-sm focus:ring-2 focus:ring-slate-400 focus:border-transparent"
|
||||
/>
|
||||
<p className="text-xs mt-1 text-content-faint">{t('settings.routingBaseHint')}</p>
|
||||
</div>
|
||||
)}
|
||||
<div style={{ position: 'relative', height: '200px', width: '100%', marginTop: 12 }}>
|
||||
{/* eslint-disable-next-line @typescript-eslint/no-explicit-any */}
|
||||
{React.createElement(MapView as any, {
|
||||
|
||||
@@ -169,7 +169,6 @@ export default function MapSettingsTab(): React.ReactElement {
|
||||
const [saving, setSaving] = useState(false)
|
||||
const [provider, setProvider] = useState<Provider>(initialProvider)
|
||||
const [mapTileUrl, setMapTileUrl] = useState<string>(settings.map_tile_url || '')
|
||||
const [routingBaseUrl, setRoutingBaseUrl] = useState<string>(settings.routing_base_url || '')
|
||||
const managed = useAuthStore((s) => s.managed)
|
||||
const [mapboxToken, setMapboxToken] = useState<string>(settings.mapbox_access_token || '')
|
||||
const [cartoKey, setCartoKey] = useState<string>(settings.carto_api_key || '')
|
||||
@@ -183,7 +182,6 @@ export default function MapSettingsTab(): React.ReactElement {
|
||||
const nextProvider = normalizeProvider(settings.map_provider)
|
||||
setProvider(nextProvider)
|
||||
setMapTileUrl(settings.map_tile_url || '')
|
||||
setRoutingBaseUrl(settings.routing_base_url || '')
|
||||
setMapboxToken(settings.mapbox_access_token || '')
|
||||
setCartoKey(settings.carto_api_key || '')
|
||||
setMapboxStyle(styleForProvider(nextProvider, slotStyle(nextProvider, settings)))
|
||||
@@ -219,7 +217,6 @@ export default function MapSettingsTab(): React.ReactElement {
|
||||
await updateSettings({
|
||||
map_provider: provider,
|
||||
map_tile_url: mapTileUrl,
|
||||
routing_base_url: routingBaseUrl.trim(),
|
||||
mapbox_access_token: mapboxToken,
|
||||
carto_api_key: cartoKey,
|
||||
...stylePatch,
|
||||
@@ -336,23 +333,6 @@ export default function MapSettingsTab(): React.ReactElement {
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Routing is the same engine whichever renderer draws the map, so this sits outside
|
||||
the provider blocks. Tokens rather than the slate-* around it: theme:lint holds
|
||||
new markup to them, and a field added today should not need converting later. */}
|
||||
{!managed && (
|
||||
<div>
|
||||
<label className="mb-1.5 block text-body font-medium text-content">{t('settings.routingBase')}</label>
|
||||
<input
|
||||
type="text"
|
||||
value={routingBaseUrl}
|
||||
onChange={(e: React.ChangeEvent<HTMLInputElement>) => setRoutingBaseUrl(e.target.value)}
|
||||
placeholder="https://osrm.example.org"
|
||||
className="w-full rounded-lg border border-edge bg-surface-input px-3 py-2 text-body text-content placeholder:text-content-faint focus:border-accent focus:outline-none"
|
||||
/>
|
||||
<p className="mt-1 text-caption text-content-faint">{t('settings.routingBaseHint')}</p>
|
||||
</div>
|
||||
)}
|
||||
|
||||
{/* Same deal as the Mapbox token: a managed install brings its own key. */}
|
||||
{provider === 'leaflet' && !managed && (
|
||||
<div>
|
||||
|
||||
@@ -2,7 +2,7 @@ import { Body, Controller, Get, HttpCode, HttpException, Post, Put, Req, UseGuar
|
||||
import type { Request } from 'express';
|
||||
import { MASKED_SETTING_VALUE } from '@trek/shared';
|
||||
import type { User } from '../../types';
|
||||
import { SettingsService, isAdminOnlyLlmSetting } from './settings.service';
|
||||
import { SettingsService, isAdminOnlyEndpointSetting } from './settings.service';
|
||||
import { SettingUpsertDto, SettingsBulkDto } from './settings.dto';
|
||||
import { AdminDefaultUserSettingsDto } from '../admin/admin.dto';
|
||||
import { JwtAuthGuard } from '../auth/jwt-auth.guard';
|
||||
@@ -46,9 +46,9 @@ export class SettingsController {
|
||||
* behind AdminGuard. A personal row would be a second, invisible place for
|
||||
* the same value.
|
||||
*/
|
||||
private assertMayWriteLlmEndpoint(settings: Record<string, unknown>) {
|
||||
private assertMayWriteInstanceEndpoint(settings: Record<string, unknown>) {
|
||||
for (const [key, value] of Object.entries(settings)) {
|
||||
if (isAdminOnlyLlmSetting(key, value)) {
|
||||
if (isAdminOnlyEndpointSetting(key, value)) {
|
||||
throw new HttpException({ error: 'Admin access required' }, 403);
|
||||
}
|
||||
}
|
||||
@@ -61,8 +61,8 @@ export class SettingsController {
|
||||
|
||||
@Put()
|
||||
upsert(@CurrentUser() user: User, @Body() body: SettingUpsertDto) {
|
||||
this.assertMayWriteLlmEndpoint({ [body.key]: body.value });
|
||||
// assertMayWriteLlmEndpoint only covers llm_base_url and provider 'local'.
|
||||
this.assertMayWriteInstanceEndpoint({ [body.key]: body.value });
|
||||
// assertMayWriteInstanceEndpoint only covers llm_base_url and provider 'local'.
|
||||
// llm_api_key and llm_model are writable by every user, and on a managed
|
||||
// install both cost the operator money, so the key list decides here.
|
||||
if (isManagedLockedKey(body.key) && this.env.isManaged()) {
|
||||
@@ -79,7 +79,7 @@ export class SettingsController {
|
||||
@Post('bulk')
|
||||
@HttpCode(200) // Express answers bulk with res.json (200), not the POST-default 201.
|
||||
bulk(@CurrentUser() user: User, @Body() body: SettingsBulkDto) {
|
||||
this.assertMayWriteLlmEndpoint(body.settings);
|
||||
this.assertMayWriteInstanceEndpoint(body.settings);
|
||||
const { allowed, blocked } = splitManagedKeys(body.settings, this.env.isManaged());
|
||||
try {
|
||||
const updated = this.settings.bulkUpsertSettings(user.id, allowed);
|
||||
|
||||
@@ -83,8 +83,9 @@ const KEY_LIST = DISPLAY_PREFERENCE_KEYS.join(', ');
|
||||
* write carries the demo gate the other write tools carry.
|
||||
*
|
||||
* The controller's own two refusals stay out of reach by construction rather
|
||||
* than by being repeated: assertMayWriteLlmEndpoint guards llm_base_url and
|
||||
* llm_provider, and isManagedLockedKey guards the operator-owned keys, and not
|
||||
* than by being repeated: assertMayWriteInstanceEndpoint guards llm_base_url,
|
||||
* llm_provider and routing_base_url, and isManagedLockedKey guards the
|
||||
* operator-owned keys, and not
|
||||
* one name from either list is on the allow-list above. tools-settings.test.ts
|
||||
* asserts that intersection is empty instead of trusting the reading.
|
||||
*/
|
||||
|
||||
@@ -79,18 +79,30 @@ const VALID_VALUES: Partial<Record<DefaultableKey, unknown[]>> = {
|
||||
const BOOLEAN_KEYS = new Set<DefaultableKey>(['blur_booking_codes', 'mapbox_3d_enabled', 'mapbox_quality_mode', 'llm_multimodal']);
|
||||
|
||||
/**
|
||||
* #1772: per-user LLM settings a non-admin must not write. Both of them pick
|
||||
* the address this server sends its own LLM requests to (provider 'local' means
|
||||
* nothing but "an endpoint I name"), and the SSRF guard in front of those
|
||||
* requests allows loopback/LAN on purpose so a self-hosted Ollama works. Only
|
||||
* whoever runs the instance can judge what is reachable from it.
|
||||
* Per-user settings that name an address, which a non-admin must not write.
|
||||
*
|
||||
* Clearing stays open to everyone: both Settings sections always send
|
||||
* `llm_base_url: ''` while the field is hidden, so a row written before this
|
||||
* rule cleans itself up on the next save instead of blocking it.
|
||||
* #1772 for the LLM pair: both pick where this server sends its own LLM
|
||||
* requests (provider 'local' means nothing but "an endpoint I name"), and the
|
||||
* SSRF guard in front of those requests allows loopback and LAN on purpose so a
|
||||
* self-hosted Ollama works. Only whoever runs the instance can judge what is
|
||||
* reachable from it.
|
||||
*
|
||||
* `routing_base_url` (#1797) is the same class for a different reason: it names
|
||||
* an origin the BROWSER connects to, so it has to appear in the CSP
|
||||
* `connect-src` the server emits at boot — and that list is built from the admin
|
||||
* default, which is the only place a value can take effect. A row on a personal
|
||||
* settings record is read by the route calculator and blocked by the browser, so
|
||||
* the whole app stops routing with no catchable error. Refusing it here means
|
||||
* the caller finds out instead of silently breaking their own map.
|
||||
*
|
||||
* Clearing stays open to everyone: the Settings sections send an empty string
|
||||
* while the field is hidden, so a row written before this rule cleans itself up
|
||||
* on the next save instead of blocking it.
|
||||
*/
|
||||
export function isAdminOnlyLlmSetting(key: string, value: unknown): boolean {
|
||||
if (key === 'llm_base_url') return typeof value === 'string' && value.trim() !== '';
|
||||
export function isAdminOnlyEndpointSetting(key: string, value: unknown): boolean {
|
||||
if (key === 'llm_base_url' || key === 'routing_base_url') {
|
||||
return typeof value === 'string' && value.trim() !== '';
|
||||
}
|
||||
if (key === 'llm_provider') return value === 'local';
|
||||
return false;
|
||||
}
|
||||
|
||||
@@ -150,6 +150,31 @@ describe('Settings e2e (real auth guard + temp SQLite)', () => {
|
||||
.toEqual({ value: 'anthropic' });
|
||||
});
|
||||
|
||||
it('PUT 403 for a non-admin naming a routing engine (#1797)', async () => {
|
||||
// Same class for a different reason: this origin has to appear in the CSP
|
||||
// connect-src the server emits at boot, and that list is built from the
|
||||
// admin default. A value on a personal row is read by the route
|
||||
// calculator and then refused by the browser, so the app stops routing
|
||||
// with no error it could report. Refusing tells the caller instead.
|
||||
const res = await request(server).put('/api/settings').set('Cookie', sessionCookie(1))
|
||||
.send({ key: 'routing_base_url', value: 'https://osrm.example.org' });
|
||||
expect(res.status).toBe(403);
|
||||
expect(res.body).toEqual({ error: 'Admin access required' });
|
||||
expect(countRows()).toBe(0);
|
||||
});
|
||||
|
||||
it('PUT lets a non-admin clear a routing engine somebody set before the rule', async () => {
|
||||
expect((await request(server).put('/api/settings').set('Cookie', sessionCookie(1))
|
||||
.send({ key: 'routing_base_url', value: '' })).status).toBe(200);
|
||||
});
|
||||
|
||||
it('POST bulk refuses a routing engine from a non-admin', async () => {
|
||||
const res = await request(server).post('/api/settings/bulk').set('Cookie', sessionCookie(1))
|
||||
.send({ settings: { routing_base_url: 'https://osrm.example.org' } });
|
||||
expect(res.status).toBe(403);
|
||||
expect(countRows()).toBe(0);
|
||||
});
|
||||
|
||||
it('PUT is unaffected for a non-LLM key with the same value', async () => {
|
||||
const res = await request(server).put('/api/settings').set('Cookie', sessionCookie(1))
|
||||
.send({ key: 'start_page', value: 'local' });
|
||||
|
||||
@@ -48,7 +48,7 @@ import { encrypt_api_key } from '../../../src/nest/common/crypto/apiKeyCrypto';
|
||||
import { MASKED_SETTING_VALUE } from '@trek/shared';
|
||||
import { DISPLAY_PREFERENCE_KEYS } from '../../../src/nest/settings/settings.mcp';
|
||||
import { MANAGED_LOCKED_SETTING_KEYS } from '../../../src/nest/common/managed';
|
||||
import { isAdminOnlyLlmSetting, ENCRYPTED_SETTING_KEYS, MASKED_SETTING_KEYS } from '../../../src/nest/settings/settings.service';
|
||||
import { isAdminOnlyEndpointSetting, ENCRYPTED_SETTING_KEYS, MASKED_SETTING_KEYS } from '../../../src/nest/settings/settings.service';
|
||||
|
||||
beforeAll(() => {
|
||||
createTables(testDb);
|
||||
@@ -493,11 +493,11 @@ describe('Display-preference allow-list', () => {
|
||||
expect(overlap).toEqual([]);
|
||||
});
|
||||
|
||||
it('holds no key assertMayWriteLlmEndpoint would have to refuse', () => {
|
||||
// isAdminOnlyLlmSetting is value-dependent, so probe it with the values
|
||||
it('holds no key assertMayWriteInstanceEndpoint would have to refuse', () => {
|
||||
// isAdminOnlyEndpointSetting is value-dependent, so probe it with the values
|
||||
// that trip it rather than matching on the key name.
|
||||
const offenders = DISPLAY_PREFERENCE_KEYS.filter((key) =>
|
||||
isAdminOnlyLlmSetting(key, 'http://127.0.0.1:11434') || isAdminOnlyLlmSetting(key, 'local'));
|
||||
isAdminOnlyEndpointSetting(key, 'http://127.0.0.1:11434') || isAdminOnlyEndpointSetting(key, 'local'));
|
||||
expect(offenders).toEqual([]);
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user