Skip to content

Tech debt

This host carries no authorization construct of any kind across all six services, exposes an unconstrained arbitrary-path filesystem surface on FileService, logs sensitive SecurityService request detail unconditionally behind a dead kill switch, mutates a shared private static field on every request, and returns full server-side exception detail to every caller through a purely observational fault pipeline.

CRITICAL

Finding Component Impact Recommended action
No authorization construct exists anywhere in this host's code — every one of the 24 operations across all six services carries "None in code" for authorization, and access relies entirely on IIS authentication and network placement for the deployed variant FileService, PreferenceService, WebContextService, ImagingService, FaxService, SecurityService Any caller that can reach a route can invoke every operation on it, including the five destructive/unconstrained FileService operations Add explicit authorization to at least the write- and delete-capable operations, or document and enforce the intended network-boundary protection
FileService exposes five operations (ReadFile, ReadEntireFile, WriteFile, DeleteFile, Length) that act directly on a caller-supplied path via System.IO with no allow-list, no canonicalization, no containment check, and no authorization SAIF.Services.WCF.Services.FileService Any caller reaching the FileService route can read, overwrite, or delete any path the IIS application-pool identity can reach, including UNC paths; includeExceptionDetailInFaults="true" additionally returns resolved paths in filesystem exception detail on failure Constrain accepted paths to an allow-listed root directory with Path.GetFullPath containment checks, and add caller-identity authorization before this surface is exposed further
SecurityService.LogRequestDetails writes a JSON fragment of every operation's name and arguments — including userId, employeeId, customerId, groupName, and role — into the IIS log unconditionally on every call, and the LOG_REQUEST_DETAILS appSetting intended to gate it is read into a field but never consulted, so the kill switch has no effect SecurityService Sensitive identity arguments are written to the IIS log on every security operation with no way to disable it in configuration Wire _logRequestDetails into an actual conditional check in LogRequestDetails, and reduce the logged fields to what is operationally necessary
The fault pipeline is purely observational: ErrorLogErrorHandler.HandleError logs and returns false, ProvideFault is an empty method body, and includeExceptionDetailInFaults="true" means every unhandled exception reaches every caller with its full type, message, and stack trace ErrorLogErrorHandler, Web.Base.config (<serviceDebug>) Any caller of any of the 24 operations can observe internal exception detail, including file paths and (for FileService/PreferenceService) caller-supplied values, on any unhandled failure Set includeExceptionDetailInFaults="false" in the deployed configuration and introduce typed [FaultContract]s so callers receive a controlled failure shape instead of raw exception detail

HIGH

Finding Component Impact Recommended action
ldap_external is a private static field mutated from an instance constructor on every request, and the static method IsAuthorizedExternal reads it; concurrent requests share and overwrite the same field SecurityService Under concurrent load, one request's resolved external LDAP host can be overwritten by another request's before the first request's authorization check runs — a cross-request shared-state hazard Make the external LDAP host resolution a per-instance (or per-call, passed as a parameter) value rather than a mutable static field
The six [ServiceContract] interfaces (IFileService, IPreferenceService, IWebContextService, IImagingService, IFaxService, ISecurityService) live outside this repository in SAIF.Services.WCF.Contracts, arriving only transitively via SAIF.Framework.Core with no direct reference and no version pin All six service classes The operation surface this host publishes can change without any corresponding change appearing in this repository's history Add a direct, version-pinned reference to SAIF.Services.WCF.Contracts if it is intended to be a first-class dependency of this host
SAIFConnection.dll 1.8.5.0 is vendored by <HintPath> outside packages\, with <SpecificVersion>False</SpecificVersion>, is not a NuGet package, and is not in packages.config — and no host source file references its types WCF.csproj (src/WCF/Libs/SAIF/SAIFConnection.dll) This binary cannot be restored, upgraded, or audited through the package pipeline; a rebuild depends on a file checked into the repository whose provenance is not tracked by any package manifest Confirm whether this assembly is actually required at run time; remove the reference if it is not, or migrate it to a package if it is
ImagingService is registered under the route name Imaging in both this host and SAIF.Services.WCF32.Host, but the two are different classes implementing different contracts (SAIF.Services.WCF.Contracts.IImagingService here vs. SAIF.Services.WCF.Imaging.WCF32.IImagingService there) on different platform targets (x64 vs. x86) SAIF.Services.WCF.Services.ImagingService; also SAIF.Services.WCF32.Host (not documented — no pipeline reference, no .nuspec reference) A change intended for one imaging surface can be mistaken for a change to the other; the two implementations can drift silently since neither references the other Rename one of the two routes to remove the ambiguity, or document the split explicitly at the point of registration
There is no dependency-injection container in this host; every collaborator across all six services is constructed with new against a concrete class inside a parameterless constructor or a per-call method, and Autofac 3.5.2 is referenced in packages.config but unused by any host source file All six service classes Collaborators cannot be substituted for testing or reconfigured without changing source; the unused Autofac reference adds confusion about whether a container is actually in use Either remove the unused Autofac reference, or adopt it as the composition root if dependency substitution is needed
ImagingService.ThrowTimeoutException(int) constructs a FaultException with FaultCode("RequestTimeout") but has no call site anywhere in the host — it is unreachable dead code SAIF.Services.WCF.Services.ImagingService The method suggests a timeout-fault contract exists for callers when none is actually ever raised, which can mislead a reader of the code into believing timeout handling exists on this path Remove the dead method, or wire it into an actual timeout-detection path if one is intended
packages.config is used exclusively for package management across the whole project, with no PackageReference migration path WCF.csproj, packages.config The project cannot take advantage of PackageReference features (transitive restore, lockfiles, floating versions) and remains on the older, more error-prone package-management model Plan a migration to PackageReference when the host is next substantially touched
ErrorLogErrorHandler.HandleError dereferences error.TargetSite.DeclaringType with no null guard; TargetSite is null for some exception shapes ErrorLogErrorHandler An exception whose TargetSite is null could cause the logging call itself to throw a secondary NullReferenceException inside the error handler, potentially masking the original failure's log entry Add a null check around error.TargetSite before dereferencing .DeclaringType

Sources

  • src/WCF/SAIF.Services.WCF.Host/Services/FileService.cs
  • src/WCF/SAIF.Services.WCF.Host/Services/Security/SecurityService.cs
  • src/WCF/SAIF.Services.WCF.Host/Services/ImagingService.cs
  • src/WCF/SAIF.Services.WCF.Host/Extensions/ErrorLogErrorHandler.cs
  • src/WCF/SAIF.Services.WCF.Host/Web.Base.config
  • src/WCF/SAIF.Services.WCF.Host/WCF.csproj
  • src/WCF/SAIF.Services.WCF.Host/packages.config