Skip to content

feat(website): Mirror backend filename validation in website - #7218

Open
tombch wants to merge 14 commits into
filename-validationfrom
website-filename-validation
Open

feat(website): Mirror backend filename validation in website#7218
tombch wants to merge 14 commits into
filename-validationfrom
website-filename-validation

Conversation

@tombch

@tombch tombch commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

resolves #

Summary

  • Mirrors the backend filename restrictions updated in feat!(backend): Stricter backend filename validation #7205 in the website, adding validation for filenames of files that are uploaded (in FolderUploadComponent), as well as filenames declared in the metadata TSV (via parseSubmissionFileMapping called in FormOrUploadWrapper).
  • Unlike the backend validation which returns errors one at a time for file names, the website validates all provided file names, and displays the first five errors in a toast.

Screenshot

Files with invalid names are rejected before upload:

image

Invalid file names declared in the metadata also raise a toast that blocks submission:

image

PR Checklist

  • All necessary documentation has been adapted.
  • The implemented feature is covered by appropriate, automated tests.
  • Any manual testing that has been done is documented (i.e. what exactly was tested?)

🚀 Preview: Add preview label to enable

@tombch tombch changed the title feat(website): Mirrored backend filename validation in website feat(website): Mirror backend filename validation in website Sep 2, 2026
@claude claude Bot added website Tasks related to the web application deployment Code changes targetting the deployment infrastructure labels Sep 2, 2026
@claude

claude Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

This PR may be related to: #7056 (Allowed characters ;, % and # in file names have encoding issues), which also concerns filename validation between the website and backend.

@tombch
tombch marked this pull request as ready for review September 2, 2026 14:09
@claude

claude Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @tombch's task in 7m 13s —— View job


Code review

Reviewed against origin/filename-validation. I checked the new website rules line-by-line against FileMappingPreconditionValidator.ktthe logic mirrors the backend faithfully: same check order, same anchoring semantics (Kotlin Regex.matches^…$, and JS $ has no trailing-newline quirk), same substringBefore('.')split('.')[0] behaviour (so .CON is accepted by both), same 0–31 control-character range, same bytes-vs-characters branch in the length message, and strict validation correctly short-circuits before the base checks exactly as the backend does. getFileSharingConfig()'s default and the backend's FileSharing() default both land on strict-on. All call sites of parseSubmissionFileMapping were updated. The cross-references added to the Kotlin KDoc are a good touch.

Findings are in inline comments; nothing blocking. The two most substantive:

  • Bulk mode is now stricter than the backend. Uploaded basenames get the full ruleset, but in bulk mode the name the backend sees is the metadata name, not the basename — reads.fastq::文件.fastq is a valid submission that the website will now refuse at upload time. (comment)
  • Error messages don't match the backend's, despite this file existing to mirror it — "cannot" vs "may not" throughout, and fileMapping.ts:308 already uses the backend spelling. Users get different text depending on which layer catches the problem. Fix this →

Smaller items, all inline: message-truncation punctuation and a per-file TextEncoder allocation (fix →), a duplicated default in config.ts, and Helm indentation that diverges from the sibling backend block (fix →).

Testing. fileNameValidation.spec.ts is thorough and tracks the Kotlin test cases closely. Two gaps: the FolderUploadComponent tests only run with strict validation disabled, so the default configuration is never exercised through the component; and the rewritten assertions there weakened to stringContaining('File'), which matches the substring inside Filenames and would pass for almost any message. (fix →)

Documentation. Neither fileSharing.disableStrictFilenameValidation nor outputFileUrlType is documented under docs/, so this PR is consistent with #7205 — but the PR checklist's documentation and manual-testing boxes are still unchecked.

No security or performance concerns. This is defence-in-depth in front of validation the backend still enforces, so bypassing the client-side check gains nothing. Validation is O(n) over filenames with no unbounded regex backtracking.

Verification note: I could not run CI=1 npm run test / npm run check-types (no node_modules in this checkout) or helm template (blocked by the sandbox), so the review is by inspection. My reading of the Helm change is that nindent 6 still renders valid YAML — the point there is consistency, not breakage.


  • Gather context (PR diff, backend validator, Helm templates, config plumbing)
  • Review new validation logic against the backend
  • Review config/wiring changes
  • Review tests
  • Post review (7 inline comments)
    • branch website-filename-validation

Comment thread website/src/components/Submission/FileUpload/fileNameValidation.ts Outdated
Comment thread website/src/components/Submission/FileUpload/fileNameValidation.ts
Comment thread website/src/components/Submission/FileUpload/fileNameValidation.ts
Comment on lines 540 to +548
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);
}

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?

Comment thread website/src/config.ts
Comment thread kubernetes/loculus/templates/_common-metadata.tpl Outdated
Comment thread website/src/components/Submission/FileUpload/FolderUploadComponent.spec.tsx Outdated
@tombch
tombch force-pushed the website-filename-validation branch from 18f74fc to cd5e8cb Compare September 2, 2026 14:55
* - 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!

});
export type SequenceFlaggingConfig = z.infer<typeof sequenceFlaggingConfig>;

export const fileSharingConfig = z.object({ disableStrictFilenameValidation: z.boolean().default(false) });

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.

the config can also have the field outputFileUrlType - will zod have any issues if this field is in the input?

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.

No zod will just ignore it as it's not declared in the fileSharingConfig. We could have it just pass the disableStrictFilenameValidation flag only in the website, but I thought to do fileSharing as I think it looks nicer and more extensible if we wanted other props in the future?

Comment thread website/src/components/Submission/SubmissionForm.spec.tsx
Comment thread website/src/components/Submission/FormOrUploadWrapper.spec.tsx
Comment thread website/src/components/Edit/EditPage.spec.tsx
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

deployment Code changes targetting the deployment infrastructure website Tasks related to the web application

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants