mirror of
https://github.com/mauriceboe/TREK.git
synced 2026-10-21 14:36:31 +00:00
fix(client): correct the second pass over this branch's own client fixes
Four of the fixes already on this branch were wrong, and an adversarial read of the diff is what turned them up. The shared merge helper claimed to implement the server's COALESCE(assignment_time, place_time) and did not: it pinned the embedded times to whatever the assignment already showed, ignoring the override column entirely. Sharing that with the WS applier traded one bug for another - a collaborator moving a place from 08:00 to 10:00 stopped reaching the day card for every assignment WITHOUT an override, which is most of them. It merges the way the server projects now, and the test that was supposed to pin this seeded a fixture the server could never produce (embedded times that differ from the pool row with no override to explain the difference). The per-iteration auth gate went into every loop in tripSyncManager except the one that matters most: the mutation-queue replay, which writes server responses straight into Dexie. After a logout that is the shared anonymous database. The fire-and-forget tags/categories writes two lines below a gate added in the same commit were ungated for the same reason - they resolve later, and the check has to happen then. safeHttpUrl is backed by placeWebsiteSchema: http(s) only, capped at 500 characters. A booking url is deliberately free-form - shared's own contract says so in as many words, because people paste bare hosts and long provider deep links - so guarding it with the stricter schema turned a link somebody saved last year into dead text. safeExternalHref refuses the schemes that execute in this origin and gives a scheme-less value https://, which is what an address bar does with the same input. And the memories timeout was 20s while the server's own budget for a Synology call is 30s, doubled again when the session has expired and it re-authenticates: a slow NAS that answered before this branch would have started reading as disconnected. Those calls had no client timeout at all before; they have none again, and the ones that need to give up early carry an AbortSignal. The notes thumbnail also loses its NUL byte cache separator - it made git treat the whole file as binary, so a security fix landed in it unreviewable - and gains the same cache cap its chat sibling got.
This commit is contained in:
@@ -971,9 +971,14 @@ export const journeyApi = {
|
||||
|
||||
// Photo providers (Immich, Synology Photos, …) behind /api/integrations/memories.
|
||||
// The provider sits on the user's own hardware and the server proxies through to
|
||||
// it, so these get a longer leash than the 8s default — a cold Immich answering a
|
||||
// date-range search regularly needs more.
|
||||
const MEMORIES_TIMEOUT = 20000
|
||||
// it, so the 8s default does not apply — and neither does any number picked
|
||||
// here. The server already holds the deadline, and its worst case is longer than
|
||||
// anything that would look reasonable in this file: a Synology call is 30s, and
|
||||
// an expired session makes it re-authenticate and try again. A client cap below
|
||||
// that turns a slow NAS into a NAS that reads as disconnected, which is what
|
||||
// these calls looked like before they moved off raw fetch (which had no timeout
|
||||
// either). Callers that need to give up early pass an AbortSignal.
|
||||
const MEMORIES_TIMEOUT = 0
|
||||
|
||||
export const memoriesApi = {
|
||||
status: (provider: string): Promise<{ connected: boolean }> =>
|
||||
|
||||
Binary file not shown.
@@ -12,7 +12,7 @@ import {
|
||||
TramFront, Footprints, StickyNote, ParkingSquare,
|
||||
} from 'lucide-react'
|
||||
import { openFile } from '../../utils/fileDownload'
|
||||
import { safeHttpUrl } from '../../utils/safeUrl'
|
||||
import { safeExternalHref } from '../../utils/safeUrl'
|
||||
import { TransitTitle, TransitLegChips, TransitMetaBadges, fmtTransitDuration } from './transitDisplay'
|
||||
import Markdown from 'react-markdown'
|
||||
import remarkGfm from 'remark-gfm'
|
||||
@@ -117,9 +117,10 @@ function ReservationCard({ r, tripId, onEdit, onDelete, files = [], onNavigateTo
|
||||
})()
|
||||
const attachedFiles = files.filter(f => f.reservation_id === r.id || (f.linked_reservation_ids || []).includes(r.id))
|
||||
const linked = r.assignment_id ? assignmentLookup[r.assignment_id] : null
|
||||
// A booking link is free-form text on rows written before the contract checked it,
|
||||
// so only an http(s) value becomes a real anchor — the rest stays readable text.
|
||||
const bookingUrl = safeHttpUrl(r.url)
|
||||
// A booking link is deliberately free-form - people paste bare hosts and long
|
||||
// provider deep links - so this refuses the schemes that execute in this
|
||||
// origin and leaves everything else linkable.
|
||||
const bookingUrl = safeExternalHref(r.url)
|
||||
const [showDeleteConfirm, setShowDeleteConfirm] = useState(false)
|
||||
|
||||
const handleDelete = async () => {
|
||||
|
||||
@@ -28,6 +28,11 @@ describe('placesSlice', () => {
|
||||
const assignment = buildAssignment({
|
||||
id: 100,
|
||||
day_id: 3,
|
||||
// The override is what makes the embedded copy differ from the pool row:
|
||||
// the server projects COALESCE(assignment_time, place_time), so without
|
||||
// it these two could not disagree in the first place.
|
||||
assignment_time: '09:00',
|
||||
assignment_end_time: '10:30',
|
||||
place: { ...place, place_time: '09:00', end_time: '10:30' },
|
||||
});
|
||||
seedStore(useTripStore, { places: [place], assignments: { '3': [assignment] } });
|
||||
@@ -43,7 +48,7 @@ describe('placesSlice', () => {
|
||||
expect(useTripStore.getState().places[0].image_url).toBe('/uploads/places/pic.jpg');
|
||||
const embedded = useTripStore.getState().assignments['3'][0].place;
|
||||
expect(embedded.image_url).toBe('/uploads/places/pic.jpg');
|
||||
// The assignment owns its own times — the fresh place must not overwrite them.
|
||||
// The assignment owns these times, so the fresh place must not overwrite them.
|
||||
expect(embedded.place_time).toBe('09:00');
|
||||
expect(embedded.end_time).toBe('10:30');
|
||||
});
|
||||
|
||||
@@ -19,12 +19,30 @@ export interface PlacesSlice {
|
||||
updatePlacesMany: (tripId: number | string, placeIds: number[], patch: Partial<Place>) => Promise<void>
|
||||
}
|
||||
|
||||
/** Swap the pool place embedded in an assignment while keeping the assignment's
|
||||
* own times: the day view shows COALESCE(assignment_time, place_time), so
|
||||
* overwriting them with the pool row would wipe a per-stop schedule. Exported
|
||||
* because the WS applier in remoteEventHandler has to merge the same way. */
|
||||
/**
|
||||
* Swap the pool place embedded in an assignment, resolving its times the way
|
||||
* the server does.
|
||||
*
|
||||
* The embedded copy is a projection, not the pool row: assignments.service
|
||||
* selects COALESCE(da.assignment_time, p.place_time). So a per-stop override
|
||||
* wins and has to survive a pool edit, and where there is no override the pool
|
||||
* time is what the day card shows - which means a pool edit must reach it.
|
||||
* Pinning the old embedded times instead would keep a renamed place's schedule
|
||||
* frozen until the next reload.
|
||||
*
|
||||
* Exported because the WS applier in remoteEventHandler has to merge
|
||||
* identically; two spellings of this rule is how the two paths drifted apart in
|
||||
* the first place.
|
||||
*/
|
||||
export function mergeAssignmentPlace(a: Assignment, place: Place): Assignment {
|
||||
return { ...a, place: { ...place, place_time: a.place.place_time, end_time: a.place.end_time } }
|
||||
return {
|
||||
...a,
|
||||
place: {
|
||||
...place,
|
||||
place_time: a.assignment_time ?? place.place_time,
|
||||
end_time: a.assignment_end_time ?? place.end_time,
|
||||
},
|
||||
}
|
||||
}
|
||||
|
||||
/** Replace a place in the pool and in every day-assignment that embeds it,
|
||||
|
||||
@@ -144,6 +144,12 @@ export const mutationQueue = {
|
||||
.sortBy('createdAt')
|
||||
|
||||
for (const mutation of pending) {
|
||||
// Re-checked every pass, not just on entry: this loop writes server
|
||||
// responses straight into Dexie, and after a logout the proxy points at
|
||||
// the shared anonymous database. A flush that started before logout
|
||||
// would otherwise seed it with the previous account's rows.
|
||||
if (!isAuthed()) break
|
||||
|
||||
// Mark as syncing so UI can show progress. The stamp is what lets the
|
||||
// next flush tell an in-flight row from one a killed tab abandoned.
|
||||
await offlineDb.mutationQueue.update(mutation.id, { status: 'syncing', syncingSince: Date.now() })
|
||||
|
||||
@@ -209,9 +209,11 @@ export const tripSyncManager = {
|
||||
}
|
||||
}
|
||||
|
||||
// Cache global user data (tags + categories) — fire-and-forget
|
||||
tagsApi.list().then(d => upsertTags(d.tags)).catch(() => {})
|
||||
categoriesApi.list().then(d => upsertCategories(d.categories)).catch(() => {})
|
||||
// Cache global user data (tags + categories) — fire-and-forget, so the
|
||||
// gate has to be re-read when the response lands, not when it was issued:
|
||||
// a logout in between would put these rows in the anonymous database.
|
||||
tagsApi.list().then(d => { if (isAuthed()) upsertTags(d.tags) }).catch(() => {})
|
||||
categoriesApi.list().then(d => { if (isAuthed()) upsertCategories(d.categories) }).catch(() => {})
|
||||
|
||||
// Cache file blobs + map tiles in background (don't block syncAll)
|
||||
const cacheTiles = getOfflinePrefs().cacheTiles
|
||||
|
||||
@@ -16,3 +16,31 @@ import { placeWebsiteSchema } from '@trek/shared'
|
||||
export function safeHttpUrl(value: string | null | undefined): string | null {
|
||||
return typeof value === 'string' && placeWebsiteSchema.safeParse(value).success ? value : null
|
||||
}
|
||||
|
||||
/**
|
||||
* A user-pasted link, made safe for an href without narrowing what people are
|
||||
* allowed to paste.
|
||||
*
|
||||
* Different job from safeHttpUrl above. That one guards fields whose contract
|
||||
* really is http(s)-only and capped. A booking url is deliberately free-form -
|
||||
* shared's reservationUrlSchema says so in as many words, because people paste
|
||||
* bare hosts and very long provider deep links - so holding it to the stricter
|
||||
* contract would turn a link somebody saved last year into dead text.
|
||||
*
|
||||
* Only the schemes that execute in this origin are refused. A value with no
|
||||
* scheme gets https://, which is what an address bar does with the same input;
|
||||
* without it the browser would resolve "www.hotel.com" against the current page
|
||||
* and navigate inside the app.
|
||||
*/
|
||||
export function safeExternalHref(value: string | null | undefined): string | null {
|
||||
if (typeof value !== 'string') return null
|
||||
const trimmed = value.trim()
|
||||
if (!trimmed) return null
|
||||
// Browsers strip control characters before they resolve a scheme, so the test
|
||||
// has to run against the same string they will see.
|
||||
const collapsed = Array.from(trimmed).filter(c => c > ' ').join('')
|
||||
if (/^https?:\/\//i.test(collapsed)) return trimmed
|
||||
// Any other scheme, executing or merely unexpected, stays unlinked.
|
||||
if (/^[a-z][a-z0-9+.-]*:/i.test(collapsed)) return null
|
||||
return `https://${trimmed}`
|
||||
}
|
||||
|
||||
@@ -51,14 +51,16 @@ describe('remoteEventHandler > places', () => {
|
||||
expect(assignments['10'][0].place?.name).toBe('Cascaded Update');
|
||||
});
|
||||
|
||||
it('FE-WSEVT-PLACE-004b: place:updated keeps the assignment its own times', () => {
|
||||
// The day view renders COALESCE(assignment_time, place_time), so the copy
|
||||
// embedded in an assignment carries the per-stop schedule. A broadcast about
|
||||
// the pool row must not overwrite it (the local reducer merges the same way).
|
||||
it('FE-WSEVT-PLACE-004b: place:updated keeps a per-stop override', () => {
|
||||
// The day view renders COALESCE(assignment_time, place_time), so an
|
||||
// assignment that carries its own time keeps it when the pool row changes
|
||||
// (the local reducer merges the same way).
|
||||
const place = buildPlace({ id: 1, name: 'Original', place_time: '08:00', end_time: '09:00' });
|
||||
const assignment = buildAssignment({
|
||||
id: 100,
|
||||
day_id: 10,
|
||||
assignment_time: '14:00',
|
||||
assignment_end_time: '15:30',
|
||||
place: { ...place, place_time: '14:00', end_time: '15:30' },
|
||||
});
|
||||
useTripStore.setState({ places: [place], assignments: { '10': [assignment] } });
|
||||
@@ -74,6 +76,31 @@ describe('remoteEventHandler > places', () => {
|
||||
expect(embedded.end_time).toBe('15:30');
|
||||
});
|
||||
|
||||
it('FE-WSEVT-PLACE-004c: without an override the pool time reaches the day card', () => {
|
||||
// The other half of the COALESCE. An assignment with no assignment_time
|
||||
// shows whatever the pool row says, so a collaborator moving the place from
|
||||
// 08:00 to 10:00 has to land here - freezing the embedded copy would leave
|
||||
// the day plan wrong until the next reload.
|
||||
const place = buildPlace({ id: 1, name: 'Original', place_time: '08:00', end_time: '09:00' });
|
||||
const assignment = buildAssignment({
|
||||
id: 100,
|
||||
day_id: 10,
|
||||
assignment_time: null,
|
||||
assignment_end_time: null,
|
||||
place: { ...place },
|
||||
});
|
||||
useTripStore.setState({ places: [place], assignments: { '10': [assignment] } });
|
||||
|
||||
useTripStore.getState().handleRemoteEvent({
|
||||
type: 'place:updated',
|
||||
place: buildPlace({ id: 1, name: 'Original', place_time: '10:00', end_time: '11:00' }),
|
||||
});
|
||||
|
||||
const embedded = useTripStore.getState().assignments['10'][0].place!;
|
||||
expect(embedded.place_time).toBe('10:00');
|
||||
expect(embedded.end_time).toBe('11:00');
|
||||
});
|
||||
|
||||
it('FE-WSEVT-PLACE-005: place:deleted removes place from places array', () => {
|
||||
seedData();
|
||||
useTripStore.getState().handleRemoteEvent({ type: 'place:deleted', placeId: 1 });
|
||||
|
||||
Reference in New Issue
Block a user