ocda db: before writing check carried file names

bugfix: content image names are now checked in a pass of their own,
before anything is created or written, and one bad name refuses every
image in the artefact rather than skipping the one, which is what the
zip reader does with a zip. The write then re-checks containment.

(prior to this an image name read out of a .ocda.db was written without
checks, and such a database can be fetched over https. A name could
climb with "..", and being handed to chainPath an absolute name dropped
everything before it, so "/x" did not land under the image directory at
all. The zip reader has guarded against this since it was written; the
database reader did not).

carried_names.d holds both rules: a bare filename, as an image is
carried, and a relative path, for the markup and conf a database is to
carry.

(assisted by Claude-Code)

2 files changed, 230 insertions, 1 deletions

diff --git a/src/sisudoc/ocda/abstraction/doc_from_artefact.d b/src/sisudoc/ocda/abstraction/doc_from_artefact.d
index 3a32d25..7cbc73a 100644
--- a/src/sisudoc/ocda/abstraction/doc_from_artefact.d
+++ b/src/sisudoc/ocda/abstraction/doc_from_artefact.d
@@ -86,6 +86,7 @@ template spineDocFromArtefact() {
   import sisudoc.ocda.meta.conf_make_meta_structs;
   import sisudoc.ocda.meta.doc_matters;
   import sisudoc.ocda.io_in.paths_source;
+  import sisudoc.ocda.io_in.carried_names;
   import sisudoc.ocda.abstraction.doc_has;
   import sisudoc.ocda.abstraction.load;
   import sisudoc.ocda.meta.topic_register;
@@ -93,6 +94,7 @@ template spineDocFromArtefact() {
   mixin spineDocHasFromAbstraction;
   mixin spineAbstractionLoad;
   mixin spineTopicRegister;
+  mixin spineCarriedNames;
   /+ ↓ a .ssp header key from a field name: the first underscore becomes the
        dot that separates the group, so title_main is title.main and
        rights_copyright_text is rights.copyright_text. The writer's keys are
@@ -220,6 +222,24 @@ template spineDocFromArtefact() {
     mixin spineAbstractionDbRead _dbr;
     auto _files = _dbr.dbReadFiles(_artefact, "image");
     if (_files.length == 0) { return ""; }
+    /+ ↓ every name checked before anything is created or written.
+         the name comes out of the database and a database can be
+         downloaded, so it is attacker-controlled: it can climb with "..",
+         and chainPath drops everything before an absolute segment, so "/x"
+         would not land under the image directory at all. One bad name means
+         the artefact cannot be trusted for the rest of them, so this
+         refuses the lot rather than skipping one, which is what the zip
+         reader does with a zip.
+    +/
+    foreach (_f; _files) {
+      string _bad = validateCarriedFileName(_f.name);
+      if (_bad.length > 0) {
+        stderr.writeln("WARNING: ", _artefact.baseName,
+          " carries an image spine will not write: ", _bad,
+          "; no image is taken from this artefact");
+        return "";
+      }
+    }
     string _root = (tempDir.chainPath("spine-ocda-"
       ~ _artefact.baseName ~ "-" ~ thisProcessID.to!string).array).to!string;
     string _img_dir = (_root.chainPath("media").chainPath("image").array).to!string;
@@ -237,8 +257,18 @@ template spineDocFromArtefact() {
           " does not match the digest recorded with it (", _f.sha256, " expected, ",
           _got, " found); it is written out as it stands");
       }
+      string _out_path = (_img_dir.chainPath(_f.name).array).to!string;
+      /+ ↓ the check on the check: the name rules above already forbid a
+           directory part, so this can only fire if they were loosened
+      +/
+      if (!(carriedPathIsWithin(_img_dir, _out_path))) {
+        stderr.writeln("WARNING: ", _f.name, " in ", _artefact.baseName,
+          " resolves outside the image directory; no image is taken from",
+          " this artefact");
+        return "";
+      }
       try {
-        (_img_dir.chainPath(_f.name).array).to!string.write(_f.data);
+        _out_path.write(_f.data);
       } catch (Exception ex) {
         stderr.writeln("WARNING: could not write ", _f.name, ": ", ex.msg);
       }
diff --git a/src/sisudoc/ocda/io_in/carried_names.d b/src/sisudoc/ocda/io_in/carried_names.d
new file mode 100644
index 0000000..09e8912
--- /dev/null
+++ b/src/sisudoc/ocda/io_in/carried_names.d
@@ -0,0 +1,199 @@
+/+
+- Name: SisuDoc Spine, Doc Reform [a part of]
+  - Description: documents, structuring, processing, publishing, search
+    - static content generator
+
+  - Author: Ralph Amissah
+    [ralph.amissah@gmail.com]
+
+  - Copyright: (C) 2015 (continuously updated, current 2026) Ralph Amissah, All Rights Reserved.
+
+  - License: AGPL 3 or later:
+
+    Spine (SiSU), a framework for document structuring, publishing and
+    search
+
+    Copyright (C) Ralph Amissah
+
+    This program is free software: you can redistribute it and/or modify it
+    under the terms of the GNU AFERO General Public License as published by the
+    Free Software Foundation, either version 3 of the License, or (at your
+    option) any later version.
+
+    This program is distributed in the hope that it will be useful, but WITHOUT
+    ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+    FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+    more details.
+
+    You should have received a copy of the GNU General Public License along with
+    this program. If not, see [https://www.gnu.org/licenses/].
+
+    If you have Internet connection, the latest version of the AGPL should be
+    available at these locations:
+    [https://www.fsf.org/licensing/licenses/agpl.html]
+    [https://www.gnu.org/licenses/agpl.html]
+
+  - Spine (by Doc Reform, related to SiSU) uses standard:
+    - docReform markup syntax
+      - standard SiSU markup syntax with modified headers and minor modifications
+    - docReform object numbering
+      - standard SiSU object citation numbering & system
+
+  - Homepages:
+    [https://www.sisudoc.org]
+    [https://www.doc-reform.org]
+
+  - Git
+    [https://git.sisudoc.org/]
+
++/
+/++
+  module carried_names;<BR>
+  - validate the names of files carried inside an artefact, before any of
+    them is written to disk<BR>
+  - one set of rules for a bare filename, one for a relative path, and a
+    containment check for either
++/
+module sisudoc.ocda.io_in.carried_names;
+@safe:
+/+ ↓ names that arrive from outside, and decide where a file gets written
+     .
+     A pod zip and a .ocda.db both carry files by name, and both can arrive
+     over https. A name read out of either is attacker-controlled: it can be
+     absolute, it can climb with "..", and chainPath silently drops
+     everything before an absolute segment, so "/etc/x" does not land under
+     the directory it was joined to. read_zip_pod.d has guarded against this
+     since the zip reader was written; the database reader did not, which is
+     what this module is for.
+     .
+     Two rules, because the two roles differ:
+     - an image is carried under a bare filename and belongs in one
+       directory, so a directory part in the name is itself the error;
+     - markup, conf and manifest are carried under their path inside the pod
+       (media/text/de/about_manual.ssi), so the rule there is the zip
+       reader's: relative, no climbing, bounded depth.
+     .
+     Both are followed by carriedPathIsWithin() at the point of writing.
+     That is deliberate duplication: the name rules are the check, and
+     containment is the check on the check.
++/
+template spineCarriedNames() {
+  import std.algorithm : canFind;
+  /+ ↓ imports stay narrow on purpose. This template is mixed in beside
+       others, and a template-level import lands in the mixing scope: an
+       "import std.array : split" here takes over the UFCS lookup for
+       split(ctRegex) at every mixin site and breaks it. Nothing below needs
+       split, and nothing below should acquire it.
+  +/
+  import std.array     : array;
+  import std.conv      : to;
+  import std.path      : asNormalizedPath, isAbsolute;
+  import std.regex;
+  import std.string    : indexOf;
+  /+ ↓ limits, the zip reader's where they overlap +/
+  enum size_t MAX_CARRIED_NAME_LENGTH = 255;               /+ one path component +/
+  enum size_t MAX_CARRIED_PATH_LENGTH = 1024;              /+ the whole path +/
+  enum size_t MAX_CARRIED_PATH_DEPTH  = 10;                /+ as read_zip_pod.d +/
+  /+ ↓ allowed characters: a filename has no separator, a path has "/" +/
+  static auto rgx_safe_carried_name = ctRegex!(`^[a-zA-Z0-9._ -]+$`);
+  static auto rgx_safe_carried_path = ctRegex!(`^[a-zA-Z0-9._/ -]+$`);
+  /+ ↓ the checks both rules share, "" when the name is acceptable +/
+  private string _carriedNameCommon(string _name) {
+    if (_name.length == 0) {
+      return "empty carried name";
+    }
+    if (_name.indexOf('\0') >= 0) {
+      return "null byte in carried name";
+    }
+    if (_name.canFind("\\")) {
+      return "backslash in carried name: " ~ _name;
+    }
+    if (_name.canFind("..")) {
+      return "path traversal in carried name: " ~ _name;
+    }
+    if (_name.isAbsolute || _name[0] == '/') {
+      return "absolute path in carried name: " ~ _name;
+    }
+    return "";
+  }
+  /+ ↓ a bare filename, as an image is carried: no directory part at all +/
+  string validateCarriedFileName(string _name) {
+    string _common = _carriedNameCommon(_name);
+    if (_common.length > 0) {
+      return _common;
+    }
+    if (_name.length > MAX_CARRIED_NAME_LENGTH) {
+      return "carried filename too long: " ~ _name;
+    }
+    if (_name.canFind("/")) {
+      return "directory part in carried filename: " ~ _name;
+    }
+    if (_name[0] == '.') {
+      return "carried filename begins with a dot: " ~ _name;
+    }
+    if (!(_name.matchFirst(rgx_safe_carried_name))) {
+      return "disallowed characters in carried filename: " ~ _name;
+    }
+    return "";
+  }
+  /+ ↓ a relative path inside a pod, as markup and conf are carried +/
+  string validateCarriedPath(string _name) {
+    string _common = _carriedNameCommon(_name);
+    if (_common.length > 0) {
+      return _common;
+    }
+    if (_name.length > MAX_CARRIED_PATH_LENGTH) {
+      return "carried path too long: " ~ _name;
+    }
+    if (_name[$-1] == '/') {
+      return "carried path names a directory: " ~ _name;
+    }
+    size_t _depth = 0;
+    foreach (_c; _name) {
+      if (_c == '/') {
+        _depth++;
+      }
+    }
+    if (_depth > MAX_CARRIED_PATH_DEPTH) {
+      return "carried path too deep: " ~ _name;
+    }
+    /+ ↓ each component in turn, walked rather than split, so that this
+         module needs no import that would leak into a mixing scope
+    +/
+    size_t _from = 0;
+    foreach (_i; 0 .. _name.length + 1) {
+      if (_i < _name.length && _name[_i] != '/') {
+        continue;
+      }
+      string _component = _name[_from .. _i];
+      if (_component.length == 0) {
+        return "empty component in carried path: " ~ _name;
+      }
+      if (_component.length > MAX_CARRIED_NAME_LENGTH) {
+        return "component too long in carried path: " ~ _name;
+      }
+      if (_component[0] == '.') {
+        return "component begins with a dot in carried path: " ~ _name;
+      }
+      _from = _i + 1;
+    }
+    if (!(_name.matchFirst(rgx_safe_carried_path))) {
+      return "disallowed characters in carried path: " ~ _name;
+    }
+    return "";
+  }
+  /+ ↓ does _path, normalised, still sit inside _root? the check on the check +/
+  bool carriedPathIsWithin(string _root, string _path) {
+    auto _canonical_root = (_root.asNormalizedPath).array.to!string;
+    auto _canonical_path = (_path.asNormalizedPath).array.to!string;
+    if (_canonical_root.length == 0
+      || _canonical_path.length <= _canonical_root.length
+    ) {
+      return false;
+    }
+    if (_canonical_path[0.._canonical_root.length] != _canonical_root) {
+      return false;
+    }
+    return (_canonical_path[_canonical_root.length] == '/');
+  }
+}