Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Desktop,Mobile,Cli: Fixes #11630: Adjust how items are queried by ID #11734

Merged
merged 1 commit into from
Jan 27, 2025
Merged

Desktop,Mobile,Cli: Fixes #11630: Adjust how items are queried by ID #11734

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions packages/lib/BaseModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -345,13 +345,17 @@ class BaseModel {
return this.modelSelectAll(q.sql, q.params);
}

public static escapeIdsForSql(ids: string[]) {
return this.db().escapeValues(ids).join(', ');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The solution and usage of escapeIdsForSql looks ok, but I just have one question:
How / are single quotes added at the start and end of the contents of the IN clause?

The previous logic used:
IN ('${ids.join('','')}')
new logic uses:
(${this.escapeIdsForSql(ids)})

So I don't understand how enclosing quotes can be added when they are omitted for usages of the escapeIdsForSql function and not added within the function.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to escaping enclosed quotes by doubling them, .escapeValues surrounds each of the IDs in 's.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that makes sense. Didn't notice the join here no longer includes quotes in the value

}

public static async byIds(ids: string[], options: LoadOptions = null) {
if (!ids.length) return [];
if (!options) options = {};
if (!options.fields) options.fields = '*';

let sql = `SELECT ${this.db().escapeFields(options.fields)} FROM \`${this.tableName()}\``;
sql += ` WHERE id IN ('${ids.join('\',\'')}')`;
sql += ` WHERE id IN (${this.escapeIdsForSql(ids)})`;
const q = this.applySqlOptions(options, sql);
return this.modelSelectAll(q.sql);
}
Expand Down Expand Up @@ -750,7 +754,7 @@ class BaseModel {

options = this.modOptions(options);
const idFieldName = options.idFieldName ? options.idFieldName : 'id';
const sql = `DELETE FROM ${this.tableName()} WHERE ${idFieldName} IN ('${ids.join('\',\'')}')`;
const sql = `DELETE FROM ${this.tableName()} WHERE ${idFieldName} IN (${this.escapeIdsForSql(ids)})`;
await this.db().exec(sql);
}

Expand Down
7 changes: 7 additions & 0 deletions packages/lib/database.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,13 @@ export default class Database {
return output;
}

public escapeValues(values: string[]) {
return values.map(value => {
// See https://www.sqlite.org/printf.html#percentq
return `'${value.replace(/[']/g, '\'\'')}'`;
});
}

public escapeFieldsToString(fields: string[] | string): string {
if (typeof fields === 'string') {
if (fields === '*') return '*';
Expand Down
12 changes: 12 additions & 0 deletions packages/lib/models/BaseItem.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,4 +149,16 @@ three line \\n no escape`)).toBe(0);
expect(await syncTime(note1.id)).toBe(newTime);
});

it.each([
'test-test!',
'This ID has spaces\ttabs\nand newlines',
'Test`;',
'Test"',
'Test\'',
'Test\'\'\'a\'\'',
'% test',
])('should support querying items with IDs containing special characters (id: %j)', async (id) => {
const note = await Note.save({ id }, { isNew: true });
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be good to add a test which includes more than 1 id passed at the same time, so that the join logic is tested

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the feedback! That would be useful.

Note that for standard IDs, there's already several tests that indirectly test this case. For example,

However, these tests only check standard IDs without 's.

expect(await BaseItem.loadItemById(note.id)).toMatchObject({ id });
});
});
13 changes: 8 additions & 5 deletions packages/lib/models/BaseItem.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,11 +243,12 @@ export default class BaseItem extends BaseModel {
if (!ids.length) return [];

const classes = this.syncItemClassNames();

// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
let output: any[] = [];
for (let i = 0; i < classes.length; i++) {
const ItemClass = this.getClass(classes[i]);
const sql = `SELECT * FROM ${ItemClass.tableName()} WHERE id IN ('${ids.join('\',\'')}')`;
const sql = `SELECT * FROM ${ItemClass.tableName()} WHERE id IN (${this.escapeIdsForSql(ids)})`;
const models = await ItemClass.modelSelectAll(sql);
output = output.concat(models);
}
Expand All @@ -261,7 +262,7 @@ export default class BaseItem extends BaseModel {
const fields = options && options.fields ? options.fields : [];
const ItemClass = this.getClassByItemType(itemType);
const fieldsSql = fields.length ? this.db().escapeFields(fields) : '*';
const sql = `SELECT ${fieldsSql} FROM ${ItemClass.tableName()} WHERE id IN ('${ids.join('\',\'')}')`;
const sql = `SELECT ${fieldsSql} FROM ${ItemClass.tableName()} WHERE id IN (${this.escapeIdsForSql(ids)})`;
return ItemClass.modelSelectAll(sql);
}

Expand Down Expand Up @@ -300,7 +301,7 @@ export default class BaseItem extends BaseModel {
// since no other client have (or should have) them.
let conflictNoteIds: string[] = [];
if (this.modelType() === BaseModel.TYPE_NOTE) {
const conflictNotes = await this.db().selectAll(`SELECT id FROM notes WHERE id IN ('${ids.join('\',\'')}') AND is_conflict = 1`);
const conflictNotes = await this.db().selectAll(`SELECT id FROM notes WHERE id IN (${this.escapeIdsForSql(ids)}) AND is_conflict = 1`);
conflictNoteIds = conflictNotes.map((n: NoteEntity) => {
return n.id;
});
Expand Down Expand Up @@ -661,7 +662,9 @@ export default class BaseItem extends BaseModel {
whereSql = [`(encryption_applied = 1 OR (${blobDownloadedButEncryptedSql})`];
}

if (exclusions.length) whereSql.push(`id NOT IN ('${exclusions.join('\',\'')}')`);
if (exclusions.length) {
whereSql.push(`id NOT IN (${this.escapeIdsForSql(exclusions)})`);
}

const sql = sprintf(
`
Expand Down Expand Up @@ -943,7 +946,7 @@ export default class BaseItem extends BaseModel {
});
if (!ids.length) continue;

await this.db().exec(`UPDATE sync_items SET force_sync = 1 WHERE item_id IN ('${ids.join('\',\'')}')`);
await this.db().exec(`UPDATE sync_items SET force_sync = 1 WHERE item_id IN (${this.escapeIdsForSql(ids)})`);
}
}

Expand Down
6 changes: 3 additions & 3 deletions packages/lib/models/Folder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -433,7 +433,7 @@ export default class Folder extends BaseItem {

const sql = ['SELECT id, parent_id FROM folders WHERE share_id != \'\''];
if (sharedFolderIds.length) {
sql.push(` AND id NOT IN ('${sharedFolderIds.join('\',\'')}')`);
sql.push(` AND id NOT IN (${Folder.escapeIdsForSql(sharedFolderIds)})`);
}

const foldersToUnshare: FolderEntity[] = await this.db().selectAll(sql.join(' '));
Expand Down Expand Up @@ -544,7 +544,7 @@ export default class Folder extends BaseItem {
SELECT resource_id, note_id, notes.share_id
FROM note_resources
LEFT JOIN notes ON notes.id = note_resources.note_id
WHERE resource_id IN ('${resourceIds.join('\',\'')}')
WHERE resource_id IN (${this.escapeIdsForSql(resourceIds)})
AND is_associated = 1
`) as NoteResourceRow[];

Expand Down Expand Up @@ -650,7 +650,7 @@ export default class Folder extends BaseItem {

const query = activeShareIds.length ? `
SELECT ${this.db().escapeFields(fields)} FROM ${tableName}
WHERE share_id != '' AND share_id NOT IN ('${activeShareIds.join('\',\'')}')
WHERE share_id != '' AND share_id NOT IN (${this.escapeIdsForSql(activeShareIds)})
` : `
SELECT ${this.db().escapeFields(fields)} FROM ${tableName}
WHERE share_id != ''
Expand Down
7 changes: 3 additions & 4 deletions packages/lib/models/Note.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ export interface PreviewsOrder {
dir: string;
}

interface PreviewsOptions {
export interface PreviewsOptions {
order?: PreviewsOrder[];
conditions?: string[];
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
Expand Down Expand Up @@ -893,8 +893,7 @@ export default class Note extends BaseItem {
'updated_time = ?',
];

// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
const params: any[] = [
const params: (string|number)[] = [
now,
now,
];
Expand All @@ -907,7 +906,7 @@ export default class Note extends BaseItem {
const sql = `
UPDATE notes
SET ${updateSql.join(', ')}
WHERE id IN ('${processIds.join('\',\'')}')
WHERE id IN (${this.escapeIdsForSql(processIds)})
`;

await this.db().exec({ sql, params });
Expand Down
2 changes: 1 addition & 1 deletion packages/lib/models/NoteResource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ export default class NoteResource extends BaseModel {
FROM note_resources
LEFT JOIN notes
ON notes.id = note_resources.note_id
WHERE resource_id IN ('${resourceIds.join('\', \'')}') AND is_associated = 1
WHERE resource_id IN (${this.escapeIdsForSql(resourceIds)}) AND is_associated = 1
`);

const output: Record<string, NoteEntity[]> = {};
Expand Down
2 changes: 1 addition & 1 deletion packages/lib/models/NoteTag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ export default class NoteTag extends BaseItem {

public static async byNoteIds(noteIds: string[]) {
if (!noteIds.length) return [];
return this.modelSelectAll(`SELECT * FROM note_tags WHERE note_id IN ('${noteIds.join('\',\'')}')`);
return this.modelSelectAll(`SELECT * FROM note_tags WHERE note_id IN (${this.escapeIdsForSql(noteIds)})`);
}

public static async tagIdsByNoteId(noteId: string) {
Expand Down
8 changes: 4 additions & 4 deletions packages/lib/models/Resource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export default class Resource extends BaseItem {
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
public static fetchStatuses(resourceIds: string[]): Promise<any[]> {
if (!resourceIds.length) return Promise.resolve([]);
return this.db().selectAll(`SELECT resource_id, fetch_status FROM resource_local_states WHERE resource_id IN ('${resourceIds.join('\',\'')}')`);
return this.db().selectAll(`SELECT resource_id, fetch_status FROM resource_local_states WHERE resource_id IN (${this.escapeIdsForSql(resourceIds)})`);
}

public static sharedResourceIds(): Promise<string[]> {
Expand Down Expand Up @@ -368,7 +368,7 @@ export default class Resource extends BaseItem {
public static async downloadedButEncryptedBlobCount(excludedIds: string[] = null) {
let excludedSql = '';
if (excludedIds && excludedIds.length) {
excludedSql = `AND resource_id NOT IN ('${excludedIds.join('\',\'')}')`;
excludedSql = `AND resource_id NOT IN (${this.escapeIdsForSql(excludedIds)})`;
}

const r = await this.db().selectOne(`
Expand Down Expand Up @@ -536,7 +536,7 @@ export default class Resource extends BaseItem {

public static async needOcr(supportedMimeTypes: string[], skippedResourceIds: string[], limit: number, options: LoadOptions): Promise<ResourceEntity[]> {
const query = this.baseNeedOcrQuery(this.selectFields(options), supportedMimeTypes);
const skippedResourcesSql = skippedResourceIds.length ? `AND resources.id NOT IN ('${skippedResourceIds.join('\',\'')}')` : '';
const skippedResourcesSql = skippedResourceIds.length ? `AND resources.id NOT IN (${this.escapeIdsForSql(skippedResourceIds)})` : '';

return await this.db().selectAll(`
${query.sql}
Expand Down Expand Up @@ -576,7 +576,7 @@ export default class Resource extends BaseItem {
public static async resourceOcrTextsByIds(ids: string[]): Promise<ResourceEntity[]> {
if (!ids.length) return [];
ids = unique(ids);
return this.modelSelectAll(`SELECT id, ocr_text FROM resources WHERE id IN ('${ids.join('\',\'')}')`);
return this.modelSelectAll(`SELECT id, ocr_text FROM resources WHERE id IN (${this.escapeIdsForSql(ids)})`);
}

public static async allForNormalization(updatedTime: number, id: string, limit = 100, options: LoadOptions = null) {
Expand Down
2 changes: 1 addition & 1 deletion packages/lib/models/Revision.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ export default class Revision extends BaseItem {

public static async itemsWithRevisions(itemType: ModelType, itemIds: string[]) {
if (!itemIds.length) return [];
const rows = await this.db().selectAll(`SELECT distinct item_id FROM revisions WHERE item_type = ? AND item_id IN ('${itemIds.join('\',\'')}')`, [itemType]);
const rows = await this.db().selectAll(`SELECT distinct item_id FROM revisions WHERE item_type = ? AND item_id IN (${this.escapeIdsForSql(itemIds)})`, [itemType]);

return rows.map((r: RevisionEntity) => r.item_id);
}
Expand Down
6 changes: 3 additions & 3 deletions packages/lib/models/Tag.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ export default class Tag extends BaseItem {

return Note.previews(
null,
{ ...options, conditions: [`id IN ('${noteIds.join('\',\'')}')`] },
{ ...options, conditions: [`id IN (${this.escapeIdsForSql(noteIds)})`] },
);
}

Expand Down Expand Up @@ -153,7 +153,7 @@ export default class Tag extends BaseItem {

const tagIds = await NoteTag.tagIdsByNoteId(noteId);
if (!tagIds.length) return [];
return this.modelSelectAll(`SELECT ${options.fields ? this.db().escapeFields(options.fields) : '*'} FROM tags WHERE id IN ('${tagIds.join('\',\'')}')`);
return this.modelSelectAll(`SELECT ${options.fields ? this.db().escapeFields(options.fields) : '*'} FROM tags WHERE id IN (${this.escapeIdsForSql(tagIds)})`);
}

public static async commonTagsByNoteIds(noteIds: string[]) {
Expand All @@ -168,7 +168,7 @@ export default class Tag extends BaseItem {
break;
}
}
return this.modelSelectAll(`SELECT * FROM tags WHERE id IN ('${commonTagIds.join('\',\'')}')`);
return this.modelSelectAll(`SELECT * FROM tags WHERE id IN (${this.escapeIdsForSql(commonTagIds)})`);
}

public static async loadByTitle(title: string): Promise<TagEntity> {
Expand Down
2 changes: 1 addition & 1 deletion packages/lib/services/ResourceService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ export default class ResourceService extends BaseService {

// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
const noteIds = changes.map((a: any) => a.item_id);
const notes = await Note.modelSelectAll(`SELECT id, title, body, encryption_applied FROM notes WHERE id IN ('${noteIds.join('\',\'')}')`);
const notes = await Note.modelSelectAll(`SELECT id, title, body, encryption_applied FROM notes WHERE id IN (${Note.escapeIdsForSql(noteIds)})`);

const noteById = (noteId: string) => {
for (let i = 0; i < notes.length; i++) {
Expand Down
4 changes: 3 additions & 1 deletion packages/lib/services/RevisionService.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,9 @@ export default class RevisionService extends BaseService {
if (!changes.length) break;

const noteIds = changes.map((a) => a.item_id);
const notes = await Note.modelSelectAll(`SELECT * FROM notes WHERE is_conflict = 0 AND encryption_applied = 0 AND id IN ('${noteIds.join('\',\'')}')`);
const notes = await Note.modelSelectAll(`SELECT * FROM notes WHERE is_conflict = 0 AND encryption_applied = 0 AND id IN (${
Note.escapeIdsForSql(noteIds)
})`);

for (let i = 0; i < changes.length; i++) {
const change = changes[i];
Expand Down
4 changes: 2 additions & 2 deletions packages/lib/services/search/SearchEngine.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ export default class SearchEngine {
const notes = await Note.modelSelectAll(`
SELECT ${SearchEngine.relevantFields}
FROM notes
WHERE id IN ('${currentIds.join('\',\'')}') AND is_conflict = 0 AND encryption_applied = 0 AND deleted_time = 0`);
WHERE id IN (${BaseModel.escapeIdsForSql(currentIds)}) AND is_conflict = 0 AND encryption_applied = 0 AND deleted_time = 0`);
const queries = [];

for (let i = 0; i < notes.length; i++) {
Expand Down Expand Up @@ -230,7 +230,7 @@ export default class SearchEngine {
const noteIds = changes.map(a => a.item_id);
const notes = await Note.modelSelectAll(`
SELECT ${SearchEngine.relevantFields}
FROM notes WHERE id IN ('${noteIds.join('\',\'')}') AND is_conflict = 0 AND encryption_applied = 0 AND deleted_time = 0`,
FROM notes WHERE id IN (${Note.escapeIdsForSql(noteIds)}) AND is_conflict = 0 AND encryption_applied = 0 AND deleted_time = 0`,
);

for (let i = 0; i < changes.length; i++) {
Expand Down
8 changes: 4 additions & 4 deletions packages/lib/services/search/SearchEngineUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import SearchEngine from './SearchEngine';
import Note from '../../models/Note';
import Note, { PreviewsOptions } from '../../models/Note';
import Setting from '../../models/Setting';

export interface NotesForQueryOptions {
Expand Down Expand Up @@ -55,10 +55,10 @@ export default class SearchEngineUtils {
todoCompletedAutoAdded = true;
}

// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
const previewOptions: any = { order: [],
const previewOptions: PreviewsOptions = { order: [],
fields: fields,
conditions: [`id IN ('${noteIds.join('\',\'')}')`], ...options };
conditions: [`id IN (${Note.escapeIdsForSql(noteIds)})`],
...options };

const notes = await Note.previews(null, previewOptions);

Expand Down
Loading