feat(diagnostics): list and retrieve retained log files by MederickDevo · Pull Request #2185 · Devolutions/devolutions-gateway

Médérick Daireaux (MederickDevo) · 2026-11-04T20:56:14Z

Summary

Add GET /jet/diagnostics/logs/files to list retained Gateway log files with size and modification time.
Add GET /jet/diagnostics/logs/files/{name} to retrieve a listed file, restricted to the configured log directory and Gateway log prefix.
Document both endpoints in OpenAPI. Both require gateway.diagnostics.read.

Jira: https://devolutions.atlassian.net/browse/DVLS-15665

Validation

cargo fmt --all -- --check
cargo check -p devolutions-gateway --features openapi
cargo test -p devolutions-log --lib (passes; this crate currently has zero library tests)

Issue: DVLS-15665

github-actions · 2026-11-04T20:56:28Z

Let maintainers know that an action is required on their side

Add the label release-required Please cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module) when you request a maintainer to cut a new release (Devolutions Gateway, Devolutions Agent, Jetsocat, PowerShell module)
Add the label release-blocker Follow-up is required before cutting a new release if a follow-up is required before cutting a new release
Add the label publish-required Please publish libraries (`Devolutions.Gateway.Utils`, OpenAPI clients, etc) when you request a maintainer to publish libraries (Devolutions.Gateway.Utils, OpenAPI clients, etc.)
Add the label publish-blocker Follow-up is required before publishing libraries if a follow-up is required before publishing libraries

Copilot

🟡 Changes recommended

A critical check-to-open race in log retrieval allows a local process with write access to the log directory to substitute a symlink and expose files outside that directory using the Gateway service’s permissions.

1 open issue

Prevent symlink swaps when opening log files · New

What changed in this PR

Adds retained-log discovery and download endpoints to Gateway diagnostics, both requiring gateway.diagnostics.read.

Changes:

Lists retained log files with their names, sizes, and modification times.
Streams a selected log file by its listed name.
Registers both endpoints and their response schema in OpenAPI.

File	Description
devolutions-gateway/src/openapi.rs	Registers the new diagnostics operations and schema.
devolutions-gateway/src/api/diagnostics.rs	Adds authenticated log listing and download handlers.
devolutions-gateway/openapi/gateway-api.yaml	Documents both endpoints and log-file metadata.
crates/devolutions-log/src/lib.rs	Enumerates matching regular log files and sorts them by modification time.

_{Review effort: Balanced}

Issue: DVLS-15665 Security: yes

Issue: DVLS-15665 Changelog: ignore

Issue: DVLS-15665

irvingouj@Devolutions (irvingoujAtDevolution)

Couple of things that go against our AGENTS.md rules, details inline.

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:06:28Z

+mod tests {
+    use super::open_regular_log_file;
+
+    // oracle: regression Devolutions/devolutions-gateway#2185 review comment 4473309953.


regression has to point at a bug report (Jira key, GitHub issue) or a fuzz crash file per test-oracle-reviewer. A review comment on this PR isn't one, and AGENTS.md says committed text states rationale, not history. Same for line 350, and that second test isn't really a regression test anyway.

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:06:28Z

+    options.read(true);
+
+    #[cfg(unix)]
+    options.custom_flags(libc::O_NOFOLLOW | libc::O_NONBLOCK);


Nothing tests that O_NONBLOCK keeps a FIFO from hanging the open, so per AGENTS.md (Untested-property comments) this needs an UNTESTED: comment saying why it's there and what to check when touching it.

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:06:28Z

+        std::os::unix::fs::symlink(&outside, &selected_log).expect("replace log with symlink");
+        #[cfg(windows)]
+        if let Err(error) = std::os::windows::fs::symlink_file(&outside, &selected_log) {
+            if error.raw_os_error() == Some(1314) {


When symlink creation fails with 1314 (ERROR_PRIVILEGE_NOT_HELD) the test returns and passes without asserting anything, so the Windows reparse-point check is never exercised there. Either make sure it runs somewhere or mark that check UNTESTED: per AGENTS.md. Named constant instead of 1314 would be nice too.

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution)

A few more, mostly non-blocking.

note: ListLogFiles and GetLogFile aren't in the generated clients yet (openapi/dotnet-client etc.). Fine to leave it for a chore(openapi) follow-up, unless DVLS needs Devolutions.Gateway.Client for this.

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:08:02Z

+        .map_err(HttpError::internal().err())?;
+    let file = files
+        .into_iter()
+        .find(|file| file.name == name)


praise: Nice, only serving an exact name from the listing means ../whatever just can't match, no path sanitizing needed. And checking the opened handle instead of the path closes the swap race properly 👍

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:08:02Z

+            continue;
+        };
+
+        if !name.starts_with(cfg.prefix) || !name.ends_with(".log") {


question: find_latest_log_file uses contains("log") while this uses ends_with(".log"), so /logs and /logs/files can disagree on what a log file is. Also get_logs still does a plain File::open that follows symlinks. Could get_logs just take the first entry of list_log_files and go through open_regular_log_file? Then we have one definition and the same hardening on both. Can be a follow up!

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:08:02Z

+    if !metadata.is_file() {
+        return Err(io::Error::new(
+            io::ErrorKind::InvalidData,
+            "log path is not a regular file",


suggestion: InvalidData ends up as a 500 here, and the OpenAPI doc says 500 is Failed to read log file. A symlink or non-regular file isn't really a server failure, from the caller's point of view it's not a log file we serve. What about mapping it to 404 like the not-listed case? Same for the reparse point one below.

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:08:02Z

+        right
+            .modified
+            .cmp(&left.modified)
+            .then_with(|| right.name.cmp(&left.name))


suggestion (non-blocking): the tie-break compares names as strings, so in descending order gateway.2026-11-09.9.log comes before gateway.2026-11-09.10.log even though 10 is newer. Only matters when mtimes are equal, so feel free to ignore.

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:08:03Z

+                modified_unix_millis: file
+                    .modified
+                    .duration_since(UNIX_EPOCH)
+                    .unwrap_or_default()


nit: unwrap_or_default() and unwrap_or(u64::MAX) silently turn odd timestamps into 0 / max. ClockDiagnostic right above uses i64 millis from the time crate, I would rather stay consistent with that (OffsetDateTime::from(file.modified)), which also removes both fallbacks.

LLM Wrote, Unaudited — Read With a Grain of Salt.

irvingouj@Devolutions (irvingoujAtDevolution) · 2026-11-09T18:08:03Z

    const LOG_FILE_PREFIX: &'static str;
 }

+pub struct LogFile {


nit: missing a doc comment on this new public type. Also the rest of this crate's API speaks Utf8Path, while path here is a std::path::PathBuf. Not a blocker.

LLM Wrote, Unaudited — Read With a Grain of Salt.

The latest-log route could select files outside the retained log filter and follow a swapped symlink. Share retained-file selection and guarded opening across both routes, return 404 for unavailable files, preserve signed modification times, and order numeric rotations correctly when timestamps match. Issue: DVLS-15665

Marc-André Moreau (mamoreau-devolutions) · 2026-11-09T18:39:42Z

+))]
+async fn get_log_file(
+    State(DgwState { conf_handle, .. }): State<DgwState>,
+    Path(name): Path<String>,


Could you validate the diagnostics scope before extracting the path? An invalid UTF-8 path segment (for example %FF) can fail extraction with 400 before the scope check, and that 400 is also missing from the OpenAPI responses. Please move the scope extractor before the path extractor and document the 400 response if it remains possible.

Note
Human-tuned, LLM-assisted content.

Marc-André Moreau (mamoreau-devolutions) · 2026-11-09T18:39:42Z

+    ),
+    security(("scope_token" = ["gateway.diagnostics.read"])),
+))]
+async fn list_log_files(


Could you add tests for the HTTP endpoints themselves? The new tests cover log-file selection and opening, but do not exercise either route's response body/status or diagnostics-scope enforcement. A route-level test would catch regressions in the listing/download contract and access control.

Note
Human-tuned, LLM-assisted content.

Marc-André Moreau (mamoreau-devolutions) · 2026-11-09T18:39:42Z

+        (status = 200, description = "Log file contents", body = String, content_type = "text/plain"),
+        (status = 401, description = "Invalid or missing authorization token"),
+        (status = 403, description = "Insufficient permissions"),
+        (status = 404, description = "Log file is no longer available"),


Could you change this description to Log file not found? The endpoint returns 404 both when a requested name is not in the retained-file list and when a listed file disappears before it is opened, so no longer available describes only one case.

Note
Human-tuned, LLM-assisted content.

Médérick Daireaux (MederickDevo) · 2026-11-09T18:50:57Z

UNTESTED: properties in the current change:

On Unix, O_NONBLOCK must keep diagnostics responsive if a FIFO replaces a listed log file before it is opened. The current tests do not perform a FIFO replacement. Exercise that race before changing the open flags.
On Windows, a reparse-point file must not be streamed. The symlink test can skip when the host lacks symlink privilege, so it does not prove this property on every test host. Exercise a file symlink on a Windows host that can create one before changing the reparse-point check.

Malformed log names could fail extraction before diagnostics scope validation. Check the scope first, document the possible bad request and both not-found cases, and exercise the list and download contract through the shipped Gateway HTTP routes. Issue: DVLS-15665 Security: yes

feat(diagnostics): expose retained Gateway log files

eb5ac9c

Issue: DVLS-15665

Copilot AI balanced review requested due to automatic review settings November 4, 2026 20:56

Copilot started reviewing on behalf of Médérick Daireaux (MederickDevo) November 4, 2026 20:56

View session

Copilot AI reviewed Nov 4, 2026

View reviewed changes

Comment thread devolutions-gateway/src/api/diagnostics.rs Resolved

Médérick Daireaux (MederickDevo) added 3 commits November 5, 2026 14:39

fix(diagnostics): reject linked log paths on open

8e155a1

Issue: DVLS-15665 Security: yes

style(diagnostics): satisfy nightly import formatting

5d1f75b

Issue: DVLS-15665 Changelog: ignore

fix: avoid stderr in symlink regression test

9cfee51

Issue: DVLS-15665

irvingouj@Devolutions (irvingoujAtDevolution) reviewed Nov 9, 2026

View reviewed changes

Marc-André Moreau (mamoreau-devolutions) reviewed Nov 9, 2026

View reviewed changes

Marc-André Moreau (mamoreau-devolutions) approved these changes Nov 9, 2026

View reviewed changes

Marc-André Moreau (mamoreau-devolutions) merged commit 3a21b56 into master Nov 9, 2026
53 checks passed

Marc-André Moreau (mamoreau-devolutions) deleted the feature/DVLS-15665-gateway-log-files branch November 9, 2026 21:34

Conversation

Médérick Daireaux (MederickDevo) commented Nov 4, 2026

Summary

Validation

Uh oh!

github-actions Bot commented Nov 4, 2026

Let maintainers know that an action is required on their side

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

irvingouj@Devolutions (irvingoujAtDevolution) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

irvingouj@Devolutions (irvingoujAtDevolution) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

irvingouj@Devolutions (irvingoujAtDevolution) Nov 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Médérick Daireaux (MederickDevo) commented Nov 9, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Milestone

Development

Uh oh!

4 participants

irvingouj@Devolutions (irvingoujAtDevolution) Nov 9, 2026 •

edited

Loading