mirror of
https://github.com/snapotter-hq/SnapOtter.git
synced 2026-08-03 07:46:42 +02:00
fix: deduplicate filenames in fetch-urls to prevent overwrites
All URLs in a batch share a single workspace directory. When multiple URLs resolve to the same filename (e.g. two different domains both serving photo.jpg), the second writeFile silently overwrites the first. Track used filenames in a Set and append _1, _2, etc. on collision, mirroring the existing getUniqueName pattern from batch.ts.
This commit is contained in:
@@ -114,6 +114,29 @@ function filenameFromUrl(url: string): string {
|
||||
return `image-${randomUUID().slice(0, 8)}`;
|
||||
}
|
||||
|
||||
/**
|
||||
* Return a filename that does not collide with any name already in `used`.
|
||||
* Appends `_1`, `_2`, etc. before the extension when a collision is found.
|
||||
* Mirrors the deduplication logic in batch.ts.
|
||||
*/
|
||||
function getUniqueName(name: string, used: Set<string>): string {
|
||||
if (!used.has(name)) {
|
||||
used.add(name);
|
||||
return name;
|
||||
}
|
||||
const dotIdx = name.lastIndexOf(".");
|
||||
const base = dotIdx > 0 ? name.slice(0, dotIdx) : name;
|
||||
const ext = dotIdx > 0 ? name.slice(dotIdx) : "";
|
||||
let counter = 1;
|
||||
let candidate = `${base}_${counter}${ext}`;
|
||||
while (used.has(candidate)) {
|
||||
counter++;
|
||||
candidate = `${base}_${counter}${ext}`;
|
||||
}
|
||||
used.add(candidate);
|
||||
return candidate;
|
||||
}
|
||||
|
||||
export async function registerFetchUrlsRoute(app: FastifyInstance): Promise<void> {
|
||||
app.post("/api/v1/fetch-urls", async (request, reply) => {
|
||||
// Validate body
|
||||
@@ -130,13 +153,17 @@ export async function registerFetchUrlsRoute(app: FastifyInstance): Promise<void
|
||||
|
||||
const queue = new PQueue({ concurrency: URL_FETCH_CONCURRENCY });
|
||||
|
||||
// Track filenames to prevent collisions when multiple URLs resolve to the
|
||||
// same name (e.g. https://a.com/photo.jpg and https://b.com/photo.jpg).
|
||||
const usedFilenames = new Set<string>();
|
||||
|
||||
// Pre-allocate result slots to preserve order
|
||||
const resultSlots: FetchResult[] = new Array(urls.length);
|
||||
|
||||
await Promise.all(
|
||||
urls.map((url, index) =>
|
||||
queue.add(async () => {
|
||||
resultSlots[index] = await fetchSingleUrl(url, jobId, outputDir);
|
||||
resultSlots[index] = await fetchSingleUrl(url, jobId, outputDir, usedFilenames);
|
||||
}),
|
||||
),
|
||||
);
|
||||
@@ -145,7 +172,12 @@ export async function registerFetchUrlsRoute(app: FastifyInstance): Promise<void
|
||||
});
|
||||
}
|
||||
|
||||
async function fetchSingleUrl(url: string, jobId: string, outputDir: string): Promise<FetchResult> {
|
||||
async function fetchSingleUrl(
|
||||
url: string,
|
||||
jobId: string,
|
||||
outputDir: string,
|
||||
usedFilenames: Set<string>,
|
||||
): Promise<FetchResult> {
|
||||
try {
|
||||
// Fetch with SSRF protection and timeout
|
||||
const controller = new AbortController();
|
||||
@@ -199,9 +231,10 @@ async function fetchSingleUrl(url: string, jobId: string, outputDir: string): Pr
|
||||
return { success: false, url, error: "Empty response body" };
|
||||
}
|
||||
|
||||
// Derive filename from URL
|
||||
// Derive filename from URL, deduplicating to prevent overwrites when
|
||||
// multiple URLs resolve to the same name (all URLs share one workspace).
|
||||
const rawFilename = filenameFromUrl(url);
|
||||
const filename = sanitizeFilename(rawFilename);
|
||||
const filename = getUniqueName(sanitizeFilename(rawFilename), usedFilenames);
|
||||
|
||||
// Validate as an image
|
||||
const validation = await validateImageBuffer(buffer, filename);
|
||||
|
||||
@@ -299,6 +299,46 @@ describe("POST /api/v1/fetch-urls", () => {
|
||||
expect(downloadRes.rawPayload.length).toBe(JPG.length);
|
||||
});
|
||||
|
||||
it("deduplicates filenames when multiple URLs resolve to the same name", async () => {
|
||||
const res = await app.inject({
|
||||
method: "POST",
|
||||
url: "/api/v1/fetch-urls",
|
||||
headers: {
|
||||
authorization: `Bearer ${adminToken}`,
|
||||
},
|
||||
payload: {
|
||||
urls: [`http://127.0.0.1:${mockPort}/photo.jpg`, `http://127.0.0.1:${mockPort}/photo.jpg`],
|
||||
},
|
||||
});
|
||||
|
||||
expect(res.statusCode).toBe(200);
|
||||
const body = JSON.parse(res.body);
|
||||
expect(body.results).toHaveLength(2);
|
||||
|
||||
expect(body.results[0].success).toBe(true);
|
||||
expect(body.results[1].success).toBe(true);
|
||||
|
||||
// Filenames must differ so one does not overwrite the other
|
||||
const names = [body.results[0].filename, body.results[1].filename];
|
||||
expect(new Set(names).size).toBe(2);
|
||||
expect(names).toContain("photo.jpg");
|
||||
expect(names).toContain("photo_1.jpg");
|
||||
|
||||
// Download URLs must also differ
|
||||
expect(body.results[0].downloadUrl).not.toBe(body.results[1].downloadUrl);
|
||||
|
||||
// Both download URLs should serve valid content
|
||||
for (const result of body.results) {
|
||||
const dl = await app.inject({
|
||||
method: "GET",
|
||||
url: result.downloadUrl,
|
||||
headers: { authorization: `Bearer ${adminToken}` },
|
||||
});
|
||||
expect(dl.statusCode).toBe(200);
|
||||
expect(dl.rawPayload.length).toBe(JPG.length);
|
||||
}
|
||||
});
|
||||
|
||||
it("returns 400 for invalid URL format", async () => {
|
||||
const res = await app.inject({
|
||||
method: "POST",
|
||||
|
||||
Reference in New Issue
Block a user