From 76dae30b5e1d2c006ad5892e054b243e2f906846 Mon Sep 17 00:00:00 2001 From: mboudet Date: Thu, 29 Oct 2026 17:39:59 +0100 Subject: [PATCH 01/12] Actually build --- .github/workflows/build.yml | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index e69de29..9474668 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -0,0 +1,37 @@ +name: Build and Push image + +on: + push: + branches: [ "master" ] + +permissions: + contents: read + packages: write + +jobs: + build-and-push: + runs-on: ubuntu-latest + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Log in to the Container registry + uses: docker/login-action@v3 + with: + registry: ghcr.io + username: ${{ github.actor }} + password: ${{ secrets.GITHUB_TOKEN }} + + - name: Extract metadata (tags, labels) + uses: docker/metadata-action@v5 + id: meta + with: + images: ghcr.io/${{ github.repository }} + + - name: Build and push Docker image + uses: docker/build-push-action@v5 + with: + context: . + push: true + tags: ${{ steps.meta.outputs.tags }} + labels: ${{ steps.meta.outputs.labels }} From 9fa47229046e6a54b6be29a9f05f3a5edcd4d4cd Mon Sep 17 00:00:00 2001 From: mboudet Date: Thu, 29 Oct 2026 17:41:13 +0100 Subject: [PATCH 02/12] Trigger? --- Dockerfile | 1 + 1 file changed, 1 insertion(+) diff --git a/Dockerfile b/Dockerfile index 98e0954..6746386 100644 --- a/Dockerfile +++ b/Dockerfile @@ -33,4 +33,5 @@ EXPOSE 8000 COPY entrypoint.sh /entrypoint.sh RUN chmod +x /entrypoint.sh + ENTRYPOINT ["/entrypoint.sh"] From ff5ac69bae90f4ff3160b988d3c1bb9acd20fd4f Mon Sep 17 00:00:00 2001 From: mboudet Date: Thu, 29 Oct 2026 17:42:35 +0100 Subject: [PATCH 03/12] Pff --- .github/workflows/build.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 9474668..cf4d634 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -2,7 +2,7 @@ name: Build and Push image on: push: - branches: [ "master" ] + branches: [ "main" ] permissions: contents: read From 42b0d04bb1a7d71929a19e27e9c4037e4c9b6898 Mon Sep 17 00:00:00 2001 From: mboudet Date: Thu, 29 Oct 2026 18:13:03 +0100 Subject: [PATCH 04/12] Update main.py --- app/main.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main.py b/app/main.py index fc7d76f..6b072aa 100644 --- a/app/main.py +++ b/app/main.py @@ -44,12 +44,12 @@ # mount if the directory actually exists (e.g. a mounted volume). if settings.static_override_dir and Path(settings.static_override_dir).is_dir(): app.mount( - f"{settings.base_url_path}/static/custom", + "/static/custom", StaticFiles(directory=settings.static_override_dir), name="custom-static", ) app.mount( - f"{settings.base_url_path}/static", + "/static", StaticFiles(directory=str(APP_DIR / "static")), name="static", ) From ef0242d853a009fac765a556d17dd5e74514daae Mon Sep 17 00:00:00 2001 From: mboudet Date: Fri, 30 Oct 2026 16:20:37 +0100 Subject: [PATCH 05/12] Increase security --- .env.example | 5 + app/command.py | 147 ++++++++++++++---------- app/config.py | 7 +- app/main.py | 127 +++++++++++++++++++-- app/models.py | 8 +- app/scheduler.py | 26 +++-- app/schemas.py | 74 ++++++++++-- bin/blast_links.py | 15 ++- requirements.txt | 1 + tests/conftest.py | 7 ++ tests/test_security.py | 250 +++++++++++++++++++++++++++++++++++++++++ 11 files changed, 566 insertions(+), 101 deletions(-) create mode 100644 tests/test_security.py diff --git a/.env.example b/.env.example index 1154663..e70a929 100644 --- a/.env.example +++ b/.env.example @@ -1,7 +1,12 @@ # --- App --- APP_TITLE=Blast server BASE_URL_PATH= +# Max size (MiB) of each uploaded file / the pasted sequence. UPLOAD_LIMIT=100 +# Max job submissions per client IP per minute (0 disables). Behind a reverse +# proxy also run uvicorn with --proxy-headers --forwarded-allow-ips= +# and set a body-size limit (client_max_body_size) on the proxy. +SUBMIT_RATE_LIMIT=10 HISTORY_LENGTH=8 # --- Template / static overrides --- diff --git a/app/command.py b/app/command.py index 5109692..b33b8e3 100644 --- a/app/command.py +++ b/app/command.py @@ -1,7 +1,16 @@ """Build the blast shell command from a BlastRequest. Mirrors the legacy ``command.txt.twig`` template. + +Security note: the returned script is executed by a shell on the compute +nodes. Every value that originates from a request (or from a path derived from +one) is therefore passed through :func:`shlex.quote`, in addition to being +validated against allow-lists in :mod:`app.schemas`. ``pre_command`` and +``link_command`` come from the administrator's configuration and are inserted +as-is on purpose (they are shell snippets). """ +import shlex + from .schemas import BlastRequest @@ -17,65 +26,74 @@ def build_blast_command( ) -> str: """Return the full shell script that runs a blast job in ``work_dir``.""" + q = shlex.quote + wd = work_dir if work_dir.endswith("/") else work_dir + "/" + + def path(name: str) -> str: + """Shell-quoted absolute path of a file inside the job directory.""" + return q(f"{wd}{name}") + # ---- db path ---------------------------------------------------------- if request.is_perso_db(): - db_path = f"{work_dir}uploadedDB" + db_path = f"{wd}uploadedDB" dbtype = "prot" if request.program in ("blastp", "blastx") else "nucl" - make_db = ( - f"makeblastdb -in {work_dir}uploadedDB.fasta -dbtype {dbtype} " - f"-out {work_dir}uploadedDB -title uploadedDB;" - ) + make_db = shlex.join([ + "makeblastdb", + "-in", f"{wd}uploadedDB.fasta", + "-dbtype", dbtype, + "-out", f"{wd}uploadedDB", + "-title", "uploadedDB", + ]) + ";" else: db_path = request.dbPath make_db = "" - # ---- blast command ---------------------------------------------------- - parts = [] + # ---- blast command (built as an argv list, never as a raw string) ----- + argv: list[str] = [] if request.program == "blastp" and request.blastpType in ("psiblast", "phiblast"): - parts.append("psiblast") + argv.append("psiblast") elif request.program == "blastp" and request.blastpType == "deltablast": - parts.append("deltablast") + argv.append("deltablast") else: - bin = request.program + argv.append(request.program) if request.program == "blastn": - bin += f" -task {request.blastnType}" + argv += ["-task", request.blastnType] elif request.program == "blastp": - bin += f" -task {request.blastpType}" - parts.append(bin) + argv += ["-task", request.blastpType] if request.fileSeq or request.pastedSeq: - parts.append(f"-query {work_dir}input.fasta") + argv += ["-query", f"{wd}input.fasta"] - parts.append(f"-db {db_path}") - parts.append(f"-evalue {request.expect}") - parts.append(f"-max_target_seqs {request.maxTargetSequences}") - parts.append(f"-soft_masking {'true' if request.softMasking else 'false'}") + argv += ["-db", db_path] + argv += ["-evalue", request.expect] + argv += ["-max_target_seqs", request.maxTargetSequences] + argv += ["-soft_masking", "true" if request.softMasking else "false"] if request.lowerCase: - parts.append("-lcase_masking") + argv.append("-lcase_masking") if request.program == "blastn" and request.blastnType == "megablast": - parts.append(f"-word_size {request.wordSizesMegablast}") + argv += ["-word_size", request.wordSizesMegablast] elif request.program == "blastn" and request.blastnType == "dc-megablast": - parts.append(f"-word_size {request.wordSizesDcMegablast}") + argv += ["-word_size", request.wordSizesDcMegablast] elif request.program == "blastn": - parts.append(f"-word_size {request.wordSizesBlastn}") + argv += ["-word_size", request.wordSizesBlastn] else: - parts.append(f"-word_size {request.wordSizesProt}") + argv += ["-word_size", request.wordSizesProt] if ( request.program == "blastn" and request.blastnType == "dc-megablast" and int(request.templateLengths) != 0 ): - parts.append(f"-template_length {request.templateLengths}") - parts.append(f"-template_type {request.templateTypes}") + argv += ["-template_length", request.templateLengths] + argv += ["-template_type", request.templateTypes] if request.program in ("tblastx", "blastx"): - parts.append(f"-query_gencode {request.queryCode}") + argv += ["-query_gencode", request.queryCode] if request.program in ("tblastx", "tblastn"): - parts.append(f"-db_gencode {request.dbCode}") + argv += ["-db_gencode", request.dbCode] if request.program == "blastn": matrix = ( @@ -84,66 +102,65 @@ def build_blast_command( else request.matricesNuc ) reward, penalty = matrix.split(",") - parts.append(f"-reward {reward}") - parts.append(f"-penalty {penalty}") + argv += ["-reward", reward, "-penalty", penalty] else: - parts.append(f"-matrix {request.matricesProt}") + argv += ["-matrix", request.matricesProt] if request.program != "tblastx": if request.program == "blastn": gap_open, gap_ext = request.gapCostsBlastn.split(",") else: gap_open, gap_ext = request.gapCostsProt.split(",") - parts.append(f"-gapopen {gap_open}") - parts.append(f"-gapextend {gap_ext}") + argv += ["-gapopen", gap_open, "-gapextend", gap_ext] if (request.program == "blastp" and request.blastpType != "deltablast") or ( request.program == "tblastn" ): - parts.append(f"-comp_based_stats {request.compositionalAdjustments}") + argv += ["-comp_based_stats", request.compositionalAdjustments] elif request.program == "blastp" and request.blastpType == "deltablast": - parts.append(f"-comp_based_stats {request.compositionalAdjustmentsDelta}") + argv += ["-comp_based_stats", request.compositionalAdjustmentsDelta] if request.program == "blastn": - parts.append(f"-dust {'yes' if request.lowComplex else 'no'}") + argv += ["-dust", "yes" if request.lowComplex else "no"] else: - parts.append(f"-seg {'yes' if request.lowComplex else 'no'}") + argv += ["-seg", "yes" if request.lowComplex else "no"] if request.program == "blastp" and request.blastpType in ( "psiblast", "phiblast", "deltablast", ): - parts.append(f"-inclusion_ethresh {request.psiThreshold}") - parts.append(f"-num_iterations {request.psiIterationNb}") - parts.append(f"-pseudocount {request.psiPseudoCount}") + argv += ["-inclusion_ethresh", str(request.psiThreshold)] + argv += ["-num_iterations", str(request.psiIterationNb)] + argv += ["-pseudocount", str(request.psiPseudoCount)] if request.blastpType in ("psiblast", "deltablast"): - parts.append(f"-out_pssm {work_dir}{job_uid}.pssm") - parts.append(f"-out_ascii_pssm {work_dir}{job_uid}.pssm.ascii") + argv += ["-out_pssm", f"{wd}{job_uid}.pssm"] + argv += ["-out_ascii_pssm", f"{wd}{job_uid}.pssm.ascii"] if request.blastpType == "psiblast" and request.psiPSSM: - parts.append(f"-in_pssm {work_dir}input.pssm") + argv += ["-in_pssm", f"{wd}input.pssm"] if request.blastpType == "phiblast" and request.phiPattern: - parts.append(f"-phi_pattern {work_dir}input.pattern") + argv += ["-phi_pattern", f"{wd}input.pattern"] if request.blastpType == "deltablast": - parts.append(f"-rpsdb {cdd_delta}") + argv += ["-rpsdb", cdd_delta] - parts.append("-num_threads 4") + argv += ["-num_threads", "4"] if request.program == "blastp" and request.blastpType == "phiblast": - parts.append(f"-outfmt 0 -html -out {work_dir}{job_uid}.html;") + argv += ["-outfmt", "0", "-html", "-out", f"{wd}{job_uid}.html"] else: - parts.append(f"-outfmt 11 -out {work_dir}{job_uid}.asn;") + argv += ["-outfmt", "11", "-out", f"{wd}{job_uid}.asn"] - blast_command = " ".join(parts) + blast_command = shlex.join(argv) + ";" # ---- script ----------------------------------------------------------- lines = [] - lines.append(f"cd {work_dir}") + lines.append(f"cd {q(wd)}") lines.append("export PATH=./bin/:$PATH;") if pre_command: lines.append(pre_command) lines.append("") - lines.append(f'echo "{blast_command}" > {work_dir}blast_command.txt;') + # printf with a single-quoted argument: no expansion of the content. + lines.append(f"printf '%s\\n' {q(blast_command)} > {path('blast_command.txt')};") lines.append("") if make_db: lines.append(make_db) @@ -152,26 +169,32 @@ def build_blast_command( lines.append("") if request.program != "blastp" or request.blastpType != "phiblast": + asn, xml = path(f"{job_uid}.asn"), path(f"{job_uid}.xml") lines += [ - f"blast_formatter -archive {work_dir}{job_uid}.asn -outfmt 5 -out {work_dir}{job_uid}.xml;", - f"blast_formatter -archive {work_dir}{job_uid}.asn -outfmt 0 -html -out {work_dir}{job_uid}.html;", - f"blast_formatter -archive {work_dir}{job_uid}.asn -outfmt 0 -out {work_dir}{job_uid}.txt;", - f"blast_formatter -archive {work_dir}{job_uid}.asn -outfmt 6 -out {work_dir}{job_uid}.tsv;", - f"blast_formatter -archive {work_dir}{job_uid}.asn -outfmt 10 -out {work_dir}{job_uid}.csv;", - f"xml2gff3.py {work_dir}{job_uid}.xml > {work_dir}{job_uid}.gff3;", + f"blast_formatter -archive {asn} -outfmt 5 -out {xml};", + f"blast_formatter -archive {asn} -outfmt 0 -html -out {path(job_uid + '.html')};", + f"blast_formatter -archive {asn} -outfmt 0 -out {path(job_uid + '.txt')};", + f"blast_formatter -archive {asn} -outfmt 6 -out {path(job_uid + '.tsv')};", + f"blast_formatter -archive {asn} -outfmt 10 -out {path(job_uid + '.csv')};", + f"xml2gff3.py {xml} > {path(job_uid + '.gff3')};", ] if link_command: gff3_url = f"{result_url}{job_uid}.gff3" + html, linked, raw = ( + path(f"{job_uid}.html"), + path(f"{job_uid}.htmllinked"), + path(f"{job_uid}.htmlraw"), + ) lines.append("") - lines.append(f"export GFF3_URL={gff3_url}") - cmd = link_command.replace("{config}", link_config) if link_config else link_command - lines.append(f"{cmd} {work_dir}{job_uid}.html > {work_dir}{job_uid}.htmllinked;") + lines.append(f"export GFF3_URL={q(gff3_url)}") + cmd = link_command.replace("{config}", q(link_config)) if link_config else link_command + lines.append(f"{cmd} {html} > {linked};") lines += [ - f"if [ -s {work_dir}{job_uid}.htmllinked ]", + f"if [ -s {linked} ]", "then", - f" mv {work_dir}{job_uid}.html {work_dir}{job_uid}.htmlraw;", - f" mv {work_dir}{job_uid}.htmllinked {work_dir}{job_uid}.html;", + f" mv {html} {raw};", + f" mv {linked} {html};", "fi", ] diff --git a/app/config.py b/app/config.py index ada5115..dece0a6 100644 --- a/app/config.py +++ b/app/config.py @@ -23,8 +23,9 @@ class Settings(BaseSettings): # --- Web / app --------------------------------------------------------- app_title: str = "Blast server (v2.2.29+)" base_url_path: str = "" - secret_key: str = "change-me" - upload_limit: int = 100 # MiB + upload_limit: int = 100 # MiB, per uploaded file / pasted sequence + # Max job submissions per client IP per minute (0 = disabled). + submit_rate_limit: int = 10 # Optional directory mounted into the container that is searched *before* # the built-in templates. Drop a custom ``layout.html`` (and any other @@ -76,7 +77,7 @@ class Settings(BaseSettings): cdd_delta_path: str = "" pre_command: str = "" link_command: str = ( - "python3 ./bin/blast_links.py" + "python3 ./bin/blast_links.py " "--config ./bin/links.yml --gff-url $GFF3_URL" ) diff --git a/app/main.py b/app/main.py index 6b072aa..a471dc1 100644 --- a/app/main.py +++ b/app/main.py @@ -4,10 +4,13 @@ keeping the blast web parameters and the ``banks.yml`` / ``links.yml`` configuration files. """ +import logging import os import re +import shutil from pathlib import Path +import nh3 from fastapi import Depends, FastAPI, File, Form, Request, UploadFile from fastapi.responses import FileResponse, HTMLResponse, JSONResponse, RedirectResponse from fastapi.staticfiles import StaticFiles @@ -27,13 +30,43 @@ APP_DIR = Path(__file__).resolve().parent +logger = logging.getLogger(__name__) + settings = get_settings() +MAX_UPLOAD_BYTES = settings.upload_limit * 1024 * 1024 +# A submission can carry up to 5 payloads (pasted sequence + 4 files). +MAX_BODY_BYTES = MAX_UPLOAD_BYTES * 5 + 1024 * 1024 + app = FastAPI(title=settings.app_title) +_SECURITY_HEADERS = { + "X-Content-Type-Options": "nosniff", + "X-Frame-Options": "DENY", + "Referrer-Policy": "same-origin", + # Deliberately light so custom templates / stylesheets keep working. + "Content-Security-Policy": "object-src 'none'; base-uri 'self'; frame-ancestors 'none'", +} + + +@app.middleware("http") +async def security_middleware(request: Request, call_next): + # Cheap early rejection of oversized submissions. This only sees the + # declared Content-Length: also set a body size limit on the reverse proxy. + length = request.headers.get("content-length") + if request.method in ("POST", "PUT", "PATCH") and length and length.isdigit(): + if int(length) > MAX_BODY_BYTES: + return HTMLResponse("Request too large", status_code=413) + response = await call_next(request) + for name, value in _SECURITY_HEADERS.items(): + response.headers.setdefault(name, value) # route-specific headers win + return response + # Template lookup: an optional override directory (e.g. mounted from a volume) # is searched *before* the built-in templates, so a custom ``layout.html`` (and # any other template) can be provided without rebuilding the image. + + templates_dirs = [str(APP_DIR / "templates")] if settings.templates_override_dir and Path(settings.templates_override_dir).is_dir(): templates_dirs.insert(0, settings.templates_override_dir) @@ -68,6 +101,10 @@ } +class UploadTooLarge(Exception): + """An uploaded file exceeds ``settings.upload_limit``.""" + + def _static_url(request: Request, path: str) -> str: return f"{settings.base_url_path}/static/{path}" @@ -116,7 +153,7 @@ def _request_from_form(form_fields: dict, file_paths: dict) -> BlastRequest: "templateTypes": form_fields.get("templateTypes") or "coding", "templateLengths": form_fields.get("templateLengths") or "18", "compositionalAdjustments": form_fields.get("compositionalAdjustments") or "2", - "compositionalAdjustmentsDelta": form_fields.get("compositionalAdjustmentsDelta") or "2", + "compositionalAdjustmentsDelta": form_fields.get("compositionalAdjustmentsDelta") or "1", "lowComplex": _bool_from_form(form_fields.get("lowComplex")), "softMasking": _bool_from_form(form_fields.get("softMasking")), "lowerCase": _bool_from_form(form_fields.get("lowerCase")), @@ -146,6 +183,33 @@ def _validation_errors(exc: ValidationError) -> dict: return errors +# BLAST's HTML report embeds attacker-influenced text (query header, database +# definition lines), so it must never reach the page as raw HTML. +_PREVIEW_TAGS = { + "a", "b", "i", "u", "em", "strong", "pre", "code", "span", "div", "p", "br", + "hr", "font", "sub", "sup", "table", "thead", "tbody", "tr", "td", "th", + "h1", "h2", "h3", "h4", "h5", "h6", "ul", "ol", "li", +} +_PREVIEW_ATTRS = { + "a": {"href", "name", "id", "title"}, + "font": {"color", "size"}, + "span": {"class", "id"}, + "div": {"class", "id"}, + "pre": {"class", "id"}, + "td": {"colspan", "rowspan", "align"}, + "th": {"colspan", "rowspan", "align"}, +} + + +def _sanitize_preview(html: str) -> str: + return nh3.clean( + html, + tags=_PREVIEW_TAGS, + attributes=_PREVIEW_ATTRS, + url_schemes={"http", "https", "mailto"}, + ) + + def _render(request: Request, name: str, ctx: dict, status_code: int = 200): return templates.TemplateResponse(request, name, ctx, status_code=status_code) @@ -223,7 +287,7 @@ async def submit( templateTypes: str = Form("coding"), templateLengths: str = Form("18"), compositionalAdjustments: str = Form("2"), - compositionalAdjustmentsDelta: str = Form("2"), + compositionalAdjustmentsDelta: str = Form("1"), lowComplex: str = Form("on"), softMasking: str = Form("on"), lowerCase: str = Form(""), @@ -255,6 +319,12 @@ async def submit( "psiIterationNb": psiIterationNb, "psiPseudoCount": psiPseudoCount, } + if len(pastedSeq.encode("utf-8", "ignore")) > MAX_UPLOAD_BYTES: + return _render_index( + request, dict(DEFAULT_VALUES, **{**form_fields, "pastedSeq": ""}), + {"pastedSeq": f"The pasted sequence is too large (limit: {settings.upload_limit} MiB)"}, + ) + # Create the job and its working directory first. scheduler = get_scheduler() job = Job() @@ -264,12 +334,21 @@ async def submit( job.email = email work_dir = scheduler.get_work_dir(job) + def discard() -> None: + shutil.rmtree(work_dir, ignore_errors=True) + def save_upload(upload: UploadFile | None, filename: str) -> str: if not upload or not upload.filename: return "" dest = os.path.join(work_dir, filename) + written = 0 with open(dest, "wb") as fh: - fh.write(upload.file.read()) + # Chunked copy with a hard cap instead of read() into memory. + while chunk := upload.file.read(1024 * 1024): + written += len(chunk) + if written > MAX_UPLOAD_BYTES: + raise UploadTooLarge(upload.filename) + fh.write(chunk) return filename # Save uploaded files. @@ -284,16 +363,26 @@ def save_upload(upload: UploadFile | None, filename: str) -> str: file_paths["fileSeq"] = uploaded_file_seq file_paths["psiPSSM"] = save_upload(psiPSSM, "input.pssm") file_paths["phiPattern"] = save_upload(phiPattern, "input.pattern") - except OSError as exc: - return _render_index(request, dict(DEFAULT_VALUES, **form_fields), {"pastedSeq": f"Could not create file: {exc}"}) + except UploadTooLarge: + discard() + return _render_index( + request, dict(DEFAULT_VALUES, **form_fields), + {"pastedSeq": f"An uploaded file is too large (limit: {settings.upload_limit} MiB)"}, + ) + except OSError: + logger.exception("Could not save uploaded files for job %s", job.job_uid) + discard() + return _render_index(request, dict(DEFAULT_VALUES, **form_fields), {"pastedSeq": "Could not store the uploaded files"}) # Build and validate the request. try: brequest = _request_from_form(form_fields, file_paths) except ValidationError as exc: + discard() return _render_index(request, dict(DEFAULT_VALUES, **form_fields), _validation_errors(exc)) if not brequest.is_perso_db() and brequest.dbPath and not db_provider.is_valid_path(brequest.dbPath): + discard() return _render_index(request, dict(DEFAULT_VALUES, **form_fields), {"bankTypeNuc": "The value you selected is not a valid choice"}) # Build the command script. @@ -344,8 +433,12 @@ def save_upload(upload: UploadFile | None, filename: str) -> str: # Launch the job. try: job = scheduler.execute(job) - except Exception as exc: # noqa: BLE001 - show a friendly error page - return _render_error(request, job.job_uid, f"Submission failed: {exc}", job) + except Exception: # noqa: BLE001 - show a friendly error page + # Log the details server side; never echo scheduler/internal errors + # (URLs, tokens in messages, stack details) back to the client. + logger.exception("Submission of job %s failed", job.job_uid) + discard() + return _render_error(request, job.job_uid, "Submission failed. Please try again later.", job) db.add(job) db.commit() @@ -460,6 +553,7 @@ def job_results(request: Request, uid: str, db: Session = Depends(get_db)): html_file = os.path.join(work_dir, file.fs_name) preview_results, crashed, truncated = _extract_preview(html_file) + preview_results = _sanitize_preview(preview_results) return _render(request, "results.html", { "request": request, @@ -475,17 +569,32 @@ def job_results(request: Request, uid: str, db: Session = Depends(get_db)): }) +# Result files are served from the application's own origin. They contain +# text derived from user input (BLAST HTML/XML reports), so they are sandboxed +# (no script execution, opaque origin) and may not be MIME-sniffed. +_RESULT_FILE_HEADERS = { + "Content-Security-Policy": ( + "sandbox; default-src 'none'; style-src 'unsafe-inline'; img-src data:" + ), + "X-Content-Type-Options": "nosniff", +} + + @app.get("/tmp/{program}/{uid}/{filename}") def result_file(program: str, uid: str, filename: str, db: Session = Depends(get_db)): job = db.get(Job, uid) if not job or job.program_name != program: return HTMLResponse("Job not found", status_code=404) + # Only the files declared for this job are downloadable (not the job + # script, stdout/stderr, mail body, uploaded databank, ...). + if filename not in {f.fs_name for f in job.result_files}: + return HTMLResponse("File not found", status_code=404) scheduler = get_scheduler() base = Path(scheduler.get_work_dir(job)).resolve() target = (base / filename).resolve() - if not str(target).startswith(str(base)) or not target.is_file(): + if not target.is_relative_to(base) or not target.is_file(): return HTMLResponse("File not found", status_code=404) - return FileResponse(str(target)) + return FileResponse(str(target), headers=_RESULT_FILE_HEADERS) def _render_error(request: Request, uid: str, message: str, job=None): diff --git a/app/models.py b/app/models.py index 1d06652..b61f65d 100644 --- a/app/models.py +++ b/app/models.py @@ -53,7 +53,7 @@ def has_valid_email(self) -> bool: @property def mail_subject(self) -> str: - title = self.title or "" + title = " ".join((self.title or "").split()) if title: title = f"'{title}' ({self.job_uid})" else: @@ -61,10 +61,12 @@ def mail_subject(self) -> str: return f"{self.program_name}: completion of job {title}" def generate_job_uid(self) -> str: - import random import re + import secrets - uid = f"{self.program_name}-{random.random()}" + # The uid is the only secret protecting a job's results and files, so + # it must be unguessable: 128 bits from the OS CSPRNG (not ``random``). + uid = f"{self.program_name}-{secrets.token_hex(16)}" uid = re.sub(r"\W", "-", uid) self.job_uid = uid.lower() return self.job_uid diff --git a/app/scheduler.py b/app/scheduler.py index 0a28a76..c722e1c 100644 --- a/app/scheduler.py +++ b/app/scheduler.py @@ -8,7 +8,9 @@ Both share the same job script / working-directory conventions as the legacy application. """ +import logging import os +import shlex import shutil from datetime import datetime from pathlib import Path @@ -16,6 +18,8 @@ from .config import get_settings from .models import Job +logger = logging.getLogger(__name__) + class Scheduler: """Base class defining the scheduler interface.""" @@ -35,7 +39,9 @@ def is_finished(self, job: Job, db=None) -> bool: def get_work_dir(self, job: Job) -> str: settings = get_settings() work_dir = os.path.join(settings.jobs_work_dir, job.program_name, job.job_uid) + "/" - Path(work_dir).mkdir(parents=True, exist_ok=True) + # Not world-readable: the directory holds uploads, the submitter's + # e-mail address and the job script. + Path(work_dir).mkdir(mode=0o750, parents=True, exist_ok=True) return work_dir def get_result_url(self, job: Job) -> str: @@ -45,22 +51,26 @@ def get_result_url(self, job: Job) -> str: def build_job_script(self, job: Job, work_dir: str, lock_file: str = "") -> str: """Build the shell script that runs the job (and sends the email).""" + q = shlex.quote email_part = "" if job.has_valid_email(): settings = get_settings() body_file = os.path.join(work_dir, "mail_body.txt") + # Everything is single-quoted: no variable/command expansion, and + # "--" ends option parsing so an address can never act as a flag. email_part = ( - f'/bin/mail -s "{job.mail_subject}" ' - f'-r "{settings.admin_email}" "{job.email}" < "{body_file}"' + f"/bin/mail -s {q(job.mail_subject)} " + f"-r {q(settings.admin_email)} -- {q(job.email)} < {q(body_file)}" ) + wd = work_dir.rstrip("/") lines = [] - lines.append(f"exec 3> {work_dir}/stdout.txt") - lines.append(f"exec 4> {work_dir}/stderr.txt") + lines.append(f"exec 3> {q(wd + '/stdout.txt')}") + lines.append(f"exec 4> {q(wd + '/stderr.txt')}") lines.append("(") - lines.append(f" if [ ! -d {work_dir} ]") + lines.append(f" if [ ! -d {q(wd)} ]") lines.append(" then") - lines.append(f" mkdir -p {work_dir}") + lines.append(f" mkdir -p {q(wd)}") lines.append(" fi") for cl in job.command.splitlines(): lines.append(f" {cl}") @@ -69,7 +79,7 @@ def build_job_script(self, job: Job, work_dir: str, lock_file: str = "") -> str: if email_part: lines.append(email_part) if lock_file: - lines.append(f"rm {lock_file}") + lines.append(f"rm {q(lock_file)}") return "\n".join(lines) diff --git a/app/schemas.py b/app/schemas.py index 3d1bf7d..bc15d69 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -5,12 +5,54 @@ from . import forms -EMAIL_RE = re.compile(r"^[^@\s]+@[^@\s]+\.[^@\s]+$") +# Conservative address check: the first character must be alphanumeric (so the +# value can never be read as a command-line option) and shell metacharacters +# (quotes, $, backticks, ;, |, &, parentheses, ...) are rejected outright. +EMAIL_RE = re.compile( + r"^[A-Za-z0-9_%+][A-Za-z0-9._%+-]*@[A-Za-z0-9](?:[A-Za-z0-9.-]*[A-Za-z0-9])?\.[A-Za-z]{2,}$" +) +MAX_EMAIL_LEN = 254 +MAX_TITLE_LEN = 255 +# Control characters (including CR/LF, which would allow mail header injection). +_CTRL_RE = re.compile(r"[\x00-\x1f\x7f]") NUCLEIC_PROGRAMS = ("blastn", "tblastn") PROTEIN_PROGRAMS = ("blastp", "blastx") +def _values(choices): + """Allowed values of a ``forms`` choice list (plain strings or (value, label)).""" + return frozenset(c[0] if isinstance(c, (tuple, list)) else c for c in choices) + + +# Every field that ends up on the blast command line is checked against the +# same list that is used to render the corresponding