paul/picpeak
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 1 Wiki Activity

fix(gallery): guest filters respect show_feedback_to_guests, and marks survive a mid-write clear (#1147)

Browse Source 
Two follow-ups from the review of #1137.

Filters were a second way to read hidden feedback. Every token on /photos is an OR of two halves: what THIS viewer marked, and what ANYONE marked. The response fields built from the second half — like_count, comment_count, color_label_count — are all gated on show_feedback_to_guests. The filter was not, so with the setting off a guest could still send ?filter=liked and get back exactly the photos other people liked, across all five tokens. Reachable by a direct API caller holding a gallery token; the frontend never sends filter to this endpoint.

The half it left standing was also the wrong half. It read guest_identifier from the guest_id QUERY PARAMETER, which never matched anything — the frontend invents that string in localStorage and never sends it when submitting feedback, while submissions store generateGuestIdentifier(req). So gating the aggregate would have emptied these filters rather than narrowing them to 'mine', and accepting a caller-supplied identifier was a way back through the gate. Resolved from the request now, hidden rows excluded to match what the viewer can see.

A mark whose row is cleared mid-write lost its value. #1137 fixed two calls both writing; this is one clearing while another sets. The clear empties the row, the row is deleted for being empty, and the setter's update matches nothing — the caller told 'no mark'. A zero-row update now reports itself and the caller re-reads, bounded at three passes, throwing rather than reporting a success that did not happen.

Merged with admin privileges: the author cannot self-approve.
This commit is contained in:
Paul Nothaft
2026-08-23 22:07:50 +02:00
committed by GitHub
parent e4a8be8e7e
commit 00b20b2d72
4 changed files with 490 additions and 53 deletions
Download Patch File Download Diff File Expand all files Collapse all files
backend/src/routes/gallery.js
+73 -22
View File
@@ -633,7 +633,10 @@ router.get('/:slug/show/:token/state', handleAsync(async (req, res) => {
router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res) => {
try {
// Get filter and sort parameters from query
const { filter, guest_id, sort = 'upload_date', order = 'desc' } = req.query;
// `guest_id` is deliberately NOT read from the query string: the viewer's
// own feedback is resolved from the request identity instead (see the
// filter block). The frontend still sends it; it is ignored.
const { filter, sort = 'upload_date', order = 'desc' } = req.query;
// Get watermark settings to generate cache-busting version for URLs
const watermarkSettings = await watermarkService.getWatermarkSettings();
@@ -692,6 +695,13 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
// Execute the query
let photos = hiddenForGuest ? [] : await photosQuery;
// Check if feedback should be visible to guests. Read BEFORE the filter
// block, not after: the filters below consult it, because a filter that
// selects on other people's feedback is a way of reading that feedback.
const feedbackService = require('../services/feedbackService');
const feedbackSettings = await feedbackService.getEventFeedbackSettings(req.event.id);
const showFeedbackToGuests = isClient || parseBooleanInput(feedbackSettings.show_feedback_to_guests, true);
// Apply filtering if requested (supports global stats + per-guest interactions)
if (filter) {
const filterTokens = new Set(
@@ -721,11 +731,39 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
});
};
// Whose feedback counts as "mine" for these filters.
//
// Resolved from the REQUEST, the same either/or the per-viewer
// is_liked and my_color_label queries below use — never from the
// `guest_id` query parameter. Two reasons, and both matter now that
// this is the only half left when feedback is hidden:
//
// - It never matched. The frontend's `gallery_guest_id` is a
// localStorage string it invents (`guest_<ts>_<rand>`) and never
// sends when submitting feedback; submissions store
// generateGuestIdentifier(req). So this lookup found nothing, and
// the filters only ever worked through the aggregate half — which
// is exactly the half now gated.
// - It is caller-controlled. Accepting an identifier from the query
// string would let anyone holding someone else's read their hidden
// memberships one token at a time, straight back through the gate.
let guestFeedbackByType = null;
let guestColorLabels = null;
if (guest_id) {
const guestFeedbackRows = await db('photo_feedback')
.where({ event_id: req.event.id, guest_identifier: guest_id })
{
// Hidden rows are excluded, matching what the viewer can actually
// SEE: getPhotoFeedback drops is_hidden for the guest's own feedback
// too, so without this a photo could come back under
// `?filter=commented` with no comment visible on it. Unapproved rows
// are NOT excluded — a comment still in the moderation queue is
// still the viewer's own, and the same guest-own read keeps it.
const viewerFeedback = db('photo_feedback')
.where({ event_id: req.event.id, is_hidden: false });
if (req.guest?.id) {
viewerFeedback.where('guest_id', req.guest.id);
} else {
viewerFeedback.where('guest_identifier', generateGuestIdentifier(req));
}
const guestFeedbackRows = await viewerFeedback
.select('photo_id', 'feedback_type', 'color_label');
guestFeedbackByType = guestFeedbackRows.reduce((acc, row) => {
@@ -753,19 +791,33 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
}
};
// Every token below is an OR of two halves: what THIS viewer marked,
// and what ANYONE marked. The second half is other people's feedback,
// so it is gated on show_feedback_to_guests exactly like the counts
// this endpoint returns.
//
// Without the gate the setting only hides the numbers. A guest could
// still send `?filter=liked` and get back precisely the set of photos
// other people liked — the membership, one token at a time, which is
// most of what the counts would have told them. The viewer's own half
// is always theirs to filter by.
const includeAggregate = (predicate) => {
if (showFeedbackToGuests) includeBy(predicate);
};
if (filterTokens.has('liked')) {
includeGuestMatches('like');
includeBy(photo => (photo.like_count || 0) > 0);
includeAggregate(photo => (photo.like_count || 0) > 0);
}
if (filterTokens.has('favorited')) {
includeGuestMatches('favorite');
includeBy(photo => (photo.favorite_count || 0) > 0);
includeAggregate(photo => (photo.favorite_count || 0) > 0);
}
if (filterTokens.has('rated')) {
includeGuestMatches('rating');
includeBy(photo => (photo.average_rating || 0) > 0);
includeAggregate(photo => (photo.average_rating || 0) > 0);
}
// Colour-label filters (#1044), one token per colour: `color:green`.
@@ -778,31 +830,30 @@ router.get('/:slug/photos', verifyGalleryAccess, resolveGuest, async (req, res)
for (const color of requestedColors) {
guestColorLabels?.[color]?.forEach(id => include.add(id));
}
const colorRows = await db('photo_feedback')
.where({ event_id: req.event.id, feedback_type: 'color_label', is_hidden: false })
.whereIn('color_label', requestedColors)
.select('photo_id');
colorRows.forEach(row => include.add(row.photo_id));
if (showFeedbackToGuests) {
const colorRows = await db('photo_feedback')
.where({ event_id: req.event.id, feedback_type: 'color_label', is_hidden: false })
.whereIn('color_label', requestedColors)
.select('photo_id');
colorRows.forEach(row => include.add(row.photo_id));
}
}
if (filterTokens.has('commented')) {
includeGuestMatches('comment');
const commentedRows = await db('photo_feedback')
.where({ event_id: req.event.id, feedback_type: 'comment', is_approved: true, is_hidden: false })
.groupBy('photo_id')
.select('photo_id');
commentedRows.forEach(row => include.add(row.photo_id));
if (showFeedbackToGuests) {
const commentedRows = await db('photo_feedback')
.where({ event_id: req.event.id, feedback_type: 'comment', is_approved: true, is_hidden: false })
.groupBy('photo_id')
.select('photo_id');
commentedRows.forEach(row => include.add(row.photo_id));
}
}
photos = photos.filter(photo => include.has(photo.id));
}
}
// Check if feedback should be visible to guests
const feedbackService = require('../services/feedbackService');
const feedbackSettings = await feedbackService.getEventFeedbackSettings(req.event.id);
const showFeedbackToGuests = isClient || parseBooleanInput(feedbackSettings.show_feedback_to_guests, true);
// Then get comment counts separately
const commentCounts = await db('photo_feedback')
.whereIn('photo_id', photos.map(p => p.id))
backend/src/services/photoAdminMarksService.js
+54 -31
View File
@@ -19,6 +19,9 @@ const { isValidColorLabel } = require('../constants/colorLabels');
* and a reworded string would silently turn a 400 into a 500.
*/
const INVALID_MARK = 'INVALID_MARK';
/** The row we were writing to was deleted by a concurrent call. */
const ROW_GONE = Symbol('row-gone');
function invalidMark(message) {
return Object.assign(new Error(message), { code: INVALID_MARK });
}
@@ -66,7 +69,12 @@ async function setMark(eventId, photoId, adminId, { rating, colorLabel } = {}) {
if (rating !== undefined) patch.rating = rating === null ? null : Number(rating);
if (colorLabel !== undefined) patch.color_label = colorLabel;
await db('photo_admin_marks').where('id', row.id).update(patch);
const updated = await db('photo_admin_marks').where('id', row.id).update(patch);
// Zero rows means a concurrent CLEAR deleted this row between our read and
// this write — the other direction of the same race the patch above
// closes. Our value landed nowhere, so reporting emptiness here would drop
// the keystroke silently. Say so, and let the caller start over.
if (!updated) return ROW_GONE;
// A mark with neither half left is deleted, not kept as an empty row — an
// empty row would still count as "marked" to anything testing existence.
@@ -86,44 +94,59 @@ async function setMark(eventId, photoId, adminId, { rating, colorLabel } = {}) {
return { rating: after.rating ?? null, color_label: after.color_label ?? null };
};
const existing = await db('photo_admin_marks')
.where({ photo_id: photoId, admin_id: adminId })
.first();
const insertFresh = async () => {
// No row yet, so there is nothing for a clear-only call to clear.
const fresh = {
rating: rating === undefined || rating === null ? null : Number(rating),
color_label: colorLabel === undefined || colorLabel === null ? null : colorLabel,
};
if (fresh.rating === null && fresh.color_label === null) return null;
if (existing) return applyToRow(existing);
const now = new Date().toISOString();
try {
await db('photo_admin_marks').insert({
photo_id: photoId,
event_id: eventId,
admin_id: adminId,
...fresh,
created_at: now,
updated_at: now,
});
} catch (error) {
// The read above and this insert are not atomic, and a triage pass fires
// them back to back — press 4 then 9 on an UNMARKED photo and both
// requests can find no row and both try to insert. The unique index
// stops the duplicate, which would otherwise surface as a 500 and a lost
// keystroke; converge onto the row the winner created, through the same
// write-only-what-you-addressed path as any other update.
const raced = await db('photo_admin_marks')
.where({ photo_id: photoId, admin_id: adminId })
.first();
if (!raced) throw error;
return applyToRow(raced);
}
// No row yet, so there is nothing for a clear-only call to clear.
const fresh = {
rating: rating === undefined || rating === null ? null : Number(rating),
color_label: colorLabel === undefined || colorLabel === null ? null : colorLabel,
return fresh;
};
if (fresh.rating === null && fresh.color_label === null) return null;
const now = new Date().toISOString();
try {
await db('photo_admin_marks').insert({
photo_id: photoId,
event_id: eventId,
admin_id: adminId,
...fresh,
created_at: now,
updated_at: now,
});
} catch (error) {
// The read above and this insert are not atomic, and a triage pass fires
// them back to back — press 4 then 9 on an UNMARKED photo and both
// requests can find no row and both try to insert. The unique index stops
// the duplicate, which would otherwise surface as a 500 and a lost
// keystroke; converge onto the row the winner created, through the same
// write-only-what-you-addressed path as any other update.
const raced = await db('photo_admin_marks')
// The row can flip between "exists" and "gone" underneath us: a concurrent
// clear deletes it after we read it, or a concurrent set recreates it after
// we find none. Each pass re-reads and acts on what it finds, and only a
// flip sends us round again — so this settles as soon as one write lands.
// Bounded because an unbounded retry on a hot row is a way to hang a
// request, and three flips in one keystroke is not a real sequence.
for (let attempt = 0; attempt < 3; attempt++) {
const existing = await db('photo_admin_marks')
.where({ photo_id: photoId, admin_id: adminId })
.first();
if (!raced) throw error;
return applyToRow(raced);
const result = existing ? await applyToRow(existing) : await insertFresh();
if (result !== ROW_GONE) return result;
}
return fresh;
// Not silent: losing the keystroke is the failure this whole path exists to
// avoid, so if it somehow cannot land, say so rather than report success.
throw new Error('Could not save mark: the row kept changing underneath');
}
/**