Support contributor author avatars

[PrzemyslawKlys]

Summary

• allow contribution author profiles to reference local avatar images
• validate avatar paths, supported extensions, and file size limits
• copy local avatars into website author assets during import and rewrite catalog paths
• add regression coverage for avatar import and path traversal rejection

Validation

• dotnet test PowerForge.Tests/PowerForge.Tests.csproj -c Release --filter WebContributionProcessorTests
• dotnet C:\Support\GitHub\PSPublishModule\PowerForge.Web.Cli\bin\Release\net10.0\PowerForge.Web.Cli.dll contributions validate --root C:\Support\GitHub\Website.Contributions

[claude]

Code Review — PR #334: Support contributor author avatars

Overview

This PR adds support for local avatar images in contribution author profiles. Authors can now reference a local relative path (e.g., ./images/jane-doe.webp), which is validated for path traversal, file existence, extension, and size — then copied into the website's asset directory and rewritten to a site-rooted URL during import. A bonus socialAutoGenerate build-step override is also included.

The implementation is generally well-structured and follows existing patterns in the codebase. Below are specific issues found.

---

Issues

1. Side effects inside a validation method (medium)

WebContributionProcessor.Validation.cs, line 108

profile.AvatarSourcePath = fullPath;

ValidateAuthorAvatar mutates profile.AvatarSourcePath as a side effect of validation. The enclosing ValidateAuthorProfiles takes an IReadOnlyDictionary, which signals "we won't change anything" — but the profile objects inside it are silently mutated. This makes validation carry hidden import-prep responsibilities. Consider splitting into ValidateAuthorAvatar (pure, returns errors) and ResolveAuthorAvatarPath (sets AvatarSourcePath, called separately in the import path), or at minimum rename the method to something like ValidateAndResolveAuthorAvatar and drop the IReadOnlyDictionary facade.

2. LINQ Select used for side effects (medium)

WebContributionProcessor.cs, lines 151–154

.Select(profile => PrepareAuthorProfileForImport(profile, options, siteRoot, errors, result))
.ToArray();
if (errors.Count > 0)
    return;

PrepareAuthorProfileForImport mutates errors and result.CopiedAssetCount — but is called inside a LINQ Select, which is semantically a pure projection. The ToArray() forces full evaluation, meaning all profiles are processed (and their avatars copied) even if an earlier one fails. The errors.Count > 0 guard only prevents writing the catalog; already-copied avatar files from successful profiles are left in place when a later copy fails. A foreach loop with early exit on failure would be clearer and more correct:

var profiles = new List<WebContributionAuthorProfile>();
foreach (var profile in authors.Values.OrderBy(...).ThenBy(...))
{
    profiles.Add(PrepareAuthorProfileForImport(profile, options, siteRoot, errors, result));
    if (errors.Count > 0) return;
}

3. Fragile static/ prefix stripping (minor)

WebContributionProcessor.cs, lines 259–261

var publicRoot = options.TargetAuthorAssetsPath.Replace('\\', '/').Trim('/');
if (publicRoot.StartsWith("static/", StringComparison.OrdinalIgnoreCase))
    publicRoot = publicRoot["static/".Length..];

This logic is also used for StaticBlogAssetsPath elsewhere, so it's consistent — but it hardcodes the convention that Hugo-style static/ is stripped for the public URL. If someone configures a non-default TargetAuthorAssetsPath that happens to start with static/ for unrelated reasons, they'd silently get a wrong URL. At minimum, a code comment explaining the Hugo convention would help future maintainers.

4. AvatarSourcePath assigned before all validation checks pass (minor)

WebContributionProcessor.Validation.cs, lines 108–121

profile.AvatarSourcePath = fullPath is set before the extension and size checks. This means a file with an unsupported extension or excessive size still ends up with a populated AvatarSourcePath. Since errors block the import, this doesn't cause a bug in practice — but it means a later reader of the profile can't trust that a non-null AvatarSourcePath implies the avatar passed all checks. Assigning only after all checks pass (or checking AvatarSourcePath only when errors.Count == 0) would be safer by convention.

5. Unquoted avatar paths in test YAML helper (minor)

WebContributionProcessorTests.cs, line ~416

var avatarLine = string.IsNullOrWhiteSpace(avatar) ? string.Empty : $"avatar: {avatar}\n";

Bare ../jane-doe.webp and ./images/jane-doe.webp happen to parse correctly with most YAML parsers, but YAML path values should ideally be quoted to be unambiguous and to avoid surprises with values like ./foo: bar. Using $"avatar: \"{avatar}\"\n" is safer and matches what a real author YAML file would likely contain.

---

Missing Test Coverage

The new tests cover the happy path (copy + catalog rewrite) and one security path (traversal rejection). The following cases are not tested:

• Unsupported extension: An avatar file ending in .exe or .txt should produce an error — currently untested.
• Size limit: A file exceeding MaxAssetBytes should be rejected — currently untested.
• Non-existent avatar file: A valid relative path pointing to a file that doesn't exist should produce an "avatar does not exist" error — currently untested.
• External URL avatar: An https:// avatar should pass through import unchanged — currently untested.
• Root-path avatar: A /assets/... avatar should pass through unchanged — currently untested.

These edge cases are well-handled in the implementation; adding tests would complete the coverage and guard against regressions.

---

Positive Notes

• The path-traversal guard (TryResolveAuthorAsset) correctly uses Path.GetFullPath + prefix check — same pattern as the existing TryResolveBundleAsset.
• Creating a clean imported copy in PrepareAuthorProfileForImport (rather than mutating the original profile) is a good choice.
• The TargetAuthorAssetsPath option has a sensible default and is validated with TryResolveInside before use.
• The socialAutoGenerate / social-auto-generate alias pair follows the established naming convention for pipeline step options.
• Schema update for the new pipeline property is included.