Skip to content

refactor: replace regex JSON parsing with DataContractJsonSerializer #40

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

Merged
hieuck merged 1 commit into from
Jul 8, 2026
Merged

refactor: replace regex JSON parsing with DataContractJsonSerializer #40

Show file tree
Hide file tree
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
2 changes: 2 additions & 0 deletions src/KeePassAutoReload.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@
<Reference Include="System" />
<Reference Include="System.Drawing" />
<Reference Include="System.Net.Http" />
<Reference Include="System.Runtime.Serialization" />
<Reference Include="System.Windows.Forms" />
<Reference Include="System.Xml" />
<Reference Include="KeePass">
<HintPath>$(KeePassReferencePath)\KeePass.exe</HintPath>
</Reference>
Expand Down
43 changes: 33 additions & 10 deletions src/UpdateChecker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,8 +4,9 @@
using System.Net;
using System.Net.Http;
using System.Reflection;
using System.Runtime.Serialization;
using System.Runtime.Serialization.Json;
using System.Security.Authentication;
using System.Text.RegularExpressions;
using System.Threading;
using System.Threading.Tasks;

Expand All @@ -20,6 +21,13 @@ internal sealed class UpdateInfo
public bool IsUpdateAvailable;
}

[DataContract]
internal sealed class GitHubRelease
{
[DataMember]
public string tag_name { get; set; }
}

internal interface IUpdateClient
{
Task<string> DownloadStringAsync(string url, CancellationToken cancellationToken = default);
Expand Down Expand Up @@ -110,7 +118,7 @@ public static async Task<UpdateInfo> CheckLatestAsync(IUpdateClient client, Plug

cancellationToken.ThrowIfCancellationRequested();
string json = await client.DownloadStringAsync(ReleasesApiUrl, cancellationToken);
string tagName = GetNewestVersionTag(ExtractJsonStrings(json, "tag_name").ToArray());
string tagName = GetNewestVersionTag(ExtractVersionTags(json));

UpdateInfo info = new UpdateInfo();
info.LatestVersion = tagName;
Comment on lines 118 to 124

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Potential Issue: Lack of Validation for tagName in UpdateInfo Construction

The code assigns tagName directly to UpdateInfo.LatestVersion and uses it to build URLs without validating that it is non-null and well-formed. If ExtractVersionTags or GetNewestVersionTag returns an empty or malformed string, this could result in broken URLs or incorrect update status.

Recommendation: Add validation for tagName before constructing URLs and setting IsUpdateAvailable. For example:

if (string.IsNullOrEmpty(tagName)) {
    // Handle the error, e.g., return null or a default UpdateInfo indicating no update
}

This will prevent propagating invalid data and improve robustness.

Expand Down Expand Up @@ -188,18 +196,33 @@ private static string BuildChecksumUrl(string tagName)
return "https://github.com/hieuck/KeePassAutoReload/releases/download/" + tagName + "/SHA256SUMS.txt";
}

private static List<string> ExtractJsonStrings(string json, string name)
private static string[] ExtractVersionTags(string json)
{
List<string> values = new List<string>();
if (string.IsNullOrEmpty(json)) return values;
if (string.IsNullOrWhiteSpace(json)) return new string[0];

MatchCollection matches = Regex.Matches(json, "\"" + Regex.Escape(name) + "\"\\s*:\\s*\"(?<value>(?:\\\\.|[^\"])*)\"");
foreach (Match match in matches)
try
{
using (MemoryStream stream = new MemoryStream(System.Text.Encoding.UTF8.GetBytes(json)))
{
DataContractJsonSerializer serializer = new DataContractJsonSerializer(typeof(List<GitHubRelease>));
List<GitHubRelease> releases = (List<GitHubRelease>)serializer.ReadObject(stream);
if (releases == null) return new string[0];

List<string> tags = new List<string>();
foreach (GitHubRelease release in releases)
{
if (release != null && !string.IsNullOrWhiteSpace(release.tag_name))
{
tags.Add(release.tag_name);
}
}
return tags.ToArray();
}
}
catch
{
if (match.Success) values.Add(Regex.Unescape(match.Groups["value"].Value));
return new string[0];
}
Comment on lines +222 to 225

@codereviewbot-ai codereviewbot-ai Bot Jul 8, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

The catch block in ExtractVersionTags (lines 222-225) silently swallows all exceptions and returns an empty array. This can obscure the root cause of failures, making debugging difficult if the JSON format changes or network responses are malformed.

Recommendation: At minimum, log the exception or provide a mechanism to surface the error, such as rethrowing or returning an error indicator. For example:

catch (Exception ex)
{
    // Log the exception or rethrow
    // Logger.LogError(ex, "Failed to extract version tags from JSON");
    return new string[0];
}


return values;
}
}
}