diff --git a/packages/cli/src/ui/commands/chatCommand.ts b/packages/cli/src/ui/commands/chatCommand.ts index 14714df3..1c9029a9 100644 --- a/packages/cli/src/ui/commands/chatCommand.ts +++ b/packages/cli/src/ui/commands/chatCommand.ts @@ -15,6 +15,7 @@ import { CommandKind, SlashCommandActionReturn, } from './types.js'; +import { decodeTagName } from '@google/gemini-cli-core'; import path from 'path'; import { HistoryItemWithoutId, MessageType } from '../types.js'; @@ -41,8 +42,9 @@ const getSavedChatTags = async ( if (file.startsWith(file_head) && file.endsWith(file_tail)) { const filePath = path.join(geminiDir, file); const stats = await fsPromises.stat(filePath); + const tagName = file.slice(file_head.length, -file_tail.length); chatDetails.push({ - name: file.slice(file_head.length, -file_tail.length), + name: decodeTagName(tagName), mtime: stats.mtime, }); } @@ -147,7 +149,7 @@ const saveCommand: SlashCommand = { return { type: 'message', messageType: 'info', - content: `Conversation checkpoint saved with tag: ${tag}.`, + content: `Conversation checkpoint saved with tag: ${decodeTagName(tag)}.`, }; } else { return { @@ -183,7 +185,7 @@ const resumeCommand: SlashCommand = { return { type: 'message', messageType: 'info', - content: `No saved checkpoint found with tag: ${tag}.`, + content: `No saved checkpoint found with tag: ${decodeTagName(tag)}.`, }; } @@ -252,13 +254,13 @@ const deleteCommand: SlashCommand = { return { type: 'message', messageType: 'info', - content: `Conversation checkpoint '${tag}' has been deleted.`, + content: `Conversation checkpoint '${decodeTagName(tag)}' has been deleted.`, }; } else { return { type: 'message', messageType: 'error', - content: `Error: No checkpoint found with tag '${tag}'.`, + content: `Error: No checkpoint found with tag '${decodeTagName(tag)}'.`, }; } }, diff --git a/packages/core/src/core/logger.test.ts b/packages/core/src/core/logger.test.ts index d032e2d4..0ad1cea9 100644 --- a/packages/core/src/core/logger.test.ts +++ b/packages/core/src/core/logger.test.ts @@ -13,8 +13,14 @@ import { afterEach, afterAll, } from 'vitest'; -import { Logger, MessageSenderType, LogEntry } from './logger.js'; -import { promises as fs } from 'node:fs'; +import { + Logger, + MessageSenderType, + LogEntry, + encodeTagName, + decodeTagName, +} from './logger.js'; +import { promises as fs, existsSync } from 'node:fs'; import path from 'node:path'; import { Content } from '@google/genai'; @@ -394,15 +400,28 @@ describe('Logger', () => { ]; it.each([ - { tag: 'test-tag', sanitizedTag: 'test-tag' }, - { tag: 'invalid/?*!', sanitizedTag: 'invalid' }, - { tag: '/?*!', sanitizedTag: 'default' }, - { tag: '../../secret', sanitizedTag: 'secret' }, - ])('should save a checkpoint', async ({ tag, sanitizedTag }) => { + { + tag: 'test-tag', + encodedTag: 'test-tag', + }, + { + tag: '你好世界', + encodedTag: '%E4%BD%A0%E5%A5%BD%E4%B8%96%E7%95%8C', + }, + { + tag: 'japanese-ひらがなひらがな形声', + encodedTag: + 'japanese-%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA%E5%BD%A2%E5%A3%B0', + }, + { + tag: '../../secret', + encodedTag: '..%2F..%2Fsecret', + }, + ])('should save a checkpoint', async ({ tag, encodedTag }) => { await logger.saveCheckpoint(conversation, tag); const taggedFilePath = path.join( TEST_GEMINI_DIR, - `checkpoint-${sanitizedTag}.json`, + `checkpoint-${encodedTag}.json`, ); const fileContent = await fs.readFile(taggedFilePath, 'utf-8'); expect(JSON.parse(fileContent)).toEqual(conversation); @@ -438,18 +457,31 @@ describe('Logger', () => { }); it.each([ - { tag: 'load-tag', sanitizedTag: 'load-tag' }, - { tag: 'inv/load?*!', sanitizedTag: 'invload' }, - { tag: '/?*!', sanitizedTag: 'default' }, - { tag: '../../secret', sanitizedTag: 'secret' }, - ])('should load from a checkpoint', async ({ tag, sanitizedTag }) => { + { + tag: 'test-tag', + encodedTag: 'test-tag', + }, + { + tag: '你好世界', + encodedTag: '%E4%BD%A0%E5%A5%BD%E4%B8%96%E7%95%8C', + }, + { + tag: 'japanese-ひらがなひらがな形声', + encodedTag: + 'japanese-%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA%E3%81%B2%E3%82%89%E3%81%8C%E3%81%AA%E5%BD%A2%E5%A3%B0', + }, + { + tag: '../../secret', + encodedTag: '..%2F..%2Fsecret', + }, + ])('should load from a checkpoint', async ({ tag, encodedTag }) => { const taggedConversation = [ ...conversation, { role: 'user', parts: [{ text: 'hello' }] }, ]; const taggedFilePath = path.join( TEST_GEMINI_DIR, - `checkpoint-${sanitizedTag}.json`, + `checkpoint-${encodedTag}.json`, ); await fs.writeFile( taggedFilePath, @@ -458,6 +490,8 @@ describe('Logger', () => { const loaded = await logger.loadCheckpoint(tag); expect(loaded).toEqual(taggedConversation); + expect(encodeTagName(tag)).toBe(encodedTag); + expect(decodeTagName(encodedTag)).toBe(tag); }); it('should return an empty array if a tagged checkpoint file does not exist', async () => { @@ -473,9 +507,10 @@ describe('Logger', () => { it('should return an empty array if the file contains invalid JSON', async () => { const tag = 'invalid-json-tag'; + const encodedTag = 'invalid-json-tag'; const taggedFilePath = path.join( TEST_GEMINI_DIR, - `checkpoint-${tag}.json`, + `checkpoint-${encodedTag}.json`, ); await fs.writeFile(taggedFilePath, 'invalid json'); const consoleErrorSpy = vi @@ -508,12 +543,13 @@ describe('Logger', () => { { role: 'user', parts: [{ text: 'Content to be deleted' }] }, ]; const tag = 'delete-me'; + const encodedTag = 'delete-me'; let taggedFilePath: string; beforeEach(async () => { taggedFilePath = path.join( TEST_GEMINI_DIR, - `${CHECKPOINT_FILE_NAME.replace('.json', '')}-${tag}.json`, + `checkpoint-${encodedTag}.json`, ); // Create a file to be deleted await fs.writeFile(taggedFilePath, JSON.stringify(conversation)); @@ -527,6 +563,30 @@ describe('Logger', () => { await expect(fs.access(taggedFilePath)).rejects.toThrow(/ENOENT/); }); + it('should delete both new and old checkpoint files if they exist', async () => { + const oldTag = 'delete-me(old)'; + const oldStylePath = path.join( + TEST_GEMINI_DIR, + `checkpoint-${oldTag}.json`, + ); + const newStylePath = logger['_checkpointPath'](oldTag); + + // Create both files + await fs.writeFile(oldStylePath, '{}'); + await fs.writeFile(newStylePath, '{}'); + + // Verify both files exist before deletion + expect(existsSync(oldStylePath)).toBe(true); + expect(existsSync(newStylePath)).toBe(true); + + const result = await logger.deleteCheckpoint(oldTag); + expect(result).toBe(true); + + // Verify both are gone + expect(existsSync(oldStylePath)).toBe(false); + expect(existsSync(newStylePath)).toBe(false); + }); + it('should return false if the checkpoint file does not exist', async () => { const result = await logger.deleteCheckpoint('non-existent-tag'); expect(result).toBe(false); @@ -535,7 +595,9 @@ describe('Logger', () => { it('should re-throw an error if file deletion fails for reasons other than not existing', async () => { // Simulate a different error (e.g., permission denied) vi.spyOn(fs, 'unlink').mockRejectedValueOnce( - new Error('EACCES: permission denied'), + Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }), ); const consoleErrorSpy = vi .spyOn(console, 'error') @@ -567,10 +629,14 @@ describe('Logger', () => { describe('checkpointExists', () => { const tag = 'exists-test'; + const encodedTag = 'exists-test'; let taggedFilePath: string; beforeEach(() => { - taggedFilePath = path.join(TEST_GEMINI_DIR, `checkpoint-${tag}.json`); + taggedFilePath = path.join( + TEST_GEMINI_DIR, + `checkpoint-${encodedTag}.json`, + ); }); it('should return true if the checkpoint file exists', async () => { @@ -595,7 +661,9 @@ describe('Logger', () => { it('should re-throw an error if fs.access fails for reasons other than not existing', async () => { vi.spyOn(fs, 'access').mockRejectedValueOnce( - new Error('EACCES: permission denied'), + Object.assign(new Error('EACCES: permission denied'), { + code: 'EACCES', + }), ); const consoleErrorSpy = vi .spyOn(console, 'error') @@ -605,12 +673,37 @@ describe('Logger', () => { 'EACCES: permission denied', ); expect(consoleErrorSpy).toHaveBeenCalledWith( - `Failed to check checkpoint existence for ${taggedFilePath}:`, + `Failed to check checkpoint existence for path for tag "${tag}":`, expect.any(Error), ); }); }); + describe('Backward compatibility', () => { + const conversation: Content[] = [ + { role: 'user', parts: [{ text: 'Hello' }] }, + { role: 'model', parts: [{ text: 'Hi there' }] }, + ]; + it('should load from a checkpoint with a raw special character tag', async () => { + const taggedConversation = [ + ...conversation, + { role: 'user', parts: [{ text: 'hello' }] }, + ]; + const tag = 'special(char)'; + const taggedFilePath = path.join( + TEST_GEMINI_DIR, + `checkpoint-${tag}.json`, + ); + await fs.writeFile( + taggedFilePath, + JSON.stringify(taggedConversation, null, 2), + ); + + const loaded = await logger.loadCheckpoint(tag); + expect(loaded).toEqual(taggedConversation); + }); + }); + describe('close', () => { it('should reset logger state', async () => { await logger.logMessage(MessageSenderType.USER, 'A message'); diff --git a/packages/core/src/core/logger.ts b/packages/core/src/core/logger.ts index f4857f47..57b5bdf2 100644 --- a/packages/core/src/core/logger.ts +++ b/packages/core/src/core/logger.ts @@ -23,6 +23,42 @@ export interface LogEntry { message: string; } +// This regex matches any character that is NOT a letter (a-z, A-Z), +// a number (0-9), a hyphen (-), an underscore (_), or a dot (.). + +/** + * Encodes a string to be safe for use as a filename. + * + * It replaces any characters that are not alphanumeric or one of `_`, `-`, `.` + * with a URL-like percent-encoding (`%` followed by the 2-digit hex code). + * + * @param str The input string to encode. + * @returns The encoded, filename-safe string. + */ +export function encodeTagName(str: string): string { + return encodeURIComponent(str); +} + +/** + * Decodes a string that was encoded with the `encode` function. + * + * It finds any percent-encoded characters and converts them back to their + * original representation. + * + * @param str The encoded string to decode. + * @returns The decoded, original string. + */ +export function decodeTagName(str: string): string { + try { + return decodeURIComponent(str); + } catch (_e) { + // Fallback for old, potentially malformed encoding + return str.replace(/%([0-9A-F]{2})/g, (_, hex) => + String.fromCharCode(parseInt(hex, 16)), + ); + } +} + export class Logger { private geminiDir: string | undefined; private logFilePath: string | undefined; @@ -231,19 +267,46 @@ export class Logger { } } - _checkpointPath(tag: string): string { + private _checkpointPath(tag: string): string { if (!tag.length) { throw new Error('No checkpoint tag specified.'); } if (!this.geminiDir) { throw new Error('Checkpoint file path not set.'); } - // Sanitize tag to prevent directory traversal attacks - let sanitizedTag = tag.replace(/[^a-zA-Z0-9-_]/g, ''); - if (!sanitizedTag) { - sanitizedTag = 'default'; + // Encode the tag to handle all special characters safely. + const encodedTag = encodeTagName(tag); + return path.join(this.geminiDir, `checkpoint-${encodedTag}.json`); + } + + private async _getCheckpointPath(tag: string): Promise { + // 1. Check for the new encoded path first. + const newPath = this._checkpointPath(tag); + try { + await fs.access(newPath); + return newPath; // Found it, use the new path. + } catch (error) { + const nodeError = error as NodeJS.ErrnoException; + if (nodeError.code !== 'ENOENT') { + throw error; // A real error occurred, rethrow it. + } + // It was not found, so we'll check the old path next. } - return path.join(this.geminiDir, `checkpoint-${sanitizedTag}.json`); + + // 2. Fallback for backward compatibility: check for the old raw path. + const oldPath = path.join(this.geminiDir!, `checkpoint-${tag}.json`); + try { + await fs.access(oldPath); + return oldPath; // Found it, use the old path. + } catch (error) { + const nodeError = error as NodeJS.ErrnoException; + if (nodeError.code !== 'ENOENT') { + throw error; // A real error occurred, rethrow it. + } + } + + // 3. If neither path exists, return the new encoded path as the canonical one. + return newPath; } async saveCheckpoint(conversation: Content[], tag: string): Promise { @@ -253,6 +316,7 @@ export class Logger { ); return; } + // Always save with the new encoded path. const path = this._checkpointPath(tag); try { await fs.writeFile(path, JSON.stringify(conversation, null, 2), 'utf-8'); @@ -269,7 +333,7 @@ export class Logger { return []; } - const path = this._checkpointPath(tag); + const path = await this._getCheckpointPath(tag); try { const fileContent = await fs.readFile(path, 'utf-8'); const parsedContent = JSON.parse(fileContent); @@ -281,6 +345,11 @@ export class Logger { } return parsedContent as Content[]; } catch (error) { + const nodeError = error as NodeJS.ErrnoException; + if (nodeError.code === 'ENOENT') { + // This is okay, it just means the checkpoint doesn't exist in either format. + return []; + } console.error(`Failed to read or parse checkpoint file ${path}:`, error); return []; } @@ -294,20 +363,39 @@ export class Logger { return false; } - const path = this._checkpointPath(tag); + let deletedSomething = false; + // 1. Attempt to delete the new encoded path. + const newPath = this._checkpointPath(tag); try { - await fs.unlink(path); - return true; + await fs.unlink(newPath); + deletedSomething = true; } catch (error) { const nodeError = error as NodeJS.ErrnoException; - if (nodeError.code === 'ENOENT') { - // File doesn't exist, which is fine. - return false; + if (nodeError.code !== 'ENOENT') { + console.error(`Failed to delete checkpoint file ${newPath}:`, error); + throw error; // Rethrow unexpected errors } - console.error(`Failed to delete checkpoint file ${path}:`, error); - throw error; + // It's okay if it doesn't exist. } + + // 2. Attempt to delete the old raw path for backward compatibility. + const oldPath = path.join(this.geminiDir!, `checkpoint-${tag}.json`); + if (newPath !== oldPath) { + try { + await fs.unlink(oldPath); + deletedSomething = true; + } catch (error) { + const nodeError = error as NodeJS.ErrnoException; + if (nodeError.code !== 'ENOENT') { + console.error(`Failed to delete checkpoint file ${oldPath}:`, error); + throw error; // Rethrow unexpected errors + } + // It's okay if it doesn't exist. + } + } + + return deletedSomething; } async checkpointExists(tag: string): Promise { @@ -316,17 +404,23 @@ export class Logger { 'Logger not initialized. Cannot check for checkpoint existence.', ); } - const filePath = this._checkpointPath(tag); + let filePath: string | undefined; try { + filePath = await this._getCheckpointPath(tag); + // We need to check for existence again, because _getCheckpointPath + // returns a canonical path even if it doesn't exist yet. await fs.access(filePath); return true; } catch (error) { const nodeError = error as NodeJS.ErrnoException; if (nodeError.code === 'ENOENT') { - return false; + return false; // It truly doesn't exist in either format. } + // A different error occurred. console.error( - `Failed to check checkpoint existence for ${filePath}:`, + `Failed to check checkpoint existence for ${ + filePath ?? `path for tag "${tag}"` + }:`, error, ); throw error;