Skip to content

Allow updating files (instead of overriding) for multiple file uploads #191

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.

Sign up for GitHub

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

Closed
janezd wants to merge 1 commit into from
Closed

Allow updating files (instead of overriding) for multiple file uploads #191

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
1 change: 1 addition & 0 deletions .gitattributes
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
next-env.d.ts merge=ours
70 changes: 64 additions & 6 deletions api/quiz.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,9 @@
import db from "@/utils/db";
import { getUserId } from "@/utils/user";
import { isAdminFor } from "@/api/user";
import { getUploadDir } from "@/utils/zip";
import path from "path";
import fs from "fs";

/* Functions in this module get user's accessToken rather than id
because id's can be faked.
Expand Down Expand Up @@ -30,6 +33,7 @@ export type PostAnswerResult =
{ status: "error",
message: string} |
{ status: "ok",
storedAnswer: string,
isCorrect: boolean | undefined;
points: number,
correctAnswer?: string };
Expand Down Expand Up @@ -59,15 +63,18 @@ export const postAnswer = async (
if (!question) {
return {status: "error", message: "Question id and book id don't match"};
}
const nPastAnswers = (await db.get(`
SELECT COUNT(*) as count FROM answers
const pastAnswers = await db.all(`
SELECT answer FROM answers
WHERE userId = ? AND bookId = ? AND groupId IS ?
AND questionId = ?
`, [userId, bookId, groupId, question.id])).count;
if (question.maxAttempts && nPastAnswers >= question.maxAttempts) {
`, [userId, bookId, groupId, question.id]);
if (question.maxAttempts && pastAnswers.length >= question.maxAttempts) {
return {status: "error", message: "Maximum number of attempts reached"};
}

const pastFiles = question.type !== "uploads" || !pastAnswers.length ? []
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The condition question.type !== "uploads" only checks for the plural form, but according to the codebase, upload questions can have type "upload" (singular) or "uploads" (plural). This means the file merging logic won't work for singular "upload" type questions. The condition should be question.type !== "uploads" && question.type !== "upload" or use !question.type.startsWith("upload") to handle both cases.

Suggested change
const pastFiles = question.type !== "uploads" || !pastAnswers.length ? []
const pastFiles = (question.type !== "uploads" && question.type !== "upload") || !pastAnswers.length ? []

Copilot uses AI. Check for mistakes.
: pastAnswers[pastAnswers.length - 1].answer.split(":");

/* If we have the answer in the database, we check the submitted answer
and assign points. Otherwise, we trust the received isCorrect and points.
*/
Expand All @@ -79,18 +86,23 @@ export const postAnswer = async (
!question.answer ? points
: actCorrect && question.maxPoints || 0;

const actAnswer = !pastFiles.length
? answer
: [...pastFiles, ...answer.split(":").filter((f) => !pastFiles.includes(f))]
.join(":");
Comment on lines +89 to +92
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The file merging logic could have issues with empty filenames. If a user uploads a file, then removes it (resulting in an empty string answer), and then uploads again, the split(":") could create an array with an empty string element. The filter !pastFiles.includes(f) won't remove truly duplicate files if there's an empty string in pastFiles. Consider filtering out empty strings: pastFiles.filter(f => f). and similar for the new files before merging.

Copilot uses AI. Check for mistakes.
await db.run(
`INSERT INTO answers (userId, bookId, groupId, questionId, answer, isCorrect, points)
VALUES (?, ?, ?, ?, ?, ?, ?)`,
[userId, bookId, groupId, question.id, answer, actCorrect, actPoints]
[userId, bookId, groupId, question.id, actAnswer, actCorrect, actPoints]
);

const shownAnswer =
(question.maxAttempts
&& nPastAnswers + 1 >= question.maxAttempts
&& pastAnswers.length + 1 >= question.maxAttempts
&& question.answer) ? { correctAnswer: question.answer } : {};
return {
status: "ok",
storedAnswer: actAnswer,
isCorrect: actCorrect,
points: actPoints,
...shownAnswer};
Expand Down Expand Up @@ -296,6 +308,52 @@ export const getUserFilesInBook = async (
)) as (Omit<UserFileAnswersInBook, "fileNames"> & {answer: string})[]
).map(({answer, ...rest}) => ({fileNames: answer.split(":"), ...rest}));

export const removeUserFile = async (
{accessToken, bookId, groupId, questionId, fileName}:
{
accessToken: string;
bookId: number;
groupId: number | null;
questionId: string;
fileName: string;
}): Promise<string | null> => {
const lastAnswer = (await db.get(`
SELECT a.id, a.answer FROM answers a
JOIN questions q ON a.questionId = q.id
JOIN books_chapters bc ON q.chapterId = bc.chapterId
JOIN users ON a.userId = users.id
WHERE users.accessToken = ? AND groupId IS ?
AND q.questionId = ? AND bc.bookId = ? AND q.type LIKE 'upload%'
ORDER BY a.createdAt DESC
LIMIT 1;
`, [accessToken, groupId, questionId, bookId]
)) as ({id: number, answer: string} | undefined);
if (!lastAnswer) {
return null;
}
const answers = lastAnswer.answer.split(":");
if (!answers.includes(fileName)) {
return null;
}

const {dir, error} = await getUploadDir(
{accessToken, bookId, groupId, qId: questionId});
if (dir && !error) {
const fname = path.join(dir, fileName);
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The removeUserFile function has a potential path traversal vulnerability. The fileName parameter is directly used in path.join without validation. A malicious user could pass a fileName like "../../../sensitive-file" to delete files outside the intended upload directory. Consider validating that fileName doesn't contain path separators or use path.basename(fileName) to ensure only the filename component is used.

Suggested change
const fname = path.join(dir, fileName);
const safeFileName = path.basename(fileName);
// Prevent path traversal: if basename changed, the input contained path separators
if (safeFileName !== fileName) {
return null;
}
const fname = path.join(dir, safeFileName);

Copilot uses AI. Check for mistakes.
if (fs.existsSync(fname)) {
fs.rmSync(fname);
}
}
const newAnswers = answers.filter((f) => f !== fileName).join(":");
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

When all files are removed, the filter operation will create an empty array, and join(":") will return an empty string. However, if there's ever a case where answer is already an empty string, split(":") returns [""], and if fileName is also "", this could match and try to filter it out. Consider adding a check to handle empty answers more explicitly, or filter out empty strings after split: answers.filter(f => f && f !== fileName)

Suggested change
const newAnswers = answers.filter((f) => f !== fileName).join(":");
const newAnswers = answers.filter((f) => f && f !== fileName).join(":");

Copilot uses AI. Check for mistakes.
await db.run(`
UPDATE answers
SET answer = ?
WHERE id = ?`,
[newAnswers, lastAnswer.id]
);
Comment on lines +332 to +353
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The removeFile function has a potential race condition. If the database update succeeds but the file system deletion fails (or vice versa), the system state becomes inconsistent. Consider either handling errors from fs.rmSync or wrapping the entire operation in a transaction-like pattern. Additionally, if answer is null, the function returns false but doesn't provide any error feedback to the user.

Suggested change
return null;
}
const answers = lastAnswer.answer.split(":");
if (!answers.includes(fileName)) {
return null;
}
const {dir, error} = await getUploadDir(
{accessToken, bookId, groupId, qId: questionId});
if (dir && !error) {
const fname = path.join(dir, fileName);
if (fs.existsSync(fname)) {
fs.rmSync(fname);
}
}
const newAnswers = answers.filter((f) => f !== fileName).join(":");
await db.run(`
UPDATE answers
SET answer = ?
WHERE id = ?`,
[newAnswers, lastAnswer.id]
);
console.warn("removeUserFile: no previous answer found for given parameters", {
bookId,
groupId,
questionId,
fileName,
});
return null;
}
const answers = lastAnswer.answer.split(":");
if (!answers.includes(fileName)) {
console.warn("removeUserFile: requested file name not found in last answer", {
bookId,
groupId,
questionId,
fileName,
});
return null;
}
const oldAnswers = lastAnswer.answer;
const newAnswers = answers.filter((f) => f !== fileName).join(":");
// First, update the database. If this fails, do not touch the filesystem.
try {
await db.run(`
UPDATE answers
SET answer = ?
WHERE id = ?`,
[newAnswers, lastAnswer.id]
);
} catch (err) {
console.error("removeUserFile: failed to update answers in database", {
error: err,
answerId: lastAnswer.id,
});
return null;
}
// After a successful DB update, attempt to remove the file. On failure, try to roll back the DB change.
const {dir, error} = await getUploadDir(
{accessToken, bookId, groupId, qId: questionId});
if (dir && !error) {
const fname = path.join(dir, fileName);
if (fs.existsSync(fname)) {
try {
fs.rmSync(fname);
} catch (fsErr) {
console.error("removeUserFile: failed to delete file from filesystem, attempting to roll back DB change", {
error: fsErr,
filePath: fname,
answerId: lastAnswer.id,
});
try {
await db.run(`
UPDATE answers
SET answer = ?
WHERE id = ?`,
[oldAnswers, lastAnswer.id]
);
} catch (rollbackErr) {
console.error("removeUserFile: failed to roll back database after filesystem error; state may be inconsistent", {
error: rollbackErr,
answerId: lastAnswer.id,
});
}
return null;
}
}
}

Copilot uses AI. Check for mistakes.
return newAnswers;
}

export type UsersPoints = {
[bookId: number]: number
}
Expand Down
7 changes: 3 additions & 4 deletions app/api/upload-answer/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import path from "path";
import { existsSync, rmSync, mkdirSync, writeFileSync } from "fs";
import { existsSync, mkdirSync, writeFileSync } from "fs";
import { NextResponse } from "next/server";
import { getUploadDir } from "@/utils/zip";

Expand Down Expand Up @@ -27,10 +27,9 @@ export async function POST(req: Request) {
{ status: 500 });
}

if (existsSync(dir)) {
rmSync(dir, { force: true, recursive: true });
if (!existsSync(dir)) {
mkdirSync(dir, { recursive: true });
}
Comment on lines +30 to 32
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The change from removing and recreating the directory to just creating it if it doesn't exist means that if a user uploads a new file with the same name as an existing file, the new file will silently overwrite the old one in the file system. However, the database merge logic in api/quiz.ts (line 91) filters out duplicates with !pastFiles.includes(f). This means the behavior is correct for preventing duplicate entries in the database, but users should be aware that re-uploading a file with the same name replaces the previous version.

Copilot uses AI. Check for mistakes.
mkdirSync(dir, { recursive: true });

for (const file of files) {
const bytes = await file.arrayBuffer();
Expand Down
2 changes: 1 addition & 1 deletion components/Quiz/Quiz.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,7 +192,7 @@ export default function Question({
options={options} answer={usersAnswers ? "" : answer} onSubmit={onSubmit} /> }
{ isUpload && <FileQuestion
id={id}
submitDisabled={submitDisabled} /* TODO: is this needed? */
submitDisabled={submitDisabled}
setSubmitted={setSubmitted}
ref={onFileDropRef}
accept={accept}
Expand Down
50 changes: 35 additions & 15 deletions components/Quiz/UploadQuestion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const FileQuestion = ({id, submitDisabled, setSubmitted, accept, multiple
ref?: React.RefObject<FileDropFunction | null>;
}) => {
const {t} = useIntl();
const {answer, uploadFiles} = useLastAnswer(id);
const {answer, uploadFiles, removeFile} = useLastAnswer(id);
const [files, setFiles] = React.useState<File[]>([]);

const onSubmitFiles = React.useCallback(async (e: React.MouseEvent) => {
Expand Down Expand Up @@ -69,14 +69,34 @@ export const FileQuestion = ({id, submitDisabled, setSubmitted, accept, multiple

React.useImperativeHandle(ref, () => onFileDrop, [onFileDrop]);

const onRemoveUploadedFile = React.useCallback(async (file: string) => {
await removeFile(file);
}, [removeFile]);

return <>
{ answer &&
{ answer && submitDisabled &&
<div className="mb-4">
{ `${t("quiz.uploaded-file")} ${answer.replaceAll(":", ", ")}.` }
</div>
}
{ !submitDisabled &&
<>
{ answer &&
<div className="mb-1 flex flex-row gap-2">
{t("quiz.uploaded-file")}:&nbsp;
{ answer.split(":").map((file, i) =>
<div key={i} className="flex flex-row">
{file}
<RiDeleteBin2Line
onClick={() => onRemoveUploadedFile(file)}
style={{cursor: "pointer"}}
className="hover:text-red-700"
/>
Comment on lines +90 to +94
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The delete icon buttons are not keyboard accessible and lack proper ARIA labels. Users navigating with a keyboard cannot tab to these buttons, and screen reader users won't know what the buttons do. Consider wrapping the icon in a button element with proper aria-label, or adding tabIndex={0} and onKeyDown handlers to support keyboard navigation.

Copilot uses AI. Check for mistakes.
</div>
)
}
</div>
}
<div className="flex flex-col gap-1 my-4 border-dashed border-1 rounded p-3"
>
<div className="grid gap-x-5 px-1 mb-3 items-center"
Expand All @@ -92,7 +112,7 @@ export const FileQuestion = ({id, submitDisabled, setSubmitted, accept, multiple
/>
}
</React.Fragment>
)}
)}
</div>
<div className="flex items-center justify-between">
<input id="file" type="file" multiple={multiple} onChange={onFileChange}
Expand All @@ -101,7 +121,7 @@ export const FileQuestion = ({id, submitDisabled, setSubmitted, accept, multiple
htmlFor="file"
className={`px-10 mr-4 submit-quiz-popup-button border border-black rounded cursor-pointer transition inline-block`}
>
{t("quiz.select-files")(files.length, multiple)}
{t("quiz.select-files")(files.length || answer, multiple)}
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The parameter passed to the translation function should be a number, but files.length || answer evaluates to a truthy value when answer is a non-empty string. This means when there are no files selected but there is an answer, the function will receive a string instead of a number. This should be changed to files.length || (answer ? 1 : 0) or similar to ensure a number is always passed.

Suggested change
{t("quiz.select-files")(files.length || answer, multiple)}
{t("quiz.select-files")(files.length || (answer ? 1 : 0), multiple)}

Copilot uses AI. Check for mistakes.
</label>

<small className="form-text text-muted" style={{lineHeight: "1.4"}}>
Expand All @@ -117,20 +137,20 @@ export const FileQuestion = ({id, submitDisabled, setSubmitted, accept, multiple
<button onClick={onSubmitFiles} disabled={!files.length}>
{t(`quiz.submit-button`)}
</button>
{ !!files.length &&
<div className="flex flex-col" style={{lineHeight: "1.4"}}>
<small className="form-text text-muted">
{ t("quiz.dont-forget-to-submit-file")(files.length) }
</small>
{!!answer &&
{!!files.length &&
<div className="flex flex-col text-red-500" style={{lineHeight: "1.4"}}>
<small className="form-text text-muted">
{t("quiz.submit-will-replace")(files.length)}
{t("quiz.dont-forget-to-submit-file")(files.length)}
Comment on lines +141 to +143
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The container div has the class "text-red-500" to make text red, but the small elements inside have class "text-muted" which likely overrides the red color. The red color class should be moved to the individual small elements or removed from the parent if it's not needed.

Copilot uses AI. Check for mistakes.
</small>
}
{!!answer && !multiple &&
<small className="form-text text-muted">
{t("quiz.submit-will-replace")(files.length)}
</small>
}
</div>
}
</div>
</>
}
</>
}
}
</>
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

Avoid automated semicolon insertion (90% of all statements in the enclosing function have an explicit semicolon).

Suggested change
</>
</>;

Copilot uses AI. Check for mistakes.
}
44 changes: 42 additions & 2 deletions context/QuizContextProvider.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import React from "react";

import { getQId, postAnswer, PostAnswerResult, CorrectAnswers } from "@/api/quiz";
import { getQId, postAnswer, PostAnswerResult, CorrectAnswers, removeUserFile } from "@/api/quiz";
import { ChapterDef } from "@/types";
import { logger } from "@/utils/logger";
import { UserContext } from "@/context/UserContextProvider";
Expand Down Expand Up @@ -124,6 +124,7 @@ export const QuizContext = React.createContext<{
threshold: number | null;
answerQuestion: (value: AnswerWithQuestionId) => Promise<boolean>;
uploadFiles: (questionId: string, files: File[]) => Promise<boolean>;
removeFile: (questionId: string, fileName: string) => Promise<boolean>;
getAnswers: (questionId: string) => Answer[];
getCorrectAnswer: (questionId: string) => string | undefined;
submissionErrored: (questionId: string) => boolean | string;
Expand All @@ -145,6 +146,7 @@ export const QuizContext = React.createContext<{
threshold: null,
answerQuestion: async () => false,
uploadFiles: async () => false,
removeFile: async () => false,
getAnswers: () => [],
getCorrectAnswer: () => undefined,
submissionErrored: () => false,
Expand Down Expand Up @@ -204,7 +206,7 @@ export const QuizContextProvider = ({
type: "ANSWER",
value: {
questionId,
answer,
answer: postResult.storedAnswer,
...postResult
}
});
Expand Down Expand Up @@ -263,6 +265,40 @@ export const QuizContextProvider = ({
},
[user, bookId, quizReducer, answerQuestion, userGroup, t]);

const removeFile = React.useCallback(
async(questionId: string, fileName: string): Promise<boolean> => {
if (!user) {
quizReducer({
type: "ERROR",
value: {questionId, error: t("quiz.not-logged-in")}});
return false;
}
const groupId = userGroup !== null ? await getGroupId(userGroup, bookId) : null;
if (userGroup && groupId === null) {
quizReducer({
type: "ERROR",
value: {questionId, error: t("quiz.invalid-group")}});
return false;
}

const answer = await removeUserFile({
accessToken: user.accessToken, bookId, questionId, groupId, fileName});
if (answer === null) {
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The removeFile callback doesn't handle errors properly. If removeUserFile returns null (indicating failure), the function returns false but doesn't set an error state in the reducer. Users won't see any feedback explaining why the file wasn't removed. Consider adding error handling similar to the uploadFiles function that calls quizReducer with type "ERROR" when the operation fails.

Suggested change
if (answer === null) {
if (answer === null) {
quizReducer({type: "ERROR", value: {questionId}});

Copilot uses AI. Check for mistakes.
return false;
}
quizReducer({
type: "ANSWER",
value: {
questionId,
answer,
isCorrect: undefined,
points: 0
}
});
Copy link

Copilot AI Feb 11, 2026

Choose a reason for hiding this comment

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

The closing brace has inconsistent indentation with 7 spaces instead of 8 (like the opening brace on line 289). This should be aligned properly for consistency.

Suggested change
});
});

Copilot uses AI. Check for mistakes.
return true;
},
[user, bookId, quizReducer, userGroup, t]);

const {
nQuestions,
achievedPoints,
Expand Down Expand Up @@ -344,6 +380,7 @@ export const QuizContextProvider = ({
threshold: quizThreshold,
answerQuestion,
uploadFiles,
removeFile,
chapterStats,
getCorrectAnswer: (questionId: string) => quizState.questions[questionId]?.correctAnswer,
getAnswers: (questionId: string) => quizState.questions[questionId]?.answers ?? [],
Expand All @@ -353,6 +390,7 @@ export const QuizContextProvider = ({
quizState,
answerQuestion,
uploadFiles,
removeFile,
nQuestions,
quizThreshold,
achievedPoints,
Expand All @@ -376,6 +414,7 @@ export const useLastAnswer = (questionId: string) => {
submissionErrored,
answerQuestion: aq,
uploadFiles: uf,
removeFile: rf,
getCorrectAnswer,
} = React.useContext(QuizContext);
const answers = getAnswers(questionId) || [];
Expand All @@ -384,6 +423,7 @@ export const useLastAnswer = (questionId: string) => {
submissionErrored: submissionErrored(questionId),
answerQuestion: async (value: Answer) => await aq({questionId, ...value}),
uploadFiles: async (files: File[]) => await uf(questionId, files),
removeFile: async (fileName: string) => await rf(questionId, fileName),
correctAnswer: getCorrectAnswer(questionId)
};
if (answers.length === 0) {
Expand Down
1 change: 0 additions & 1 deletion next.config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import type { NextConfig } from "next";

const nextConfig: NextConfig = {
/* config options here */
};

export default nextConfig;
Loading