From 5086aa2916f2b85e2badd2a3c79a4ef4acf9bc01 Mon Sep 17 00:00:00 2001 From: Paul Lizer Date: Fri, 2 Oct 2026 18:45:01 -0400 Subject: [PATCH 1/2] Fix orchestration file rendering when /app is root-owned (V2) Since the OneNote runtime stage (c9faeae08), /app in the image is owned by root while the app runs as UID 65532 with /app as its working directory. The structured file renderer and the generated-file download stream created scratch files with tempfile.TemporaryFile(dir='.'), so every CSV, JSON, XML, YAML, TXT and Markdown render failed with PermissionError, reported as output_access_denied. - Add functions_temp_files.scratch_file_dir(): /sc-temp-files when it is writable, otherwise the platform temp directory, never the working directory. Use it in both call sites. - Dockerfile: onenote-runtime no longer creates /app. Copy the extractor binary in the final stage after the app code, so the runtime user's chowned copy creates /app. The binary stays root-owned. - Record error_type, error_cause_type and error_errno in render-attempt diagnostics, without messages or paths. - Add regression tests. Update two tests that pinned dir='.' and one source-loaded namespace. - Docs: fix document, logging tags, OneNote build layout, release notes. Bump to 0.261.218. Fixes #1623 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- application/single_app/Dockerfile | 7 +- application/single_app/config.py | 2 +- .../functions_orchestration_rendering.py | 32 +++ .../functions_simplechat_operations.py | 3 +- .../functions_structured_file_renderers.py | 5 +- .../single_app/functions_temp_files.py | 22 ++ .../explanation/features/ONENOTE_INGESTION.md | 11 +- ...RCHESTRATION_FILE_RENDER_PERMISSION_FIX.md | 145 +++++++++++++ docs/explanation/release_notes.md | 11 + docs/reference/logging-tags.md | 14 +- ...est_generated_file_structured_renderers.py | 3 +- ...t_orchestration_file_render_scratch_dir.py | 190 ++++++++++++++++++ .../test_orchestration_output_lifecycle.py | 5 +- .../test_workflow_saved_output_artifacts.py | 2 + 14 files changed, 441 insertions(+), 11 deletions(-) create mode 100644 application/single_app/functions_temp_files.py create mode 100644 docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md create mode 100644 functional_tests/test_orchestration_file_render_scratch_dir.py diff --git a/application/single_app/Dockerfile b/application/single_app/Dockerfile index 6fcdd26cd..ba5c89c99 100644 --- a/application/single_app/Dockerfile +++ b/application/single_app/Dockerfile @@ -61,7 +61,8 @@ RUN cargo build --release --locked \ FROM mcr.microsoft.com/azurelinux/distroless/python:3.12 AS onenote-runtime -COPY --from=onenote-builder /build/target/release/simplechat-onenote-extractor /app/native/onenote_extractor/bin/simplechat-onenote-extractor +# Nothing in this stage may create /app. The final stage's chowned copy from the builder +# must be what creates /app, so the non-root runtime user owns its working directory. COPY --from=onenote-builder /build/THIRD_PARTY_NOTICES.txt /usr/share/simplechat/onenote-extractor/THIRD_PARTY_NOTICES.txt COPY --from=onenote-builder /build/licenses/ /usr/share/simplechat/onenote-extractor/licenses/ COPY --from=onenote-builder /onenote-source/ /usr/share/simplechat/onenote-extractor/source/ @@ -285,6 +286,10 @@ COPY --chown=${UID}:${GID} application/single_app ./ # ever arrives from the v2uibuilder stage. COPY --from=v2uibuilder --chown=${UID}:${GID} /single_app/static/v2 ./static/v2 +# The native OneNote extractor is copied after /app already exists with runtime-user +# ownership. The binary itself stays root-owned: the app user can run it but not replace it. +COPY --from=onenote-builder /build/target/release/simplechat-onenote-extractor /app/native/onenote_extractor/bin/simplechat-onenote-extractor + # Expose port EXPOSE 5000 diff --git a/application/single_app/config.py b/application/single_app/config.py index 32111db14..4ef56809c 100644 --- a/application/single_app/config.py +++ b/application/single_app/config.py @@ -100,7 +100,7 @@ EXECUTOR_TYPE = 'thread' EXECUTOR_MAX_WORKERS = 30 SESSION_TYPE = 'filesystem' -VERSION = "0.261.217" +VERSION = "0.261.218" IS_DEVELOPMENT = is_development_env_enabled() # Opt-out for deployments where App Service Easy Auth is active but the platform diff --git a/application/single_app/functions_orchestration_rendering.py b/application/single_app/functions_orchestration_rendering.py index 9bef81cf7..05ba6abc7 100644 --- a/application/single_app/functions_orchestration_rendering.py +++ b/application/single_app/functions_orchestration_rendering.py @@ -362,10 +362,34 @@ def __init__(self, service, *, stop=None): self._last_full = None self._last_recheck = None self._due_at = None + self.error_type = None + self.error_cause_type = None + self.error_errno = None def bind(self, claim): self.claim = claim + def record_error(self, error): + """Keep the failure's class names and OS error number, never its message. + + The output code alone can hide the cause: a local file-system PermissionError and + a revoked source both classify as ``output_access_denied``. + """ + if self.error_type is not None or not isinstance(error, BaseException): + return + self.error_type = type(error).__name__ + chain, current = [], error + while current is not None and len(chain) < 8 and all(current is not seen for seen in chain): + chain.append(current) + current = current.__cause__ + if len(chain) > 1: + self.error_cause_type = type(chain[-1]).__name__ + for item in chain: + number = getattr(item, "errno", None) if isinstance(item, OSError) else None + if type(number) is int: + self.error_errno = number + break + def release_stop(self): """Staged bytes are always committed; a later budget overrun is the owner's to report.""" self.stop = None @@ -448,6 +472,12 @@ def facts(self, outcome=None, failure=None): "status": "raised", "output_code": output_failure(failure)[0] if failure is not None else None, }) + if self.error_type is not None: + facts.update({ + "error_type": self.error_type, + "error_cause_type": self.error_cause_type, + "error_errno": self.error_errno, + }) return facts @@ -931,6 +961,7 @@ def render_attempt(self, output_id, *, claim=None, worker_id=None, stop=None, ob return outcome except Exception as exc: failure = exc + attempt.record_error(exc) raise finally: if observe is not None and attempt.claim is not None: @@ -1020,6 +1051,7 @@ def _render_attempt(self, output_id, claim, worker_id, attempt): except Exception as exc: if observing: raise + attempt.record_error(exc) return self._record_failure( output_id, exc, claim, skip_commit_reconciliation=skip_commit_reconciliation, ) diff --git a/application/single_app/functions_simplechat_operations.py b/application/single_app/functions_simplechat_operations.py index 05a87844b..9a5efaa96 100644 --- a/application/single_app/functions_simplechat_operations.py +++ b/application/single_app/functions_simplechat_operations.py @@ -120,6 +120,7 @@ ) from functions_settings import get_settings, is_user_workflows_enabled_for_user from functions_saved_analysis import analysis_artifact_metadata, authorize_analysis_artifact +from functions_temp_files import scratch_file_dir from utils_cache import invalidate_group_search_cache, invalidate_personal_search_cache @@ -3346,7 +3347,7 @@ def open_generated_chat_artifact_stream(artifact, *, check=None): raise RuntimeError("Blob storage client not available") blob_client = blob_service_client.get_blob_client(container=artifact["blob_container"], blob=artifact["blob_path"]) try: - with tempfile.TemporaryFile(mode="w+b", dir=".") as content: + with tempfile.TemporaryFile(mode="w+b", dir=scratch_file_dir()) as content: digest, size = hashlib.sha256(), 0 if check is not None: check() diff --git a/application/single_app/functions_structured_file_renderers.py b/application/single_app/functions_structured_file_renderers.py index dda22c389..9284de908 100644 --- a/application/single_app/functions_structured_file_renderers.py +++ b/application/single_app/functions_structured_file_renderers.py @@ -22,6 +22,7 @@ GeneratedFileExportStream, ) from functions_generated_export_registry import resolve_generated_file_export_format +from functions_temp_files import scratch_file_dir _CHUNK_CHARACTERS = 16384 @@ -441,7 +442,9 @@ def _render_generated_file_source(source, request, *, max_output_bytes, check=No checks = _SourceChecks(source, check) checks.run() with ExitStack() as resources: - stream = resources.enter_context(ClosingExportResource(tempfile.TemporaryFile(mode='w+b', dir='.'))) + stream = resources.enter_context(ClosingExportResource( + tempfile.TemporaryFile(mode='w+b', dir=scratch_file_dir()), + )) try: writer = _OutputWriter(stream, max_output_bytes, checks) count = _RENDERERS[entry.renderer_id](writer, source, request, limits, checks) diff --git a/application/single_app/functions_temp_files.py b/application/single_app/functions_temp_files.py new file mode 100644 index 000000000..95f4a133e --- /dev/null +++ b/application/single_app/functions_temp_files.py @@ -0,0 +1,22 @@ +# functions_temp_files.py +"""Writable scratch locations for short-lived server-side files. + +Import-independent on purpose: renderers and download streams use it without +pulling in configuration or Azure clients. +""" + +import os + + +SC_TEMP_FILES_DIR = '/sc-temp-files' + + +def scratch_file_dir(): + """Return the container's dedicated scratch directory, or None for the platform default. + + Never the process working directory. The container runs as a non-root user, and + the application code directory is not guaranteed to be writable by that user. + """ + if os.path.isdir(SC_TEMP_FILES_DIR) and os.access(SC_TEMP_FILES_DIR, os.W_OK | os.X_OK): + return SC_TEMP_FILES_DIR + return None diff --git a/docs/explanation/features/ONENOTE_INGESTION.md b/docs/explanation/features/ONENOTE_INGESTION.md index 72b8a29ca..b9613718e 100644 --- a/docs/explanation/features/ONENOTE_INGESTION.md +++ b/docs/explanation/features/ONENOTE_INGESTION.md @@ -145,8 +145,15 @@ requires correcting the native installation, not re-uploading the same file. ## Build and file layout The Dockerfile has an `onenote-builder` stage with a pinned Rust 1.85.1 image. -The application inherits the `onenote-runtime` stage, containing the native -executable on the same Azure Linux distroless Python 3.12 base. +The application inherits the `onenote-runtime` stage, which carries the +extractor's third-party notices and corresponding source on the same Azure Linux +distroless Python 3.12 base. The final stage copies the native executable to +`/app/native/onenote_extractor/bin/` after `/app` already exists with +runtime-user ownership. Nothing in `onenote-runtime` may create `/app`: a +root-owned `/app` stops the non-root application from writing to its working +directory (see `docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md`). +The executable stays root-owned, so the application user can run it but not +replace it. The React branch retains its independent `v2uibuilder` stage and copies its fresh bundle into that final application image. Publishing the branch is not a deployment; an existing test app needs a rebuild from this branch before it diff --git a/docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md b/docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md new file mode 100644 index 000000000..1491af8f3 --- /dev/null +++ b/docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md @@ -0,0 +1,145 @@ +# Orchestration File Render Permission Fix + +**Version: 0.261.218** + +Fixed in version: **0.261.218**, recorded in +`application/single_app/config.py`. + +Issue: [#1623](https://github.com/microsoft/simplechat/issues/1623). This affects +the React V2 branch (`paullizer-react-v2-ui`) only. The causal commits are not on +`main` or `Development`. + +## Issue + +Chat orchestration could not create downloadable files. A request such as +"create a csv showing the states and their capitals" planned correctly, and the +compose step that prepared the rows completed. The file step then failed, and +the chat showed: + +- `us_states_and_capitals.csv: could not be created.` +- `This operation could not complete.` + +Production telemetry recorded the render attempt as `output_access_denied` +after about 260 ms of rendering. That code describes a source-access refusal, +but the source was general knowledge with no documents involved. + +## Root cause + +Two changes combined to cause the failure. Neither caused it on its own. + +1. **Container image.** The OneNote native extractor work (2026-09-26) made the + final image stage `FROM onenote-runtime`. That stage copied the extractor + binary to `/app/native/onenote_extractor/bin/`, which implicitly created + `/app` owned by `root:root` with mode `0755`. The later + `COPY --from=builder --chown=${UID}:${GID} /app /app` copies only the + directory's contents, so `/app` itself stayed root-owned. The application + runs as UID `65532` with `/app` as its working directory. +2. **Scratch-file location.** The shared structured file renderer + (`functions_structured_file_renderers.py`) and the verified generated-file + download stream (`open_generated_chat_artifact_stream` in + `functions_simplechat_operations.py`) created their scratch files with + `tempfile.TemporaryFile(dir='.')`, the process working directory. + +Creating a scratch file in `/app` therefore raised +`PermissionError: [Errno 13]`. `output_failure()` classifies every +`PermissionError` as `output_access_denied`, and the original exception was not +logged, so the cause was hidden. + +Inspection of the deployed image layers confirmed the regression: + +| Image | `/app` owner | CSV render | +|---|---|---| +| `20260926-113853` (before the OneNote stage change) | `65532:65532` | Completed | +| `20260930-185829`, `20261002-145750` (after) | `0:0`, mode `0755` | `output_access_denied` | + +### Affected behavior + +- Every orchestration file in the structured formats (CSV, JSON, XML, YAML, + TXT, and Markdown) failed to render. +- Office formats (DOCX, XLSX, PPTX, PDF) render in memory, but downloading any + orchestration-generated file streams it through the same working-directory + scratch file, so those downloads would fail as well. + +## Technical details + +### Files modified + +| File | Change | +|---|---| +| `application/single_app/functions_temp_files.py` | New import-independent `scratch_file_dir()` helper. | +| `application/single_app/functions_structured_file_renderers.py` | The renderer's scratch file uses `scratch_file_dir()` instead of `dir='.'`. | +| `application/single_app/functions_simplechat_operations.py` | `open_generated_chat_artifact_stream` uses `scratch_file_dir()` instead of `dir="."`. | +| `application/single_app/functions_orchestration_rendering.py` | A failed render attempt records its error class names and OS error number in its diagnostics. | +| `application/single_app/Dockerfile` | `onenote-runtime` no longer creates `/app`. The extractor binary is copied in the final stage after the app code. | +| `docs/explanation/features/ONENOTE_INGESTION.md` | Build layout updated for the new copy location. | +| `docs/reference/logging-tags.md` | New render-attempt diagnostic fields documented. | +| `functional_tests/test_orchestration_file_render_scratch_dir.py` | New regression tests. | +| `functional_tests/test_generated_file_structured_renderers.py`, `functional_tests/test_orchestration_output_lifecycle.py` | Their stream-tracking doubles required `dir='.'`, which pinned the defect. They now require the `scratch_file_dir()` location. | +| `functional_tests/test_workflow_saved_output_artifacts.py` | Supplies `scratch_file_dir` to the download function it loads from source. | + +### Scratch directory + +`scratch_file_dir()` returns `/sc-temp-files` when that directory exists and the +process can write to it. The image creates `/sc-temp-files` for the runtime +user, and document uploads already use it. Otherwise the function returns +`None`, which selects the platform temp directory (`/tmp` in the image, `%TEMP%` +on Windows development machines). It never returns the working directory. + +### Image layout + +The `onenote-runtime` stage keeps the extractor's notices and corresponding +source under `/usr/share/simplechat/onenote-extractor/`, but no longer writes +under `/app`. In the final stage, the chowned copy from the builder stage +creates `/app` owned by the runtime user, as it did before 2026-09-26. The +extractor binary is copied afterward to the path `functions_onenote.py` +expects. The binary remains root-owned: the application user can run it but +cannot replace it. + +### Render diagnostics + +The `[ORCHESTRATION_EXECUTOR] A file render attempt finished.` event now also +includes, for a failed attempt: + +- `error_type`: the class name of the exception that failed the attempt. +- `error_cause_type`: the class name at the end of its explicit `__cause__` + chain, when the error was wrapped. +- `error_errno`: the first OS error number in that chain, when there is one. + +Exception messages, file paths, and identifiers are never recorded. A +successful attempt does not include these fields. The user-facing +classification is unchanged. A file-system `PermissionError` still reports +`output_access_denied`, but the log now shows `error_type=PermissionError` and +`error_errno=13`, so the two cases can be told apart. + +## Validation + +### Tests + +`functional_tests/test_orchestration_file_render_scratch_dir.py`: + +- `scratch_file_dir()` returns the dedicated directory only when it exists and + is writable. +- No application `tempfile` call passes `dir='.'`, `dir=''`, or + `dir=os.getcwd()`. +- A CSV file renders, commits, and downloads through the real renderer and the + real download stream while any scratch file aimed at the working directory + fails with `EACCES`, as it does in the container. This test fails on the + previous code with `output_access_denied`. +- A failed render logs `PermissionError` and errno `13` without the message or + path. A wrapped failure logs its outer class, root cause class, and errno. +- The Dockerfile creates `/app` only through the runtime user's chowned copy, + and adds the extractor afterward. + +Related suites that pass with the change: `test_orchestration_render_check_cadence.py`, +`test_orchestration_output_lifecycle.py`, `test_generated_file_structured_renderers.py`, +`test_workflow_saved_output_artifacts.py`, `test_orchestration_artifact_publication.py`, +`test_document_provenance_publication.py`, and `test_v2_onenote_uploads.py`. + +### Deployment + +Rebuild and redeploy the image. Each change would fix the failure on its own: +the code no longer writes scratch files to the working directory, and the +rebuilt image gives the runtime user ownership of `/app` again. Both are kept +so a later image-layout change cannot silently break file generation. After +deploying, a failed file render can be diagnosed from the `error_type` and +`error_errno` fields of the render-attempt event. diff --git a/docs/explanation/release_notes.md b/docs/explanation/release_notes.md index 4d17e9a75..d64cb7275 100644 --- a/docs/explanation/release_notes.md +++ b/docs/explanation/release_notes.md @@ -2,6 +2,17 @@ For feature-focused and fix-focused drill-downs by version, see [Features by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/features) and [Fixes by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/fixes). +### **(v0.261.218)** + +#### Bug Fixes + +* **Orchestration File Generation Fix** + * Chat orchestration could no longer create downloadable files. A request such as "create a csv showing the states and their capitals" planned correctly and prepared its rows, then the file step failed with "This file could not be created." Every CSV, JSON, XML, YAML, TXT and Markdown file failed this way. Word and PDF files rendered, but downloading any orchestration-generated file used the same failing step, so those downloads would have failed too. + * Two changes interacted to cause this. Since the OneNote extractor was added to the container image, `/app` has been owned by root, while the app runs as a non-root user with `/app` as its working directory. The file renderer and the verified download stream created their scratch files in the working directory, so the operating system refused them. The refusal was reported as `output_access_denied`, which is a source-access category, and the real cause wasn't logged. + * Scratch files now go to `/sc-temp-files` when it's writable, and otherwise to the platform temp directory, never to the working directory. The image creates `/app` for the runtime user again. The OneNote extractor binary is added after that and stays root-owned. + * For a failed attempt, the `[ORCHESTRATION_EXECUTOR] A file render attempt finished.` event now also records `sc_error_type`, `sc_error_cause_type` and `sc_error_errno`, so a file-system error can be told apart from a source-access refusal. Messages and paths aren't logged. + * (Ref: #1623, `functions_temp_files.py`, `functions_structured_file_renderers.py`, `functions_simplechat_operations.py`, `functions_orchestration_rendering.py`, `Dockerfile`, [Orchestration File Render Permission Fix](fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md)) + ### **(v0.261.217)** #### New Features diff --git a/docs/reference/logging-tags.md b/docs/reference/logging-tags.md index db131ede5..398717a7d 100644 --- a/docs/reference/logging-tags.md +++ b/docs/reference/logging-tags.md @@ -376,7 +376,7 @@ file names, file content, prompts, or model responses. | Event message | Severity | Properties | | --- | --- | --- | -| `[ORCHESTRATION_EXECUTOR] A file render attempt finished.` | Information when the file completed; otherwise Warning | `sc_status`, `sc_output_code`, `sc_output_format`, `sc_size_bytes`, and the timing and check counts described below. | +| `[ORCHESTRATION_EXECUTOR] A file render attempt finished.` | Information when the file completed; otherwise Warning | `sc_status`, `sc_output_code`, `sc_output_format`, `sc_size_bytes`, the timing and check counts described below, and `sc_error_type`, `sc_error_cause_type`, and `sc_error_errno` when the attempt failed with an exception. | | `[ORCHESTRATION_EXECUTOR] A step finished after its time budget; its finished result was kept.` | Warning | `sc_failure_code` (`step_timeout` or `run_timeout`), `sc_elapsed_ms`, `sc_step_timeout_seconds`. | | `[ORCHESTRATION_EXECUTOR] Prepared content did not match its declared outputs; asking once more.` | Information | `sc_reason` (`compose_output_retry`), `sc_error_type`, and `sc_execution_code` when the mismatch has one. | | `[ORCHESTRATION_EXECUTOR] Content preparation could not complete.` | Warning | `sc_failure_code`, `sc_execution_code`, `sc_error_type`, and `sc_authority_reason` when a retained source was refused. | @@ -408,6 +408,14 @@ A render attempt records how its time was spent: exception. `sc_output_code` carries the output failure category when there is one; a render stopped by the step time limit reports `failed` with `output_step_time_limit`. +- Since **0.261.218**, an attempt that failed with an exception also records + `sc_error_type`, the exception's class name; `sc_error_cause_type`, the class at + the end of its explicit cause chain when the error was wrapped; and + `sc_error_errno`, the first operating-system error number in that chain. Messages, + paths, and identifiers are never recorded. These fields separate causes that share + an output code: a source-access refusal and a local file-system `PermissionError` + both report `output_access_denied`, but only the file-system error carries + `sc_error_errno` (`13` for permission denied). To see where a run's file rendering spent its time: @@ -426,7 +434,9 @@ AppTraces verifyMs = toint(Properties.sc_verify_ms), fullChecks = toint(Properties.sc_full_checks), lightChecks = toint(Properties.sc_light_checks), - stopObserved = tobool(Properties.sc_stop_observed) + stopObserved = tobool(Properties.sc_stop_observed), + errorType = tostring(Properties.sc_error_type), + errorErrno = toint(Properties.sc_error_errno) | order by TimeGenerated asc ``` diff --git a/functional_tests/test_generated_file_structured_renderers.py b/functional_tests/test_generated_file_structured_renderers.py index 01a9677be..420477d85 100644 --- a/functional_tests/test_generated_file_structured_renderers.py +++ b/functional_tests/test_generated_file_structured_renderers.py @@ -211,7 +211,8 @@ def opened_streams(monkeypatch): opened = [] def tracked(*args, **kwargs): - assert kwargs.get('dir') == '.' + # Never the working directory: the container's non-root user cannot write /app. + assert kwargs.get('dir') == renderers.scratch_file_dir() stream = original(*args, **kwargs) opened.append(stream) return stream diff --git a/functional_tests/test_orchestration_file_render_scratch_dir.py b/functional_tests/test_orchestration_file_render_scratch_dir.py new file mode 100644 index 000000000..131ba85ed --- /dev/null +++ b/functional_tests/test_orchestration_file_render_scratch_dir.py @@ -0,0 +1,190 @@ +#!/usr/bin/env python3 +# test_orchestration_file_render_scratch_dir.py +""" +Functional test for orchestration file rendering when the app directory is not writable. +Version: 0.261.218 +Implemented in: 0.261.218 + +This test ensures that generated-file rendering and generated-file downloads never write +scratch files into the process working directory (the container's non-root user cannot +write /app there), that the container image creates /app with runtime-user ownership, and +that a failed render attempt logs its error class names and OS error number so a local +file-system fault can be told apart from a source-access refusal. +""" + +import ast +import csv +import errno +import io +import json +import os +import tempfile +from pathlib import Path + +import functions_temp_files +from test_orchestration_output_lifecycle import lifecycle, production_modules # noqa: F401 + + +ROOT = Path(__file__).resolve().parents[1] +APP = ROOT / "application" / "single_app" +DOCKERFILE = APP / "Dockerfile" +TEMP_FACTORIES = { + "TemporaryFile", "NamedTemporaryFile", "SpooledTemporaryFile", "TemporaryDirectory", + "mkstemp", "mkdtemp", +} +SKIPPED_DIRECTORIES = {"__pycache__", "node_modules", "static", ".venv", "venv"} +INSTRUCTIONS = ("COPY", "ADD", "RUN", "WORKDIR") + + +def _deny_working_directory_scratch(monkeypatch): + """Fail a temp file aimed at the working directory the way the container's /app does.""" + real = tempfile.TemporaryFile + working = os.path.realpath(os.getcwd()) + targets = [] + + def guarded(*args, **kwargs): + target = kwargs.get("dir", args[6] if len(args) > 6 else None) + targets.append(target) + if target is not None and os.path.realpath(target) == working: + raise PermissionError(errno.EACCES, "Permission denied", os.path.join(working, "tmp-denied")) + return real(*args, **kwargs) + + monkeypatch.setattr(tempfile, "TemporaryFile", guarded) + return targets + + +def _working_directory_temp_calls(): + """Every application tempfile call whose dir argument resolves to the working directory.""" + findings = [] + for path in sorted(APP.rglob("*.py")): + if SKIPPED_DIRECTORIES.intersection(path.relative_to(APP).parts): + continue + tree = ast.parse(path.read_text(encoding="utf-8"), filename=str(path)) + for node in ast.walk(tree): + if not isinstance(node, ast.Call): + continue + name = node.func.attr if isinstance(node.func, ast.Attribute) else getattr(node.func, "id", None) + if name not in TEMP_FACTORIES: + continue + for keyword in node.keywords: + value = keyword.value + relative = isinstance(value, ast.Constant) and value.value in {".", "./", ""} + current = isinstance(value, ast.Call) and getattr(value.func, "attr", None) == "getcwd" + if keyword.arg == "dir" and (relative or current): + findings.append(f"{path.relative_to(ROOT).as_posix()}:{node.lineno}") + return findings + + +def test_scratch_directory_is_the_dedicated_directory_or_the_platform_default(tmp_path, monkeypatch): + print("🔍 Testing scratch files use /sc-temp-files or the platform default, never the working directory...") + monkeypatch.setattr(functions_temp_files, "SC_TEMP_FILES_DIR", str(tmp_path / "missing")) + missing = functions_temp_files.scratch_file_dir() + assert missing is None + + dedicated = tmp_path / "sc-temp-files" + dedicated.mkdir() + monkeypatch.setattr(functions_temp_files, "SC_TEMP_FILES_DIR", str(dedicated)) + chosen = functions_temp_files.scratch_file_dir() + assert chosen == str(dedicated) + + monkeypatch.setattr(functions_temp_files.os, "access", lambda path, mode: False) + unwritable = functions_temp_files.scratch_file_dir() + assert unwritable is None + print("✅ The dedicated directory is used only when it exists and is writable.") + + +def test_application_code_never_creates_temp_files_in_the_working_directory(): + print("🔍 Testing no application tempfile call targets the working directory...") + findings = _working_directory_temp_calls() + assert findings == [], f"Temp files must not be created in the working directory: {findings}" + print("✅ No tempfile call uses dir='.' or the current working directory.") + + +def test_csv_renders_and_downloads_when_the_working_directory_is_not_writable(lifecycle, monkeypatch): + print("🔍 Testing a CSV file renders and downloads with an unwritable working directory...") + targets = _deny_working_directory_scratch(monkeypatch) + output = lifecycle.prepare("csv") + facts = [] + completed = lifecycle.service.render_attempt(output["output_id"], observe=facts.append) + assert completed["state"] == "completed", completed + assert completed["error_code"] is None + rows = list(csv.reader(io.StringIO(lifecycle.download(completed).decode("utf-8")))) + assert rows[0] == ["id", "amount", "enabled"] + assert rows[-1] == ["last", "", "false"] + # One scratch file for the render, at least one more for the verified download stream. + assert len(targets) >= 2 + working = os.path.realpath(os.getcwd()) + assert all(target is None or os.path.realpath(target) != working for target in targets) + assert "error_type" not in facts[0] + print("✅ The file rendered, committed and downloaded without writing to the working directory.") + + +def test_failed_render_logs_error_class_and_errno_without_details(lifecycle): + print("🔍 Testing a failed render records its error class and OS error number only...") + private_path = "/app/tmp-private-scratch-name" + lifecycle.failures["csv"] = [PermissionError(errno.EACCES, "Permission denied", private_path)] + output = lifecycle.prepare("csv") + facts = [] + failed = lifecycle.service.render_attempt(output["output_id"], observe=facts.append) + assert failed["state"] == "failed" + assert len(facts) == 1 + fact = facts[0] + # The user-facing classification is unchanged; the diagnostics now show the real cause. + assert fact["output_code"] == "output_access_denied" + assert fact["error_type"] == "PermissionError" + assert fact["error_errno"] == errno.EACCES + assert fact["error_cause_type"] is None + text = json.dumps(fact) + assert private_path not in text and "Permission denied" not in text + assert all(type(value) in (int, bool, str) or value is None for value in fact.values()) + print("✅ The PermissionError and errno were logged without its message or path.") + + +def test_failed_render_logs_the_root_cause_class_of_a_wrapped_error(lifecycle): + print("🔍 Testing a wrapped failure records the root cause class and its errno...") + + def wrapped(): + try: + raise OSError(errno.ENOSPC, "No space left on device", "/sc-temp-files/tmp-private") + except OSError as exc: + raise RuntimeError("private render detail") from exc + + lifecycle.failures["json"] = [wrapped] + output = lifecycle.prepare("json") + facts = [] + failed = lifecycle.service.render_attempt(output["output_id"], observe=facts.append) + assert failed["state"] != "completed" + fact = facts[0] + assert (fact["error_type"], fact["error_cause_type"], fact["error_errno"]) == ( + "RuntimeError", "OSError", errno.ENOSPC, + ) + assert "private" not in json.dumps(fact) + print("✅ The outer class, root cause class and errno were logged.") + + +def test_container_image_creates_app_directory_for_the_runtime_user(): + print("🔍 Testing the image's /app is created by the runtime user's chowned copy...") + dockerfile = DOCKERFILE.read_text(encoding="utf-8") + runtime_stage = dockerfile.split("AS onenote-runtime\n", 1)[1].split("\nFROM ", 1)[0] + final_stage = dockerfile.rsplit("\nFROM ", 1)[1] + assert final_stage.startswith("onenote-runtime\n") + for line in runtime_stage.splitlines(): + if line.startswith(INSTRUCTIONS): + assert not line.split()[-1].startswith("/app"), f"The base stage must not create /app: {line}" + app_copy = final_stage.index("COPY --from=builder --chown=${UID}:${GID} /app /app") + for line in final_stage[:app_copy].splitlines(): + if line.startswith(INSTRUCTIONS): + assert not line.split()[-1].startswith("/app"), f"/app must not exist before its chowned copy: {line}" + code_copy = final_stage.index("COPY --chown=${UID}:${GID} application/single_app ./") + extractor_copy = final_stage.index( + "COPY --from=onenote-builder /build/target/release/simplechat-onenote-extractor " + "/app/native/onenote_extractor/bin/simplechat-onenote-extractor" + ) + assert app_copy < code_copy < extractor_copy + print("✅ /app is created by the chowned copy and the extractor is added afterward.") + + +if __name__ == "__main__": + import pytest + + raise SystemExit(pytest.main([__file__, "-q"])) diff --git a/functional_tests/test_orchestration_output_lifecycle.py b/functional_tests/test_orchestration_output_lifecycle.py index 14441aa36..afadc86fa 100644 --- a/functional_tests/test_orchestration_output_lifecycle.py +++ b/functional_tests/test_orchestration_output_lifecycle.py @@ -57,6 +57,7 @@ ) from functions_orchestration_rendering import OrchestrationRenderingService, execute_render_file from functions_orchestration_results import NamedOutput, OrchestrationResultReader +from functions_temp_files import scratch_file_dir from test_support import offline_bootstrap from test_support.orchestration_results import COLUMNS, ROWS, ResultFixture, complete, source from test_support.orchestration_revisions import AtomicMemoryContainer @@ -1666,8 +1667,8 @@ def test_output_bounds_and_all_private_streams_close_on_failure(lifecycle, monke original = tempfile.TemporaryFile def tracked(*args, **kwargs): - if kwargs.get("dir") != ".": - raise AssertionError("Output scratch files must stay in the project.") + if kwargs.get("dir") != scratch_file_dir(): + raise AssertionError("Output scratch files must use the scratch directory, never the working directory.") stream = original(*args, **kwargs) created.append(stream) return stream diff --git a/functional_tests/test_workflow_saved_output_artifacts.py b/functional_tests/test_workflow_saved_output_artifacts.py index 729cf8438..353fb4656 100644 --- a/functional_tests/test_workflow_saved_output_artifacts.py +++ b/functional_tests/test_workflow_saved_output_artifacts.py @@ -38,6 +38,7 @@ from test_workflow_result_store import FakeBlobService from functions_analysis_access import AnalysisResultUnavailable from content_screening.contracts import DocumentHeldError +from functions_temp_files import scratch_file_dir from functions_workflow_artifacts import ( WorkflowRecordExportSource, authorize_workflow_saved_output_artifact, @@ -129,6 +130,7 @@ def create_message(body): namespace = { "Any": Any, "Dict": Dict, "Optional": Optional, "hashlib": hashlib, "datetime": datetime, "timezone": timezone, "os": os, "uuid": uuid, "tempfile": tempfile, + "scratch_file_dir": scratch_file_dir, "ResourceExistsError": ResourceExistsError, "CosmosResourceExistsError": CosmosResourceExistsError, "CosmosResourceNotFoundError": CosmosResourceNotFoundError, "cosmos_conversations_container": services.conversations, "cosmos_messages_container": services.messages, From 6b98087dedbfe80b5b0e44a1c088d5e6058d2599 Mon Sep 17 00:00:00 2001 From: Paul Lizer Date: Sat, 3 Oct 2026 14:36:19 -0400 Subject: [PATCH 2/2] Build the final image stage from the pinned distroless image The final stage inherited the onenote-runtime build stage. That stage once created /app as root, and the Malicious PR Security Review flags FROM onenote-runtime as a floating base image on any PR that touches the Dockerfile, because a stage name has no tag or digest. After the previous commit the stage held only the OneNote extractor's notices, licenses and source. Remove it, build the final stage FROM mcr.microsoft.com/azurelinux/distroless/python:3.12 as before c9faeae08, and copy those files from onenote-builder at the top of the final stage. The image contents and paths are unchanged, and the checker is not modified. - The scratch-dir regression test now requires that the final stage does not inherit any build stage and that its base is pinned. - test_v2_onenote_uploads requires the distroless base and the extractor binary, notices, licenses and source in the final stage itself. - Update the OneNote build layout and the fix document. Refs #1623 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- application/single_app/Dockerfile | 17 +++++------ .../explanation/features/ONENOTE_INGESTION.md | 15 +++++----- ...RCHESTRATION_FILE_RENDER_PERMISSION_FIX.md | 29 ++++++++++++------- ...t_orchestration_file_render_scratch_dir.py | 24 +++++++++++---- functional_tests/test_v2_onenote_uploads.py | 14 ++++++--- 5 files changed, 63 insertions(+), 36 deletions(-) diff --git a/application/single_app/Dockerfile b/application/single_app/Dockerfile index ba5c89c99..e6dd249ca 100644 --- a/application/single_app/Dockerfile +++ b/application/single_app/Dockerfile @@ -59,14 +59,6 @@ RUN cargo build --release --locked \ && cp Cargo.toml Cargo.lock /onenote-source/wrapper/ \ && cp -R src /onenote-source/wrapper/ -FROM mcr.microsoft.com/azurelinux/distroless/python:3.12 AS onenote-runtime - -# Nothing in this stage may create /app. The final stage's chowned copy from the builder -# must be what creates /app, so the non-root runtime user owns its working directory. -COPY --from=onenote-builder /build/THIRD_PARTY_NOTICES.txt /usr/share/simplechat/onenote-extractor/THIRD_PARTY_NOTICES.txt -COPY --from=onenote-builder /build/licenses/ /usr/share/simplechat/onenote-extractor/licenses/ -COPY --from=onenote-builder /onenote-source/ /usr/share/simplechat/onenote-extractor/source/ - FROM mcr.microsoft.com/azurelinux/base/python:3.12 AS builder ARG UID @@ -243,11 +235,18 @@ RUN set -eux; \ ;; \ esac -FROM onenote-runtime +FROM mcr.microsoft.com/azurelinux/distroless/python:3.12 ARG UID ARG GID +# OneNote extractor notices and corresponding source, required when redistributing the image. +# Nothing may be copied under /app before the chowned copy from the builder below creates it, +# so the non-root runtime user owns its working directory. +COPY --from=onenote-builder /build/THIRD_PARTY_NOTICES.txt /usr/share/simplechat/onenote-extractor/THIRD_PARTY_NOTICES.txt +COPY --from=onenote-builder /build/licenses/ /usr/share/simplechat/onenote-extractor/licenses/ +COPY --from=onenote-builder /onenote-source/ /usr/share/simplechat/onenote-extractor/source/ + COPY --from=builder /etc/pki /etc/pki COPY --from=builder /etc/ssl/certs /etc/ssl/certs COPY --from=builder /home/nonroot /home/nonroot diff --git a/docs/explanation/features/ONENOTE_INGESTION.md b/docs/explanation/features/ONENOTE_INGESTION.md index b9613718e..68282220b 100644 --- a/docs/explanation/features/ONENOTE_INGESTION.md +++ b/docs/explanation/features/ONENOTE_INGESTION.md @@ -145,14 +145,15 @@ requires correcting the native installation, not re-uploading the same file. ## Build and file layout The Dockerfile has an `onenote-builder` stage with a pinned Rust 1.85.1 image. -The application inherits the `onenote-runtime` stage, which carries the -extractor's third-party notices and corresponding source on the same Azure Linux -distroless Python 3.12 base. The final stage copies the native executable to +The final application stage builds directly from the Azure Linux distroless +Python 3.12 image and copies the extractor's third-party notices and +corresponding source from `onenote-builder`. It copies the native executable to `/app/native/onenote_extractor/bin/` after `/app` already exists with -runtime-user ownership. Nothing in `onenote-runtime` may create `/app`: a -root-owned `/app` stops the non-root application from writing to its working -directory (see `docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md`). -The executable stays root-owned, so the application user can run it but not +runtime-user ownership. The final stage must not inherit an intermediate stage +or copy anything under `/app` before that point: a root-owned `/app` stops the +non-root application from writing to its working directory (see +`docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md`). The +executable stays root-owned, so the application user can run it but not replace it. The React branch retains its independent `v2uibuilder` stage and copies its fresh bundle into that final application image. Publishing the branch is not diff --git a/docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md b/docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md index 72d723c01..03c8f77b2 100644 --- a/docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md +++ b/docs/explanation/fixes/ORCHESTRATION_FILE_RENDER_PERMISSION_FIX.md @@ -70,12 +70,13 @@ Inspection of the deployed image layers confirmed the regression: | `application/single_app/functions_structured_file_renderers.py` | The renderer's scratch file uses `scratch_file_dir()` instead of `dir='.'`. | | `application/single_app/functions_simplechat_operations.py` | `open_generated_chat_artifact_stream` uses `scratch_file_dir()` instead of `dir="."`. | | `application/single_app/functions_orchestration_rendering.py` | A failed render attempt records its error class names and OS error number in its diagnostics. | -| `application/single_app/Dockerfile` | `onenote-runtime` no longer creates `/app`. The extractor binary is copied in the final stage after the app code. | +| `application/single_app/Dockerfile` | The `onenote-runtime` stage is removed. The final stage builds from the pinned distroless image and copies the extractor's notices and source directly. The extractor binary is copied after the app code. | | `docs/explanation/features/ONENOTE_INGESTION.md` | Build layout updated for the new copy location. | | `docs/reference/logging-tags.md` | New render-attempt diagnostic fields documented. | | `functional_tests/test_orchestration_file_render_scratch_dir.py` | New regression tests. | | `functional_tests/test_generated_file_structured_renderers.py`, `functional_tests/test_orchestration_output_lifecycle.py` | Their stream-tracking doubles required `dir='.'`, which pinned the defect. They now require the `scratch_file_dir()` location. | | `functional_tests/test_workflow_saved_output_artifacts.py` | Supplies `scratch_file_dir` to the download function it loads from source. | +| `functional_tests/test_v2_onenote_uploads.py` | The image-wiring test required the final stage to inherit `onenote-runtime`. It now requires the distroless base and the extractor binary, notices, licenses, and source in the final stage. | ### Scratch directory @@ -87,13 +88,20 @@ on Windows development machines). It never returns the working directory. ### Image layout -The `onenote-runtime` stage keeps the extractor's notices and corresponding -source under `/usr/share/simplechat/onenote-extractor/`, but no longer writes -under `/app`. In the final stage, the chowned copy from the builder stage -creates `/app` owned by the runtime user, as it did before 2026-09-26. The -extractor binary is copied afterward to the path `functions_onenote.py` -expects. The binary remains root-owned: the application user can run it but -cannot replace it. +The `onenote-runtime` stage is removed. The final stage builds directly from +`mcr.microsoft.com/azurelinux/distroless/python:3.12`, as it did before +2026-09-26. It copies the extractor's notices and corresponding source from +`onenote-builder` to `/usr/share/simplechat/onenote-extractor/`, so the +redistribution notices are unchanged. The chowned copy from the builder stage +then creates `/app` owned by the runtime user. The extractor binary is copied +afterward to the path `functions_onenote.py` expects. The binary remains +root-owned: the application user can run it but cannot replace it. + +Removing the stage also clears a false positive in the Malicious PR Security +Review. That check inspects the whole Dockerfile whenever a pull request +changes it. It requires each `FROM` to name an image with a tag or digest, so it +flagged `FROM onenote-runtime`, a build-stage name. The final `FROM` now names +the pinned image, and the check itself is unchanged. ### Render diagnostics @@ -127,8 +135,9 @@ classification is unchanged. A file-system `PermissionError` still reports previous code with `output_access_denied`. - A failed render logs `PermissionError` and errno `13` without the message or path. A wrapped failure logs its outer class, root cause class, and errno. -- The Dockerfile creates `/app` only through the runtime user's chowned copy, - and adds the extractor afterward. +- The Dockerfile's final stage builds from a pinned image rather than another + build stage, creates `/app` only through the runtime user's chowned copy, and + adds the extractor afterward. Related suites that pass with the change: `test_orchestration_render_check_cadence.py`, `test_orchestration_output_lifecycle.py`, `test_generated_file_structured_renderers.py`, diff --git a/functional_tests/test_orchestration_file_render_scratch_dir.py b/functional_tests/test_orchestration_file_render_scratch_dir.py index dfef19282..4461db638 100644 --- a/functional_tests/test_orchestration_file_render_scratch_dir.py +++ b/functional_tests/test_orchestration_file_render_scratch_dir.py @@ -18,6 +18,7 @@ import io import json import os +import re import tempfile from pathlib import Path @@ -162,15 +163,26 @@ def wrapped(): print("✅ The outer class, root cause class and errno were logged.") +def _build_stage_names(dockerfile): + """Stage aliases declared with FROM ... AS; Docker matches them case-insensitively.""" + return { + match.group(1).lower() + for match in re.finditer(r"^FROM\s+\S+\s+AS\s+(\S+)", dockerfile, re.MULTILINE | re.IGNORECASE) + } + + def test_container_image_creates_app_directory_for_the_runtime_user(): print("🔍 Testing the image's /app is created by the runtime user's chowned copy...") dockerfile = DOCKERFILE.read_text(encoding="utf-8") - runtime_stage = dockerfile.split("AS onenote-runtime\n", 1)[1].split("\nFROM ", 1)[0] final_stage = dockerfile.rsplit("\nFROM ", 1)[1] - assert final_stage.startswith("onenote-runtime\n") - for line in runtime_stage.splitlines(): - if line.startswith(INSTRUCTIONS): - assert not line.split()[-1].startswith("/app"), f"The base stage must not create /app: {line}" + final_base = final_stage.split(None, 1)[0] + # An inherited build stage can create /app as root first, as the former onenote-runtime stage did. + assert final_base.lower() not in _build_stage_names(dockerfile), ( + f"The final stage must not inherit another build stage: {final_base}" + ) + assert "@sha256:" in final_base or ":" in final_base.rsplit("/", 1)[-1], ( + f"The final base image must be pinned to a tag or digest: {final_base}" + ) app_copy = final_stage.index("COPY --from=builder --chown=${UID}:${GID} /app /app") for line in final_stage[:app_copy].splitlines(): if line.startswith(INSTRUCTIONS): @@ -181,7 +193,7 @@ def test_container_image_creates_app_directory_for_the_runtime_user(): "/app/native/onenote_extractor/bin/simplechat-onenote-extractor" ) assert app_copy < code_copy < extractor_copy - print("✅ /app is created by the chowned copy and the extractor is added afterward.") + print("✅ /app is created by the chowned copy from a pinned base, and the extractor is added afterward.") if __name__ == "__main__": diff --git a/functional_tests/test_v2_onenote_uploads.py b/functional_tests/test_v2_onenote_uploads.py index 3eaa4df60..007ad6365 100644 --- a/functional_tests/test_v2_onenote_uploads.py +++ b/functional_tests/test_v2_onenote_uploads.py @@ -137,13 +137,19 @@ def forbidden(*args, **kwargs): def test_final_image_contains_both_native_runtime_and_built_spa(self): dockerfile = (APP_ROOT / "Dockerfile").read_text(encoding="utf-8") final_stage = dockerfile.rsplit("\nFROM ", 1)[1] - self.assertTrue(final_stage.startswith("onenote-runtime\n")) + # The final stage builds from the distroless image directly, not an intermediate stage + # (0.261.232): an inherited stage created /app as root before the runtime user's copy. + self.assertTrue(final_stage.startswith("mcr.microsoft.com/azurelinux/distroless/python:")) self.assertIn("AS v2uibuilder", dockerfile) self.assertIn("AS onenote-builder", dockerfile) - self.assertIn("AS onenote-runtime", dockerfile) self.assertIn("cargo build --release --locked", dockerfile) - self.assertIn("COPY --from=onenote-builder /build/target/release/simplechat-onenote-extractor", dockerfile) - self.assertIn("COPY --from=onenote-builder /onenote-source/", dockerfile) + for native_copy in ( + "COPY --from=onenote-builder /build/target/release/simplechat-onenote-extractor", + "COPY --from=onenote-builder /build/THIRD_PARTY_NOTICES.txt", + "COPY --from=onenote-builder /build/licenses/", + "COPY --from=onenote-builder /onenote-source/", + ): + self.assertIn(native_copy, final_stage) self.assertIn("COPY --from=v2uibuilder", final_stage) self.assertLess( final_stage.index("application/single_app ./"),