-
Notifications
You must be signed in to change notification settings - Fork 68
Use Central Package Management (cpm), move TargetFramework to BuildProps #519
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.
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
Conversation
WalkthroughCentralized NuGet package version management via a new Directory.Packages.props, removed per-project PackageReference Version pins, and standardized the build target by replacing AvaloniaVersion with TargetFramework set to net8.0 in Directory.Build.props. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (2)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ImmichFrame.Core.Tests/ImmichFrame.Core.Tests.csproj (1)
30-32: Remove unnecessary hardcoded assembly reference.Microsoft.Extensions.Logging is already available transitively through the
Microsoft.Extensions.Logging.ConsolePackageReference (line 10). The hardcoded Reference with its absolute path to a specific .NET version (lines 30-32) is redundant and should be deleted. This will ensure the dependency is managed properly through NuGet rather than relying on a fragile environment-specific file path.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Directory.Build.props(1 hunks)Directory.Packages.props(1 hunks)ImmichFrame.Core.Tests/ImmichFrame.Core.Tests.csproj(1 hunks)ImmichFrame.Core/ImmichFrame.Core.csproj(2 hunks)ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj(1 hunks)ImmichFrame.WebApi/ImmichFrame.WebApi.csproj(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-09T16:12:49.488Z
Learnt from: JoeRu
Repo: immichFrame/ImmichFrame PR: 481
File: ImmichFrame.Core.Tests/Logic/Pool/ChronologicalAssetsPoolWrapperTests.cs:306-306
Timestamp: 2025-10-09T16:12:49.488Z
Learning: When testing the ChronologicalAssetsPoolWrapper in ImmichFrame.Core.Tests, use `Is.SupersetOf` rather than `Is.EquivalentTo` or `Is.EqualTo` assertions because the wrapper uses Fisher-Yates shuffle to randomize set order, making output non-deterministic between runs. The wrapper also uses a 10x fetch multiplier (capped at 1000) that may return more assets than requested, which is legitimate behavior.
Applied to files:
ImmichFrame.Core.Tests/ImmichFrame.Core.Tests.csproj
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (7)
ImmichFrame.WebApi/ImmichFrame.WebApi.csproj (1)
12-14: LGTM! CPM migration correctly implemented.The package references correctly omit Version attributes, deferring to Directory.Packages.props for centralized version management.
Directory.Build.props (1)
4-4: LGTM! Centralized target framework configuration.Setting TargetFramework globally ensures consistency across all projects in the solution.
ImmichFrame.Core.Tests/ImmichFrame.Core.Tests.csproj (1)
10-22: LGTM! CPM migration correctly implemented for test packages.All package references correctly omit Version attributes, with central version management handled by Directory.Packages.props. PrivateAssets and IncludeAssets metadata is properly retained.
Directory.Packages.props (2)
3-4: LGTM! CPM configuration is correct.The ManagePackageVersionsCentrally setting enables central package management, and disabling CentralPackageTransitivePinningEnabled allows NuGet to resolve transitive dependencies more flexibly.
14-14: Remove this review comment — no issue exists.Microsoft.Extensions.Caching.Memory 9.0.5 is designed for and compatible with .NET 8.0 (targets .NET 8.0 and higher). The version difference between this package (9.0.5) and other Microsoft.Extensions packages (8.0.x) is normal and expected—different packages follow independent version lifecycles and do not cause assembly conflicts. This configuration is correct.
ImmichFrame.WebApi.Tests/ImmichFrame.WebApi.Tests.csproj (1)
10-23: LGTM! CPM migration correctly implemented for WebApi tests.All package references correctly omit Version attributes, with PrivateAssets and IncludeAssets metadata properly retained for analyzers and collectors.
ImmichFrame.Core/ImmichFrame.Core.csproj (1)
14-28: LGTM! CPM migration correctly implemented for Core library.All package references correctly omit Version attributes, deferring to Directory.Packages.props. PrivateAssets and IncludeAssets metadata is properly preserved for build-time tools.
| <PackageVersion Include="AwesomeAssertions" Version="9.2.0" /> | ||
| <PackageVersion Include="BloomFilter.NetCore" Version="2.5.3" /> | ||
| <PackageVersion Include="coverlet.collector" Version="6.0.4" /> | ||
| <PackageVersion Include="Ical.Net" Version="4.3.1" /> | ||
| <PackageVersion Include="Microsoft.AspNetCore.Mvc.Testing" Version="8.0.20" /> | ||
| <PackageVersion Include="Microsoft.AspNetCore.SpaProxy" Version="8.0.20" /> | ||
| <PackageVersion Include="Microsoft.Extensions.ApiDescription.Client" Version="8.0.20" /> | ||
| <PackageVersion Include="Microsoft.Extensions.Caching.Memory" Version="9.0.5" /> | ||
| <PackageVersion Include="Microsoft.Extensions.Http" Version="8.0.1" /> | ||
| <PackageVersion Include="Microsoft.Extensions.Logging.Console" Version="8.0.1" /> | ||
| <PackageVersion Include="Microsoft.NET.Test.Sdk" Version="17.14.1" /> | ||
| <PackageVersion Include="Moq" Version="4.20.72" /> | ||
| <PackageVersion Include="Newtonsoft.Json" Version="13.0.4" /> | ||
| <PackageVersion Include="NSwag.ApiDescription.Client" Version="13.20.0" /> | ||
| <PackageVersion Include="NUnit" Version="4.4.0" /> | ||
| <PackageVersion Include="NUnit.Analyzers" Version="4.10.0" /> | ||
| <PackageVersion Include="NUnit3TestAdapter" Version="5.2.0" /> | ||
| <PackageVersion Include="OpenWeatherMap.API" Version="2.1.2" /> | ||
| <PackageVersion Include="Swashbuckle.AspNetCore" Version="8.1.4" /> | ||
| <PackageVersion Include="ThumbHash" Version="2.1.1" /> | ||
| <PackageVersion Include="YamlDotNet" Version="16.3.0" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🌐 Web query:
Swashbuckle.AspNetCore 8.1.4 security vulnerabilities CVE
💡 Result:
Short answer: There is no known CVE that directly affects Swashbuckle.AspNetCore v8.1.4 itself. However:
-
A transitive dependency risk exists: Newtonsoft.Json versions < 13.0.1 are affected by CVE-2024-21907 (DoS via deeply nested JSON); if your project (or any dependency in the chain used by Swashbuckle) pulls an affected Newtonsoft.Json, you are exposed. [1][3]
-
Old swagger-ui packages had SSRF/click‑hijack issues (e.g. CVE-2021-46708) but that affects Swagger UI packages prior to their fixed releases (upgrade to fixed versions); it does not apply to Swashbuckle.AspNetCore.SwaggerUI >= 6.3. Confirm your installed swagger-ui subpackage version. [2]
Recommendation: verify your effective dependency tree (dotnet list package --include-transitive or a SCA tool) for Newtonsoft.Json < 13.0.1 or for swashbuckle.aspnetcore.swaggerui versions < 6.3 and upgrade those packages if present. [3][1][2]
Sources:
- NVD — CVE-2024-21907 (Newtonsoft.Json). [1]
- Snyk advisory — CVE-2021-46708 / swagger-ui SSRF. [2]
- Swashbuckle.AspNetCore 8.1.4 NuGet page (package metadata). [3]
🌐 Web query:
Newtonsoft.Json 13.0.4 security vulnerabilities known issues
💡 Result:
Short answer
- No publicly disclosed CVE affects Newtonsoft.Json 13.0.4 itself — the known high‑severity CVE (CVE‑2024‑21907) affects versions before 13.0.1 and was fixed in 13.0.1. [1][4]
Known non‑security issues reported for 13.0.4
- Users have reported regressions/behavior changes (SerializerSettings not respected) after upgrading to 13.0.4. [2]
- Runtime issues when mixing assemblies (apps built against 13.0.4 loading 13.0.3 from GAC) have been reported. These are runtime/compatibility bugs, not CVEs. [3]
Recommendations
- 13.0.4 includes the security fix for CVE‑2024‑21907; if you need protection against that DoS issue, ensure you are on >=13.0.1 (13.0.4 is fine). [1][4]
- Before upgrading to 13.0.4 in production, test for the reported regressions (SerializerSettings and GAC/assembly-binding scenarios) and apply binding redirects or avoid mixed GAC installs if necessary. [2][3]
Sources
- GitHub Advisory / CVE details (CVE‑2024‑21907) — fix in 13.0.1. [1]
- GitHub issues reporting 13.0.4 regressions (#3080). [2]
- GitHub issue reporting runtime/GAC compatibility (#3084). [3]
- Rapid7 / vulnerability databases summary of CVE‑2024‑21907. [4]
🌐 Web query:
Microsoft.AspNetCore.Mvc.Testing 8.0.20 security vulnerabilities
💡 Result:
Short answer: Yes — Microsoft.AspNetCore.Mvc.Testing 8.0.20 (and ASP.NET Core 8.0.20 and earlier) is covered by a critical ASP.NET Core HTTP request‑smuggling vulnerability tracked as CVE‑2025‑55315. Microsoft patched it in ASP.NET Core 8.0.21; you should update to 8.0.21 or later. [1][2]
Sources
- Microsoft / dotnet security advisory (CVE‑2025‑55315) — affected: ASP.NET Core 8.0.20 and earlier; guidance to update to patched releases. [1]
- NuGet package page showing Microsoft.AspNetCore.Mvc.Testing 8.0.20 (package/version referenced). Update to 8.0.21+ is recommended. [2]
Would you like exact upgrade steps for your project (csproj/central package management/Docker)?
🌐 Web query:
NUnit latest stable version December 2025
💡 Result:
Latest stable NUnit version (as of December 4, 2025) is 4.4.0. [1][2]
Sources:
- NuGet package profile for NUnit (latest version 4.4.0, last updated Aug 10, 2025). [1]
- NUnit GitHub repository (NUnit 4 framework). [2]
🌐 Web query:
Microsoft.NET.Test.Sdk latest version 2025
💡 Result:
The latest Microsoft.NET.Test.Sdk release is version 18.0.1, published November 11, 2025. [1]
Sources:
[1] NuGet — Microsoft.NET.Test.Sdk package page (version list). [2] NuGet — Microsoft.NET.Test.SDK (package) (same listing).
Update Microsoft.AspNetCore.Mvc.Testing to 8.0.21 or later—CVE-2025-55315 critical HTTP request-smuggling vulnerability.
Additionally:
- Microsoft.NET.Test.Sdk 17.14.1 is outdated. Update to 18.0.1 (latest as of November 2025).
- Newtonsoft.Json 13.0.4 has reported regressions (SerializerSettings not respected in some scenarios). Test thoroughly before production deployment, or consider downgrading to 13.0.3 if issues occur.
- Verify the effective dependency tree (e.g.,
dotnet list package --include-transitive) for any transitive Newtonsoft.Json < 13.0.1 pulled by Swashbuckle or other dependencies, and confirm swagger-ui subpackage version is ≥ 6.3.
🤖 Prompt for AI Agents
In Directory.Packages.props around lines 7 to 27, update vulnerable/outdated
package versions: bump Microsoft.AspNetCore.Mvc.Testing from 8.0.20 to 8.0.21 or
later to address CVE-2025-55315, update Microsoft.NET.Test.Sdk from 17.14.1 to
18.0.1, and evaluate Newtonsoft.Json 13.0.4 for regressions—either test
thoroughly or pin to 13.0.3 if issues arise; after changes run a full transitive
check (dotnet list package --include-transitive) to ensure no transitive
Newtonsoft.Json < 13.0.1 is pulled and confirm any swagger-ui subpackage is >=
6.3, then update Directory.Packages.props accordingly and re-run tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will resolve this in another PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JW-CH, understood! Would you like me to create a follow-up issue to track the package updates (including the critical CVE-2025-55315 fix for Microsoft.AspNetCore.Mvc.Testing)?
| <Project> | ||
| <PropertyGroup> | ||
| <Nullable>enable</Nullable> | ||
| <AvaloniaVersion>11.0.6</AvaloniaVersion> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LOL, how was this still in there 😄
|
Nice, much cleaner |
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.