Fix screenshot by frankiejol · Pull Request #2292 · UPC/ravada

frankiejol · 2026-02-17T10:24:38Z

Fixed call to screenshot request, added security checks and improved text

also show screenshot as soon as it appears

Copilot

Pull request overview

This PR fixes the VM screenshot/copy-screenshot request wiring across the UI, backend controller, and test suite, aiming to ensure screenshot requests are properly attributed to a user and guarded by admin checks where appropriate.

Changes:

Pass uid (and _force) into screenshot and copy-screenshot backend requests; add an explicit admin check for copy-screenshot.
Update the screenshot UI to show/hide the copy button based on screenshot availability and add a spinner during screenshot capture.
Refactor test VM creation helpers and add a new screenshot/copy-screenshot test.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
templates/main/vm_screenshot.html.ep	Adjusts screenshot/copy UI visibility and adds a waiting spinner/message gating.
public/js/ravada.js	Switches screenshot UI feedback to a `screenshot_waiting` flag and clears current screenshot on request.
script/rvd_front	Includes `uid`/`_force` in screenshot-related requests; adds admin enforcement for copy-screenshot.
t/mojo/10_login.t	Refactors domain creation/wait helpers and adds a new screenshot/copy-screenshot test.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-17T10:36:48Z

public/js/ravada.js

+              $scope.screenshot_waiting=true;
                  $http.get('/machine/screenshot/'+machineId+'.json');
+              $scope.showmachine.screenshot='';


$scope.screenshot_waiting is set to true when requesting a screenshot, but it is never set back to false (and is not updated anywhere else in this file). This can leave the UI stuck in the "waiting" state on failures/timeouts and also prevents the "need a screenshot" message from ever appearing again. Consider resetting it in the $http.get(...).then/.catch handlers and/or when the screenshot field is refreshed via websocket/polling.

Suggested change

$scope.screenshot_waiting=true;

$http.get('/machine/screenshot/'+machineId+'.json');

$scope.showmachine.screenshot='';

$scope.screenshot_waiting = true;

$scope.showmachine.screenshot = '';

$http.get('/machine/screenshot/' + machineId + '.json')

.then(function(response) {

$scope.screenshot_waiting = false;

})

.catch(function(error) {

$scope.screenshot_waiting = false;

});

Copilot · 2026-02-17T10:36:48Z

public/js/ravada.js

+              $scope.screenshot_waiting=true;
                  $http.get('/machine/screenshot/'+machineId+'.json');
+              $scope.showmachine.screenshot='';


reload_page_msg is still referenced by the template to show the "Saving the screenshot..." message, but the screenshot action no longer sets it to true anywhere (only initializes it to false). Either remove this unused flag/message or reintroduce consistent state updates so the user gets feedback while the screenshot request is in progress.

Suggested change

$scope.screenshot_waiting=true;

$http.get('/machine/screenshot/'+machineId+'.json');

$scope.showmachine.screenshot='';

$scope.screenshot_waiting = true;

$scope.reload_page_msg = true;

$scope.fail_page_msg = false;

$http.get('/machine/screenshot/' + machineId + '.json')

.then(function(response) {

$scope.screenshot_waiting = false;

$scope.reload_page_msg = false;

}, function(error) {

$scope.screenshot_waiting = false;

$scope.reload_page_msg = false;

$scope.fail_page_msg = true;

});

$scope.showmachine.screenshot = '';

Copilot · 2026-02-17T10:36:48Z

t/mojo/10_login.t

+sub test_new_machine_default($t, $vm_name, $empty_iso_file=undef) {
+
+    my $domain = _new_machine($t, $vm_name, $empty_iso_file);
+    die if !$domain;


The test uses die if !$domain; which produces an unhelpful failure (no context about which VM type/name failed). Prefer an ok($domain, ...)/BAIL_OUT(...) with a message (and ideally include $vm_name and $name) so CI failures are actionable.

Suggested change

die if !$domain;

ok($domain, "Expected domain to be created for backend '$vm_name'")

or BAIL_OUT("Failed to create domain for backend '$vm_name'");

Copilot · 2026-02-17T10:36:49Z

t/mojo/10_login.t

+    $t->get_ok("/machine/screenshot/".$domain->id.".json")->status_is(200);
+    $t->get_ok("/machine/copy_screenshot/".$domain->id.".json")->status_is(200);


test_screenshot only checks that the endpoints return HTTP 200, but it doesn't verify that the returned request actually completes successfully (or that a screenshot exists before attempting copy_screenshot). This can make the test pass even if the backend request fails asynchronously. Consider using the existing request-waiting helper (e.g. mojo_request_url(...) / _wait_mojo_request(...)) and asserting request status=done with empty error, plus waiting for the screenshot request to finish before triggering the copy.

Suggested change

$t->get_ok("/machine/screenshot/".$domain->id.".json")->status_is(200);

$t->get_ok("/machine/copy_screenshot/".$domain->id.".json")->status_is(200);

# request screenshot and wait for backend request to complete successfully

my $screenshot_req = mojo_request_url($t, "/machine/screenshot/" . $domain->id . ".json");

my $screenshot_done = _wait_mojo_request($screenshot_req);

is($screenshot_done->{status}, 'done', 'screenshot request finished successfully');

is($screenshot_done->{error}, '', 'screenshot request has no error');

# only after screenshot is ready, copy it and wait for completion

my $copy_req = mojo_request_url($t, "/machine/copy_screenshot/" . $domain->id . ".json");

my $copy_done = _wait_mojo_request($copy_req);

is($copy_done->{status}, 'done', 'copy_screenshot request finished successfully');

is($copy_done->{error}, '', 'copy_screenshot request has no error');

Copilot · 2026-02-17T10:36:49Z

script/rvd_front

    my $req = Ravada::Request->screenshot(
        id_domain => $domain->id
+        ,uid => $USER->id
+        ,_force => 1
    );
    $c->render(json => { request => $req->id});
 }

 sub copy_screenshot {
    my $c = shift;
    return login($c)    if !_logged_in($c);
+    return access_denied_json($c) if !$USER->is_admin;

    my $domain = _search_requested_machine($c);

    my $req = Ravada::Request->copy_screenshot (
        id_domain => $domain->id
+        ,uid => $USER->id
+        ,_force => 1
    );


Both screenshot endpoints now pass _force => 1, which bypasses request de-duplication / done-recently checks in Ravada::Request. This makes it easier to spam screenshot/copy requests (potentially expensive) and can have operational impact. If forcing is necessary, consider limiting it (e.g., only for admins, or only when explicitly requested) or adding some throttling/rate limiting at the controller level.

issue #2291

frankiejol added 5 commits February 16, 2026 16:46

wip: call request with uid

d3662c8

also show screenshot as soon as it appears

wip: removed check for already done

c031810

wip: hide button, fixed typo

b48ef4d

issue #2291

wip: removed duplicated unsetting

0ea9ba9

issue #2291

wip: fixed wrong quote

f2500e5

issue #2291

frankiejol added this to the v2.4.5 milestone Feb 17, 2026

Copilot AI review requested due to automatic review settings February 17, 2026 10:24

Copilot started reviewing on behalf of frankiejol February 17, 2026 10:25 View session

Copilot AI reviewed Feb 17, 2026

View reviewed changes

frankiejol added 3 commits February 17, 2026 12:58

wip(test): check the screenshot

b7b80cc

issue #2291

wip: force screenshot for operators

cbe91ff

issue #2291

wip: reset screenshot waiting if defined

d23584a

issue #2291

frankiejol merged commit 463a328 into main Feb 18, 2026
0 of 2 checks passed

frankiejol deleted the fix/2291_screenshot branch February 18, 2026 08:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix screenshot#2292

Fix screenshot#2292
frankiejol merged 8 commits intomainfrom
fix/2291_screenshot

frankiejol commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Copilot AI Feb 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

-              $scope.screenshot_waiting=true;
-                  $http.get('/machine/screenshot/'+machineId+'.json');
-              $scope.showmachine.screenshot='';
+              $scope.screenshot_waiting = true;
+              $scope.showmachine.screenshot = '';
+              $http.get('/machine/screenshot/' + machineId + '.json')
+                  .then(function(response) {
+                      $scope.screenshot_waiting = false;
+                  })
+                  .catch(function(error) {
+                      $scope.screenshot_waiting = false;
+                  });

	die if !$domain;
	ok($domain, "Expected domain to be created for backend '$vm_name'")
	or BAIL_OUT("Failed to create domain for backend '$vm_name'");

		$t->get_ok("/machine/screenshot/".$domain->id.".json")->status_is(200);
		$t->get_ok("/machine/copy_screenshot/".$domain->id.".json")->status_is(200);

-    $t->get_ok("/machine/screenshot/".$domain->id.".json")->status_is(200);
-    $t->get_ok("/machine/copy_screenshot/".$domain->id.".json")->status_is(200);
+    # request screenshot and wait for backend request to complete successfully
+    my $screenshot_req = mojo_request_url($t, "/machine/screenshot/" . $domain->id . ".json");
+    my $screenshot_done = _wait_mojo_request($screenshot_req);
+    is($screenshot_done->{status}, 'done', 'screenshot request finished successfully');
+    is($screenshot_done->{error},  '',     'screenshot request has no error');
+    # only after screenshot is ready, copy it and wait for completion
+    my $copy_req = mojo_request_url($t, "/machine/copy_screenshot/" . $domain->id . ".json");
+    my $copy_done = _wait_mojo_request($copy_req);
+    is($copy_done->{status}, 'done', 'copy_screenshot request finished successfully');
+    is($copy_done->{error},  '',     'copy_screenshot request has no error');

Conversation

frankiejol commented Feb 17, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 17, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants