-
Notifications
You must be signed in to change notification settings - Fork 7
feat: implement optional SMTP password support #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,92 @@ | ||
| import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; | ||
|
|
||
| vi.mock('conf', () => { | ||
| const mockConfigStore = new Map<string, any>(); | ||
| const mockConfInstance = { | ||
| store: {}, | ||
| set: vi.fn((key, val) => { | ||
| mockConfigStore.set(key, val); | ||
| }), | ||
| get: vi.fn((key) => { | ||
| return mockConfigStore.get(key); | ||
| }), | ||
| delete: vi.fn((key) => { | ||
| mockConfigStore.delete(key); | ||
| }), | ||
| clear: vi.fn(() => { | ||
| mockConfigStore.clear(); | ||
| }), | ||
| }; | ||
|
|
||
| (globalThis as any).mockConfigStore = mockConfigStore; | ||
| (globalThis as any).mockConfInstance = mockConfInstance; | ||
|
|
||
| return { | ||
| default: class MockConf { | ||
| constructor() { | ||
| return mockConfInstance; | ||
| } | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| import { getSMTPSettings, clearNotificationCredentials } from '../utils/config'; | ||
|
|
||
| describe('config utils', () => { | ||
| const originalEnv = process.env; | ||
| let mockConfInstance: any; | ||
| let mockConfigStore: any; | ||
|
|
||
| beforeEach(() => { | ||
| mockConfInstance = (globalThis as any).mockConfInstance; | ||
| mockConfigStore = (globalThis as any).mockConfigStore; | ||
| mockConfigStore.clear(); | ||
| vi.clearAllMocks(); | ||
| process.env = { ...originalEnv }; | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| process.env = originalEnv; | ||
| }); | ||
|
|
||
| it('should clear all notification credentials, including email_password', () => { | ||
| mockConfigStore.set('discord_webhook', 'http://webhook'); | ||
| mockConfigStore.set('email_host', 'smtp.test.com'); | ||
| mockConfigStore.set('email_port', 587); | ||
| mockConfigStore.set('email_user', 'user@test.com'); | ||
| mockConfigStore.set('email_to', 'to@test.com'); | ||
| mockConfigStore.set('email_password', 'secret'); | ||
|
|
||
| clearNotificationCredentials(); | ||
|
|
||
| expect(mockConfInstance.delete).toHaveBeenCalledWith('discord_webhook'); | ||
| expect(mockConfInstance.delete).toHaveBeenCalledWith('email_host'); | ||
| expect(mockConfInstance.delete).toHaveBeenCalledWith('email_port'); | ||
| expect(mockConfInstance.delete).toHaveBeenCalledWith('email_user'); | ||
| expect(mockConfInstance.delete).toHaveBeenCalledWith('email_to'); | ||
| expect(mockConfInstance.delete).toHaveBeenCalledWith('email_password'); | ||
| }); | ||
|
|
||
| it('should get SMTP settings with precedence given to environment variables over email_password', () => { | ||
| mockConfigStore.set('email_host', 'smtp.test.com'); | ||
| mockConfigStore.set('email_port', 587); | ||
| mockConfigStore.set('email_user', 'user@test.com'); | ||
| mockConfigStore.set('email_to', 'to@test.com'); | ||
| mockConfigStore.set('email_password', 'config-password'); | ||
|
|
||
| // Case 1: No env variable set, should use stored config password | ||
| delete process.env.KDM_SMTP_PASSWORD; | ||
| let settings = getSMTPSettings(); | ||
| expect(settings.auth.pass).toBe('config-password'); | ||
|
|
||
| // Case 2: Env variable set, should take precedence | ||
| process.env.KDM_SMTP_PASSWORD = 'env-password'; | ||
| settings = getSMTPSettings(); | ||
| expect(settings.auth.pass).toBe('env-password'); | ||
|
|
||
| // Case 3: Env variable set to empty string, should be honored instead of falling back to config password | ||
| process.env.KDM_SMTP_PASSWORD = ''; | ||
| settings = getSMTPSettings(); | ||
| expect(settings.auth.pass).toBe(''); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -7,6 +7,7 @@ interface KDMConfig { | |
| email_port?: number; | ||
| email_user?: string; | ||
| email_to?: string; | ||
| email_password?: string; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid storing SMTP password in plain-text config storage. Persisting As per coding guidelines "Verify that sensitive information is not stored in plain text if possible." 🤖 Prompt for AI Agents |
||
| alert_cooldown?: number; // in seconds | ||
| } | ||
|
|
||
|
|
@@ -25,6 +26,7 @@ export const clearNotificationCredentials = () => { | |
| config.delete('email_port'); | ||
| config.delete('email_user'); | ||
| config.delete('email_to'); | ||
| config.delete('email_password'); | ||
| }; | ||
|
|
||
| // Helper for sensitive data - always use environment variables | ||
|
|
@@ -34,7 +36,10 @@ export const getSMTPSettings = () => { | |
| port: config.get('email_port') || 587, | ||
| auth: { | ||
| user: config.get('email_user'), | ||
| pass: process.env.KDM_SMTP_PASSWORD, | ||
| pass: | ||
| process.env.KDM_SMTP_PASSWORD !== undefined | ||
| ? process.env.KDM_SMTP_PASSWORD | ||
| : config.get('email_password'), | ||
| }, | ||
|
Comment on lines
32
to
43
|
||
| to: config.get('email_to'), | ||
| }; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.