fix(search): stop escapeLikePattern corrupting bound search values
Verified against a real SQLite connection -- each of these returned zero rows before and the right row after: "Sarah's" before=[] after=["Sarah's Birthday"] "100%" before=[] after=["Summer 100% Sale"] "Gala_" before=[] after=["Gala_Night"] Two bugs in one helper. It did .replace(/'/g, "''"), which is SQL string-quote doubling -- meaningless and actively corrupting for a value that is bound, so any search containing an apostrophe matched nothing. And its \% escaping had no ESCAPE clause on the LIKE, which is engine-dependent: honoured on Postgres, a literal backslash on SQLite, so % and _ stayed wildcards there. Now mirrors the correct implementation from 59666b59: escape \ % _ only, and a new likeWithEscape(column) emits `col LIKE ? ESCAPE '\'`. Both call sites move to whereRaw with the value still bound; the column argument is a literal, documented in the JSDoc. Callers checked before changing the contract: adminPhotos.js, adminEvents/crud.js, and sqlSecurity's own addLikeCondition(), which has no callers anywhere -- pre-existing dead export, updated to the new shape rather than deleted. Behavioural change: searches containing ' % _ or \ now return the right rows instead of nothing. Case sensitivity is unchanged. Refs testplan REPORT.md, escapeLikePattern finding.
This commit is contained in:
@@ -0,0 +1,116 @@
|
||||
/**
|
||||
* The admin event search must treat the search box as literal text.
|
||||
*
|
||||
* escapeLikePattern() used to double single quotes — SQL string-literal syntax
|
||||
* applied to a value that is BOUND, so "Sarah's Birthday" went to the database
|
||||
* as "Sarah''s Birthday" and matched nothing. It also emitted `\%` with no
|
||||
* ESCAPE clause on the LIKE, which Postgres honours and SQLite does not: on
|
||||
* SQLite the backslash was matched literally, so a search for a name containing
|
||||
* a real `%` or `_` returned nothing while the same search worked on Postgres.
|
||||
*
|
||||
* These run against SQLite (the engine the suite boots), which is the side that
|
||||
* silently returned zero rows.
|
||||
*/
|
||||
const path = require('path');
|
||||
const fs = require('fs');
|
||||
const os = require('os');
|
||||
|
||||
process.env.NODE_ENV = 'test';
|
||||
process.env.TEST_DATABASE_PATH = path.join(
|
||||
fs.mkdtempSync(path.join(os.tmpdir(), 'picpeak-events-search-')), 'db.sqlite'
|
||||
);
|
||||
process.env.JWT_SECRET = process.env.JWT_SECRET || 'admin-events-search-secret';
|
||||
|
||||
const express = require('express');
|
||||
const cookieParser = require('cookie-parser');
|
||||
const request = require('supertest');
|
||||
const { bootCrmDb, seedMinimal, assignAdminRole, mintAdminToken } = require('../integration/helpers/crmDb');
|
||||
const { escapeLikePattern } = require('../../src/utils/sqlSecurity');
|
||||
|
||||
async function insertEvent(db, adminId, eventName) {
|
||||
const rand = Math.random().toString(16).slice(2);
|
||||
await db('events').insert({
|
||||
slug: `ev-${rand}`,
|
||||
event_type: 'wedding',
|
||||
event_name: eventName,
|
||||
event_date: '2026-05-29',
|
||||
host_email: '[email protected]',
|
||||
admin_email: '[email protected]',
|
||||
password_hash: 'x',
|
||||
share_link: `/gallery/share-${rand}`,
|
||||
share_token: `st-${rand}`,
|
||||
expires_at: new Date(Date.now() + 7 * 24 * 3600 * 1000).toISOString(),
|
||||
is_active: 1, is_archived: 0, is_draft: 0,
|
||||
created_by: adminId,
|
||||
created_at: new Date().toISOString(),
|
||||
});
|
||||
}
|
||||
|
||||
describe('GET /api/admin/events search — LIKE metacharacters and quotes', () => {
|
||||
let db; let cleanup; let app; let token;
|
||||
|
||||
// Each metacharacter name is paired with a name a wildcard reading of the
|
||||
// search term would also match, so "matches only itself" is testable.
|
||||
const fixtures = [
|
||||
'Sarah\'s Birthday',
|
||||
'Sarahs Birthday',
|
||||
'Summer 100% Sale',
|
||||
'Summer 100X Sale',
|
||||
'Gala_Night',
|
||||
'GalaXNight',
|
||||
];
|
||||
|
||||
const search = async (term) => {
|
||||
const res = await request(app)
|
||||
.get('/api/admin/events')
|
||||
.query({ search: term })
|
||||
.set('Authorization', `Bearer ${token}`);
|
||||
expect(res.status).toBe(200);
|
||||
return res.body.events.map((e) => e.event_name).sort();
|
||||
};
|
||||
|
||||
beforeAll(async () => {
|
||||
({ db, cleanup } = await bootCrmDb());
|
||||
const { adminId } = await seedMinimal(db);
|
||||
await assignAdminRole(db, adminId, 'super_admin');
|
||||
token = mintAdminToken(adminId);
|
||||
|
||||
for (const name of fixtures) {
|
||||
await insertEvent(db, adminId, name);
|
||||
}
|
||||
|
||||
app = express();
|
||||
app.use(express.json());
|
||||
app.use(cookieParser());
|
||||
app.use('/api/admin/events', require('../../src/routes/adminEvents'));
|
||||
// eslint-disable-next-line no-unused-vars
|
||||
app.use((err, req, res, next) => {
|
||||
res.status(err.statusCode || err.status || 500).json({ error: err.message, code: err.code });
|
||||
});
|
||||
}, 120000);
|
||||
|
||||
afterAll(async () => { await cleanup(); });
|
||||
|
||||
it('finds a name containing an apostrophe', async () => {
|
||||
expect(await search('Sarah\'s')).toEqual(['Sarah\'s Birthday']);
|
||||
});
|
||||
|
||||
it('matches a literal % against itself only, not as a wildcard', async () => {
|
||||
expect(await search('100%')).toEqual(['Summer 100% Sale']);
|
||||
});
|
||||
|
||||
it('matches a literal _ against itself only, not as a single-char wildcard', async () => {
|
||||
expect(await search('Gala_')).toEqual(['Gala_Night']);
|
||||
});
|
||||
|
||||
it('still does substring matching for ordinary terms', async () => {
|
||||
expect(await search('Summer')).toEqual(['Summer 100% Sale', 'Summer 100X Sale']);
|
||||
});
|
||||
|
||||
it('escapes only the LIKE metacharacters, leaving quotes untouched', () => {
|
||||
expect(escapeLikePattern('Sarah\'s Birthday')).toBe('Sarah\'s Birthday');
|
||||
expect(escapeLikePattern('100%_x')).toBe('100\\%\\_x');
|
||||
expect(escapeLikePattern('back\\slash')).toBe('back\\\\slash');
|
||||
expect(escapeLikePattern(null)).toBe('');
|
||||
});
|
||||
});
|
||||
@@ -13,7 +13,7 @@ const bcrypt = require('bcrypt');
|
||||
const crypto = require('crypto');
|
||||
const fs = require('fs').promises;
|
||||
const path = require('path');
|
||||
const { escapeLikePattern } = require('../../utils/sqlSecurity');
|
||||
const { escapeLikePattern, likeWithEscape } = require('../../utils/sqlSecurity');
|
||||
const { validatePasswordInContext, getBcryptRounds } = require('../../utils/passwordValidation');
|
||||
const logger = require('../../utils/logger');
|
||||
const { sanitizeForLog, sanitizeValidationErrors } = require('../../utils/sanitizeForLog');
|
||||
@@ -866,12 +866,12 @@ module.exports = (router) => {
|
||||
|
||||
// Apply search filter
|
||||
if (search) {
|
||||
const escapedSearch = escapeLikePattern(search);
|
||||
const pattern = `%${escapeLikePattern(search)}%`;
|
||||
query = query.where((builder) => {
|
||||
builder.where('event_name', 'like', `%${escapedSearch}%`)
|
||||
.orWhere('admin_email', 'like', `%${escapedSearch}%`)
|
||||
.orWhere('customer_email', 'like', `%${escapedSearch}%`)
|
||||
.orWhere('slug', 'like', `%${escapedSearch}%`);
|
||||
builder.whereRaw(likeWithEscape('event_name'), [pattern])
|
||||
.orWhereRaw(likeWithEscape('admin_email'), [pattern])
|
||||
.orWhereRaw(likeWithEscape('customer_email'), [pattern])
|
||||
.orWhereRaw(likeWithEscape('slug'), [pattern]);
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -12,7 +12,7 @@ const {
|
||||
getUseOriginalFilenames,
|
||||
pickRawDownloadName,
|
||||
} = require('../services/downloadFilenameService');
|
||||
const { escapeLikePattern } = require('../utils/sqlSecurity');
|
||||
const { escapeLikePattern, likeWithEscape } = require('../utils/sqlSecurity');
|
||||
const { COLOR_LABELS, dominantColorLabel, SHARED_COLOR_LABEL_IDENTITY } = require('../constants/colorLabels');
|
||||
const feedbackService = require('../services/feedbackService');
|
||||
const photoAdminMarksService = require('../services/photoAdminMarksService');
|
||||
@@ -1228,8 +1228,10 @@ router.get('/:eventId/photos', adminAuth, requirePermission('photos.view'), requ
|
||||
|
||||
// Search by filename
|
||||
if (search) {
|
||||
const escapedSearch = escapeLikePattern(search);
|
||||
query = query.where('photos.filename', 'like', `%${escapedSearch}%`);
|
||||
query = query.whereRaw(
|
||||
likeWithEscape('photos.filename'),
|
||||
[`%${escapeLikePattern(search)}%`]
|
||||
);
|
||||
}
|
||||
|
||||
// Feedback filters (has likes / favorites / comments / min rating) with AND/OR logic
|
||||
|
||||
@@ -29,25 +29,49 @@ function sanitizeDays(days) {
|
||||
}
|
||||
|
||||
/**
|
||||
* Escape special characters in LIKE queries
|
||||
* Escape the LIKE metacharacters in a search term so it matches itself.
|
||||
*
|
||||
* The result is a BOUND value — it goes into the `?` of a prepared statement,
|
||||
* never into SQL text. That is why single quotes are left alone: doubling them
|
||||
* is SQL string-literal syntax, and inside a bound value it is not escaping
|
||||
* anything, it is corrupting the search term (a search for "Sarah's Birthday"
|
||||
* would be sent as "Sarah''s Birthday" and match nothing).
|
||||
*
|
||||
* Only the three characters LIKE itself reads are escaped:
|
||||
* % matches any sequence of characters
|
||||
* _ matches any single character
|
||||
* \ the escape character named by the ESCAPE clause
|
||||
*
|
||||
* Pair the result with likeWithEscape() — without an explicit ESCAPE clause
|
||||
* the backslashes below mean different things on Postgres and SQLite.
|
||||
*
|
||||
* @param {string} input - The search string from user input
|
||||
* @returns {string} Escaped string safe for LIKE queries
|
||||
* @returns {string} Escaped string safe to bind into a LIKE pattern
|
||||
*/
|
||||
function escapeLikePattern(input) {
|
||||
if (!input || typeof input !== 'string') {
|
||||
return '';
|
||||
}
|
||||
|
||||
// Escape special LIKE pattern characters
|
||||
// In SQL LIKE patterns:
|
||||
// % matches any sequence of characters
|
||||
// _ matches any single character
|
||||
// \ is the escape character
|
||||
return input
|
||||
.replace(/\\/g, '\\\\') // Escape backslashes first
|
||||
.replace(/%/g, '\\%') // Escape percent signs
|
||||
.replace(/_/g, '\\_') // Escape underscores
|
||||
.replace(/'/g, '\'\''); // Escape single quotes for safety
|
||||
|
||||
return input.replace(/[\\%_]/g, '\\$&');
|
||||
}
|
||||
|
||||
/**
|
||||
* Build a `LIKE ? ESCAPE '\'` comparison for a column.
|
||||
*
|
||||
* The ESCAPE clause is load-bearing rather than decorative: Postgres treats a
|
||||
* backslash in a LIKE pattern as an escape character by default, SQLite has no
|
||||
* default escape character at all and would match the backslash literally. Naming
|
||||
* it makes escapeLikePattern()'s output mean the same thing on both engines.
|
||||
*
|
||||
* `column` is interpolated into the SQL text, so callers must pass a literal
|
||||
* column name — never user input. The search term stays bound.
|
||||
*
|
||||
* @param {string} column - Literal column name (optionally table-qualified)
|
||||
* @returns {string} Raw SQL fragment with a single `?` binding placeholder
|
||||
*/
|
||||
function likeWithEscape(column) {
|
||||
return `${column} LIKE ? ESCAPE '\\'`;
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -78,9 +102,8 @@ function addLikeCondition(query, column, pattern) {
|
||||
return query;
|
||||
}
|
||||
|
||||
const escapedPattern = escapeLikePattern(pattern);
|
||||
// Knex handles parameterization of the LIKE value
|
||||
return query.where(column, 'like', `%${escapedPattern}%`);
|
||||
return query.whereRaw(likeWithEscape(column), [`%${escapeLikePattern(pattern)}%`]);
|
||||
}
|
||||
|
||||
/**
|
||||
@@ -110,6 +133,7 @@ function validateSortOrder(order) {
|
||||
module.exports = {
|
||||
sanitizeDays,
|
||||
escapeLikePattern,
|
||||
likeWithEscape,
|
||||
addDateRangeCondition,
|
||||
addLikeCondition,
|
||||
validateSortColumn,
|
||||
|
||||
Reference in New Issue
Block a user