mirror of
https://github.com/CherryHQ/cherry-studio.git
synced 2025-12-22 08:40:08 +08:00
fix: pr review
This commit is contained in:
parent
8bab2c8ebc
commit
0dc9658846
@ -797,10 +797,11 @@ class BackupManager {
|
|||||||
async deleteTempBackup(_: Electron.IpcMainInvokeEvent, filePath: string): Promise<boolean> {
|
async deleteTempBackup(_: Electron.IpcMainInvokeEvent, filePath: string): Promise<boolean> {
|
||||||
try {
|
try {
|
||||||
// Security check: only allow deletion within temp directory
|
// Security check: only allow deletion within temp directory
|
||||||
const tempBase = path.join(app.getPath('temp'), 'cherry-studio', 'lan-transfer')
|
const tempBase = path.normalize(path.join(app.getPath('temp'), 'cherry-studio', 'lan-transfer'))
|
||||||
const resolvedPath = path.resolve(filePath)
|
const resolvedPath = path.normalize(path.resolve(filePath))
|
||||||
|
|
||||||
if (!resolvedPath.startsWith(tempBase)) {
|
// Use normalized paths with trailing separator to prevent prefix attacks (e.g., /temp-evil)
|
||||||
|
if (!resolvedPath.startsWith(tempBase + path.sep) && resolvedPath !== tempBase) {
|
||||||
logger.warn(`[BackupManager] Attempted to delete file outside temp directory: ${filePath}`)
|
logger.warn(`[BackupManager] Attempted to delete file outside temp directory: ${filePath}`)
|
||||||
return false
|
return false
|
||||||
}
|
}
|
||||||
|
|||||||
@ -53,6 +53,9 @@ class LanTransferClientService {
|
|||||||
private isConnecting = false
|
private isConnecting = false
|
||||||
private activeTransfer?: ActiveFileTransfer
|
private activeTransfer?: ActiveFileTransfer
|
||||||
private lastConnectOptions?: LocalTransferConnectPayload
|
private lastConnectOptions?: LocalTransferConnectPayload
|
||||||
|
private consecutiveJsonErrors = 0
|
||||||
|
private static readonly MAX_CONSECUTIVE_JSON_ERRORS = 3
|
||||||
|
private reconnectPromise: Promise<void> | null = null
|
||||||
|
|
||||||
constructor() {
|
constructor() {
|
||||||
this.responseManager.setTimeoutCallback(() => void this.disconnect())
|
this.responseManager.setTimeoutCallback(() => void this.disconnect())
|
||||||
@ -296,8 +299,9 @@ class LanTransferClientService {
|
|||||||
transferId,
|
transferId,
|
||||||
reason: 'Cancelled by user'
|
reason: 'Cancelled by user'
|
||||||
})
|
})
|
||||||
} catch {
|
} catch (error) {
|
||||||
// Ignore errors when sending cancel message
|
// Expected when connection is already broken
|
||||||
|
logger.warn('Failed to send cancel message', error as Error)
|
||||||
}
|
}
|
||||||
|
|
||||||
abortTransfer(this.activeTransfer, new Error('Transfer cancelled by user'))
|
abortTransfer(this.activeTransfer, new Error('Transfer cancelled by user'))
|
||||||
@ -317,8 +321,24 @@ class LanTransferClientService {
|
|||||||
throw new Error('No active connection. Please connect to a peer first.')
|
throw new Error('No active connection. Please connect to a peer first.')
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// Prevent concurrent reconnection attempts
|
||||||
|
if (this.reconnectPromise) {
|
||||||
|
logger.debug('Waiting for existing reconnection attempt...')
|
||||||
|
await this.reconnectPromise
|
||||||
|
return
|
||||||
|
}
|
||||||
|
|
||||||
logger.info('Connection lost, attempting to reconnect...')
|
logger.info('Connection lost, attempting to reconnect...')
|
||||||
await this.connectAndHandshake(this.lastConnectOptions)
|
this.reconnectPromise = this.connectAndHandshake(this.lastConnectOptions)
|
||||||
|
.then(() => {
|
||||||
|
this.reconnectPromise = null
|
||||||
|
})
|
||||||
|
.catch((error) => {
|
||||||
|
this.reconnectPromise = null
|
||||||
|
throw error
|
||||||
|
})
|
||||||
|
|
||||||
|
await this.reconnectPromise
|
||||||
}
|
}
|
||||||
|
|
||||||
private async performFileTransfer(
|
private async performFileTransfer(
|
||||||
@ -393,8 +413,21 @@ class LanTransferClientService {
|
|||||||
let payload: Record<string, unknown>
|
let payload: Record<string, unknown>
|
||||||
try {
|
try {
|
||||||
payload = JSON.parse(line)
|
payload = JSON.parse(line)
|
||||||
|
this.consecutiveJsonErrors = 0 // Reset on successful parse
|
||||||
} catch {
|
} catch {
|
||||||
logger.warn('Received invalid JSON control message', { line })
|
this.consecutiveJsonErrors++
|
||||||
|
logger.warn('Received invalid JSON control message', { line, consecutiveErrors: this.consecutiveJsonErrors })
|
||||||
|
|
||||||
|
if (this.consecutiveJsonErrors >= LanTransferClientService.MAX_CONSECUTIVE_JSON_ERRORS) {
|
||||||
|
const message = `Protocol error: ${this.consecutiveJsonErrors} consecutive invalid messages`
|
||||||
|
logger.error(message)
|
||||||
|
this.broadcastClientEvent({
|
||||||
|
type: 'error',
|
||||||
|
message,
|
||||||
|
timestamp: Date.now()
|
||||||
|
})
|
||||||
|
this.consecutiveJsonErrors = 0
|
||||||
|
}
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@ -23,6 +23,10 @@ export const BINARY_TYPE_FILE_CHUNK = 0x01
|
|||||||
* @returns true if data was buffered, false if backpressure should be applied
|
* @returns true if data was buffered, false if backpressure should be applied
|
||||||
*/
|
*/
|
||||||
export function sendBinaryChunk(socket: Socket, transferId: string, chunkIndex: number, data: Buffer): boolean {
|
export function sendBinaryChunk(socket: Socket, transferId: string, chunkIndex: number, data: Buffer): boolean {
|
||||||
|
if (!socket || socket.destroyed || !socket.writable) {
|
||||||
|
throw new Error('Socket is not writable')
|
||||||
|
}
|
||||||
|
|
||||||
const tidBuffer = Buffer.from(transferId, 'utf8')
|
const tidBuffer = Buffer.from(transferId, 'utf8')
|
||||||
const tidLen = tidBuffer.length
|
const tidLen = tidBuffer.length
|
||||||
|
|
||||||
|
|||||||
@ -9,6 +9,9 @@ import type { ConnectionContext } from '../types'
|
|||||||
|
|
||||||
export const HANDSHAKE_PROTOCOL_VERSION = '1'
|
export const HANDSHAKE_PROTOCOL_VERSION = '1'
|
||||||
|
|
||||||
|
/** Maximum size for line buffer to prevent memory exhaustion from malicious peers */
|
||||||
|
const MAX_LINE_BUFFER_SIZE = 1024 * 1024 // 1MB limit for control messages
|
||||||
|
|
||||||
const logger = loggerService.withContext('LanTransferConnection')
|
const logger = loggerService.withContext('LanTransferConnection')
|
||||||
|
|
||||||
/**
|
/**
|
||||||
@ -74,6 +77,14 @@ export function createDataHandler(onControlLine: (line: string) => void): {
|
|||||||
},
|
},
|
||||||
handleData(chunk: Buffer) {
|
handleData(chunk: Buffer) {
|
||||||
lineBuffer += chunk.toString('utf8')
|
lineBuffer += chunk.toString('utf8')
|
||||||
|
|
||||||
|
// Prevent memory exhaustion from malicious peers sending data without newlines
|
||||||
|
if (lineBuffer.length > MAX_LINE_BUFFER_SIZE) {
|
||||||
|
logger.error('Line buffer exceeded maximum size, resetting')
|
||||||
|
lineBuffer = ''
|
||||||
|
throw new Error('Control message too large')
|
||||||
|
}
|
||||||
|
|
||||||
let newlineIndex = lineBuffer.indexOf('\n')
|
let newlineIndex = lineBuffer.indexOf('\n')
|
||||||
while (newlineIndex !== -1) {
|
while (newlineIndex !== -1) {
|
||||||
const line = lineBuffer.slice(0, newlineIndex).trim()
|
const line = lineBuffer.slice(0, newlineIndex).trim()
|
||||||
|
|||||||
@ -32,8 +32,17 @@ export async function validateFile(filePath: string): Promise<{ stats: fs.Stats;
|
|||||||
let stats: fs.Stats
|
let stats: fs.Stats
|
||||||
try {
|
try {
|
||||||
stats = await fs.promises.stat(filePath)
|
stats = await fs.promises.stat(filePath)
|
||||||
} catch {
|
} catch (error) {
|
||||||
|
const nodeError = error as NodeJS.ErrnoException
|
||||||
|
if (nodeError.code === 'ENOENT') {
|
||||||
throw new Error(`File not found: ${filePath}`)
|
throw new Error(`File not found: ${filePath}`)
|
||||||
|
} else if (nodeError.code === 'EACCES') {
|
||||||
|
throw new Error(`Permission denied: ${filePath}`)
|
||||||
|
} else if (nodeError.code === 'ENOTDIR') {
|
||||||
|
throw new Error(`Invalid path: ${filePath}`)
|
||||||
|
} else {
|
||||||
|
throw new Error(`Cannot access file: ${filePath} (${nodeError.code || 'unknown error'})`)
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if (!stats.isFile()) {
|
if (!stats.isFile()) {
|
||||||
|
|||||||
Loading…
Reference in New Issue
Block a user