Build the config editor out of the defaults we ship

The /config form hardcoded four fields, so every other setting in
config/default.cfg -- the site title, the provisioner checkouts -- was
only editable by hand-editing config/main.cfg, and adding a setting
meant editing the template, the route, and the save.

Make default.cfg the description of what exists: Trog::Config::schema()
parses it for its sections, keys, defaults and comments (the comments
being the help text), and the editor is built from that.  Each section
renders as its own collapsed <details>, so an admin who came to change
the theme is not scrolled past the provisioner paths to find it.  The
same schema is the allowlist on save, so a posted key the defaults never
named writes nothing.

The two settings which are a choice among what is installed rather than
free text -- the theme and the data model -- still get their menus,
since default.cfg can say a key exists but not that it has to be one of
the themes on disk.  Both of those, plus the hostname, are now described
in default.cfg so they have somewhere to be described from.

A value the instance blanked on purpose stays blank rather than falling
back to what ships, or clearing the embed allowlist would hand the
shipped one back and re-save it on the next commit.

Also drops the state variables config() cached its values in, which made
the page rendered straight after a save show the pre-save configuration
until the workers restarted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Aevq96J12mMxJnexthkMHv

config/default.cfg

@@ -1,15 +1,26 @@
 [general]
     # FlatFile or SQLite.  See the Readme, and bin/migrate.pl
     # for moving an existing FlatFile site across.
-    data_model=FlatFile
+    data_model=SQLite
+    # What the site calls itself in page titles and feeds.
     title=tCMS
+    # Which directory under www/themes to render with.
+    # Empty means the stock look.
+    theme=
+    # The name this site answers to.  Used wherever an absolute URL has to be
+    # built, and by the virtual machine data sources to tell which guest is
+    # this one.
+    hostname=
 [provisioner]
     # Where the two repositories which own the VM provisioning lifecycle are
     # checked out.  Only needed by Trog::DataSource::ProvisionedVirt, which
     # degrades to a plain guest listing without them.
-    # provisioners=/home/you/Code/provisioners
-    # trog_provisioner=/home/you/Code/trog-provisioner
+    # Example: /home/you/Code/provisioners
+    provisioners=
+    # Example: /home/you/Code/trog-provisioner
+    trog_provisioner=
 [security]
+    # Space separated list of hosts embeds (iframes, remote media) may come
+    # from.  Wildcards are allowed, e.g. *.vimeo.com.  Anything not listed here
+    # is refused by the Content-Security-Policy.
     allow_embeds_from=vimeo.com *.vimeo.com youtube.com *.youtube.com www.youtube-nocookie.com
-[nginx-unit]
-    socket=/var/run/control.unit.sock

lib/Trog/Config.pm

@@ -42,4 +42,79 @@ sub get {
     return $cf;
 }
 
+=head2 Trog::Config::schema( STRING file = $default ) = ARRAYREF
+
+The shape of the configuration, as described by the file we ship.
+
+config/default.cfg is the only place which says what settings exist at all, so
+it is also what the /config editor is built out of -- add a key there and it
+becomes editable, leave one out and it isn't.  Sections and keys come back in
+file order:
+
+    [
+        {
+            section => 'general',
+            fields  => [
+                {
+                    key     => 'data_model',
+                    name    => 'general.data_model',
+                    default => 'SQLite',
+                    comment => 'FlatFile or SQLite. ...',
+                },
+            ],
+        },
+    ]
+
+Comment lines accumulate until a key claims them, so a comment block is the
+help text for whatever setting follows it.  Config::Simple throws both comments
+and ordering away when it reads a file, which is why this reads it again rather
+than asking the object we already have.
+
+Keys appearing before any section header are skipped -- Config::Simple has no
+name for them, so neither do we.
+
+=cut
+
+sub schema ( $file = $default ) {
+    open( my $fh, '<', $file ) or die "Could not read $file: $!";
+
+    my ( @sections, $section, @comments );
+    while ( my $line = readline($fh) ) {
+        chomp $line;
+        $line =~ s/^\s+//;
+        $line =~ s/\s+$//;
+        next unless length $line;
+
+        if ( $line =~ m/^\[(.+)\]$/ ) {
+            $section = { section => $1, fields => [] };
+            push( @sections, $section );
+            @comments = ();
+            next;
+        }
+
+        if ( $line =~ m/^[#;]\s?(.*)$/ ) {
+            push( @comments, $1 );
+            next;
+        }
+
+        my ( $key, $value ) = $line =~ m/^([^=]+?)\s*=\s*(.*)$/;
+        next unless defined $key;
+        next unless $section;
+
+        push(
+            @{ $section->{fields} },
+            {
+                key     => $key,
+                name    => "$section->{section}.$key",
+                default => $value,
+                comment => join( ' ', @comments ),
+            }
+        );
+        @comments = ();
+    }
+    close($fh);
+
+    return \@sections;
+}
+
 1;

lib/Trog/Routes/HTML.pm

@@ -784,40 +784,119 @@ sub config ( $query = {} ) {
 
     $query->{failure} //= -1;
 
-    #XXX ACHTUNG config::simple has this brain damaged behavior of returning a multiple element array when you access something that does not exist.
-    #XXX straight up dying would be preferrable.
-    #XXX anyways, this means you can NEVER NEVER NEVER access a param from within a hash directly.  YOU HAVE BEEN WARNED!
-    my $conf = Trog::Config::get();
-    state $theme    = $conf->param('general.theme')              // '';
-    state $dm       = $conf->param('general.data_model')         // 'DUMMY';
-    state $embeds   = $conf->param('security.allow_embeds_from') // '';
-    state $hostname = $conf->param('general.hostname')           // '';
-
     return Trog::Routes::HTML::index(
         {
-            title              => 'Configure tCMS',
-            theme_dir          => Trog::Themes::td(),
-            stylesheets        => [qw{config.css}],
-            scripts            => [qw{post.js}],
-            themes             => _get_themes() || [],
-            data_models        => _get_data_models(),
-            current_theme      => $theme,
-            current_data_model => $dm,
-            message            => $query->{message},
-            failure            => $query->{failure},
-            to                 => '/config',
-            scheme             => $query->{scheme},
-            embeds             => $embeds,
-            is_admin           => 1,
-            template           => 'config.tx',
+            title           => 'Configure tCMS',
+            theme_dir       => Trog::Themes::td(),
+            stylesheets     => [qw{config.css}],
+            scripts         => [qw{post.js}],
+            themes          => _get_themes() || [],
+            config_sections => _config_sections(),
+            message         => $query->{message},
+            failure         => $query->{failure},
+            to              => '/config',
+            scheme          => $query->{scheme},
+            is_admin        => 1,
+            template        => 'config.tx',
             %$query,
-            hostname => $hostname,
         },
         undef,
         [qw{config.css}],
     );
 }
 
+# The settings which are a choice among what this instance actually has
+# installed rather than free text.  default.cfg can say a key exists, but not
+# that it has to be one of the themes on disk, so that part lives here.
+my %config_choices = (
+    'general.theme'      => sub { return [ '', @{ _get_themes() || [] } ] },
+    'general.data_model' => \&_get_data_models,
+);
+
+=head2 _config_sections() = ARRAYREF
+
+The configuration editor's form, one entry per section of config/default.cfg,
+each holding the fields that section describes:
+
+    { section => 'general', title => 'General', fields => [ { name, key, title, comment, value, options, multiline } ] }
+
+Fields carry the value this instance is running with, defaulting to what
+default.cfg ships.  A field with 'options' renders as a menu, one with
+'multiline' as a textarea -- a list of embed sources is not something to edit
+through a one line input.
+
+=cut
+
+sub _config_sections {
+    my %configured = Trog::Config::get()->vars();
+
+    my @sections;
+    foreach my $section ( @{ Trog::Config::schema() } ) {
+        my @fields;
+        foreach my $field ( @{ $section->{fields} } ) {
+            my $value = _config_value( \%configured, $field->{name} );
+            $value = $field->{default} unless defined $value;
+
+            my %rendered = (
+                %$field,
+                title => _config_title( $field->{key} ),
+                value => $value,
+            );
+
+            if ( my $choices = $config_choices{ $field->{name} } ) {
+                $rendered{options} = [
+                    map {
+                        {
+                            value    => $_,
+                            label    => length($_)   ? $_ : 'default',
+                            selected => $_ eq $value ? 1  : 0,
+                        }
+                    } @{ $choices->() }
+                ];
+            }
+            else {
+                # Long values (an embed allowlist, a list of paths) are miserable
+                # to edit in a box you can only see the end of.
+                $rendered{multiline} = length($value) > 60 ? 1 : 0;
+            }
+
+            push( @fields, \%rendered );
+        }
+        push(
+            @sections,
+            {
+                section => $section->{section},
+                title   => _config_title( $section->{section} ),
+                fields  => \@fields,
+            }
+        );
+    }
+    return \@sections;
+}
+
+# What this instance has configured a setting as, or undef if it never has --
+# which is not the same thing as having set it to nothing, or the editor would
+# hand back the shipped default to anyone who deliberately cleared a setting,
+# and quietly restore it on the next save.
+#
+# XXX ACHTUNG config::simple's param() answers a key which does not exist with a
+# multi element array, so it can never tell you those two apart; vars() can, and
+# is the only reason this does not just ask for the one key.  A key holding
+# commas comes back as an arrayref too, hence the join.
+sub _config_value ( $configured, $name ) {
+    return undef unless exists $configured->{$name};    ## no critic (ProhibitExplicitReturnUndef) -- 'never configured' has to be distinguishable from ''
+
+    my $value = $configured->{$name};
+    return '' unless defined $value;
+    return join( ', ', @$value ) if ref $value eq 'ARRAY';
+    return ref $value ? '' : $value;
+}
+
+sub _config_title ($name) {
+    $name =~ s/[_-]/ /g;
+    return join( ' ', map { ucfirst } split( ' ', $name ) );
+}
+
 =head2 resetpass
 
 =head2 do_resetpass
@@ -980,7 +1059,11 @@ sub _datasource_editable ($module) {
 
 =head2 config_save
 
-Implements /config/save route.  Saves what little configuration we actually use to config/main.cfg
+Implements /config/save route.  Saves the configuration to config/main.cfg.
+
+Only the keys config/default.cfg describes are saved, so a form which posts
+something the defaults never heard of writes nothing -- the schema is what is
+editable, not whatever turned up in the request.
 
 =cut
 
@@ -989,10 +1072,7 @@ sub config_save ($query) {
     return $query->{tpsgi}->forbidden($query)  unless grep { $_ eq 'admin' } @{ $query->{user_acls} };
 
     my $conf = Trog::Config::get();
-    $conf->param( 'general.theme',              $query->{theme} )      if defined $query->{theme};
-    $conf->param( 'general.data_model',         $query->{data_model} ) if $query->{data_model};
-    $conf->param( 'security.allow_embeds_from', $query->{embeds} )     if $query->{embeds};
-    $conf->param( 'general.hostname',           $query->{hostname} )   if $query->{hostname};
+    _config_apply( $conf, $query );
 
     $query->{failure} = 1;
     $query->{message} = "Failed to save configuration!";
@@ -1007,6 +1087,35 @@ sub config_save ($query) {
     return config($query);
 }
 
+=head2 _config_apply( Config::Simple conf, HASHREF query )
+
+Copy whatever settings the form posted onto $conf, keyed by their full
+'section.key' name.
+
+The schema is the allowlist: a request naming something config/default.cfg does
+not describe writes nothing, so a stray form field cannot invent configuration.
+Values are taken as posted, empty string included -- clearing a setting is how
+you go back to having none.
+
+=cut
+
+sub _config_apply ( $conf, $query ) {
+    foreach my $section ( @{ Trog::Config::schema() } ) {
+        foreach my $field ( @{ $section->{fields} } ) {
+            my $value = $query->{ $field->{name} };
+            next unless defined $value;
+
+            # Duplicate inputs arrive as an array; the last one wins, same as
+            # the browser's own idea of which field you filled in last.
+            $value = $value->[-1] if ref $value eq 'ARRAY';
+            next                  if ref $value;
+
+            $conf->param( $field->{name}, $value );
+        }
+    }
+    return;
+}
+
 =head2 themeclone
 
 Clone a theme by copying a directory.

t/Trog-Config.t

@@ -74,4 +74,34 @@ subtest 'the shipped defaults are left alone' => sub {
     );
 };
 
+subtest 'the defaults describe what is editable' => sub {
+
+    # The /config editor is built out of this, so the parse has to keep the
+    # comments (they are the help text) and the file order (they are the form).
+    Path::Tiny->new('config/schema.cfg')->spew_utf8(<<'CFG');
+[general]
+    # FlatFile or SQLite.
+    # Pick one.
+    data_model=FlatFile
+    title=Stock
+[security]
+    # Where embeds may come from.
+    allow_embeds_from=vimeo.com *.vimeo.com
+CFG
+
+    my $schema = Trog::Config::schema('config/schema.cfg');
+
+    is_deeply( [ map { $_->{section} } @$schema ], [qw{general security}], 'the sections come back in file order' );
+
+    my $general = $schema->[0]{fields};
+    is_deeply( [ map { $_->{key} } @$general ], [qw{data_model title}], 'and so do the keys in them' );
+
+    is( $general->[0]{name},    'general.data_model',            'a field knows its full name' );
+    is( $general->[0]{default}, 'FlatFile',                      'and what the defaults ship' );
+    is( $general->[0]{comment}, 'FlatFile or SQLite. Pick one.', 'a comment block above a key is that key\'s help' );
+    is( $general->[1]{comment}, '',                              'and does not leak onto the key after it' );
+
+    is( $schema->[1]{fields}[0]{default}, 'vimeo.com *.vimeo.com', 'values with spaces survive intact' );
+};
+
 done_testing();

t/Trog-Routes-HTML-config.t

@@ -0,0 +1,119 @@
+use strict;
+use warnings;
+
+use Test::More;
+use Cwd();
+use File::Path();
+use File::Temp();
+use Path::Tiny();
+use FindBin;
+
+use lib "$FindBin::Bin/../lib";
+
+# The config editor is built out of config/default.cfg rather than a hardcoded
+# list of fields, so what it can edit is whatever that file describes.  These
+# tests stand inside an instance root, since every path in this app is
+# cwd-relative -- and they stand there before the SUT is loaded, because reading
+# the configuration is memoized and would otherwise be the one in this checkout.
+our $ROOT   = File::Temp::tempdir( 'tcms-config-route-XXXXXX', TMPDIR => 1, CLEANUP => 0 );
+our $OLDCWD = Cwd::getcwd();
+
+END {
+    chdir($OLDCWD)                 if $OLDCWD;
+    File::Path::remove_tree($ROOT) if $ROOT && -d $ROOT;
+}
+
+File::Path::make_path("$ROOT/config");
+File::Path::make_path("$ROOT/www/themes/spec");
+chdir($ROOT) or die $!;
+
+Path::Tiny->new('config/default.cfg')->spew_utf8(<<'CFG');
+[general]
+    # FlatFile or SQLite.
+    data_model=FlatFile
+    # Which directory under www/themes to render with.
+    theme=
+    title=Stock
+    hostname=stock.example.com
+[security]
+    allow_embeds_from=vimeo.com *.vimeo.com youtube.com *.youtube.com www.youtube-nocookie.com
+CFG
+
+# What this instance is actually running with, which is not what ships -- and
+# one setting it deliberately cleared.
+Path::Tiny->new('config/main.cfg')->spew_utf8(<<'CFG');
+[general]
+theme=spec
+hostname=spec.example.com
+title=
+CFG
+
+require_ok('Trog::Routes::HTML') or BAIL_OUT("Can't find SUT");
+
+my %field;
+my $sections = Trog::Routes::HTML::_config_sections();
+foreach my $section (@$sections) {
+    $field{ $_->{name} } = $_ foreach @{ $section->{fields} };
+}
+
+subtest 'the form is the shape of the defaults' => sub {
+    is_deeply( [ map { $_->{section} } @$sections ], [qw{general security}],    'a details block per section, in file order' );
+    is_deeply( [ map { $_->{title} } @$sections ],   [ 'General', 'Security' ], 'each with a legible summary' );
+
+    is_deeply(
+        [ map { $_->{name} } @{ $sections->[0]{fields} } ],
+        [qw{general.data_model general.theme general.title general.hostname}],
+        'and the keys that section describes, in order'
+    );
+    is( $field{'general.data_model'}{title},   'Data Model',          'a key gets a legible label' );
+    is( $field{'general.data_model'}{comment}, 'FlatFile or SQLite.', '...and the comment above it for help' );
+};
+
+subtest 'fields carry what the instance is running with' => sub {
+    is( $field{'general.hostname'}{value},   'spec.example.com', 'a setting this instance overrode shows its value' );
+    is( $field{'general.data_model'}{value}, 'FlatFile',         '...and one it never touched falls back to the default' );
+
+    # Blanking a setting is how you turn it off, so the defaults must not creep
+    # back in and get re-saved on the next commit.
+    is( $field{'general.title'}{value}, '', '...while one cleared on purpose stays cleared' );
+};
+
+subtest 'the settings which are a choice are menus' => sub {
+
+    # default.cfg can say general.theme exists; it cannot say it has to be one
+    # of the themes on disk, so that part is not read off the file.
+    my $themes = $field{'general.theme'}{options};
+    is_deeply( [ map { $_->{value} } @$themes ], [ '', 'spec' ], 'the theme menu is what is installed, plus none' );
+    is( $themes->[0]{label}, 'default', 'the empty theme is named rather than blank' );
+    is_deeply( [ map { $_->{selected} } @$themes ], [ 0, 1 ], 'and the running theme is the selected one' );
+
+    ok( scalar @{ $field{'general.data_model'}{options} }, 'the data model is a menu too' );
+    ok( !$field{'general.title'}{options},                 'while a free text setting is not' );
+    ok( $field{'security.allow_embeds_from'}{multiline},   'a long value gets a textarea' );
+    ok( !$field{'general.hostname'}{multiline},            '...and a short one does not' );
+};
+
+subtest 'saving only writes what the defaults describe' => sub {
+    require Config::Simple;
+    my $conf = Config::Simple->new( syntax => 'ini' );
+
+    Trog::Routes::HTML::_config_apply(
+        $conf,
+        {
+            'general.title' => 'Renamed',
+            'general.theme' => '',
+
+            # Neither of these is in the schema: one is a leftover from the
+            # form this replaced, the other is somebody trying it on.
+            'theme'          => 'nope',
+            'general.pwnage' => 'nope',
+        }
+    );
+
+    is( $conf->param('general.title'), 'Renamed', 'a posted setting is saved' );
+    is( $conf->param('general.theme'), '',        'including one cleared back to empty' );
+    is_deeply( [ $conf->param('general.pwnage') ], [], 'a key the defaults never named is ignored' );
+    is_deeply( [ $conf->param('theme') ],          [], '...as is the bare name the old form used' );
+};
+
+done_testing();

www/styles/config.css

@@ -49,3 +49,33 @@ a.topbar {
 #posttype, #subtitle {
     display: none;
 }
+
+/* The configuration form, one <details> per section of config/default.cfg. */
+.config-section {
+    margin: .5rem 0;
+    padding: .25rem .5rem;
+    border: 1px solid #CCC;
+    border-radius: .25rem;
+}
+.config-section > summary {
+    cursor: pointer;
+    font-weight: bold;
+    padding: .25rem;
+}
+.config-field {
+    margin: .75rem .25rem;
+}
+.config-field > label {
+    display: block;
+    font-weight: bold;
+}
+.config-field > .config-help {
+    margin: .125rem 0 .25rem 0;
+    font-size: .875rem;
+    color: #555;
+}
+.config-field > input, .config-field > select, .config-field > textarea {
+    width: 100%;
+    max-width: 40rem;
+    box-sizing: border-box;
+}

www/templates/html/components/config.tx

@@ -2,34 +2,38 @@
 <p class="title">
  General settings:
 </p>
-This controls your Theme and Data Model used.
+Everything tCMS reads out of its configuration file lives here, grouped the way
+config/default.cfg groups it.  Open the section you care about and leave the
+rest folded up.
+<br />
 The Data Model <em>must</em> be 0-configuration.
 If for example, you use mysql it will have to rely on either a local server, valid config file or connection proxy/pooler locally.
 <hr />
 <form class="Submissions Config" id="mainConfig" method="post" action="/config/save">
-    Theme:
-    <select class="cooltext" name="theme">
-        <option value="" <: if ( $current_theme == '' ) { :>selected<: } :> >default</option>
-        : for $themes -> $theme {
-        <option value="<: $theme :>" <: if ( $current_theme == $theme ) { :>selected<: } :> ><: $theme :></option>
+    : for $config_sections -> $section {
+    <details class="config-section">
+        <summary><: $section.title :></summary>
+        : for $section.fields -> $field {
+        <div class="config-field">
+            <label for="config-<: $field.name :>"><: $field.title :></label>
+            : if ( $field.comment ) {
+            <p class="config-help"><: $field.comment :></p>
+            : }
+            : if ( $field.options ) {
+            <select class="cooltext" id="config-<: $field.name :>" name="<: $field.name :>">
+                : for $field.options -> $option {
+                <option value="<: $option.value :>" <: if ( $option.selected ) { :>selected<: } :> ><: $option.label :></option>
+                : }
+            </select>
+            : } else if ( $field.multiline ) {
+            <textarea class="cooltext" id="config-<: $field.name :>" name="<: $field.name :>" rows="3" form="mainConfig"><: $field.value :></textarea>
+            : } else {
+            <input class="cooltext" type="text" id="config-<: $field.name :>" name="<: $field.name :>" value="<: $field.value :>" />
+            : }
+        </div>
         : }
-    </select>
-    <div>
-    Data Model:
-    <select class="cooltext" name="data_model">
-        : for $data_models -> $dm {
-        <option value="<: $dm :>" <: if ( $current_data_model == $dm ) { :>selected<: } :> ><: $dm :></option>
-        : }
-    </select>
-    </div>
-    <div>
-    Allow Embeds from:
-    <input class="cooltext" type="textarea" name="embeds" value="<: $embeds :>" />
-    </div>
-    <div>
-    Hostname:
-    <input class="cooltext" type="textarea" name="hostname" value="<: $hostname :>" />
-    </div>
+    </details>
+    : }
     <br />
     <input type="submit" class="coolbutton" value="Commit Changes" />