From 4aad1da72d0ab08b423ff30d86c453414e2a43cc Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Thu, 5 Mar 2026 22:56:43 +0100 Subject: [PATCH] =?utf8?q?fix(charging-station):=20audit=20fixes=20?= =?utf8?q?=E2=80=94=20error=20handling,=20OCPP=20conformance,=20DRY,=20typ?= =?utf8?q?e=20safety?= MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit - Replace .catch(EMPTY_FUNCTION) with proper error logging (F-001/F-002/F-003) - Add fail-closed security comment in UIServerSecurity (F-004) - Convert FIXME to actionable TODO in Bootstrap (F-005) - Implement relative charging profile duration handling per K01.FR.41-42 (F-006) - Restart heartbeat and WebSocket ping on template change (F-007) - Consolidate repeated type casts in convertDeprecatedTemplateKey (F-008) - Remove duplicate console.warn calls and unused chalk import in UIServerFactory (F-010) - Extract logPrefix to AbstractUIServer base class, removing duplicates (F-011) - Extract matchesConfigurationKey predicate in ConfigurationKeyUtils (F-012) - Add failure counter and error summary for reservation removal (F-013) --- .../AutomaticTransactionGenerator.ts | 4 +- src/charging-station/Bootstrap.ts | 6 ++- src/charging-station/ChargingStation.ts | 7 +++- src/charging-station/ConfigurationKeyUtils.ts | 17 +++++--- src/charging-station/Helpers.ts | 39 +++++++++++++++---- .../ui-server/AbstractUIServer.ts | 14 ++++++- .../ui-server/UIHttpServer.ts | 19 ++------- .../ui-server/UIServerFactory.ts | 5 --- .../ui-server/UIServerSecurity.ts | 1 + .../ui-server/UIWebSocketServer.ts | 14 +------ 10 files changed, 73 insertions(+), 53 deletions(-) diff --git a/src/charging-station/AutomaticTransactionGenerator.ts b/src/charging-station/AutomaticTransactionGenerator.ts index 8106d29a..22da82a8 100644 --- a/src/charging-station/AutomaticTransactionGenerator.ts +++ b/src/charging-station/AutomaticTransactionGenerator.ts @@ -101,7 +101,9 @@ export class AutomaticTransactionGenerator { throw new BaseError(`Connector ${connectorId.toString()} does not exist`) } if (this.connectorsStatus.get(connectorId)?.start === false) { - this.internalStartConnector(connectorId, stopAbsoluteDuration).catch(Constants.EMPTY_FUNCTION) + this.internalStartConnector(connectorId, stopAbsoluteDuration).catch((error: unknown) => + logger.error(`${this.logPrefix(connectorId)} Error while starting connector:`, error) + ) } else if (this.connectorsStatus.get(connectorId)?.start === true) { logger.warn(`${this.logPrefix(connectorId)} is already started on connector`) } diff --git a/src/charging-station/Bootstrap.ts b/src/charging-station/Bootstrap.ts index 9117c09f..65831ac5 100644 --- a/src/charging-station/Bootstrap.ts +++ b/src/charging-station/Bootstrap.ts @@ -553,7 +553,7 @@ export class Bootstrap extends EventEmitter { this.uiServerStarted = false } this.initializeCounters() - // FIXME: initialize worker implementation only if the worker section has changed + // TODO: compare worker configuration hash to skip unnecessary re-initialization this.initializeWorkerImplementation( Configuration.getConfigurationSection(ConfigurationSection.worker) ) @@ -616,7 +616,9 @@ export class Bootstrap extends EventEmitter { this.storage.storePerformanceStatistics as ( performanceStatistics: Statistics ) => Promise - )(data).catch(Constants.EMPTY_FUNCTION) + )(data).catch((error: unknown) => { + logger.error(`${this.logPrefix()} Error while storing performance statistics:`, error) + }) } else { ;(this.storage?.storePerformanceStatistics as (performanceStatistics: Statistics) => void)( data diff --git a/src/charging-station/ChargingStation.ts b/src/charging-station/ChargingStation.ts index db4469a3..f3df70b3 100644 --- a/src/charging-station/ChargingStation.ts +++ b/src/charging-station/ChargingStation.ts @@ -813,7 +813,9 @@ export class ChargingStation extends EventEmitter { // Handle WebSocket message this.wsConnection.on('message', data => { - this.onMessage(data).catch(Constants.EMPTY_FUNCTION) + this.onMessage(data).catch((error: unknown) => + logger.error(`${this.logPrefix()} Error while processing WebSocket message:`, error) + ) }) // Handle WebSocket error this.wsConnection.on('error', this.onError.bind(this)) @@ -951,7 +953,8 @@ export class ChargingStation extends EventEmitter { } else { this.performanceStatistics?.stop() } - // FIXME?: restart heartbeat and WebSocket ping when their interval values have changed + this.restartHeartbeat() + this.restartWebSocketPing() } catch (error) { logger.error( `${this.logPrefix()} ${FileType.ChargingStationTemplate} file monitoring error:`, diff --git a/src/charging-station/ConfigurationKeyUtils.ts b/src/charging-station/ConfigurationKeyUtils.ts index 709914b8..e3979136 100644 --- a/src/charging-station/ConfigurationKeyUtils.ts +++ b/src/charging-station/ConfigurationKeyUtils.ts @@ -18,6 +18,15 @@ interface DeleteConfigurationKeyParams { save?: boolean } +const matchesConfigurationKey = ( + configElement: ConfigurationKey, + key: ConfigurationKeyType, + caseInsensitive: boolean +): boolean => + caseInsensitive + ? configElement.key.toLowerCase() === key.toLowerCase() + : configElement.key === key + export const getConfigurationKey = ( chargingStation: ChargingStation, key: ConfigurationKeyType, @@ -25,9 +34,7 @@ export const getConfigurationKey = ( ): ConfigurationKey | undefined => { if (!Array.isArray(chargingStation.ocppConfiguration?.configurationKey)) return undefined return chargingStation.ocppConfiguration.configurationKey.find(configElement => - caseInsensitive - ? configElement.key.toLowerCase() === key.toLowerCase() - : configElement.key === key + matchesConfigurationKey(configElement, key, caseInsensitive) ) } @@ -40,9 +47,7 @@ const getConfigurationKeyIndex = ( return -1 } return chargingStation.ocppConfiguration.configurationKey.findIndex(configElement => - caseInsensitive - ? configElement.key.toLowerCase() === key.toLowerCase() - : configElement.key === key + matchesConfigurationKey(configElement, key, caseInsensitive) ) } diff --git a/src/charging-station/Helpers.ts b/src/charging-station/Helpers.ts index 231ddda5..761d976a 100644 --- a/src/charging-station/Helpers.ts +++ b/src/charging-station/Helpers.ts @@ -169,13 +169,20 @@ export const removeExpiredReservations = async ( chargingStation.removeReservation(reservation, ReservationTerminationReason.EXPIRED) ) ) + let failureCount = 0 for (const result of results) { if (result.status === 'rejected') { + ++failureCount logger.warn( `${chargingStation.logPrefix()} ${moduleName}.removeExpiredReservations: reservation removal failed: ${String(result.reason)}` ) } } + if (failureCount > 0) { + logger.error( + `${chargingStation.logPrefix()} ${moduleName}.removeExpiredReservations: ${failureCount.toString()}/${reservations.length.toString()} expired reservation removal(s) failed` + ) + } } export const getNumberOfReservableConnectors = ( @@ -993,13 +1000,13 @@ const convertDeprecatedTemplateKey = ( deprecatedKey: string, key?: string ): void => { - if (template[deprecatedKey as keyof ChargingStationTemplate] != null) { + const templateRecord = template as unknown as Record + if (templateRecord[deprecatedKey] != null) { if (key != null) { - ;(template as unknown as Record)[key] = - template[deprecatedKey as keyof ChargingStationTemplate] + templateRecord[key] = templateRecord[deprecatedKey] } // eslint-disable-next-line @typescript-eslint/no-dynamic-delete - delete template[deprecatedKey as keyof ChargingStationTemplate] + delete templateRecord[deprecatedKey] } } @@ -1218,10 +1225,28 @@ export const prepareChargingProfileKind = ( ) delete chargingSchedule.startSchedule } - if (connectorStatus?.transactionStarted === true) { - chargingSchedule.startSchedule = connectorStatus.transactionStart + if (connectorStatus?.transactionStarted !== true) { + logger.debug( + `${logPrefix} ${moduleName}.prepareChargingProfileKind: Relative charging profile id ${chargingProfileId} has no active transaction, cannot be evaluated` + ) + return false + } + chargingSchedule.startSchedule = connectorStatus.transactionStart + if (chargingSchedule.startSchedule == null) { + logger.warn( + `${logPrefix} ${moduleName}.prepareChargingProfileKind: Relative charging profile id ${chargingProfileId} has active transaction without start date` + ) + return false + } + if (chargingSchedule.duration != null) { + const elapsedSeconds = differenceInSeconds(currentDate, chargingSchedule.startSchedule) + if (elapsedSeconds > chargingSchedule.duration) { + logger.debug( + `${logPrefix} ${moduleName}.prepareChargingProfileKind: Relative charging profile id ${chargingProfileId} duration ${chargingSchedule.duration.toString()}s exceeded (elapsed: ${elapsedSeconds.toString()}s)` + ) + return false + } } - // FIXME: handle relative charging profile duration break } return true diff --git a/src/charging-station/ui-server/AbstractUIServer.ts b/src/charging-station/ui-server/AbstractUIServer.ts index 20bf50fc..3505f65f 100644 --- a/src/charging-station/ui-server/AbstractUIServer.ts +++ b/src/charging-station/ui-server/AbstractUIServer.ts @@ -20,7 +20,7 @@ import { type UIServerConfiguration, type UUIDv4, } from '../../types/index.js' -import { isEmpty, logger } from '../../utils/index.js' +import { isEmpty, isNotEmptyString, logger, logPrefix } from '../../utils/index.js' import { UIServiceFactory } from './ui-services/UIServiceFactory.js' import { isValidCredential } from './UIServerSecurity.js' import { getUsernameAndPasswordFromAuthorizationToken } from './UIServerUtils.js' @@ -31,6 +31,8 @@ export abstract class AbstractUIServer { protected readonly httpServer: Http2Server | Server protected readonly responseHandlers: Map + protected abstract readonly uiServerType: string + protected readonly uiServices: Map private readonly chargingStations: Map @@ -105,7 +107,15 @@ export abstract class AbstractUIServer { return [...this.chargingStations.values()] } - public abstract logPrefix (moduleName?: string, methodName?: string, prefixSuffix?: string): string + public logPrefix = (modName?: string, methodName?: string, prefixSuffix?: string): string => { + const logMsgPrefix = + prefixSuffix != null ? `${this.uiServerType} ${prefixSuffix}` : this.uiServerType + const logMsg = + isNotEmptyString(modName) && isNotEmptyString(methodName) + ? ` ${logMsgPrefix} | ${modName}.${methodName}:` + : ` ${logMsgPrefix} |` + return logPrefix(logMsg) + } public async sendInternalRequest (request: ProtocolRequest): Promise { const protocolVersion = ProtocolVersion['0.0.1'] diff --git a/src/charging-station/ui-server/UIHttpServer.ts b/src/charging-station/ui-server/UIHttpServer.ts index 7b50c195..cf6fee38 100644 --- a/src/charging-station/ui-server/UIHttpServer.ts +++ b/src/charging-station/ui-server/UIHttpServer.ts @@ -17,13 +17,7 @@ import { type UIServerConfiguration, type UUIDv4, } from '../../types/index.js' -import { - generateUUID, - isNotEmptyString, - JSONStringify, - logger, - logPrefix, -} from '../../utils/index.js' +import { generateUUID, JSONStringify, logger } from '../../utils/index.js' import { AbstractUIServer } from './AbstractUIServer.js' import { createBodySizeLimiter, @@ -47,6 +41,8 @@ enum HttpMethods { } export class UIHttpServer extends AbstractUIServer { + protected override readonly uiServerType = 'UI HTTP Server' + private readonly acceptsGzip: Map public constructor (protected override readonly uiServerConfiguration: UIServerConfiguration) { @@ -54,15 +50,6 @@ export class UIHttpServer extends AbstractUIServer { this.acceptsGzip = new Map() } - public logPrefix = (modName?: string, methodName?: string, prefixSuffix?: string): string => { - const logMsgPrefix = prefixSuffix != null ? `UI HTTP Server ${prefixSuffix}` : 'UI HTTP Server' - const logMsg = - isNotEmptyString(modName) && isNotEmptyString(methodName) - ? ` ${logMsgPrefix} | ${modName}.${methodName}:` - : ` ${logMsgPrefix} |` - return logPrefix(logMsg) - } - public sendRequest (request: ProtocolRequest): void { switch (this.uiServerConfiguration.version) { case ApplicationProtocolVersion.VERSION_20: diff --git a/src/charging-station/ui-server/UIServerFactory.ts b/src/charging-station/ui-server/UIServerFactory.ts index 3bdf68f6..b0c87bba 100644 --- a/src/charging-station/ui-server/UIServerFactory.ts +++ b/src/charging-station/ui-server/UIServerFactory.ts @@ -1,5 +1,3 @@ -import chalk from 'chalk' - import type { AbstractUIServer } from './AbstractUIServer.js' import { BaseError } from '../../exception/index.js' @@ -48,7 +46,6 @@ export class UIServerFactory { ) { const logMsg = `Non loopback address in '${ConfigurationSection.uiServer}' configuration section without authentication enabled. This is not recommended` logger.warn(`${UIServerFactory.logPrefix()} ${logMsg}`) - console.warn(chalk.yellow(logMsg)) } if ( uiServerConfiguration.type === ApplicationProtocol.WS && @@ -56,7 +53,6 @@ export class UIServerFactory { ) { const logMsg = `Only version ${ApplicationProtocolVersion.VERSION_11} with application protocol type '${uiServerConfiguration.type}' is supported in '${ConfigurationSection.uiServer}' configuration section. Falling back to version ${ApplicationProtocolVersion.VERSION_11}` logger.warn(`${UIServerFactory.logPrefix()} ${logMsg}`) - console.warn(chalk.yellow(logMsg)) uiServerConfiguration.version = ApplicationProtocolVersion.VERSION_11 } switch (uiServerConfiguration.type) { @@ -77,7 +73,6 @@ export class UIServerFactory { ApplicationProtocol.WS }'` logger.warn(`${UIServerFactory.logPrefix()} ${logMsg}`) - console.warn(logMsg) } return new UIWebSocketServer(uiServerConfiguration) } diff --git a/src/charging-station/ui-server/UIServerSecurity.ts b/src/charging-station/ui-server/UIServerSecurity.ts index 306a0644..6295cf54 100644 --- a/src/charging-station/ui-server/UIServerSecurity.ts +++ b/src/charging-station/ui-server/UIServerSecurity.ts @@ -27,6 +27,7 @@ export const isValidCredential = (provided: string, expected: string): boolean = return timingSafeEqual(paddedProvided, paddedExpected) } catch { + // Intentional: fail closed on any comparison error (e.g., encoding issues) return false } } diff --git a/src/charging-station/ui-server/UIWebSocketServer.ts b/src/charging-station/ui-server/UIWebSocketServer.ts index d42cf798..332d27ff 100644 --- a/src/charging-station/ui-server/UIWebSocketServer.ts +++ b/src/charging-station/ui-server/UIWebSocketServer.ts @@ -13,10 +13,8 @@ import { } from '../../types/index.js' import { getWebSocketCloseEventStatusString, - isNotEmptyString, JSONStringify, logger, - logPrefix, validateUUID, } from '../../utils/index.js' import { AbstractUIServer } from './AbstractUIServer.js' @@ -30,6 +28,8 @@ import { const moduleName = 'UIWebSocketServer' export class UIWebSocketServer extends AbstractUIServer { + protected override readonly uiServerType = 'UI WebSocket Server' + private readonly webSocketServer: WebSocketServer public constructor (protected override readonly uiServerConfiguration: UIServerConfiguration) { @@ -56,16 +56,6 @@ export class UIWebSocketServer extends AbstractUIServer { }) } - public logPrefix = (modName?: string, methodName?: string, prefixSuffix?: string): string => { - const logMsgPrefix = - prefixSuffix != null ? `UI WebSocket Server ${prefixSuffix}` : 'UI WebSocket Server' - const logMsg = - isNotEmptyString(modName) && isNotEmptyString(methodName) - ? ` ${logMsgPrefix} | ${modName}.${methodName}:` - : ` ${logMsgPrefix} |` - return logPrefix(logMsg) - } - public sendRequest (request: ProtocolRequest): void { this.broadcastToClients(JSON.stringify(request)) } -- 2.53.0