Fix guest screenshots: a capture named 'domain' never survived routing

The console images were broken because the guest name never reached the
handler. TPSGI applies a route's captures in extract_query and then overwrites
$query->{domain} with the request's own host, so the screenshot route was
asking libvirt for a guest called 'dux.troglodyne.net:5000' and getting the
'bad domain name' rejection every time.

/guest/act read the same field, so the power, snapshot and destroy buttons had
the same defect: they were aiming at a guest named after the web host. They
failed closed rather than acting on the wrong guest, which is the only reason
this is a bug report and not an incident. Both now use 'guest', and a test
asserts neither the route's captures nor the action form's field names collide
with the keys the router sets for itself.

Two further things found while fixing it:

The cache path ignored the hypervisor, so two hosts each with a guest called
'staging' -- which is the normal state of affairs, not a corner case -- would
have shown each other's console. It is keyed by hypervisor id now, and that id
is validated, since it is part of a path.

The cache went under www/assets, which is served statically with no
authentication: anyone who guessed the path could read a console capture, and
those show login prompts and whatever the last command printed. It goes under
www/assets/private now, which is where _handle_upload already puts a private
post's assets. The screenshot route serves them to admins itself and does not
depend on how www/assets happens to be exposed.

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

lib/Trog/DataSource/Virt.pm

@@ -40,7 +40,16 @@ is reached only from an explicit admin-gated POST.  Keep it that way.
 # captures, so they change slowly and are cheap to refetch.
 our $screenshot_ttl = 30;
 
-our $screenshot_dir = 'www/assets/guests';
+# Under www/assets like every other asset, in its own subtree because these are
+# not uploads somebody made -- they are cached datasource output, and anything
+# in here can be deleted at any time and will simply be refetched.
+#
+# Under private/ specifically because a console capture can show a login
+# prompt, a hostname, whatever the last command printed.  That is the same
+# reasoning _handle_upload uses to put a private post's assets there, and the
+# screenshot route serves these to admins itself rather than relying on
+# anything about how www/assets is exposed.
+our $screenshot_dir = 'www/assets/private/datasource/virt';
 
 # Reconnecting per guest would be absurd -- one ssh handshake per hypervisor
 # per process is enough.  Not a state var: a connection that has gone away
@@ -173,7 +182,7 @@ sub _unreachable_post ( $hypervisor, $series, $form, $why ) {
     };
 }
 
-=head2 screenshot($conn_uri, $domain_name) = ($path, $error)
+=head2 screenshot($hypervisor, $guest_name) = ($path, $error)
 
 Path to a reasonably fresh PNG of the guest's console.
 
@@ -181,13 +190,24 @@ Cached on disk, because a page of guests means one of these per guest and they
 are only interesting to about the nearest half minute.  libvirt hands back the
 mimetype; qemu gives us PNG already, so there is nothing to convert.
 
+Keyed by hypervisor as well as by guest -- guest names are only unique within
+one hypervisor, and two of them called 'staging' would otherwise show each
+other's console.
+
 =cut
 
-sub screenshot ( $conn_uri, $domain_name ) {
-    my ($safe) = $domain_name =~ m/^([A-Za-z0-9._-]+)$/;
-    return ( undef, 'bad domain name' ) unless $safe;
+sub screenshot ( $hypervisor, $guest_name ) {
+    my ($safe) = ( $guest_name // '' ) =~ m/^([A-Za-z0-9._-]+)$/;
+    return ( undef, 'bad guest name' ) unless $safe;
+
+    my ($host) = ( $hypervisor->{id} // '' ) =~ m/^([A-Za-z0-9._-]+)$/;
+    return ( undef, 'bad hypervisor id' ) unless $host;
+
+    my $conn_uri = $hypervisor->{conn_uri};
+    return ( undef, 'hypervisor has no connection uri' ) unless $conn_uri;
 
-    my $path = "$screenshot_dir/$safe.png";
+    my $dir  = "$screenshot_dir/$host";
+    my $path = "$dir/$safe.png";
     return ( $path, undef ) if -f $path && ( time() - ( stat($path) )[9] ) < $screenshot_ttl;
 
     my ( $conn, $why ) = _connect($conn_uri);
@@ -212,7 +232,7 @@ sub screenshot ( $conn_uri, $domain_name ) {
         return ( undef, $err );
     }
 
-    File::Path::make_path($screenshot_dir);
+    File::Path::make_path($dir);
     open( my $fh, '>', $path ) or return ( undef, "could not cache the screenshot: $!" );
     binmode $fh;
     print {$fh} $data;

lib/Trog/Routes/HTML.pm

@@ -168,7 +168,11 @@ our %routes = (
         method   => 'GET',
         auth     => 1,
         callback => \&Trog::Routes::HTML::guest_screenshot,
-        captures => [qw{hypervisor domain}],
+
+        # 'guest', not 'domain': the router sets $query->{domain} to the
+        # request's own host *after* it applies captures, so a capture by that
+        # name never survives to the callback.
+        captures => [qw{hypervisor guest}],
         noindex  => 1,
         nocache  => 1,
     },
@@ -2044,9 +2048,9 @@ sub guest_screenshot ($query) {
     return $query->{tpsgi}->notfound($query) unless $hypervisor;
 
     require Trog::DataSource::Virt;
-    my ( $path, $why ) = Trog::DataSource::Virt::screenshot( $hypervisor->{conn_uri}, $query->{domain} );
+    my ( $path, $why ) = Trog::DataSource::Virt::screenshot( $hypervisor, $query->{guest} );
     if ( !$path ) {
-        WARN("Could not screenshot '$query->{domain}': $why");
+        WARN("Could not screenshot '$query->{guest}': $why");
         return $query->{tpsgi}->notfound($query);
     }
 
@@ -2076,11 +2080,13 @@ sub guest_act ($query) {
     return _feedback_redirect( $query, $to, 1, 'No such hypervisor.' ) unless $hypervisor;
 
     require Trog::DataSource::Virt;
+    # Same reason the screenshot route captures 'guest': a form field called
+    # 'domain' would be overwritten by the router with the request's host.
     my ( $ok, $message ) = Trog::DataSource::Virt::act(
-        $query->{action}, $hypervisor->{conn_uri}, $query->{domain}, $query->{user},
+        $query->{action}, $hypervisor->{conn_uri}, $query->{guest}, $query->{user},
     );
 
-    return _feedback_redirect( $query, $to, $ok ? 0 : 1, "$query->{domain}: $message" );
+    return _feedback_redirect( $query, $to, $ok ? 0 : 1, "$query->{guest}: $message" );
 }
 
 # The hypervisor post a guest route was asked about.  Looked up rather than

t/Trog-DataSource-Virt.t

@@ -34,11 +34,22 @@ our @CALLS;
     sub shutdown      { push @main::CALLS, [ 'shutdown',      $_[0]{name} ]; 1 }
     sub destroy       { push @main::CALLS, [ 'destroy',       $_[0]{name} ]; 1 }
     sub create_snapshot { push @main::CALLS, [ 'create_snapshot', $_[0]{name} ]; 1 }
+
+    # Reading a console is not a mutation, so this one really runs.
+    sub screenshot { return 'image/png' }
+}
+
+{
+    package FakeStream;
+    sub new { bless {}, shift }
+    sub recv_all { my ( $self, $cb ) = @_; $cb->( $self, 'fake-png-bytes' ); return 1 }
+    sub finish   { 1 }
 }
 
 {
     package FakeConn;
     sub new { bless {}, shift }
+    sub new_stream { FakeStream->new() }
     sub list_all_domains {
         return (
             FakeDomain->new( name => 'alpha', uuid => 'uuid-alpha', active => 1, state => 1 ),
@@ -138,13 +149,39 @@ subtest 'act() refuses what it should' => sub {
     is_deeply( \@CALLS, [], 'having done nothing' );
 };
 
-subtest 'screenshot() validates the guest name before anything else' => sub {
-    my ( $path, $why ) = Trog::DataSource::Virt::screenshot( 'test:///default', '../../../etc/passwd' );
+subtest 'screenshot() validates its inputs before anything else' => sub {
+    my $hv = { id => 'hv-1', conn_uri => 'test:///default' };
+
+    my ( $path, $why ) = Trog::DataSource::Virt::screenshot( $hv, '../../../etc/passwd' );
     is( $path, undef, 'a path-shaped guest name gets no screenshot' );
-    like( $why, qr/bad domain name/, 'and is named as the reason' );
+    like( $why, qr/bad guest name/, 'and is named as the reason' );
 
-    ( $path, $why ) = Trog::DataSource::Virt::screenshot( 'test:///default', 'alpha/../../beta' );
+    ( $path, $why ) = Trog::DataSource::Virt::screenshot( $hv, 'alpha/../../beta' );
     is( $path, undef, 'nor does one with traversal in the middle' );
+
+    # The cache path is built from the hypervisor id too, so it has to be
+    # checked as well -- otherwise it is a directory traversal of its own.
+    ( $path, $why ) = Trog::DataSource::Virt::screenshot( { id => '../../..', conn_uri => 'test:///default' }, 'alpha' );
+    is( $path, undef, 'nor does a path-shaped hypervisor id' );
+    like( $why, qr/bad hypervisor id/, 'which is reported separately' );
+};
+
+subtest 'the guest cache is scoped per hypervisor' => sub {
+
+    # Two hypervisors, each with a guest called 'alpha' -- which is the normal
+    # state of affairs, not a corner case.
+    my $root = Path::Tiny->tempdir();
+    local $Trog::DataSource::Virt::screenshot_dir = "$root";
+
+    my @paths;
+    foreach my $id (qw{hv-1 hv-2}) {
+        my ( $path, $why ) = Trog::DataSource::Virt::screenshot( { id => $id, conn_uri => 'test:///default' }, 'alpha' );
+        ok( $path, "screenshot taken for $id" ) or diag($why);
+        push( @paths, $path );
+    }
+
+    isnt( $paths[0], $paths[1], "two hypervisors' guests do not share a cache entry" );
+    like( $paths[0], qr{/hv-1/alpha\.png$}, 'the path names the hypervisor and the guest' );
 };
 
 subtest 'the datasource hook only loads what it should' => sub {
@@ -184,4 +221,28 @@ subtest 'the datasource hook only loads what it should' => sub {
     is( $got[0]{title}, 'alpha', 'with what it built' );
 };
 
+subtest 'the guest routes avoid the router\'s own query keys' => sub {
+    require Trog::Routes::HTML;
+
+    # TPSGI applies a route's captures in extract_query, and *then* overwrites
+    # $query->{domain} with the request's own host.  A capture or form field by
+    # that name therefore never reaches the callback -- which silently aimed
+    # the screenshot and action routes at a guest named after the web host.
+    my @reserved = qw{domain route method scheme port user acls to};
+
+    my $captures = $Trog::Routes::HTML::routes{'/guest/screenshot/(.*)/(.*)'}{captures};
+    foreach my $capture (@$captures) {
+        ok( !( grep { $_ eq $capture } @reserved ), "the screenshot route's '$capture' capture is not a router key" );
+    }
+
+    # The action form posts the same field names the handler reads, so check
+    # the template rather than trusting that they still agree.
+    my $form = Path::Tiny->new("$FindBin::Bin/../www/templates/html/components/forms/guests.tx")->slurp_utf8;
+    foreach my $reserved (@reserved) {
+        unlike( $form, qr/name="\Q$reserved\E"/, "the guest action form does not post a field called '$reserved'" )
+          unless $reserved eq 'to';    # 'to' is the redirect target, and is meant to be the router's
+    }
+    like( $form, qr/name="guest"/, 'it posts the guest under a name that survives routing' );
+};
+
 done_testing();

www/templates/html/components/forms/guests.tx

@@ -18,7 +18,7 @@
                 <br />
                 <form class="Submissions guest-actions" action="/guest/act" method="POST">
                     <input type="hidden" name="hypervisor" value="<: $post.hypervisor :>" />
-                    <input type="hidden" name="domain" value="<: $post.domain :>" />
+                    <input type="hidden" name="guest" value="<: $post.domain :>" />
                     <input type="hidden" name="to" value="<: $route :>" />
                     : if ( $post.is_active ) {
                     <button class="coolbutton" type="submit" name="action" value="poweroff">Power Off</button>