mirror of
https://github.com/bookorbit/bookorbit.git
synced 2026-11-03 00:11:37 -06:00
fix(email): add skip-TLS-verification option and surface provider delete errors (#35)
Node.js inside Docker validates the full TLS certificate chain against its bundled CA store. When the chain cannot be verified (e.g. outdated CA bundle, self-signed cert, or Let's Encrypt chain not trusted), nodemailer reports a misleading error like "certificate has expired". Add a per-provider tlsRejectUnauthorized flag (default: true) that passes tls.rejectUnauthorized=false to nodemailer when disabled, letting self-hosted SMTP servers opt out of cert verification. A visible warning is shown in the provider form whenever the flag is turned off. The delete provider API already returned a clear 400 message explaining that the system designation must be removed first, but the frontend threw a hardcoded "Failed to delete provider" string and never read the response body. Fix the composable to parse and rethrow the actual API error message. Also disable the delete button for system providers with a tooltip stating the constraint, so users cannot trigger the error in the first place. Adds tlsRejectUnauthorized boolean column (NOT NULL DEFAULT true) to email_providers via Drizzle migration 0004. Closes #26
This commit is contained in:
@@ -1,7 +1,7 @@
|
||||
<script setup lang="ts">
|
||||
import { ref, reactive, computed, watch } from 'vue'
|
||||
import { toast } from 'vue-sonner'
|
||||
import { ChevronDown, ChevronUp, Plus, Pencil, Trash2, Star, Share2, Wifi, Server } from 'lucide-vue-next'
|
||||
import { ChevronDown, ChevronUp, Plus, Pencil, Trash2, Star, Share2, Wifi, Server, AlertTriangle } from 'lucide-vue-next'
|
||||
import { Permission } from '@bookorbit/types'
|
||||
import { useEmailProviders, type EmailProvider, type EmailProviderForm } from '../composables/useEmailProviders'
|
||||
import { usePermissions } from '@/features/auth/composables/usePermissions'
|
||||
@@ -43,6 +43,7 @@ const emptyForm = (): EmailProviderForm => ({
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
})
|
||||
|
||||
const form = reactive<EmailProviderForm>(emptyForm())
|
||||
@@ -67,6 +68,7 @@ function openEdit(p: EmailProvider) {
|
||||
auth: p.auth,
|
||||
ssl: p.ssl,
|
||||
startTls: p.startTls,
|
||||
tlsRejectUnauthorized: p.tlsRejectUnauthorized,
|
||||
})
|
||||
editingId.value = p.id
|
||||
formError.value = null
|
||||
@@ -293,6 +295,17 @@ watch(
|
||||
<input v-model="form.startTls" type="checkbox" class="rounded border-border" />
|
||||
<span class="text-xs text-foreground">STARTTLS</span>
|
||||
</label>
|
||||
<label class="flex items-center gap-2 cursor-pointer">
|
||||
<input v-model="form.tlsRejectUnauthorized" type="checkbox" class="rounded border-border" />
|
||||
<span class="text-xs text-foreground">Verify TLS certificate</span>
|
||||
</label>
|
||||
</div>
|
||||
|
||||
<div v-if="!form.tlsRejectUnauthorized" class="flex items-start gap-2 rounded-md border border-amber-500/40 bg-amber-500/10 px-3 py-2">
|
||||
<AlertTriangle :size="14" class="mt-0.5 shrink-0 text-amber-500" />
|
||||
<p class="text-xs text-amber-700 dark:text-amber-400">
|
||||
TLS verification is disabled. The server's certificate will not be validated - use only for trusted internal servers.
|
||||
</p>
|
||||
</div>
|
||||
|
||||
<div v-if="formError" class="text-xs text-destructive">{{ formError }}</div>
|
||||
@@ -418,13 +431,21 @@ watch(
|
||||
<Tooltip v-if="canManageEmail && p.userId !== null">
|
||||
<TooltipTrigger as-child>
|
||||
<button
|
||||
class="flex items-center justify-center w-7 h-7 rounded text-muted-foreground hover:text-destructive hover:bg-destructive/10 transition-colors"
|
||||
class="flex items-center justify-center w-7 h-7 rounded transition-colors"
|
||||
:class="
|
||||
p.isSystemProvider
|
||||
? 'cursor-not-allowed text-muted-foreground/40'
|
||||
: 'text-muted-foreground hover:text-destructive hover:bg-destructive/10'
|
||||
"
|
||||
:disabled="p.isSystemProvider"
|
||||
@click="requestRemove(p)"
|
||||
>
|
||||
<Trash2 :size="13" />
|
||||
</button>
|
||||
</TooltipTrigger>
|
||||
<TooltipContent>Delete</TooltipContent>
|
||||
<TooltipContent>
|
||||
{{ p.isSystemProvider ? 'Remove the system designation before deleting' : 'Delete' }}
|
||||
</TooltipContent>
|
||||
</Tooltip>
|
||||
</div>
|
||||
</div>
|
||||
|
||||
@@ -13,6 +13,7 @@ export interface EmailProvider {
|
||||
auth: boolean
|
||||
ssl: boolean
|
||||
startTls: boolean
|
||||
tlsRejectUnauthorized: boolean
|
||||
isDefault: boolean
|
||||
isShared: boolean
|
||||
isSystemProvider: boolean
|
||||
@@ -31,6 +32,7 @@ export interface EmailProviderForm {
|
||||
auth: boolean
|
||||
ssl: boolean
|
||||
startTls: boolean
|
||||
tlsRejectUnauthorized: boolean
|
||||
}
|
||||
|
||||
const providers = ref<EmailProvider[]>([])
|
||||
@@ -82,7 +84,10 @@ export function useEmailProviders() {
|
||||
|
||||
async function deleteProvider(id: number): Promise<void> {
|
||||
const res = await api(`/api/v1/email/providers/${id}`, { method: 'DELETE' })
|
||||
if (!res.ok) throw new Error('Failed to delete provider')
|
||||
if (!res.ok) {
|
||||
const err = await res.json().catch(() => ({}))
|
||||
throw new Error((err as { message?: string }).message ?? 'Failed to delete provider')
|
||||
}
|
||||
providers.value = providers.value.filter((p) => p.id !== id)
|
||||
}
|
||||
|
||||
|
||||
@@ -0,0 +1,3 @@
|
||||
ALTER TABLE "achievements" DROP CONSTRAINT "achievements_category_chk";--> statement-breakpoint
|
||||
ALTER TABLE "email_providers" ADD COLUMN "tls_reject_unauthorized" boolean DEFAULT true NOT NULL;--> statement-breakpoint
|
||||
ALTER TABLE "achievements" ADD CONSTRAINT "achievements_category_chk" CHECK ("achievements"."category" in ('reading', 'library', 'exploration', 'dedication'));
|
||||
File diff suppressed because it is too large
Load Diff
@@ -29,6 +29,13 @@
|
||||
"when": 1778890017858,
|
||||
"tag": "0003_add-achievements-tables",
|
||||
"breakpoints": true
|
||||
},
|
||||
{
|
||||
"idx": 4,
|
||||
"version": "7",
|
||||
"when": 1779128487353,
|
||||
"tag": "0004_add_tls_reject_unauthorized_to_email_providers",
|
||||
"breakpoints": true
|
||||
}
|
||||
]
|
||||
}
|
||||
}
|
||||
@@ -21,6 +21,7 @@ export const emailProviders = pgTable(
|
||||
isDefault: boolean('is_default').notNull().default(false),
|
||||
isShared: boolean('is_shared').notNull().default(false),
|
||||
isSystemProvider: boolean('is_system_provider').notNull().default(false),
|
||||
tlsRejectUnauthorized: boolean('tls_reject_unauthorized').notNull().default(true),
|
||||
createdAt: timestamp('created_at', { withTimezone: true }).defaultNow().notNull(),
|
||||
updatedAt: timestamp('updated_at', { withTimezone: true })
|
||||
.defaultNow()
|
||||
|
||||
@@ -42,4 +42,7 @@ export class CreateEmailProviderDto {
|
||||
|
||||
@IsBoolean()
|
||||
startTls: boolean;
|
||||
|
||||
@IsBoolean()
|
||||
tlsRejectUnauthorized: boolean;
|
||||
}
|
||||
|
||||
@@ -42,6 +42,7 @@ describe('Email DTO validation', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
})
|
||||
).length,
|
||||
).toBe(0);
|
||||
@@ -55,12 +56,29 @@ describe('Email DTO validation', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
})
|
||||
).length,
|
||||
).toBeGreaterThan(0);
|
||||
|
||||
expect(
|
||||
(
|
||||
await errorsFor(CreateEmailProviderDto, {
|
||||
name: 'SMTP',
|
||||
host: 'smtp.example.com',
|
||||
port: 465,
|
||||
auth: true,
|
||||
ssl: true,
|
||||
startTls: false,
|
||||
})
|
||||
).length,
|
||||
'tlsRejectUnauthorized is required on create',
|
||||
).toBeGreaterThan(0);
|
||||
|
||||
expect((await errorsFor(UpdateEmailProviderDto, { port: 0 })).length).toBeGreaterThan(0);
|
||||
expect((await errorsFor(UpdateEmailProviderDto, { port: 2525, auth: false })).length).toBe(0);
|
||||
expect((await errorsFor(UpdateEmailProviderDto, { tlsRejectUnauthorized: false })).length).toBe(0);
|
||||
expect((await errorsFor(UpdateEmailProviderDto, { tlsRejectUnauthorized: 'yes' })).length).toBeGreaterThan(0);
|
||||
});
|
||||
|
||||
it('enforces recipient enums and allows null resets on update', async () => {
|
||||
|
||||
@@ -48,4 +48,8 @@ export class UpdateEmailProviderDto {
|
||||
@IsOptional()
|
||||
@IsBoolean()
|
||||
startTls?: boolean;
|
||||
|
||||
@IsOptional()
|
||||
@IsBoolean()
|
||||
tlsRejectUnauthorized?: boolean;
|
||||
}
|
||||
|
||||
@@ -36,6 +36,7 @@ describe('EmailProviderResolver', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
};
|
||||
|
||||
beforeEach(async () => {
|
||||
@@ -74,6 +75,15 @@ describe('EmailProviderResolver', () => {
|
||||
expect(result.config.host).toBe('smtp.test.com');
|
||||
expect(result.config.fromName).toBe('BookOrbit Bot');
|
||||
expect(result.config.fromAddress).toBe('bot@example.com');
|
||||
expect(result.config.tlsRejectUnauthorized).toBe(true);
|
||||
});
|
||||
|
||||
it('should include tlsRejectUnauthorized=false in resolved config when provider has it disabled', async () => {
|
||||
(providerService.getProviderWithDecryptedPassword as vi.Mock).mockResolvedValue({ ...mockProvider, tlsRejectUnauthorized: false });
|
||||
|
||||
const result = await resolver.resolve(mockUser, 10);
|
||||
|
||||
expect(result.config.tlsRejectUnauthorized).toBe(false);
|
||||
});
|
||||
|
||||
it('should resolve from default provider if no ID requested', async () => {
|
||||
|
||||
@@ -50,6 +50,7 @@ export class EmailProviderResolver {
|
||||
auth: provider.auth,
|
||||
ssl: provider.ssl,
|
||||
startTls: provider.startTls,
|
||||
tlsRejectUnauthorized: provider.tlsRejectUnauthorized,
|
||||
},
|
||||
providerId: provider.id,
|
||||
};
|
||||
|
||||
@@ -62,6 +62,7 @@ describe('EmailProviderController', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
};
|
||||
|
||||
await controller.create(dto, user);
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
import { ConflictException, ForbiddenException, NotFoundException } from '@nestjs/common';
|
||||
import { BadRequestException, ConflictException, ForbiddenException, NotFoundException } from '@nestjs/common';
|
||||
import { EmailProviderService } from './email-provider.service';
|
||||
import { EmailProviderRepository } from './email-provider.repository';
|
||||
import { EmailEncryptionService } from './email-encryption.service';
|
||||
@@ -40,9 +40,11 @@ describe('EmailProviderService', () => {
|
||||
username: 'testuser',
|
||||
passwordEnc: 'encrypted',
|
||||
isShared: false,
|
||||
isSystemProvider: false,
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
@@ -81,15 +83,39 @@ describe('EmailProviderService', () => {
|
||||
|
||||
describe('create', () => {
|
||||
it('should encrypt password and insert provider', async () => {
|
||||
const dto = { name: 'New SMTP', host: 'new.test.com', port: 465, password: 'new-password', auth: true, ssl: true, startTls: false };
|
||||
const dto = {
|
||||
name: 'New SMTP',
|
||||
host: 'new.test.com',
|
||||
port: 465,
|
||||
password: 'new-password',
|
||||
auth: true,
|
||||
ssl: true,
|
||||
startTls: false,
|
||||
tlsRejectUnauthorized: true,
|
||||
};
|
||||
await service.create(dto, mockUser);
|
||||
expect(encryption.encrypt).toHaveBeenCalledWith('new-password');
|
||||
expect(repo.insert).toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('should persist tlsRejectUnauthorized=false when provided', async () => {
|
||||
const dto = { name: 'Self-Hosted', host: 'smtp.home.lan', port: 465, auth: true, ssl: true, startTls: false, tlsRejectUnauthorized: false };
|
||||
await service.create(dto, mockUser);
|
||||
expect(repo.insert).toHaveBeenCalledWith(expect.objectContaining({ tlsRejectUnauthorized: false }));
|
||||
});
|
||||
|
||||
it('should map duplicate provider names to ConflictException', async () => {
|
||||
(repo.insert as vi.Mock).mockRejectedValue({ code: '23505' });
|
||||
const dto = { name: 'New SMTP', host: 'new.test.com', port: 465, password: 'new-password', auth: true, ssl: true, startTls: false };
|
||||
const dto = {
|
||||
name: 'New SMTP',
|
||||
host: 'new.test.com',
|
||||
port: 465,
|
||||
password: 'new-password',
|
||||
auth: true,
|
||||
ssl: true,
|
||||
startTls: false,
|
||||
tlsRejectUnauthorized: true,
|
||||
};
|
||||
await expect(service.create(dto, mockUser)).rejects.toThrow(ConflictException);
|
||||
});
|
||||
});
|
||||
@@ -111,6 +137,11 @@ describe('EmailProviderService', () => {
|
||||
expect(result.id).toBe(10);
|
||||
});
|
||||
|
||||
it('should patch tlsRejectUnauthorized when provided', async () => {
|
||||
await service.update(10, { tlsRejectUnauthorized: false }, mockUser);
|
||||
expect(repo.update).toHaveBeenCalledWith(10, 1, expect.objectContaining({ tlsRejectUnauthorized: false }));
|
||||
});
|
||||
|
||||
it('should throw NotFoundException if update fails', async () => {
|
||||
(repo.update as vi.Mock).mockResolvedValue([]);
|
||||
await expect(service.update(10, {}, mockUser)).rejects.toThrow(NotFoundException);
|
||||
@@ -127,6 +158,11 @@ describe('EmailProviderService', () => {
|
||||
await service.remove(10, mockUser);
|
||||
expect(repo.delete).toHaveBeenCalledWith(10, 1);
|
||||
});
|
||||
|
||||
it('should throw BadRequestException when provider is the system provider', async () => {
|
||||
(repo.findById as vi.Mock).mockResolvedValue([{ ...mockProvider, isSystemProvider: true }]);
|
||||
await expect(service.remove(10, mockUser)).rejects.toThrow(BadRequestException);
|
||||
});
|
||||
});
|
||||
|
||||
describe('setDefault', () => {
|
||||
@@ -169,6 +205,13 @@ describe('EmailProviderService', () => {
|
||||
expect(result.success).toBe(false);
|
||||
expect(result.error).toBe('Connection failed');
|
||||
});
|
||||
|
||||
it('should pass tlsRejectUnauthorized to the transporter builder', async () => {
|
||||
(repo.findById as vi.Mock).mockResolvedValue([{ ...mockProvider, tlsRejectUnauthorized: false }]);
|
||||
(transport.verifyTransporter as vi.Mock).mockResolvedValue(undefined);
|
||||
await service.testConnection(10, mockUser);
|
||||
expect(transport.buildTransporter).toHaveBeenCalledWith(expect.objectContaining({ tlsRejectUnauthorized: false }));
|
||||
});
|
||||
});
|
||||
|
||||
describe('getOwnedOrShared', () => {
|
||||
|
||||
@@ -43,6 +43,7 @@ export class EmailProviderService {
|
||||
auth: dto.auth,
|
||||
ssl: dto.ssl,
|
||||
startTls: dto.startTls,
|
||||
tlsRejectUnauthorized: dto.tlsRejectUnauthorized,
|
||||
});
|
||||
return this.sanitize(created);
|
||||
} catch (error) {
|
||||
@@ -66,6 +67,7 @@ export class EmailProviderService {
|
||||
if (dto.auth !== undefined) patch.auth = dto.auth;
|
||||
if (dto.ssl !== undefined) patch.ssl = dto.ssl;
|
||||
if (dto.startTls !== undefined) patch.startTls = dto.startTls;
|
||||
if (dto.tlsRejectUnauthorized !== undefined) patch.tlsRejectUnauthorized = dto.tlsRejectUnauthorized;
|
||||
if (dto.password !== undefined) {
|
||||
patch.passwordEnc = dto.password ? this.encryption.encrypt(dto.password) : null;
|
||||
}
|
||||
@@ -130,6 +132,7 @@ export class EmailProviderService {
|
||||
auth: provider.auth,
|
||||
ssl: provider.ssl,
|
||||
startTls: provider.startTls,
|
||||
tlsRejectUnauthorized: provider.tlsRejectUnauthorized,
|
||||
});
|
||||
|
||||
try {
|
||||
|
||||
@@ -22,6 +22,7 @@ describe('EmailTransportService', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
});
|
||||
|
||||
expect(nodemailer.createTransport).toHaveBeenCalledWith(
|
||||
@@ -44,6 +45,7 @@ describe('EmailTransportService', () => {
|
||||
auth: false,
|
||||
ssl: false,
|
||||
startTls: false,
|
||||
tlsRejectUnauthorized: true,
|
||||
});
|
||||
|
||||
expect(nodemailer.createTransport).toHaveBeenCalledWith(
|
||||
@@ -71,6 +73,7 @@ describe('EmailTransportService', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
});
|
||||
|
||||
expect(nodemailer.createTransport).toHaveBeenCalledWith(
|
||||
@@ -80,7 +83,7 @@ describe('EmailTransportService', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('should not disable TLS certificate verification by default', () => {
|
||||
it('should not set tls option when tlsRejectUnauthorized is true', () => {
|
||||
(nodemailer.createTransport as vi.Mock).mockReturnValue({});
|
||||
|
||||
service.buildTransporter({
|
||||
@@ -91,10 +94,32 @@ describe('EmailTransportService', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
});
|
||||
|
||||
const calls = (nodemailer.createTransport as vi.Mock).mock.calls;
|
||||
const callArg = calls[calls.length - 1][0] as Record<string, unknown>;
|
||||
expect(callArg).not.toHaveProperty('tls');
|
||||
});
|
||||
|
||||
it('should set tls.rejectUnauthorized=false when tlsRejectUnauthorized is false', () => {
|
||||
(nodemailer.createTransport as vi.Mock).mockReturnValue({});
|
||||
|
||||
service.buildTransporter({
|
||||
host: 'smtp.selfhosted.com',
|
||||
port: 465,
|
||||
auth: true,
|
||||
username: 'user',
|
||||
password: 'pass',
|
||||
ssl: true,
|
||||
startTls: false,
|
||||
tlsRejectUnauthorized: false,
|
||||
});
|
||||
|
||||
expect(nodemailer.createTransport).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
tls: { rejectUnauthorized: false },
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -13,6 +13,7 @@ export interface SmtpConfig {
|
||||
auth: boolean;
|
||||
ssl: boolean;
|
||||
startTls: boolean;
|
||||
tlsRejectUnauthorized: boolean;
|
||||
}
|
||||
|
||||
@Injectable()
|
||||
@@ -29,6 +30,7 @@ export class EmailTransportService {
|
||||
secure,
|
||||
requireTLS: requireTls,
|
||||
...(auth ? { auth } : {}),
|
||||
...(!config.tlsRejectUnauthorized ? { tls: { rejectUnauthorized: false } } : {}),
|
||||
};
|
||||
|
||||
return nodemailer.createTransport(options);
|
||||
|
||||
@@ -24,6 +24,7 @@ describe('SystemMailService', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
fromName: 'BookOrbit',
|
||||
fromAddress: 'no-reply@example.com',
|
||||
};
|
||||
@@ -82,6 +83,7 @@ describe('SystemMailService', () => {
|
||||
auth: true,
|
||||
ssl: false,
|
||||
startTls: true,
|
||||
tlsRejectUnauthorized: true,
|
||||
});
|
||||
expect(transporter.sendMail).toHaveBeenCalledWith(
|
||||
expect.objectContaining({
|
||||
|
||||
@@ -49,6 +49,7 @@ export class SystemMailService {
|
||||
auth: provider.auth,
|
||||
ssl: provider.ssl,
|
||||
startTls: provider.startTls,
|
||||
tlsRejectUnauthorized: provider.tlsRejectUnauthorized,
|
||||
});
|
||||
|
||||
const from = provider.fromAddress ? (provider.fromName ? `${provider.fromName} <${provider.fromAddress}>` : provider.fromAddress) : undefined;
|
||||
|
||||
Reference in New Issue
Block a user