Skip to content
Open
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
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,8 @@ class FileMappingPreconditionValidator(
* - Trailing periods: Windows silently strips these, and single or double period names break path normalisation
* - Whitespace characters
*
* The website mirrors this validation in fileNameValidation.ts - any changes here must also be made there.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nice!

*
* References:
* - https://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits
* - https://learn.microsoft.com/en-us/windows/win32/fileio/naming-a-file
Expand Down Expand Up @@ -206,6 +208,8 @@ class FileMappingPreconditionValidator(
* - Underscores (_)
* - Hyphens (-)
* - Periods (.)
*
* The website mirrors this validation in fileNameValidation.ts - any changes here must also be made there.
*/
private fun strictValidateFilename(filename: String, category: FileCategory) {
if (!STRICT_FILENAME_REGEX.matches(filename)) {
Expand Down
4 changes: 4 additions & 0 deletions kubernetes/loculus/templates/_common-metadata.tpl
Original file line number Diff line number Diff line change
Expand Up @@ -239,6 +239,10 @@ enableDataUseTerms: {{ $.Values.dataUseTerms.enabled }}
{{ if $.Values.dataUseTerms.agreementHTML }}
dataUseTermsAgreementHTML: {{ quote $.Values.dataUseTerms.agreementHTML }}
{{- end }}
{{- if .Values.fileSharing }}
fileSharing:
{{ .Values.fileSharing | toYaml | nindent 2 }}
{{- end }}
accessionPrefix: {{ quote $.Values.accessionPrefix }}
dateFieldForGroupGraph: {{ if $.Values.dateFieldForGroupGraph }}{{ quote $.Values.dateFieldForGroupGraph }}{{ else }}null{{ end }}
{{- $commonMetadata := (include "loculus.commonMetadata" . | fromYaml).fields }}
Expand Down
1 change: 1 addition & 0 deletions website/src/components/Edit/EditPage.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,7 @@ function renderEditPage({
maxSequencesPerEntry: 1,
}}
sequenceEntryHistory={sequenceEntryHistory}
fileSharingConfig={{ disableStrictFilenameValidation: false }}
Comment thread
anna-parker marked this conversation as resolved.
/>
</QueryClientProvider>,
);
Expand Down
5 changes: 4 additions & 1 deletion website/src/components/Edit/EditPage.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { routes } from '../../routes/routes.ts';
import { backendApi } from '../../services/backendApi.ts';
import { backendClientHooks } from '../../services/serviceHooks.ts';
import { type FilesByCategory, type SequenceEntryToEdit, approvedForReleaseStatus } from '../../types/backend.ts';
import { type InputField, type SubmissionDataTypes } from '../../types/config.ts';
import { type FileSharingConfig, type InputField, type SubmissionDataTypes } from '../../types/config.ts';
import {
getLatestAccessionVersionForRevision,
isLatestVersionRevocation,
Expand Down Expand Up @@ -41,6 +41,7 @@ type EditPageProps = {
accessToken: string;
groupedInputFields: Map<string, InputField[]>;
submissionDataTypes: SubmissionDataTypes;
fileSharingConfig: FileSharingConfig;
sequenceEntryHistory?: SequenceEntryHistory;
};

Expand All @@ -66,6 +67,7 @@ const InnerEditPage: FC<EditPageProps> = ({
accessToken,
groupedInputFields,
submissionDataTypes,
fileSharingConfig,
sequenceEntryHistory,
}) => {
const [editableMetadata, setEditableMetadata] = useState(EditableMetadata.fromInitialData(dataToEdit));
Expand Down Expand Up @@ -256,6 +258,7 @@ const InnerEditPage: FC<EditPageProps> = ({
fileUploadStates={fileUploadStates}
setFileUploadStates={setFileUploadStates}
onError={(msg) => toast.error(msg, { position: 'top-center', autoClose: false })}
fileSharingConfig={fileSharingConfig}
/>
</div>
)}
Expand Down
9 changes: 8 additions & 1 deletion website/src/components/Submission/DataUploadForm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
openDataUseTermsOption,
restrictedDataUseTermsOption,
} from '../../types/backend.ts';
import type { FileCategory, InputField } from '../../types/config.ts';
import type { FileCategory, FileSharingConfig, InputField } from '../../types/config.ts';
import type { SubmissionDataTypes } from '../../types/config.ts';
import type { ClientConfig } from '../../types/runtimeConfig.ts';
import { createAuthorizationHeader } from '../../utils/createAuthorizationHeader.ts';
Expand Down Expand Up @@ -57,6 +57,7 @@ type DataUploadFormProps = {
onError: (message: string) => void;
submissionDataTypes: SubmissionDataTypes;
dataUseTermsEnabled: boolean;
fileSharingConfig: FileSharingConfig;
};

const logger = getClientLogger('DataUploadForm');
Expand All @@ -74,6 +75,7 @@ const InnerDataUploadForm = ({
metadataTemplateFields,
submissionDataTypes,
dataUseTermsEnabled,
fileSharingConfig,
}: DataUploadFormProps) => {
const extraFilesEnabled = submissionDataTypes.files?.enabled ?? false;

Expand Down Expand Up @@ -240,6 +242,7 @@ const InnerDataUploadForm = ({
metadataTemplateFields={metadataTemplateFields}
submissionDataTypes={submissionDataTypes}
onError={onError}
fileSharingConfig={fileSharingConfig}
/>
<hr />
{extraFilesEnabled && (
Expand All @@ -254,6 +257,7 @@ const InnerDataUploadForm = ({
fileUploadStates={fileUploadStates}
setFileUploadStates={setFileUploadStates}
fileLinkage={fileLinkage}
fileSharingConfig={fileSharingConfig}
/>
<hr />
</>
Expand Down Expand Up @@ -441,6 +445,7 @@ export const ExtraFilesUpload = ({
setFileUploadStates,
fileLinkage,
onError,
fileSharingConfig,
}: {
accessToken: string;
clientConfig: ClientConfig;
Expand All @@ -451,6 +456,7 @@ export const ExtraFilesUpload = ({
setFileUploadStates: Dispatch<SetStateAction<Map<string, FileUploadState>>>;
fileLinkage?: FileLinkage;
onError: (message: string) => void;
fileSharingConfig: FileSharingConfig;
}) => {
const setCategoryFileUploadState =
(category: string): Dispatch<SetStateAction<FileUploadState | undefined>> =>
Expand Down Expand Up @@ -490,6 +496,7 @@ export const ExtraFilesUpload = ({
onError={onError}
fileUploadState={fileUploadStates.get(fileCategory.name)}
setFileUploadState={setCategoryFileUploadState(fileCategory.name)}
fileSharingConfig={fileSharingConfig}
/>
{inputMode === 'bulk' && (
<CategoryLinkageStatus categoryLinkage={fileLinkage?.get(fileCategory.name)} />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@ const defaultProps = {
clientConfig: { backendUrl: 'http://test-backend', lapisUrls: {} },
groupId: 1,
onError: mockOnError,
fileSharingConfig: { disableStrictFilenameValidation: true },
};

const previousUploadsState = (files: { fileId: string; path: string }[]): FileUploadState => ({
Expand Down Expand Up @@ -258,7 +259,8 @@ describe('FolderUploadComponent', () => {

await userEvent.upload(screen.getByTestId('extraFiles'), file);

expect(mockOnError).toHaveBeenCalledWith('File names cannot contain whitespace.');
expect(mockOnError).toHaveBeenCalledWith(expect.stringContaining('File'));
expect(mockOnError).toHaveBeenCalledWith(expect.stringContaining('may not contain whitespace'));
expect(mockRequestMultipartUpload).not.toHaveBeenCalled();
});

Expand All @@ -273,7 +275,8 @@ describe('FolderUploadComponent', () => {

await userEvent.upload(screen.getByTestId('extraFiles'), file);

expect(mockOnError).toHaveBeenCalledWith('Folder names cannot contain whitespace.');
expect(mockOnError).toHaveBeenCalledWith(expect.stringContaining('Folder'));
expect(mockOnError).toHaveBeenCalledWith(expect.stringContaining('cannot contain whitespace'));
expect(mockRequestMultipartUpload).not.toHaveBeenCalled();
});

Expand All @@ -285,7 +288,8 @@ describe('FolderUploadComponent', () => {

await userEvent.upload(screen.getByTestId('add_extraFiles'), file);

expect(mockOnError).toHaveBeenCalledWith('File names cannot contain whitespace.');
expect(mockOnError).toHaveBeenCalledWith(expect.stringContaining('File'));
expect(mockOnError).toHaveBeenCalledWith(expect.stringContaining('may not contain whitespace'));
expect(mockRequestMultipartUpload).not.toHaveBeenCalled();
});
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,18 +2,19 @@ import { produce } from 'immer';
import React, { useEffect, useState, type Dispatch, type FC, type SetStateAction } from 'react';
import { toast } from 'react-toastify';

import type {
Awaiting,
FileUploadState,
Pending,
PreviousUpload,
SingleFileUpload,
Uploaded,
UploadStatus,
import { validateFileNames, getFileNameErrorMessage } from './fileNameValidation';
import {
type Awaiting,
type FileUploadState,
type Pending,
type PreviousUpload,
type SingleFileUpload,
type Uploaded,
type UploadStatus,
} from './fileUpload';
import useClientFlag from '../../../hooks/isClient';
import { BackendClient } from '../../../services/backendClient';
import { type FileCategory } from '../../../types/config';
import { type FileCategory, type FileSharingConfig } from '../../../types/config';
import type { ClientConfig } from '../../../types/runtimeConfig';
import { calculatePartSizeAndCount, splitFileIntoParts, uploadPart } from '../../../utils/multipartUpload';
import { displayConfirmationDialog } from '../../ConfirmationDialog';
Expand All @@ -32,6 +33,7 @@ type FolderUploadComponentProps = {
fileUploadState: FileUploadState | undefined;
setFileUploadState: Dispatch<SetStateAction<FileUploadState | undefined>>;
onError: (message: string) => void;
fileSharingConfig: FileSharingConfig;
};

const FileInput = ({
Expand Down Expand Up @@ -89,6 +91,7 @@ export const FolderUploadComponent: FC<FolderUploadComponentProps> = ({
fileUploadState,
setFileUploadState,
onError,
fileSharingConfig,
}) => {
const [isDragging, setIsDragging] = useState(false);

Expand Down Expand Up @@ -227,7 +230,7 @@ export const FolderUploadComponent: FC<FolderUploadComponentProps> = ({
// Reset the input so the same folder can be selected again
e.target.value = '';

const error = isFilesArrayValid(filesArray, inputMode);
const error = isFilesArrayValid(filesArray, inputMode, fileSharingConfig);
if (error) {
onError(error);
return;
Expand Down Expand Up @@ -269,7 +272,7 @@ export const FolderUploadComponent: FC<FolderUploadComponentProps> = ({
// Reset the input so the same file can be selected again
e.target.value = '';

const error = isFilesArrayValid(filesArray, inputMode);
const error = isFilesArrayValid(filesArray, inputMode, fileSharingConfig);
if (error) {
onError(error);
return;
Expand Down Expand Up @@ -524,15 +527,23 @@ const filterDotFiles = (files: File[]): File[] => {
/**
* Returns `undefined` if the files are fine, or an error otherwise.
*/
const isFilesArrayValid = (files: File[], inputMode: InputMode): string | undefined => {
const isFilesArrayValid = (
files: File[],
inputMode: InputMode,
fileSharingConfig: FileSharingConfig,
): string | undefined => {
if (inputMode === 'form') {
if (files.some((f) => f.webkitRelativePath.split('/').length > 2)) {
return 'Subdirectories are not supported for individual submissions.';
}
}
const fileNames = files.map((f) => f.name);
const folderNames = files.flatMap((f) => f.webkitRelativePath.split('/').slice(1, -1));
const fileNames = files.map((f) => f.name);

if (fileNames.some((n) => /\s/.test(n))) return 'File names cannot contain whitespace.';
if (folderNames.some((p) => /\s/.test(p))) return 'Folder names cannot contain whitespace.';

const fileNameValidationResult = validateFileNames(fileNames, fileSharingConfig);
if (fileNameValidationResult.isErr()) {
return 'Encountered errors in uploaded files: ' + getFileNameErrorMessage(fileNameValidationResult.error);
}
Comment on lines 540 to +548

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two things worth confirming are intentional:

  1. Bulk mode is now stricter than the backend. In bulk mode the name that reaches the backend is the metadata name, not the uploaded basename — fileMapping.ts:321 lets a row declare reads.fastq::文件.fastq, and the backend only validates reads.fastq. That submission is valid server-side, but this check rejects the upload because the file on disk is 文件.fastq. In form/individual mode the path is used as its own name (fileMapping.ts:233), so validating basenames there is correct and necessary. If the over-strictness in bulk mode isn't wanted, the strict portion could be limited to inputMode === 'form'.

  2. Folder names are still only whitespace-checked (line 543) while file names now get the full ruleset, so under strict config a folder data#1 or 文件 passes but a file with those characters doesn't. That may be fine given folder names never reach the backend, but the error strings also now differ in style (Folder names cannot contain whitespace. vs Encountered errors in uploaded files: Invalid filename '…': …), which reads inconsistently when both appear in the same UI.

@tombch tombch Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The situation described here although valid, is basically never going to happen and it'd be more confusing if the folder component sometimes validated and sometimes didn't

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree we should not check the folder names, I think it would also be consistent to only check the defined fileNames that are sent to the backend and not the "true" fileNames - but I also think this is an uncommon situation and is more complicated as this comes from parsing the metadata file and not the actual file names - so we could also do that in a later PR

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

or actually, could we just remove the validation here any only do the validation on the file mapping? that seems more correct, less code and potentially less passing down of the FileSharingConfig config?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yeah could do it just on the file mapping! the only thing you'd lose is the rejecting of files with invalid names before they can even be uploaded. But I guess that's not really a huge benefit and only makes total sense for the form upload anyway?

};
22 changes: 17 additions & 5 deletions website/src/components/Submission/FileUpload/fileMapping.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const RAW_READS_COLUMN = `${FILES_HEADER_PREFIX}${RAW_READS}`;
const OTHER_FILES = 'otherFiles';
const OTHER_FILES_COLUMN = `${FILES_HEADER_PREFIX}${OTHER_FILES}`;
const FILE_CATEGORIES = [RAW_READS, OTHER_FILES];
const FILE_SHARING_CONFIG = { disableStrictFilenameValidation: false };

const tsv = (rows: string[][]) => rows.map((row) => row.join('\t')).join('\n');
const declaredFile = (name: string, path: string = name) => ({ type: 'declaredFile' as const, name, path });
Expand All @@ -44,12 +45,12 @@ const errorMessageOf = <T>(result: Result<T, Error>): string => {
};

const entriesOf = (text: string, submissionId: string, category: string) => {
const mapping = valueOf(parseSubmissionFileMapping(text, FILE_CATEGORIES));
const mapping = valueOf(parseSubmissionFileMapping(text, FILE_CATEGORIES, FILE_SHARING_CONFIG));
return [...(mapping.get(submissionId)?.get(category)?.values() ?? [])];
};

const errorOf = (text: string, categories: string[] = FILE_CATEGORIES): string => {
const result = parseSubmissionFileMapping(text, categories);
const result = parseSubmissionFileMapping(text, categories, FILE_SHARING_CONFIG);
if (result.isOk()) throw new Error('expected the parse to fail');
return result.error.message;
};
Expand Down Expand Up @@ -138,9 +139,12 @@ describe('parseSubmissionFileMapping', () => {
['e1', 'CH'],
]),
FILE_CATEGORIES,
FILE_SHARING_CONFIG,
);
expect(valueOf(result)).toEqual(new Map());
expect(valueOf(parseSubmissionFileMapping(tsv([['country'], ['CH']]), FILE_CATEGORIES))).toEqual(new Map());
expect(
valueOf(parseSubmissionFileMapping(tsv([['country'], ['CH']]), FILE_CATEGORIES, FILE_SHARING_CONFIG)),
).toEqual(new Map());
});

it('rejects a file column without an id column', () => {
Expand Down Expand Up @@ -188,6 +192,14 @@ describe('parseSubmissionFileMapping', () => {
expect(errorOf(text)).toContain(`Found duplicate file names for entry e1 in the ${RAW_READS} category: a.txt`);
});

it.each(['CON.txt', 'CON.txt::sub/reads.fastq', 'CON.txt:fileId'])('rejects invalid file name %s', (entry) => {
const text = tsv([
['id', RAW_READS_COLUMN],
['e1', entry],
]);
expect(errorOf(text)).toContain('may not use Windows reserved device names');
});

it('allows two different names to share the same explicit path', () => {
const text = tsv([
['id', RAW_READS_COLUMN],
Expand All @@ -201,7 +213,7 @@ describe('parseSubmissionFileMapping', () => {
['id', RAW_READS_COLUMN, OTHER_FILES_COLUMN],
['e1', 'a.txt', ''],
]);
const result = valueOf(parseSubmissionFileMapping(text, FILE_CATEGORIES));
const result = valueOf(parseSubmissionFileMapping(text, FILE_CATEGORIES, FILE_SHARING_CONFIG));
expect([...result.get('e1')!.keys()]).toEqual([RAW_READS]);
});

Expand All @@ -211,7 +223,7 @@ describe('parseSubmissionFileMapping', () => {
['e1', 'CH', 'a.txt', 'a.json'],
['e2', 'DE', 'b.txt', 'b.json'],
]);
const result = valueOf(parseSubmissionFileMapping(text, FILE_CATEGORIES));
const result = valueOf(parseSubmissionFileMapping(text, FILE_CATEGORIES, FILE_SHARING_CONFIG));
expect([...result.keys()]).toEqual(['e1', 'e2']);
expect(entriesOf(text, 'e1', OTHER_FILES).map((f) => f.name)).toEqual(['a.json']);
expect(entriesOf(text, 'e2', RAW_READS).map((f) => f.name)).toEqual(['b.txt']);
Expand Down
20 changes: 20 additions & 0 deletions website/src/components/Submission/FileUpload/fileMapping.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import { type Result, ok, err } from 'neverthrow';
import Papa from 'papaparse';

import { validateFileNames, getFileNameErrorMessage } from './fileNameValidation';
import { FILES_HEADER_PREFIX, SUBMISSION_ID_INPUT_FIELD } from '../../../settings';
import type { FileSharingConfig } from '../../../types/config';

const ID_COLUMNS = [SUBMISSION_ID_INPUT_FIELD, 'submissionId'];

Expand Down Expand Up @@ -378,6 +380,7 @@ const parseMetadataText = (text: string): Result<ParsedMetadata, Error> => {
export function parseSubmissionFileMapping(
text: string,
categories: FileCategory[],
fileSharingConfig: FileSharingConfig,
): Result<SubmissionFileMapping, Error> {
const parsedMetadataResult = parseMetadataText(text);
if (parsedMetadataResult.isErr()) return err(parsedMetadataResult.error);
Expand Down Expand Up @@ -456,6 +459,23 @@ export function parseSubmissionFileMapping(
submissionFileMapping.set(submissionId, categoryMapping);
}

// Validate file names
const fileNames = [
...new Set(
[...submissionFileMapping.values()]
.flatMap((categoryMapping) => [...categoryMapping.values()])
.flatMap((fileEntries) => [...fileEntries.keys()]),
),
];
const fileNameValidationResult = validateFileNames(fileNames, fileSharingConfig);
if (fileNameValidationResult.isErr())
return err(
new Error(
'Encountered errors in file names within metadata: ' +
getFileNameErrorMessage(fileNameValidationResult.error),
),
);

return ok(submissionFileMapping);
}

Expand Down
Loading
Loading