Skip to content
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

fix(upload): bug sur l'ajout de l'extension #1367

Merged
merged 3 commits into from
Apr 24, 2024
Merged
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
77 changes: 77 additions & 0 deletions targets/frontend/src/lib/__tests__/secu.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import { isUploadFileSafe, isAllowedFile } from "../secu";
import * as formidable from "formidable";
import fs from "fs";

jest.mock("fs");

describe("secu.ts", () => {
describe("isAllowedFile", () => {
test("should return true for allowed extensions", () => {
const file: formidable.File = {
originalFilename: "test.png",
} as formidable.File;

expect(isAllowedFile(file)).toBe(true);
});

test("should return false for disallowed extensions", () => {
const file: formidable.File = {
originalFilename: "test.exe",
} as formidable.File;

expect(isAllowedFile(file)).toBe(false);
});

test("should return false if filename is undefined", () => {
const file: formidable.File = {
originalFilename: undefined,
} as any;

expect(isAllowedFile(file)).toBe(false);
});
});

describe("isUploadFileSafe", () => {
test("should resolve false for disallowed file extensions", async () => {
const file: formidable.File = {
originalFilename: "test.exe",
mimetype: "application/octet-stream",
} as formidable.File;

await expect(isUploadFileSafe(file)).resolves.toBe(false);
});

test("should resolve true for non-SVG file types", async () => {
const file: formidable.File = {
originalFilename: "test.png",
mimetype: "image/png",
} as formidable.File;

await expect(isUploadFileSafe(file)).resolves.toBe(true);
});

test("should resolve true for SVG files without script tags", async () => {
(fs.readFileSync as jest.Mock).mockReturnValue("<svg></svg>");

const file: formidable.File = {
originalFilename: "test.svg",
mimetype: "image/svg+xml",
} as formidable.File;

await expect(isUploadFileSafe(file)).resolves.toBe(true);
});

test("should resolve false for SVG files with script tags", async () => {
(fs.readFileSync as jest.Mock).mockReturnValue(
'<svg><script>alert("XSS")</script></svg>'
);

const file: formidable.File = {
originalFilename: "test.svg",
mimetype: "image/svg+xml",
} as formidable.File;

await expect(isUploadFileSafe(file)).resolves.toBe(false);
});
});
});
4 changes: 2 additions & 2 deletions targets/frontend/src/lib/secu.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,13 +15,13 @@ const ALLOWED_EXTENSIONS = [
...ALLOWED_DOC,
];

const isAllowedFile = (file: formidable.File) => {
export const isAllowedFile = (file: formidable.File) => {
const extension = file.originalFilename
?.toLowerCase()
.split(".")
.reverse()[0];
if (!extension) return false;
return ALLOWED_EXTENSIONS.includes(extension);
return ALLOWED_EXTENSIONS.includes("." + extension);
Copy link
Member Author

Choose a reason for hiding this comment

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

tout ça à cause de ça x)

};

export const isUploadFileSafe = (file: formidable.File): Promise<boolean> => {
Expand Down
17 changes: 8 additions & 9 deletions targets/frontend/src/pages/api/storage/index.ts
Original file line number Diff line number Diff line change
@@ -1,22 +1,19 @@
import Boom from "@hapi/boom";
import formidable, { IncomingForm } from "formidable";
import { createErrorFor } from "src/lib/apiError";
import { isUploadFileSafe } from "src/lib/secu";
import { NextApiRequest, NextApiResponse } from "next";
import { getApiAllFiles, uploadApiFiles } from "src/lib/upload";
import fs from "fs";

async function endPoint(req: NextApiRequest, res: NextApiResponse) {
const apiError = createErrorFor(res);

switch (req.method) {
case "POST":
return uploadFiles(req, res);
case "GET":
return getFiles(req, res);
default: {
res.setHeader("Allow", "GET, POST");
apiError(Boom.methodNotAllowed(`${req.method} not allowed`));
return res
.status(400)
.json({ success: false, errorMessage: `${req.method} not allowed` });
}
}
}
Expand All @@ -34,14 +31,16 @@ function uploadFiles(req: NextApiRequest, res: NextApiResponse) {
const file = allFiles[i];
const isSafe = await isUploadFileSafe(file);
if (!isSafe) {
console.error("A malicious code was find in the upload");
return res.status(400).json({ success: false });
console.error("Malicious code detected");
return res
.status(400)
.json({ success: false, errorMessage: "Malicious code detected" });
}
Comment on lines +35 to +37
Copy link
Member Author

Choose a reason for hiding this comment

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

Je gère mieux les erreurs comme ça, car ça venait du fait que le fichier était pas considéré comme safe

const fileContent = fs.readFileSync(file.filepath);
await uploadApiFiles(`${file.originalFilename}`, fileContent);
}
res.status(200).json({ success: true });
});
res.status(200).json({ success: true });
}

async function getFiles(_req: NextApiRequest, res: NextApiResponse) {
Expand Down
7 changes: 5 additions & 2 deletions targets/frontend/src/pages/fichiers.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,11 @@ function FilesPage() {
mutate();
}, 3000);
})
.catch(() => {
alert("Impossible de supprimer le fichier :/");
.catch((err) => {
alert(
"Impossible de supprimer le fichier :/, le message d'erreur est : " +
JSON.stringify(err.data)
);
})
.finally(() => {
setUploading(false);
Expand Down
Loading