Add input validation for trading bot IPC handler by FarahR01 · Pull Request #36 · NubleX/NeutronTrader

FarahR01 · 2026-03-02T13:59:13Z

Problem

The start-trading-bot IPC handler does not validate trading configuration before execution.
This allows a compromised renderer (XSS) to start a bot with:

Unlimited trade amounts (e.g., 999999999 units)
Invalid intervals that bypass risk manager
No validation against position size limits

Solution

Created TradingBotValidator class that validates:
✓ Amount is positive and within risk manager limits
✓ Symbol format is valid (BASE/QUOTE)
✓ Interval is one of allowed values (1m, 5m, 15m, etc.)
✓ Strategy exists (simpleMovingAverage, relativeStrengthIndex, bollingerBands)
✓ API credentials have valid format

Changes

New: electron/validators/tradingBotValidator.js (132 lines)
Modified: main.js (added 1 import + validation call)

Impact

Prevents uncontrolled trades from compromised renderer while respecting risk manager position size limits.

- Create TradingBotValidator class to validate: - Amount is positive and within risk manager limits - Symbol format is valid (BASE/QUOTE) - Interval is one of allowed values - Strategy exists - API credentials are properly formatted - Integrate validator into start-trading-bot IPC handler - Prevents uncontrolled trades from compromised renderer - Respects risk manager position size limits Fixes: [CRITICAL] Missing input validation in trading bot IPC handler

Copilot

Pull request overview

Adds a main-process validator to harden the start-trading-bot IPC path by rejecting malformed/unsafe bot configurations before scheduling execution.

Changes:

Introduces TradingBotValidator to validate bot config fields (amount, symbol, interval, strategy, API config).
Wires validation into the start-trading-bot IPC handler in main.js.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
`main.js`	Imports and invokes `TradingBotValidator` before starting a trading bot via IPC.
`electron/validators/tradingBotValidator.js`	New validator class implementing config validation rules for the trading bot.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-02T14:04:20Z

electron/validators/tradingBotValidator.js

+    }
+
+    // Does amount exceed risk manager's max position size?
+    const maxPosition = this.riskConfig.maxPositionSize || 100;


_validateAmount reads this.riskConfig.maxPositionSize, but RiskManager exposes config.maxPositionUSDT (and this bot’s amount is a base-asset quantity, not USDT). As written, the risk-manager limit is effectively ignored and the check falls back to 100, which can incorrectly block/allow trades. Consider aligning this with riskManager.config.maxPositionUSDT by validating position size in USDT (e.g., quantity * price for USDT-quoted pairs) or passing the correct limit field into the validator.

Suggested change

const maxPosition = this.riskConfig.maxPositionSize || 100;

const maxPosition =

Number.isFinite(this.riskConfig.maxPositionUSDT)

? this.riskConfig.maxPositionUSDT

: (Number.isFinite(this.riskConfig.maxPositionSize)

? this.riskConfig.maxPositionSize

: 100);

Wrong risk manager field name (functional regression)
The validator reads riskConfig.maxPositionSize with a fallback of 100, but RiskManager uses maxPositionUSDT. This means the upper bound check silently falls back to 100 units on every run, effectively ignoring actual configured limit. The fix is to read maxPositionUSDT and do the USDT conversion (quantity × price) before comparing.

Copilot · 2026-03-02T14:04:20Z

electron/validators/tradingBotValidator.js

+    // Does symbol match pattern: BASE/QUOTE (e.g., BTC/USD)?
+    if (!/^[A-Z0-9]{1,10}\/[A-Z0-9]{1,10}$/.test(symbol)) {
+      throw new Error('Symbol must match format: BASE/QUOTE (e.g., BTC/USD, ETH/USDT)');
+    }


Symbol validation requires BASE/QUOTE (e.g. BTC/USDT), but the existing trading-bot UI sends Binance-style symbols like BNBUSDT and electron/binanceApi.getCurrentPrice() expects BTCUSDT-style symbols. With this regex, valid existing inputs will be rejected and bots won’t start. Either accept both formats and normalize, or switch the bot flow to use the exchange adapter’s normalizeSymbol() consistently.

Symbol format breaks all existing bots (critical regression)

The regex enforces BASE/QUOTE (e.g. BTC/USDT), but your entire codebase — binanceApi.js, the exchange adapters, and the UI TradeSetup component — uses Binance-style concatenated symbols like BTCUSDT. Every existing bot config will fail validation immediately. The fix is to either accept both formats and normalize, or use your exchange adapters' normalizeSymbol() consistently.

Copilot · 2026-03-02T14:04:20Z

electron/validators/tradingBotValidator.js

+
+    // Define what values are allowed
+    this.VALID_INTERVALS = ['1m', '5m', '15m', '30m', '1h', '4h', '1d'];
+    this.VALID_STRATEGIES = ['simpleMovingAverage', 'relativeStrengthIndex', 'bollingerBands'];


VALID_STRATEGIES omits macd, but the renderer’s strategy list (src/utils/tradingStrategies.js) includes it and users can select it (e.g. in TradeSetup). This change will make previously selectable strategies fail validation. Either add macd support end-to-end (including execution logic), or remove/disable it in the UI so the allowed values stay in sync.

Suggested change

this.VALID_STRATEGIES = ['simpleMovingAverage', 'relativeStrengthIndex', 'bollingerBands'];

this.VALID_STRATEGIES = ['simpleMovingAverage', 'relativeStrengthIndex', 'bollingerBands', 'macd'];

Missing macd strategy (breaks user selection)

VALID_STRATEGIES only lists simpleMovingAverage, relativeStrengthIndex, bollingerBands, but src/utils/tradingStrategies.js exposes macd as a selectable option. Users who pick MACD will get a validation error. Either add 'macd' to the allowed list or remove it from the UI.

Copilot · 2026-03-02T14:04:21Z

electron/validators/tradingBotValidator.js

+    // Does apiKey exist and is it a string?
+    if (!apiConfig.apiKey || typeof apiConfig.apiKey !== 'string') {
+      throw new Error('API key is missing or invalid');
+    }
+
+    // Is apiKey long enough? (Real keys are 50+ chars)
+    if (apiConfig.apiKey.length < 10) {
+      throw new Error('API key appears to be invalid (too short)');
+    }


API key validation logic here duplicates (and is weaker than) the existing validateApiKeyFormat() in electron/security/encryption.js (used by the security:validateApiKey IPC). To avoid divergent rules between main-process validation points, consider reusing the shared validator (and applying an equivalent format check for apiSecret if desired).

Duplicate and weaker API key validation

The PR validates API key length (>= 10 chars) independently, but electron/security/encryption.js already has validateApiKeyFormat() used by the security:validateApiKey IPC handler. Two divergent validation rules is a maintenance hazard. The fix is to import and reuse the shared function.

NubleX · 2026-03-10T08:08:26Z

Hi @FarahR01, thank you for this contribution — the structure and intent are exactly right, and this is a genuinely useful addition to the codebase. Before merging, there are four issues that need to be addressed to prevent regressions. GitHub Copilot flagged them in the automated review as well.

Issue 1 — Wrong risk manager field name

_validateAmount reads this.riskConfig.maxPositionSize, but RiskManager exposes maxPositionUSDT. The fallback of 100 will silently apply on every run, ignoring the actual configured limit.

// tradingBotValidator.js - current (broken)
const maxPosition = this.riskConfig.maxPositionSize || 100;

// Fix: align with the real field name and add USDT conversion
// amount here is a base-asset quantity, so we need quantity * price
// Pass currentPrice into the validator or validate in USDT directly
const maxPositionUSDT =
  Number.isFinite(this.riskConfig.maxPositionUSDT)
    ? this.riskConfig.maxPositionUSDT
    : (Number.isFinite(this.riskConfig.maxPositionSize)
        ? this.riskConfig.maxPositionSize
        : 100);

Issue 2 — Symbol regex breaks all existing bot configs

The BASE/QUOTE format regex (BTC/USDT) conflicts with the Binance-style symbols used everywhere in the codebase (BTCUSDT). Every existing bot config will fail validation immediately.

// tradingBotValidator.js - current (breaks existing symbols)
if (!/^[A-Z0-9]{1,10}\/[A-Z0-9]{1,10}$/.test(symbol)) {
  throw new Error('Symbol must match format: BASE/QUOTE (e.g., BTC/USD, ETH/USDT)');
}

// Fix: accept both formats
const normalizedSymbol = symbol.includes('/')
  ? symbol.replace('/', '')
  : symbol;

if (!/^[A-Z0-9]{2,20}$/.test(normalizedSymbol)) {
  throw new Error('Symbol must be a valid trading pair (e.g., BTCUSDT or BTC/USDT)');
}

Alternatively, run the symbol through the exchange adapter's normalizeSymbol() before validation and work with the normalized form consistently.

Issue 3 — macd missing from VALID_STRATEGIES

src/utils/tradingStrategies.js exposes macd as a selectable strategy in the UI (TradeSetup). Users who select it will hit a validation error after this PR lands.

// tradingBotValidator.js - current (missing macd)
this.VALID_STRATEGIES = ['simpleMovingAverage', 'relativeStrengthIndex', 'bollingerBands'];

// Fix: add macd to stay in sync with the renderer's strategy list
this.VALID_STRATEGIES = ['simpleMovingAverage', 'relativeStrengthIndex', 'bollingerBands', 'macd'];

If macd execution logic isn't implemented in the main process yet, it should be removed from the UI at the same time — but either way the allowed lists need to match.

Issue 4 — Duplicate and weaker API key validation

electron/security/encryption.js already exports validateApiKeyFormat(), used by the security:validateApiKey IPC handler. Introducing a separate length check (>= 10 chars) creates two divergent validation rules for the same field.

// tradingBotValidator.js - current (reimplements existing logic weakly)
if (apiConfig.apiKey.length < 10) {
  throw new Error('API key appears to be invalid (too short)');
}

// Fix: reuse the shared validator instead
const { validateApiKeyFormat } = require('../security/encryption');

if (!validateApiKeyFormat(apiConfig.apiKey)) {
  throw new Error('API key format is invalid');
}
// Apply the same for apiSecret if the function supports it

Once these four points are addressed the PR should be good to merge. Happy to review a follow-up commit. Thanks again for the careful work on this.

Copilot AI review requested due to automatic review settings March 2, 2026 13:59

Copilot started reviewing on behalf of FarahR01 March 2, 2026 13:59 View session

Copilot AI reviewed Mar 2, 2026

View reviewed changes

NubleX assigned FarahR01 Mar 10, 2026

NubleX added enhancement New feature or request Security labels Mar 10, 2026

NubleX added this to @NubleX NeutronTrader Mar 10, 2026

NubleX moved this to Done in @NubleX NeutronTrader Mar 10, 2026

NubleX moved this from Done to In Progress in @NubleX NeutronTrader Mar 10, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add input validation for trading bot IPC handler#36

Add input validation for trading bot IPC handler#36
FarahR01 wants to merge 1 commit intoNubleX:mainfrom
FarahR01:fix/ipc-input-validation-security

FarahR01 commented Mar 2, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

NubleX Mar 10, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

NubleX Mar 10, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

NubleX Mar 10, 2026

Uh oh!

Copilot AI Mar 2, 2026

Uh oh!

NubleX Mar 10, 2026

Uh oh!

NubleX commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	this.VALID_STRATEGIES = ['simpleMovingAverage', 'relativeStrengthIndex', 'bollingerBands'];
	this.VALID_STRATEGIES = ['simpleMovingAverage', 'relativeStrengthIndex', 'bollingerBands', 'macd'];

Conversation

FarahR01 commented Mar 2, 2026

Problem

Solution

Changes

Impact

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

NubleX Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

NubleX Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

NubleX Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

Uh oh!

NubleX Mar 10, 2026

Choose a reason for hiding this comment

Uh oh!

NubleX commented Mar 10, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants