Finding (LOW)
SynologyApiClient.GetAsync appends the caller-supplied extraQuery to the entry.cgi URL raw, with no escaping:
// src/SynoSharp/SynologyApiClient.cs
var query = $"webapi/entry.cgi?api={api}&version={version}&method={method}&_sid={_sid}";
if (!string.IsNullOrEmpty(extraQuery))
{
query += "&" + extraQuery; // appended raw (per the XML doc comment)
}Why it matters
Today all callers are internal and pass trusted, pre-formed query fragments, so real risk is low. But the contract is a footgun: if extraQuery is ever built from external/untrusted input, an attacker could inject or override query parameters (e.g. smuggle &method=… or additional API params). It also pushes escaping responsibility onto every caller.
Suggested fix
Accept structured key/value parameters and escape each value with Uri.EscapeDataString, e.g.:
public Task<JsonElement> GetAsync(
string api, int version, string method,
IEnumerable<KeyValuePair<string, string>>? extraParams = null,
CancellationToken cancellationToken = default)…or, if the raw string is kept for ergonomics, document the trust constraint explicitly and validate against unexpected keys.
Context
Surfaced during a security review of the C# clients (alongside two MEDIUM findings — DSM credentials in the login URL query string, and record ToString() leaking secrets — both already fixed on branch fix/secret-leaks-and-login-post). Filing this one separately for later as it needs an API-shape decision.
Severity: LOW. Read-only client, internal callers only at present.
Finding (LOW)
SynologyApiClient.GetAsyncappends the caller-suppliedextraQueryto theentry.cgiURL raw, with no escaping:Why it matters
Today all callers are internal and pass trusted, pre-formed query fragments, so real risk is low. But the contract is a footgun: if
extraQueryis ever built from external/untrusted input, an attacker could inject or override query parameters (e.g. smuggle&method=…or additional API params). It also pushes escaping responsibility onto every caller.Suggested fix
Accept structured key/value parameters and escape each value with
Uri.EscapeDataString, e.g.:…or, if the raw string is kept for ergonomics, document the trust constraint explicitly and validate against unexpected keys.
Context
Surfaced during a security review of the C# clients (alongside two MEDIUM findings — DSM credentials in the login URL query string, and record
ToString()leaking secrets — both already fixed on branchfix/secret-leaks-and-login-post). Filing this one separately for later as it needs an API-shape decision.Severity: LOW. Read-only client, internal callers only at present.