mirror of
https://github.com/bretzel-app/crumbs.git
synced 2026-10-12 12:06:57 +02:00
fix: address PR review — sanitize attachments, SSR safety, a11y
- Strip server paths from public attachment responses (new PublicAttachment type) - Use $page.url.origin instead of window.location.origin (SSR-safe) - Fix toggle switch left-4.5 → left-[1.125rem] (valid Tailwind) - Add aria-label to SharingIndicator button for screen readers - Add opportunistic pruning to IP rate limiter to prevent unbounded growth - Reduce shared attachment cache to 1 hour (respect share revocation) - Update OpenAPI schema with PublicAttachment type - Add unit test asserting server paths are not leaked Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
committed by
Fred Cabassut
co-authored by
Claude Opus 4.6
parent
29eb77f9a2
commit
92d0581186
+1
-2
@@ -480,11 +480,10 @@ No authentication required. Returns the note content for a valid share token. Ra
|
||||
"attachments": [
|
||||
{
|
||||
"id": "id",
|
||||
"noteId": "noteId",
|
||||
"filename": "filename",
|
||||
"mimeType": "mimeType",
|
||||
"size": 1,
|
||||
"thumbnailPath": "thumbnailPath",
|
||||
"featured": true,
|
||||
"createdAt": "2025-01-01T00:00:00.000Z"
|
||||
}
|
||||
],
|
||||
|
||||
+19
-1
@@ -618,6 +618,24 @@ components:
|
||||
description: API key (crumbs_...)
|
||||
|
||||
schemas:
|
||||
PublicAttachment:
|
||||
type: object
|
||||
description: Sanitized attachment metadata for public sharing (no server paths)
|
||||
properties:
|
||||
id:
|
||||
type: string
|
||||
filename:
|
||||
type: string
|
||||
mimeType:
|
||||
type: string
|
||||
size:
|
||||
type: integer
|
||||
featured:
|
||||
type: boolean
|
||||
createdAt:
|
||||
type: string
|
||||
format: date-time
|
||||
|
||||
SharedNoteData:
|
||||
type: object
|
||||
properties:
|
||||
@@ -633,7 +651,7 @@ components:
|
||||
attachments:
|
||||
type: array
|
||||
items:
|
||||
$ref: '#/components/schemas/Attachment'
|
||||
$ref: '#/components/schemas/PublicAttachment'
|
||||
createdAt:
|
||||
type: string
|
||||
format: date-time
|
||||
|
||||
@@ -8,6 +8,7 @@
|
||||
import Copy from 'lucide-svelte/icons/copy';
|
||||
import Check from 'lucide-svelte/icons/check';
|
||||
import { showToast } from '$lib/stores/toast.js';
|
||||
import { page } from '$app/state';
|
||||
|
||||
interface UserResult {
|
||||
id: number;
|
||||
@@ -35,7 +36,7 @@
|
||||
let isSearching = $state(false);
|
||||
let searchTimeout: ReturnType<typeof setTimeout> | undefined;
|
||||
|
||||
const shareUrl = $derived(publicShareToken ? `${window.location.origin}/s/${publicShareToken}` : null);
|
||||
const shareUrl = $derived(publicShareToken ? `${page.url.origin}/s/${publicShareToken}` : null);
|
||||
|
||||
async function togglePublicShare() {
|
||||
togglingShare = true;
|
||||
@@ -193,7 +194,7 @@
|
||||
>
|
||||
<span
|
||||
class="absolute top-0.5 h-4 w-4 rounded-sm bg-[var(--bg-surface)] shadow-[1px_1px_0px_var(--border-subtle)] transition-all {publicShareToken
|
||||
? 'left-4.5'
|
||||
? 'left-[1.125rem]'
|
||||
: 'left-0.5'}"
|
||||
></span>
|
||||
</button>
|
||||
|
||||
@@ -53,6 +53,7 @@
|
||||
type="button"
|
||||
class="rounded-sm p-1 {hasPublicLink ? 'text-[var(--primary)]' : 'text-[var(--text-muted)]'} hover:text-[var(--primary)]"
|
||||
title={tooltipLines.join(', ')}
|
||||
aria-label={tooltipLines.join(', ')}
|
||||
onclick={handleClick}
|
||||
data-testid="sharing-indicator"
|
||||
>
|
||||
|
||||
@@ -1,14 +1,30 @@
|
||||
const store = new Map<string, { count: number; windowStart: number }>();
|
||||
const WINDOW_MS = 60_000; // 1 minute
|
||||
const MAX_REQUESTS = 60;
|
||||
const MAX_ENTRIES = 10_000;
|
||||
let cleanupCounter = 0;
|
||||
|
||||
export function checkIpRateLimit(ip: string): boolean {
|
||||
const now = Date.now();
|
||||
const entry = store.get(ip);
|
||||
if (!entry || now - entry.windowStart > WINDOW_MS) {
|
||||
store.set(ip, { count: 1, windowStart: now });
|
||||
pruneIfNeeded(now);
|
||||
return true;
|
||||
}
|
||||
entry.count++;
|
||||
return entry.count <= MAX_REQUESTS;
|
||||
}
|
||||
|
||||
function pruneIfNeeded(now: number) {
|
||||
cleanupCounter++;
|
||||
if (cleanupCounter < 100 || store.size < MAX_ENTRIES) {
|
||||
return;
|
||||
}
|
||||
cleanupCounter = 0;
|
||||
for (const [key, entry] of store) {
|
||||
if (now - entry.windowStart > WINDOW_MS) {
|
||||
store.delete(key);
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
@@ -139,6 +139,9 @@ describe('getSharedNote', () => {
|
||||
const data = getSharedNote(db, token);
|
||||
expect(data!.attachments).toHaveLength(1);
|
||||
expect(data!.attachments[0].filename).toBe('image.png');
|
||||
// Verify server paths are not exposed
|
||||
expect(data!.attachments[0]).not.toHaveProperty('path');
|
||||
expect(data!.attachments[0]).not.toHaveProperty('thumbnailPath');
|
||||
});
|
||||
});
|
||||
|
||||
|
||||
@@ -3,7 +3,7 @@ import { sharedNotes, notes, attachments } from './db/schema.js';
|
||||
import { eq, and, inArray } from 'drizzle-orm';
|
||||
import { nanoid } from 'nanoid';
|
||||
import { requireNoteOwnership } from './api-utils.js';
|
||||
import type { SharedNoteData, Attachment } from '$lib/types/index.js';
|
||||
import type { SharedNoteData } from '$lib/types/index.js';
|
||||
|
||||
/**
|
||||
* Batch fetch share tokens for multiple notes.
|
||||
@@ -93,7 +93,16 @@ export function getSharedNote(db: Db, token: string): (SharedNoteData & { noteId
|
||||
.select()
|
||||
.from(attachments)
|
||||
.where(eq(attachments.noteId, share.noteId))
|
||||
.all() as Attachment[];
|
||||
.all();
|
||||
|
||||
const publicAttachments = noteAttachments.map((a) => ({
|
||||
id: a.id,
|
||||
filename: a.filename,
|
||||
mimeType: a.mimeType,
|
||||
size: a.size,
|
||||
featured: a.featured,
|
||||
createdAt: a.createdAt
|
||||
}));
|
||||
|
||||
return {
|
||||
noteId: note.id,
|
||||
@@ -101,7 +110,7 @@ export function getSharedNote(db: Db, token: string): (SharedNoteData & { noteId
|
||||
content: note.content,
|
||||
checklistMode: note.checklistMode,
|
||||
color: note.color as SharedNoteData['color'],
|
||||
attachments: noteAttachments,
|
||||
attachments: publicAttachments,
|
||||
createdAt: note.createdAt,
|
||||
updatedAt: note.updatedAt
|
||||
};
|
||||
|
||||
+10
-1
@@ -126,12 +126,21 @@ export interface NoteShare {
|
||||
expiresAt: Date | null;
|
||||
}
|
||||
|
||||
export interface PublicAttachment {
|
||||
id: string;
|
||||
filename: string;
|
||||
mimeType: string;
|
||||
size: number;
|
||||
featured: boolean;
|
||||
createdAt: Date;
|
||||
}
|
||||
|
||||
export interface SharedNoteData {
|
||||
title: string;
|
||||
content: string;
|
||||
checklistMode: boolean;
|
||||
color: NoteColor;
|
||||
attachments: Attachment[];
|
||||
attachments: PublicAttachment[];
|
||||
createdAt: Date;
|
||||
updatedAt: Date;
|
||||
}
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
<script lang="ts">
|
||||
import { NOTE_COLORS } from '$lib/utils/colors.js';
|
||||
import { renderMarkdown } from '$lib/utils/markdown.js';
|
||||
import type { NoteColor, Attachment } from '$lib/types/index.js';
|
||||
import type { NoteColor, PublicAttachment } from '$lib/types/index.js';
|
||||
|
||||
interface ChecklistItem {
|
||||
text: string;
|
||||
@@ -29,7 +29,7 @@
|
||||
const doneChecklistItems = checklistItems.filter((i) => i.checked);
|
||||
const sortedChecklistItems = [...activeChecklistItems, ...doneChecklistItems];
|
||||
|
||||
const imageAttachments = (data.attachments as Attachment[]).filter((a) =>
|
||||
const imageAttachments = (data.attachments as PublicAttachment[]).filter((a) =>
|
||||
a.mimeType.startsWith('image/')
|
||||
);
|
||||
|
||||
|
||||
@@ -31,7 +31,7 @@ export const GET: RequestHandler = async ({ params, url, getClientAddress }) =>
|
||||
headers: {
|
||||
'Content-Type': mimeType,
|
||||
'Content-Disposition': `inline; filename="${attachment.filename}"`,
|
||||
'Cache-Control': 'public, max-age=31536000, immutable'
|
||||
'Cache-Control': 'public, max-age=3600'
|
||||
}
|
||||
});
|
||||
};
|
||||
|
||||
Reference in New Issue
Block a user