Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
67 changes: 67 additions & 0 deletions src/__tests__/config-migration.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
import { describe, expect, it } from 'vitest';
import { mergeLegacyConfig, notificationFromLegacy } from '../config/migration';

describe('config migration', () => {
it('returns undefined for empty legacy input', () => {
expect(notificationFromLegacy({})).toBeUndefined();
});

it('converts legacy notification keys into the new notification shape', () => {
const notifications = notificationFromLegacy({
notification_service: 'email',
discord_webhook: 'https://discord.com/api/webhooks/123/token',
email_host: 'smtp.test.com',
email_port: 587,
email_user: 'user@test.com',
email_to: 'to@test.com',
email_password: 'secret',
alert_cooldown: 120,
});

expect(notifications).toEqual({
service: 'email',
discordWebhook: 'https://discord.com/api/webhooks/123/token',
emailHost: 'smtp.test.com',
emailPort: 587,
emailUser: 'user@test.com',
emailTo: 'to@test.com',
emailPassword: 'secret',
alertCooldown: 120,
});
});

it('prefers explicit new notification config over migrated legacy values', () => {
const config = mergeLegacyConfig({
notification_service: 'discord',
discord_webhook: 'legacy-webhook',
notifications: {
service: 'email',
discordWebhook: 'new-webhook',
},
});

expect(config.notifications).toMatchObject({
service: 'email',
discordWebhook: 'new-webhook',
});
});

it('ignores malformed legacy notification values', () => {
const notifications = notificationFromLegacy({
notification_service: { invalid: 'object' } as never,
email_port: 'not-a-number' as never,
alert_cooldown: 'not-a-number' as never,
});

expect(notifications).toEqual({
service: 'none',
emailPort: undefined,
alertCooldown: undefined,
discordWebhook: undefined,
emailHost: undefined,
emailUser: undefined,
emailTo: undefined,
emailPassword: undefined,
});
});
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
122 changes: 122 additions & 0 deletions src/__tests__/config-store.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,122 @@
import { beforeEach, describe, expect, it, vi } from 'vitest';

vi.mock('conf', () => {
const mockConfigStore = new Map<string, any>();
const mockConfInstance = {
get store() {
return Object.fromEntries(mockConfigStore.entries());
},
set: vi.fn((key, val) => {
mockConfigStore.set(key, val);
}),
get: vi.fn((key) => mockConfigStore.get(key)),
delete: vi.fn((key) => {
mockConfigStore.delete(key);
}),
clear: vi.fn(() => {
mockConfigStore.clear();
}),
};

(globalThis as any).mockConfigStore = mockConfigStore;

return {
default: class MockConf {
constructor() {
return mockConfInstance;
}
},
};
});
import {
clearConfig,
getActiveFilters,
getCacheConfig,
getConfig,
getLegacyConfig,
getLegacyValue,
getOutputConfig,
setActiveFilters,
setAIConfig,
setCacheConfig,
setConfigValue,
setKubernetesConfig,
setLegacyValue,
setOutputConfig,
} from '../config/store';

describe('config store', () => {
beforeEach(() => {
clearConfig();
vi.clearAllMocks();
});

it('returns defaults for new configuration sections', () => {
expect(getActiveFilters()).toEqual([]);
expect(getCacheConfig()).toEqual({ type: 'file', enabled: true });
expect(getOutputConfig()).toEqual({ format: 'text', language: 'english' });
});

it('stores and reads new typed configuration sections', () => {
setAIConfig({
providers: [{ name: 'openai', model: 'gpt-4o' }],
defaultProvider: 'openai',
});
setActiveFilters(['Pod', 'Deployment']);
setKubernetesConfig({ namespace: 'default', kubecontext: 'minikube' });
setCacheConfig({ type: 'file', enabled: false, path: '/tmp/kdm-cache' });
setOutputConfig({ format: 'json', language: 'english' });

expect(getConfig()).toMatchObject({
ai: {
providers: [{ name: 'openai', model: 'gpt-4o' }],
defaultProvider: 'openai',
},
activeFilters: ['Pod', 'Deployment'],
kubernetes: { namespace: 'default', kubecontext: 'minikube' },
cache: { type: 'file', enabled: false, path: '/tmp/kdm-cache' },
output: { format: 'json', language: 'english' },
});
});

it('keeps legacy notification reads compatible', () => {
setLegacyValue('notification_service', 'discord');
setLegacyValue('discord_webhook', 'https://discord.com/api/webhooks/123/token');
setLegacyValue('alert_cooldown', 300);

expect(getLegacyConfig()).toEqual({
notification_service: 'discord',
discord_webhook: 'https://discord.com/api/webhooks/123/token',
alert_cooldown: 300,
email_host: undefined,
email_port: undefined,
email_user: undefined,
email_to: undefined,
email_password: undefined,
});
});

it('keeps legacy value reads compatible with migrated notification config', () => {
setConfigValue('notifications', {
service: 'email',
emailHost: 'smtp.test.com',
emailPort: 587,
emailUser: 'user@test.com',
emailTo: 'to@test.com',
});

expect(getLegacyValue('notification_service')).toBe('email');
expect(getLegacyValue('email_host')).toBe('smtp.test.com');
expect(getLegacyValue('email_port')).toBe(587);
});

it('returns safe legacy defaults for malformed notification values', () => {
setLegacyValue('notification_service', { invalid: 'object' } as never);
setLegacyValue('alert_cooldown', 'not-a-number' as never);

expect(getLegacyConfig()).toMatchObject({
notification_service: 'none',
alert_cooldown: undefined,
});
});
});
Comment thread
coderabbitai[bot] marked this conversation as resolved.
11 changes: 9 additions & 2 deletions src/__tests__/config-utils.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
vi.mock('conf', () => {
const mockConfigStore = new Map<string, any>();
const mockConfInstance = {
store: {},
get store() {
return Object.fromEntries(mockConfigStore.entries());
},
set: vi.fn((key, val) => {
mockConfigStore.set(key, val);
}),
Expand All @@ -30,7 +32,7 @@ vi.mock('conf', () => {
};
});

import { getSMTPSettings, clearNotificationCredentials } from '../utils/config';
import { getSMTPSettings, clearNotificationCredentials, setConfig } from '../utils/config';

describe('config utils', () => {
const originalEnv = process.env;
Expand Down Expand Up @@ -89,4 +91,9 @@ describe('config utils', () => {
settings = getSMTPSettings();
expect(settings.auth.pass).toBe('');
});

it('should reject storing SMTP passwords in config', () => {
expect(() => setConfig('email_password', 'secret')).toThrow(/KDM_SMTP_PASSWORD/);
expect(mockConfInstance.set).not.toHaveBeenCalledWith('email_password', 'secret');
});
Comment on lines +95 to +98
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Strengthen the negative assertion to reject any email_password write.

The current check only blocks one exact value ('secret'). A write with the same key and a different value would still pass.

Suggested fix
   it('should reject storing SMTP passwords in config', () => {
     expect(() => setConfig('email_password', 'secret')).toThrow(/KDM_SMTP_PASSWORD/);
-    expect(mockConfInstance.set).not.toHaveBeenCalledWith('email_password', 'secret');
+    expect(
+      mockConfInstance.set.mock.calls.some(([key]: [string]) => key === 'email_password'),
+    ).toBe(false);
   });
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
it('should reject storing SMTP passwords in config', () => {
expect(() => setConfig('email_password', 'secret')).toThrow(/KDM_SMTP_PASSWORD/);
expect(mockConfInstance.set).not.toHaveBeenCalledWith('email_password', 'secret');
});
it('should reject storing SMTP passwords in config', () => {
expect(() => setConfig('email_password', 'secret')).toThrow(/KDM_SMTP_PASSWORD/);
expect(
mockConfInstance.set.mock.calls.some(([key]: [string]) => key === 'email_password'),
).toBe(false);
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/__tests__/config-utils.test.ts` around lines 95 - 98, The test currently
only forbids writing the specific value 'secret' for the 'email_password' key;
update the negative assertion so it rejects any write to that key regardless of
value. In the test that calls setConfig('email_password', 'secret'), replace the
assertion against mockConfInstance.set that references the exact value with one
that asserts mockConfInstance.set was not called with the key 'email_password'
and any value (e.g., using expect.anything()) so the check targets the key
rather than a single value; keep the existing
expect(...).toThrow(/KDM_SMTP_PASSWORD/) and the call to setConfig in place.

});
21 changes: 8 additions & 13 deletions src/__tests__/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,8 +95,7 @@ describe('config command', () => {
.mockResolvedValueOnce('smtp.gmail.com') // host
.mockResolvedValueOnce('587') // port
.mockResolvedValueOnce('user@test.com') // user
.mockResolvedValueOnce('to@test.com') // to
.mockResolvedValueOnce(''); // password (empty)
.mockResolvedValueOnce('to@test.com'); // to

await program.parseAsync(['node', 'test', 'config', 'setup']);

Expand All @@ -113,37 +112,33 @@ describe('config command', () => {
expect(guideOrder).toBeLessThan(firstInputOrder());
});

it('should save email_password if provided during email setup', async () => {
it('should not ask for or persist SMTP password during email setup', async () => {
vi.mocked(tui.select).mockResolvedValue('email');
vi.mocked(tui.input)
.mockResolvedValueOnce('smtp.gmail.com') // host
.mockResolvedValueOnce('587') // port
.mockResolvedValueOnce('user@test.com') // user
.mockResolvedValueOnce('to@test.com') // to
.mockResolvedValueOnce('pass123'); // password
.mockResolvedValueOnce('to@test.com'); // to

await program.parseAsync(['node', 'test', 'config', 'setup']);

expect(configUtils.setConfig).toHaveBeenCalledWith('email_password', 'pass123');
expect(tui.input).toHaveBeenCalledTimes(4);
expect(configUtils.setConfig).not.toHaveBeenCalledWith('email_password', expect.any(String));
expect(consoleLogSpy).toHaveBeenCalledWith(expect.stringContaining('KDM_SMTP_PASSWORD'));
});

it('should require an SMTP host during email setup and validate optional SMTP password', async () => {
it('should require an SMTP host during email setup', async () => {
vi.mocked(tui.select).mockResolvedValue('email');
vi.mocked(tui.input)
.mockResolvedValueOnce('smtp.gmail.com')
.mockResolvedValueOnce('587')
.mockResolvedValueOnce('user@test.com')
.mockResolvedValueOnce('to@test.com')
.mockResolvedValueOnce('');
.mockResolvedValueOnce('to@test.com');

await program.parseAsync(['node', 'test', 'config', 'setup']);

const smtpHostPrompt = vi.mocked(tui.input).mock.calls[0][0];
expect(smtpHostPrompt.validate('')).toBe('Host is required');

const smtpPasswordPrompt = vi.mocked(tui.input).mock.calls[4][0];
expect(smtpPasswordPrompt.validate('')).toBe(true);
expect(smtpPasswordPrompt.validate('anything')).toBe(true);
});

it('should call setConfig on config set', async () => {
Expand Down
12 changes: 3 additions & 9 deletions src/commands/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,22 +68,16 @@ export const registerConfigCommand = (program: Command) => {
message: 'Alert Recipient Email:',
validate: (v) => /^[^\s@]+@[^\s@]+\.[^\s@]+$/.test(v) || 'Must be a valid email address',
});
const password = await input({
message: 'SMTP Password (optional, press Enter to skip):',
validate: () => true,
});

clearNotificationCredentials();
setConfig('email_host', host);
setConfig('email_port', parseInt(portStr, 10));
setConfig('email_user', user);
setConfig('email_to', to);
if (password) {
setConfig('email_password', password);
}
setConfig('notification_service', 'email');

console.log(chalk.green('\n✓ Email SMTP configured.'));
console.log(chalk.dim(' Set KDM_SMTP_PASSWORD in your environment to provide the SMTP password.'));
}

console.log(chalk.green(`✓ Notification service set to: ${chalk.bold(choice.toUpperCase())}`));
Expand Down Expand Up @@ -127,7 +121,7 @@ export const registerConfigCommand = (program: Command) => {
}

console.log(chalk.gray('──────────────────────────────────────────────────'));
console.log(chalk.dim('\n Note: SMTP password can be set either in config or via the KDM_SMTP_PASSWORD environment variable, which takes precedence if both are set.\n'));
console.log(chalk.dim('\n Note: SMTP password is not stored in config. Set it with the KDM_SMTP_PASSWORD environment variable.\n'));
});

config
Expand Down Expand Up @@ -156,7 +150,7 @@ const printEmailSmtpGuide = () => {
console.log(chalk.white(' 1. Find your provider SMTP settings before continuing.'));
console.log(chalk.white(' 2. Common hosts: smtp.gmail.com for Gmail, smtp.office365.com for Outlook.'));
console.log(chalk.white(' 3. Use port 587 for STARTTLS unless your provider says otherwise.'));
console.log(chalk.white(' 4. Provide the SMTP password during setup or via the KDM_SMTP_PASSWORD environment variable.'));
console.log(chalk.white(' 4. Provide the SMTP password via the KDM_SMTP_PASSWORD environment variable.'));
console.log(chalk.dim(' Gmail accounts with 2FA usually require an App Password.'));
console.log(chalk.gray('──────────────────────────────────────────────────\n'));
};
51 changes: 51 additions & 0 deletions src/config/migration.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
import type { KDMConfig, LegacyNotificationConfig, NotificationConfig, StoredKDMConfig } from './schema';

const isNotificationService = (value: unknown): value is NotificationConfig['service'] =>
value === 'discord' || value === 'email' || value === 'none';

const stringOrUndefined = (value: unknown) => (typeof value === 'string' ? value : undefined);
const numberOrUndefined = (value: unknown) => (typeof value === 'number' ? value : undefined);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Validate migrated numeric fields before accepting them.

On Line 30 and Line 34, any numeric value is accepted, including negatives/out-of-range values. That can disable cooldown behavior downstream (e.g., negative alertCooldown) and accept invalid SMTP ports.

Suggested fix
-const numberOrUndefined = (value: unknown) => (typeof value === 'number' ? value : undefined);
+const finiteNumberOrUndefined = (value: unknown) =>
+  typeof value === 'number' && Number.isFinite(value) ? value : undefined;
+
+const emailPortOrUndefined = (value: unknown) => {
+  const n = finiteNumberOrUndefined(value);
+  return n !== undefined && Number.isInteger(n) && n >= 1 && n <= 65535 ? n : undefined;
+};
+
+const alertCooldownOrUndefined = (value: unknown) => {
+  const n = finiteNumberOrUndefined(value);
+  return n !== undefined && n >= 0 ? n : undefined;
+};
@@
-    emailPort: numberOrUndefined(legacy.email_port),
+    emailPort: emailPortOrUndefined(legacy.email_port),
@@
-    alertCooldown: numberOrUndefined(legacy.alert_cooldown),
+    alertCooldown: alertCooldownOrUndefined(legacy.alert_cooldown),

Also applies to: 30-35

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/config/migration.ts` at line 7, The migration currently uses
numberOrUndefined which accepts any number (including negatives) and lets
invalid values like negative alertCooldown or out-of-range smtpPort through;
update the migration to validate numeric fields before accepting them: replace
or extend numberOrUndefined to perform range checks (e.g., non-negative for
alertCooldown/timeout fields, valid TCP port range 1-65535 for smtpPort) or add
field-specific validators where numbers are parsed (referencing
numberOrUndefined and the migration code paths that assign alertCooldown and
smtpPort) so only numbers within the allowed ranges are returned; on invalid
values return undefined (or fallback defaults) and log or record the rejection.


const hasLegacyNotificationConfig = (config: LegacyNotificationConfig) =>
config.notification_service !== undefined ||
config.discord_webhook !== undefined ||
config.email_host !== undefined ||
config.email_port !== undefined ||
config.email_user !== undefined ||
config.email_to !== undefined ||
config.email_password !== undefined ||
config.alert_cooldown !== undefined;

export const notificationFromLegacy = (
legacy: LegacyNotificationConfig,
): NotificationConfig | undefined => {
if (!hasLegacyNotificationConfig(legacy)) {
return undefined;
}

return {
service: isNotificationService(legacy.notification_service) ? legacy.notification_service : 'none',
discordWebhook: stringOrUndefined(legacy.discord_webhook),
emailHost: stringOrUndefined(legacy.email_host),
emailPort: numberOrUndefined(legacy.email_port),
emailUser: stringOrUndefined(legacy.email_user),
emailTo: stringOrUndefined(legacy.email_to),
emailPassword: stringOrUndefined(legacy.email_password),
alertCooldown: numberOrUndefined(legacy.alert_cooldown),
};
};
export const mergeLegacyConfig = (stored: StoredKDMConfig): KDMConfig => {
const legacyNotifications = notificationFromLegacy(stored);

return {
ai: stored.ai,
activeFilters: stored.activeFilters,
kubernetes: stored.kubernetes,
cache: stored.cache,
output: stored.output,
notifications: {
...legacyNotifications,
...stored.notifications,
},
};
};
Loading