Skip to content

fix(agent): improve productinfo.json parsing resilience #1594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix(agent): improve productinfo.json parsing resilience #1594

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
166 changes: 159 additions & 7 deletions devolutions-agent/src/updater/productinfo/db.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,13 @@
use serde::{Deserialize, Serialize};
use std::collections::HashMap;
use std::str::FromStr;
use tracing::warn;

use crate::updater::UpdaterError;

/// Information about a product file available for download
#[derive(Debug, Clone, Deserialize, Serialize)]
#[serde(deny_unknown_fields)]
Copy link
Member

@CBenoit CBenoit Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Does we need to make it stricter here?

pub(crate) struct ProductFile {
#[serde(rename = "Arch")]
pub arch: String,
Expand All @@ -24,8 +26,15 @@ pub(crate) struct ProductFile {
pub(crate) struct ChannelData {
#[serde(rename = "Version")]
pub version: String,
#[serde(rename = "Date", skip_serializing_if = "Option::is_none")]
pub date: Option<String>,
#[serde(rename = "Files")]
pub files: Vec<ProductFile>,
// Allow unknown fields at channel level for forward compatibility.
// New marketing fields or metadata won't break parsing.
#[serde(flatten)]
#[serde(skip_serializing)]
pub _other: HashMap<String, serde_json::Value>,
Comment on lines +33 to +37
Copy link
Member

@CBenoit CBenoit Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: I suppose it’s to keep information when performing round-trip encoding?

}

/// Product information containing multiple channels
Expand All @@ -39,6 +48,10 @@ pub(crate) struct ProductData {
pub update: Option<ChannelData>,
#[serde(rename = "Stable")]
pub stable: Option<ChannelData>,
// Allow unknown fields at product level for forward compatibility
#[serde(flatten)]
#[serde(skip_serializing)]
pub _other: HashMap<String, serde_json::Value>,
}

#[derive(Debug, Clone, Default)]
Expand Down Expand Up @@ -80,8 +93,11 @@ impl FromStr for ProductInfoDb {
type Err = UpdaterError;

fn from_str(s: &str) -> Result<Self, Self::Err> {
// Parse the JSON content
let json: serde_json::Value = serde_json::from_str(s).map_err(|_| UpdaterError::ProductInfo)?;
// Parse the JSON content with better error context
let json: serde_json::Value = serde_json::from_str(s).map_err(|e| {
warn!(%e, "Failed to parse productinfo.json as valid JSON");
UpdaterError::ProductInfo
})?;

let mut records = HashMap::new();
let target_arch = get_target_arch();
Expand All @@ -90,15 +106,44 @@ impl FromStr for ProductInfoDb {
if let Some(obj) = json.as_object() {
for (product_name, product_value) in obj {
// Try to deserialize the product data
let product_data: ProductData =
serde_json::from_value(product_value.clone()).map_err(|_| UpdaterError::ProductInfo)?;
let product_data: ProductData = serde_json::from_value(product_value.clone()).map_err(|e| {
warn!(%product_name, %e, "Failed to deserialize product data");
UpdaterError::ProductInfo
})?;

// Use Current channel for now (as specified)
let channel = product_data.current.ok_or(UpdaterError::ProductInfo)?;
let channel = product_data.current.ok_or_else(|| {
warn!(%product_name, "Product is missing 'Current' channel");
UpdaterError::ProductInfo
})?;

// Validate that we have files
if channel.files.is_empty() {
warn!(%product_name, "Product Current channel has no files");
return Err(UpdaterError::ProductInfo);
}

// Select the appropriate file based on architecture and type (msi)
let selected_file =
select_file(&channel.files, &target_arch, "msi").ok_or(UpdaterError::ProductInfo)?;
let selected_file = select_file(&channel.files, &target_arch, "msi").ok_or_else(|| {
warn!(
%product_name,
%target_arch,
available_archs = ?channel.files.iter().map(|f| &f.arch).collect::<Vec<_>>(),
"No MSI file found for target architecture"
);
UpdaterError::ProductInfo
})?;

// Basic validation of the selected file
if selected_file.url.is_empty() {
warn!(%product_name, "Selected file has empty URL");
return Err(UpdaterError::ProductInfo);
}

if selected_file.hash.is_empty() {
warn!(%product_name, "Selected file has empty hash");
return Err(UpdaterError::ProductInfo);
}

let product_info = ProductInfo {
version: channel.version.clone(),
Expand Down Expand Up @@ -150,4 +195,111 @@ mod tests {
Some("72D7A836A6AF221D4E7631D27B91A358915CF985AA544CC0F7F5612B85E989AA")
);
}

#[test]
fn test_productinfo_parse_with_date_field() {
// Test that the Date field is properly handled (ignoring or parsing it)
let input = r#"{
"Gateway": {
"Current": {
"Version": "2025.3.2.0",
"Date": "2025-10-10",
"Files": [
{
"Arch": "x64",
"Type": "msi",
"Url": "https://cdn.devolutions.net/download/DevolutionsGateway-x86_64-2025.3.2.0.msi",
"Hash": "9670B9B7D8B4D145708EE5F7F1F7053111E620541D67CFA04CF711065C4C3B27"
}
]
}
}
}"#;

let db: ProductInfoDb = input
.parse()
.expect("failed to parse product info database with Date field");

assert_eq!(db.get("Gateway").expect("product not found").version, "2025.3.2.0");
assert_eq!(
db.get("Gateway").expect("product not found").url,
"https://cdn.devolutions.net/download/DevolutionsGateway-x86_64-2025.3.2.0.msi"
);
assert_eq!(
db.get("Gateway").expect("product not found").hash.as_deref(),
Some("9670B9B7D8B4D145708EE5F7F1F7053111E620541D67CFA04CF711065C4C3B27")
);
}

#[test]
fn test_productinfo_parse_with_unknown_fields() {
// Test forward compatibility - new fields at product and channel level shouldn't break parsing
let input = r#"{
"Gateway": {
"NewMarketingField": "some value",
"Current": {
"Version": "2025.3.2.0",
"Date": "2025-10-10",
"ReleaseNotes": "https://example.com/notes",
"Files": [
{
"Arch": "x64",
"Type": "msi",
"Url": "https://cdn.devolutions.net/download/DevolutionsGateway-x86_64-2025.3.2.0.msi",
"Hash": "9670B9B7D8B4D145708EE5F7F1F7053111E620541D67CFA04CF711065C4C3B27"
}
]
}
}
}"#;

let db: ProductInfoDb = input
.parse()
.expect("failed to parse product info database with unknown fields");

assert_eq!(db.get("Gateway").expect("product not found").version, "2025.3.2.0");
}

#[test]
fn test_productinfo_parse_validation() {
// Test that empty URLs are rejected
let input = r#"{
"Gateway": {
"Current": {
"Version": "2025.3.2.0",
"Files": [
{
"Arch": "x64",
"Type": "msi",
"Url": "",
"Hash": "9670B9B7D8B4D145708EE5F7F1F7053111E620541D67CFA04CF711065C4C3B27"
}
]
}
}
}"#;

let result: Result<ProductInfoDb, _> = input.parse();
assert!(result.is_err(), "Should reject empty URL");

// Test that empty hashes are rejected
let input = r#"{
"Gateway": {
"Current": {
"Version": "2025.3.2.0",
"Files": [
{
"Arch": "x64",
"Type": "msi",
"Url": "https://cdn.devolutions.net/download/test.msi",
"Hash": ""
}
]
}
}
}"#;

let result: Result<ProductInfoDb, _> = input.parse();
assert!(result.is_err(), "Should reject empty hash");
}
}