Cdn improvements #55
Cdn improvements #55
Conversation
WalkthroughThe changes introduce several updates across multiple files in the Argon.Api project. Key modifications include enhancements to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (13)
src/Argon.Api/Extensions/SwaggerExtension.cs (1)
Line range hint
20-31: Consider documenting the security requirement scope.While the empty array for scopes is valid, consider adding a comment explaining why no scopes are needed or if they might be added in the future. This will help other developers understand the security configuration.
} }, - [] + Array.Empty<string>() // No scopes needed for JWT Bearer authenticationsrc/Argon.Api/Features/Captcha/CloudflareCaptcha.cs (2)
7-7: Good improvement using IHttpContextAccessor!The switch from direct
HttpContexttoIHttpContextAccessoris a better architectural choice as it:
- Follows ASP.NET Core best practices for accessing HttpContext
- Improves testability through proper dependency injection
- Ensures thread-safety when accessing the HttpContext
Line range hint
10-26: Consider adding rate limiting and additional security measuresWhile the implementation is solid, consider these security enhancements:
- Add rate limiting to prevent brute force attempts
- Implement token format validation before making the API call
- Consider adding timeout to the HTTP request to prevent hanging connections
Example enhancement:
public async ValueTask<bool> ValidateAsync(string token) { if (string.IsNullOrEmpty(token)) return false; + // Basic token format validation (example for Turnstile) + if (!token.StartsWith("0x4AAAAAAA") || token.Length < 32) + return false; var config = options.Value; var remoteIp = accessor.HttpContext!.GetIpAddress(); try { var response = await config.ChallengeEndpoint .PostMultipartAsync(content => content .AddString("secret", config.SiteSecret) .AddString("response", token) .AddString("remoteip", remoteIp)) + .WithTimeout(TimeSpan.FromSeconds(5)) .ReceiveJson<CloudflareTurnstileResponse>();src/Argon.Api/Extensions/HttpContextExtensions.cs (1)
32-41: Consider aligning error handling with existing patternsThe new
GetUserIdmethod handles missing/invalid values differently from other methods in the class. While other methods provide fallback values (e.g., "unknown" for IP), this method throws exceptions. Consider aligning the error handling approach with the existing pattern for consistency.Options to consider:
- Return
Guid.Emptyfor invalid/missing values (following the pattern)- Update other methods to throw exceptions (breaking change)
- Add a
TryGetUserIdmethod that returns bool (similar to Guid.TryParse pattern)src/Argon.Api/Features/Jwt/JwtFeature.cs (2)
28-35: Consider revising the authorization policy implementation.While adding authorization policies is good practice, creating a policy that simply mirrors the authentication scheme doesn't provide additional value. The policy currently only enforces what the authentication middleware already handles.
Consider either:
- Removing this policy if no additional requirements are needed, as JWT authentication is already enforced by the authentication middleware, or
- Enhancing the policy with specific authorization requirements relevant to your CDN implementation:
builder.Services.AddAuthorization(options => { - options.AddPolicy(JwtBearerDefaults.AuthenticationScheme, policy => + options.AddPolicy("CdnAccess", policy => { policy.AuthenticationSchemes.Add(JwtBearerDefaults.AuthenticationScheme); policy.RequireAuthenticatedUser(); + policy.RequireClaim("permission", "cdn:access"); // Add specific requirements }); });
Security configuration requires critical updates
Several security concerns need to be addressed:
- Missing CORS configuration which is required for secure CDN integration
- No HSTS configuration found, leaving the application vulnerable to protocol downgrade attacks
- JWT secret key is hardcoded in appsettings files
- Missing security headers configuration (X-Frame-Options, X-Content-Type-Options, etc.)
- No rate limiting configuration found to prevent abuse
Required actions:
- Add CORS policy configuration with specific allowed origins
- Enable HSTS with
app.UseHsts()- Move JWT secret to secure configuration management/secrets
- Add security headers middleware
- Implement rate limiting for authentication endpoints
🔗 Analysis chain
Line range hint
1-44: Verify security configuration completeness.Let's ensure all security-related configurations are properly set up across the application, particularly for CDN integration.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify security configurations across the application # Check for CORS configuration which is critical for CDN echo "Checking CORS configuration..." rg -l "AddCors|UseCors" --type cs # Check for security headers middleware echo "Checking security headers configuration..." rg -l "UseHsts|UseHttpsRedirection|AddSecurityHeaders" --type cs # Verify no hardcoded JWT secrets echo "Checking for potential hardcoded secrets..." rg -l "SigningKey|Secret|Password" --type cs # Check for rate limiting configuration echo "Checking rate limiting configuration..." rg -l "AddRateLimiting|UseRateLimiting" --type csLength of output: 1967
Script:
#!/bin/bash # Let's examine the security configurations in Program.cs files and check HTTPS/HSTS setup echo "Checking Program.cs security configurations:" rg "UseHsts|UseHttpsRedirection" -A 5 -B 5 src/Argon.Entry/Program.cs src/Argon.Api/Program.cs # Let's check the actual JWT configuration in appsettings echo "Checking JWT configuration patterns:" fd appsettings --exec cat {} \; # Let's verify CORS setup which is critical for CDN echo "Checking CORS setup in Program.cs:" rg "cors|CORS" -i src/Argon.Entry/Program.cs src/Argon.Api/Program.cs # Let's check if there are any security middleware configurations echo "Checking security middleware configurations:" rg "app\.Use|builder\.Services\.Add.*Security" src/Argon.Entry/Program.cs src/Argon.Api/Program.csLength of output: 5357
src/Argon.Api/Features/MediaStorage/AssetId.cs (2)
6-6: Document the purpose of the reserved "00" in the file ID formatThe comment indicates that the "00" is reserved, but its intended future use isn't clear. This should be documented for maintainability.
- => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00.{extensions}"; // last two zero reserved + => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00.{extensions}"; // Reserved for future versioning or metadata flags
Line range hint
3-52: Improved separation of concernsThe removal of the
GetMimemethod and delegation of MIME type handling toIContentTypeProvider(as mentioned in the summary) is a good architectural decision that better adheres to the Single Responsibility Principle.src/Argon.Api/Program.cs (1)
32-32: Consider moving HttpContextAccessor registration.For better code organization, consider moving this registration inside the
if (!builder.Environment.IsManaged())block since it appears to be primarily used by components that are only active in non-managed environments (FilesController, CloudflareCaptcha).-builder.Services.AddHttpContextAccessor(); if (!builder.Environment.IsManaged()) { + builder.Services.AddHttpContextAccessor(); builder.AddJwt(); builder.Services.AddAuthorization();src/Argon.Api/Controllers/FilesController.cs (1)
48-60: Address TODO comment and enhance permission checkThe permission check could be more comprehensive, and there's an unaddressed TODO comment.
Would you like me to help implement a more detailed permission check system or create an issue to track this TODO?
Additionally, consider enhancing the permission check:
-if (!permissions.CanAccess("server.avatar.upload", PropertyBag.Empty.Set(serverId))) +if (!permissions.CanAccess("server.avatar.upload", PropertyBag.Empty + .Set(serverId) + .Set("userId", HttpContext.GetUserId())))src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs (3)
24-24: Consider configuring custom MIME typesWhile
FileExtensionContentTypeProvideris appropriate for standard MIME types, you might want to configure it for any custom file types specific to your application.Here's how you could configure custom MIME types:
-builder.Services.TryAddSingleton<IContentTypeProvider, FileExtensionContentTypeProvider>(); +builder.Services.TryAddSingleton<IContentTypeProvider>(serviceProvider => { + var provider = new FileExtensionContentTypeProvider(); + // Add custom mappings if needed + // provider.Mappings[".custom"] = "application/x-custom"; + return provider; +});
Line range hint
43-48: Refactor to eliminate redundant storage kind checksThe storage kind is already checked in the switch expression above, making these conditions redundant. This could lead to maintenance issues if new
StorageKindvalues are added.Consider refactoring to this pattern:
- if (keyName == StorageKind.Disk) - { - builder.Services.AddKeyedSingleton<IContentStorage, DiskContentStorage>(IContentStorage.DiskContentStorageKey); - builder.Services.AddSingleton<IContentDeliveryNetwork, T>(); - } - else if (keyName == StorageKind.GenericS3) - { - builder.Services.AddKeyedSingleton<IContentStorage, S3ContentStorage>(IContentStorage.GenericS3StorageKey); - builder.Services.AddSingleton<IContentDeliveryNetwork, T>(); - builder.AddS3Storage(keyName, options); - } + switch (keyName) + { + case StorageKind.Disk: + builder.Services.AddKeyedSingleton<IContentStorage, DiskContentStorage>(IContentStorage.DiskContentStorageKey); + builder.Services.AddSingleton<IContentDeliveryNetwork, T>(); + break; + case StorageKind.GenericS3: + builder.Services.AddKeyedSingleton<IContentStorage, S3ContentStorage>(IContentStorage.GenericS3StorageKey); + builder.Services.AddSingleton<IContentDeliveryNetwork, T>(); + builder.AddS3Storage(keyName, options); + break; + default: + throw new ArgumentException($"Unsupported storage kind: {keyName}", nameof(keyName)); + }This approach:
- Eliminates redundant checks
- Provides better error messages
- Uses pattern matching for cleaner code
- Makes it easier to add new storage kinds in the future
Line range hint
54-73: Review lifecycle management of S3 service providerCreating a separate
ServiceProviderfor S3 services could lead to lifecycle management issues. The S3 services might not be properly disposed when the application shuts down.Consider these alternatives:
- Register S3 services directly in the main container
- If separation is necessary, implement
IDisposableon the containing service and properly dispose of thestorageServices- Use
IServiceScopeFactoryfor better lifecycle managementWould you like me to provide an example implementation for any of these approaches?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
src/Argon.Api/Controllers/FilesController.cs(3 hunks)src/Argon.Api/Extensions/HttpContextExtensions.cs(2 hunks)src/Argon.Api/Extensions/SwaggerExtension.cs(1 hunks)src/Argon.Api/Features/Captcha/CloudflareCaptcha.cs(1 hunks)src/Argon.Api/Features/Jwt/JwtFeature.cs(1 hunks)src/Argon.Api/Features/MediaStorage/AssetId.cs(3 hunks)src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs(3 hunks)src/Argon.Api/Features/MediaStorage/YandexContentDeliveryNetwork.cs(1 hunks)src/Argon.Api/Program.cs(1 hunks)
🔇 Additional comments (10)
src/Argon.Api/Extensions/SwaggerExtension.cs (1)
14-16: LGTM! Standard compliant authentication header configuration.
The changes properly align with standard JWT Bearer authentication practices by using the "Authorization" header.
Let's verify the impact on existing API clients:
✅ Verification successful
The authentication header changes are safe and properly integrated
The search results confirm that:
- The JWT Bearer authentication is consistently configured across the codebase:
- JwtFeature.cs properly sets up JWT Bearer as the default authentication scheme
- All authenticated endpoints use the standard JwtBearerDefaults.AuthenticationScheme
- The "Authorization" header is used correctly in all authentication flows:
- Swagger configuration aligns with the actual implementation
- SFU integration also uses standard Bearer token format
- No references to the old "x-argon-token" header were found, suggesting this is a new implementation rather than a breaking change
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining usages of the old header name
# to ensure all clients have been updated
# Search for old header name usage
rg -l "x-argon-token"
# Search for JWT/Bearer token validation logic
ast-grep --pattern 'new JwtBearerOptions { $$$}'
Length of output: 72
Script:
#!/bin/bash
# Let's try a broader search for authentication-related configurations and API clients
# Search for authentication header references
rg -i "authorization.*header" -A 3
# Search for HTTP client configurations
rg -i "httpclient" -A 3
# Search for any Bearer token references
rg -i "bearer" -A 3
# Look for API client classes
fd -e cs --exec grep -l "HttpClient\|IHttpClient"
Length of output: 9961
src/Argon.Api/Features/Captcha/CloudflareCaptcha.cs (1)
14-14: Verify HttpContext null handling and IP address extraction
The current implementation has potential null reference risks:
- Using the null-forgiving operator (
!) onHttpContextwithout a null check could lead to runtime exceptions - The
GetIpAddress()extension method's implementation needs verification for proper IP extraction, especially with reverse proxies or CDN scenarios
Let's verify the implementation:
✅ Verification successful
The null-forgiving operator usage is acceptable in this context
The implementation is safe because:
HttpContextAccessoris properly registered viaAddHttpContextAccessor()in Program.cs- The
GetIpAddress()extension method handles null cases gracefully by:- Checking Cloudflare headers first
- Falling back to Connection.RemoteIpAddress with null coalescing
- Providing "unknown" as a final fallback
- The pattern is consistently used across the codebase (AuthorizationController examples)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the GetIpAddress extension method implementation
echo "Searching for GetIpAddress implementation..."
rg -l "GetIpAddress"
# Check for other usages of IHttpContextAccessor to compare null handling patterns
echo "Checking other IHttpContextAccessor usage patterns..."
ast-grep --pattern 'accessor.HttpContext'
Length of output: 4772
Script:
#!/bin/bash
# Let's check the implementation of GetIpAddress extension method
echo "GetIpAddress implementation:"
rg -A 10 "GetIpAddress" src/Argon.Api/Extensions/HttpContextExtensions.cs
# Check for null checks in AuthorizationController
echo -e "\nUsage in AuthorizationController:"
rg -A 3 "GetIpAddress" src/Argon.Api/Controllers/AuthorizationController.cs
# Check if there's any middleware or service setup handling null HttpContext
echo -e "\nChecking for HttpContext setup in Program.cs or Startup:"
rg "AddHttpContextAccessor|UseMiddleware"
Length of output: 1400
src/Argon.Api/Extensions/HttpContextExtensions.cs (2)
3-3: LGTM!
The System.Security.Claims import is appropriate for the new functionality.
32-41:
Add authentication check and improve error handling
The current implementation has several potential issues:
- No authentication check before accessing claims
- Incomplete error message when "id" claim fails
- Possible NullReferenceException if ctx.User is null
Consider this improved implementation:
public static Guid GetUserId(this HttpContext ctx)
{
+ if (ctx?.User?.Identity?.IsAuthenticated != true)
+ throw new UnauthorizedAccessException("User is not authenticated");
+
var userId = ctx.User.FindFirstValue(ClaimTypes.NameIdentifier);
-
userId ??= ctx.User.FindFirstValue("id");
if (Guid.TryParse(userId, out var result))
return result;
- throw new FormatException($"UserId by '{ClaimTypes.NameIdentifier} claim has value: '{userId}' - incorrect guid");
+ throw new FormatException($"Invalid user ID format. Value '{userId}' could not be parsed as GUID");
}Let's verify how this method is used in the codebase:
src/Argon.Api/Features/MediaStorage/YandexContentDeliveryNetwork.cs (1)
38-38: Verify security implications of removing IP-based restrictions.
The removal of IP-based restrictions from signed URLs simplifies the implementation but might affect security. While the URLs are still protected by expiration time and signatures, consider documenting this architectural decision and its implications.
Let's verify if this change is consistent across the codebase and if there are any security compensating controls:
✅ Verification successful
Removal of IP-based restrictions is an acceptable security trade-off
The codebase analysis shows that:
- The interface
IContentDeliveryNetworkdoesn't mandate IP-based restrictions - The signed URL generation is consistently implemented using expiration time and secret signing
- The system uses SimpleS3 authentication mechanisms for the underlying storage
The removal of IP restrictions simplifies the API while maintaining core security through time-based expiration and cryptographic signing, which is a common and secure approach for CDN implementations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for IP-based security controls and documentation
# Check for any IP-based security controls in the codebase
rg -l "(?i)(ip.*restrict|restrict.*ip|whitelist.*ip|ip.*whitelist)"
# Look for security documentation
fd -g "*security*.md" -g "*readme*.md" -x cat {}
Length of output: 221
Script:
#!/bin/bash
# Let's try a more comprehensive analysis
# 1. Check for any IP-based security controls with broader pattern
rg -i "ip.{0,20}(restrict|allow|block|check)"
# 2. Look for documentation files correctly
fd "README" --type f
fd "SECURITY" --type f
# 3. Check the interface and its implementations
ast-grep --pattern 'interface IContentDeliveryNetwork {
$$$
}'
# 4. Look for other CDN implementations
fd -e cs -x rg -l "class.*implements.*IContentDeliveryNetwork"
# 5. Check for any security-related configurations
fd -e json -e xml -e config -x rg -l "security|authentication"
Length of output: 288
Script:
#!/bin/bash
# Let's focus on the specific CDN implementation and interface
# 1. Find and show the interface definition
rg -A 10 "interface IContentDeliveryNetwork"
# 2. Look for all implementations of GenerateSignedLink
rg -A 5 "GenerateSignedLink"
# 3. Check for any security-related comments or documentation in the CDN files
rg -i "security|authentication|authorization" src/Argon.Api/Features/MediaStorage/
Length of output: 2204
src/Argon.Api/Features/MediaStorage/AssetId.cs (1)
23-34: LGTM! Improved readability of dictionary initialization
The new formatting makes the dictionary initialization more readable and maintainable.
src/Argon.Api/Program.cs (1)
32-32: Verify CDN integration with HttpContext.
The HttpContextAccessor registration looks good and aligns with the changes in FilesController and CloudflareCaptcha. However, since this PR focuses on CDN improvements, let's verify the CDN integration.
src/Argon.Api/Controllers/FilesController.cs (2)
4-14: LGTM! Constructor changes follow DI best practices
The addition of IContentTypeProvider and related using statements properly supports the new MIME type detection functionality.
Also applies to: 20-21
63-64: Verify JWT authentication configuration
The endpoint uses JWT authentication. Let's verify the authentication setup and user ID claim configuration.
✅ Verification successful
JWT authentication and user ID claim configuration are properly set up
The verification shows that:
- JWT authentication is correctly configured in
JwtFeature.cswith proper default schemes and token validation - User ID retrieval is implemented in
HttpContextExtensions.GetUserId()with fallback support:- Primary:
ClaimTypes.NameIdentifier - Fallback:
idclaim
- Primary:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check JWT authentication configuration and user ID claim setup
rg -A 10 "AddJwtBearer|JwtBearerDefaults"
rg -A 5 "GetUserId"
Length of output: 4426
src/Argon.Api/Features/MediaStorage/CdnFeatureExtensions.cs (1)
10-11: LGTM: Using directives are properly organized
The new using directives are necessary for the added functionality and follow proper organization (framework imports after domain imports).
| builder.Services.AddAuthentication(options => | ||
| { | ||
| options.DefaultAuthenticateScheme = JwtBearerDefaults.AuthenticationScheme; | ||
| options.DefaultChallengeScheme = JwtBearerDefaults.AuthenticationScheme; | ||
| }) | ||
| .AddJwtBearer((options) => options.TokenValidationParameters = tokenValidator); | ||
| .AddJwtBearer(JwtBearerDefaults.AuthenticationScheme, (options) => options.TokenValidationParameters = tokenValidator); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance JWT configuration with CDN-relevant security headers.
Given this PR's focus on CDN improvements, consider adding security headers to protect against token theft and ensure secure content delivery.
Add these security-focused options to the JWT bearer configuration:
.AddJwtBearer(JwtBearerDefaults.AuthenticationScheme, (options) =>
{
options.TokenValidationParameters = tokenValidator;
+ options.IncludeErrorDetails = false; // Security: Don't expose unnecessary error details
+ options.RequireHttpsMetadata = true; // Ensure HTTPS in production
+ options.SaveToken = true; // Required for CDN token forwarding
+ options.Events = new JwtBearerEvents
+ {
+ OnMessageReceived = context =>
+ {
+ // Allow token from CDN-specific custom header
+ var token = context.Request.Headers["X-CDN-Authorization"].FirstOrDefault();
+ if (!string.IsNullOrEmpty(token))
+ {
+ context.Token = token;
+ }
+ return Task.CompletedTask;
+ }
+ };
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| .AddJwtBearer(JwtBearerDefaults.AuthenticationScheme, (options) => options.TokenValidationParameters = tokenValidator); | |
| .AddJwtBearer(JwtBearerDefaults.AuthenticationScheme, (options) => | |
| { | |
| options.TokenValidationParameters = tokenValidator; | |
| options.IncludeErrorDetails = false; // Security: Don't expose unnecessary error details | |
| options.RequireHttpsMetadata = true; // Ensure HTTPS in production | |
| options.SaveToken = true; // Required for CDN token forwarding | |
| options.Events = new JwtBearerEvents | |
| { | |
| OnMessageReceived = context => | |
| { | |
| // Allow token from CDN-specific custom header | |
| var token = context.Request.Headers["X-CDN-Authorization"].FirstOrDefault(); | |
| if (!string.IsNullOrEmpty(token)) | |
| { | |
| context.Token = token; | |
| } | |
| return Task.CompletedTask; | |
| } | |
| }; | |
| }); |
| { | ||
| var expires = DateTimeOffset.UtcNow.ToUnixTimeSeconds() + expiryInSeconds; | ||
| var tokenData = expires + path + (ip ?? "") + secret; | ||
| var tokenData = $"{expires}{path} {secret}"; |
There was a problem hiding this comment.
Security: Strengthen the token generation implementation.
The current implementation has several security considerations:
- MD5 is cryptographically weak and should be replaced with a stronger algorithm.
- Manual Base64 URL-safe encoding could be replaced with built-in methods.
- String concatenation of sensitive data might be vulnerable to timing attacks.
Consider applying these security improvements:
- var tokenData = $"{expires}{path} {secret}";
-
- using var md5 = MD5.Create();
- var hashBytes = md5.ComputeHash(Encoding.UTF8.GetBytes(tokenData));
- var token = Convert.ToBase64String(hashBytes)
- .Replace("\n", "")
- .Replace("+", "-")
- .Replace("/", "_")
- .Replace("=", "");
+ byte[] pathBytes = Encoding.UTF8.GetBytes(path);
+ byte[] expiresBytes = BitConverter.GetBytes(expires);
+
+ using var hmac = new HMACSHA256(Encoding.UTF8.GetBytes(secret));
+ hmac.TransformBlock(expiresBytes, 0, expiresBytes.Length, null, 0);
+ hmac.TransformFinalBlock(pathBytes, 0, pathBytes.Length);
+
+ var token = WebEncoders.Base64UrlEncode(hmac.Hash!);Committable suggestion skipped: line range outside the PR's diff.
| public static AssetId Avatar() => new(Guid.NewGuid(), AssetScope.ProfileAsset, AssetKind.Image, "png"); | ||
| public static AssetId VideoAvatar() => new(Guid.NewGuid(), AssetScope.ProfileAsset, AssetKind.VideoNoSound, "mp4"); |
There was a problem hiding this comment.
💡 Codebase verification
Inconsistency found between VideoAvatar implementation and AssetKind enum documentation
The verification confirms there's a clear discrepancy:
AssetKind.VideoNoSoundenum is documented to use "gif" format in the commentVideoAvatar()method implements it with "mp4" format
This inconsistency needs to be resolved by either:
- Updating the enum comment to reflect the actual mp4 implementation, or
- Changing the VideoAvatar implementation to use gif format as per the enum documentation
The suggestion to make extensions configurable remains valid as it would provide more flexibility.
🔗 Analysis chain
Resolve discrepancy between enum comment and implementation
There's a mismatch between the AssetKind.VideoNoSound enum comment (which suggests "gif") and the VideoAvatar() implementation (which uses "mp4").
Additionally, consider making the extensions configurable:
+ private const string DEFAULT_IMAGE_EXT = "png";
+ private const string DEFAULT_VIDEO_EXT = "mp4";
+
- public static AssetId Avatar() => new(Guid.NewGuid(), AssetScope.ProfileAsset, AssetKind.Image, "png");
- public static AssetId VideoAvatar() => new(Guid.NewGuid(), AssetScope.ProfileAsset, AssetKind.VideoNoSound, "mp4");
+ public static AssetId Avatar(string extension = DEFAULT_IMAGE_EXT)
+ => new(Guid.NewGuid(), AssetScope.ProfileAsset, AssetKind.Image, extension);
+ public static AssetId VideoAvatar(string extension = DEFAULT_VIDEO_EXT)
+ => new(Guid.NewGuid(), AssetScope.ProfileAsset, AssetKind.VideoNoSound, extension);🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any other usages of VideoNoSound to understand the correct format
rg -l "VideoNoSound" | xargs rg -A 5 "VideoNoSound"
Length of output: 311
| public readonly struct AssetId(Guid assetId, AssetScope scope, AssetKind kind, string extensions) | ||
| { | ||
| public string ToFileId() | ||
| => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00"; // last two zero reserved | ||
| => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00.{extensions}"; // last two zero reserved |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the extensions parameter
The extensions parameter should be validated to prevent potential issues with empty or invalid file extensions.
public readonly struct AssetId(Guid assetId, AssetScope scope, AssetKind kind, string extensions)
{
+ private readonly string _extensions = string.IsNullOrWhiteSpace(extensions)
+ ? throw new ArgumentException("Extension cannot be empty", nameof(extensions))
+ : extensions.TrimStart('.');
+
public string ToFileId()
- => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00.{extensions}";
+ => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00.{_extensions}";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public readonly struct AssetId(Guid assetId, AssetScope scope, AssetKind kind, string extensions) | |
| { | |
| public string ToFileId() | |
| => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00"; // last two zero reserved | |
| => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00.{extensions}"; // last two zero reserved | |
| public readonly struct AssetId(Guid assetId, AssetScope scope, AssetKind kind, string extensions) | |
| { | |
| private readonly string _extensions = string.IsNullOrWhiteSpace(extensions) | |
| ? throw new ArgumentException("Extension cannot be empty", nameof(extensions)) | |
| : extensions.TrimStart('.'); | |
| public string ToFileId() | |
| => $"{assetId:D}-{((byte)scope):X2}-{((byte)kind):X2}-00.{_extensions}"; // last two zero reserved |
| var ext = fileId.Split('.').Last(); | ||
| return new AssetId(guid, (AssetScope)scope, (AssetKind)kind, ext); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize extension extraction and add validation
The current implementation has potential performance and reliability issues:
- Using
Split()creates unnecessary array allocations - No validation for the presence of an extension
- Could break with multiple dots in filename
- var ext = fileId.Split('.').Last();
- return new AssetId(guid, (AssetScope)scope, (AssetKind)kind, ext);
+ var lastDotIndex = fileId.LastIndexOf('.');
+ if (lastDotIndex == -1 || lastDotIndex == fileId.Length - 1)
+ throw new InvalidOperationException("File ID must contain a valid extension");
+ var ext = fileId[(lastDotIndex + 1)..];
+ return new AssetId(guid, (AssetScope)scope, (AssetKind)kind, ext);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ext = fileId.Split('.').Last(); | |
| return new AssetId(guid, (AssetScope)scope, (AssetKind)kind, ext); | |
| var lastDotIndex = fileId.LastIndexOf('.'); | |
| if (lastDotIndex == -1 || lastDotIndex == fileId.Length - 1) | |
| throw new InvalidOperationException("File ID must contain a valid extension"); | |
| var ext = fileId[(lastDotIndex + 1)..]; | |
| return new AssetId(guid, (AssetScope)scope, (AssetKind)kind, ext); |
| var ns = new StorageNameSpace(nsPath, nsId); | ||
| var assetId = AssetId.FromFileId(fileId); | ||
| var mem = DiskContentStorage.OpenFileRead(ns, assetId); | ||
|
|
||
| return File(mem, assetId.GetMime()); | ||
| if (contentType.TryGetContentType(fileId, out var mime)) | ||
| return File(mem, mime); | ||
| return File(mem, "application/octet-stream"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null check and error handling for file operations
While the MIME type detection is well implemented, consider adding proper error handling for cases where the file doesn't exist or cannot be read.
var ns = new StorageNameSpace(nsPath, nsId);
var assetId = AssetId.FromFileId(fileId);
+try {
var mem = DiskContentStorage.OpenFileRead(ns, assetId);
+ if (mem == null)
+ return NotFound();
if (contentType.TryGetContentType(fileId, out var mime))
return File(mem, mime);
return File(mem, "application/octet-stream");
+} catch (IOException ex) {
+ return StatusCode(500, "Unable to read file");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var ns = new StorageNameSpace(nsPath, nsId); | |
| var assetId = AssetId.FromFileId(fileId); | |
| var mem = DiskContentStorage.OpenFileRead(ns, assetId); | |
| return File(mem, assetId.GetMime()); | |
| if (contentType.TryGetContentType(fileId, out var mime)) | |
| return File(mem, mime); | |
| return File(mem, "application/octet-stream"); | |
| var ns = new StorageNameSpace(nsPath, nsId); | |
| var assetId = AssetId.FromFileId(fileId); | |
| try { | |
| var mem = DiskContentStorage.OpenFileRead(ns, assetId); | |
| if (mem == null) | |
| return NotFound(); | |
| if (contentType.TryGetContentType(fileId, out var mime)) | |
| return File(mem, mime); | |
| return File(mem, "application/octet-stream"); | |
| } catch (IOException ex) { | |
| return StatusCode(500, "Unable to read file"); | |
| } |
| [HttpPost("/files/user/@me/avatar"), Authorize(JwtBearerDefaults.AuthenticationScheme)] | ||
| public async ValueTask<IActionResult> UploadUserAvatar(IFormFile file) | ||
| { | ||
| // TODO | ||
| if (!permissions.CanAccess("user.avatar.upload", PropertyBag.Empty.Set(userId))) | ||
| return StatusCode(401); | ||
| var userId = HttpContext.GetUserId(); | ||
| var assetId = AssetId.Avatar(); | ||
| var result = await cdn.CreateAssetAsync(StorageNameSpace.ForUser(userId), assetId, file); | ||
| var ns = StorageNameSpace.ForUser(userId); | ||
| var result = await cdn.CreateAssetAsync(ns, assetId, file); | ||
|
|
||
| if (result.HasValue) | ||
| return Ok(result); | ||
|
|
||
| await grainFactory.GetGrain<IUserGrain>(userId) | ||
| .UpdateUser(new UserEditInput(null, null, assetId.ToFileId())); | ||
|
|
||
| return Ok(); | ||
| return Ok(await cdn.GenerateAssetUrl(ns, assetId)); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add file validation for avatar uploads
While the user ID handling has been improved, consider adding file validation to ensure security and consistency.
public async ValueTask<IActionResult> UploadUserAvatar(IFormFile file)
{
+ if (file == null || file.Length == 0)
+ return BadRequest("No file uploaded");
+
+ if (file.Length > 5 * 1024 * 1024) // 5MB limit
+ return BadRequest("File size exceeds limit");
+
+ if (!file.ContentType.StartsWith("image/"))
+ return BadRequest("Only image files are allowed");
+
var userId = HttpContext.GetUserId();
// ... rest of the method📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [HttpPost("/files/user/@me/avatar"), Authorize(JwtBearerDefaults.AuthenticationScheme)] | |
| public async ValueTask<IActionResult> UploadUserAvatar(IFormFile file) | |
| { | |
| // TODO | |
| if (!permissions.CanAccess("user.avatar.upload", PropertyBag.Empty.Set(userId))) | |
| return StatusCode(401); | |
| var userId = HttpContext.GetUserId(); | |
| var assetId = AssetId.Avatar(); | |
| var result = await cdn.CreateAssetAsync(StorageNameSpace.ForUser(userId), assetId, file); | |
| var ns = StorageNameSpace.ForUser(userId); | |
| var result = await cdn.CreateAssetAsync(ns, assetId, file); | |
| if (result.HasValue) | |
| return Ok(result); | |
| await grainFactory.GetGrain<IUserGrain>(userId) | |
| .UpdateUser(new UserEditInput(null, null, assetId.ToFileId())); | |
| return Ok(); | |
| return Ok(await cdn.GenerateAssetUrl(ns, assetId)); | |
| [HttpPost("/files/user/@me/avatar"), Authorize(JwtBearerDefaults.AuthenticationScheme)] | |
| public async ValueTask<IActionResult> UploadUserAvatar(IFormFile file) | |
| { | |
| if (file == null || file.Length == 0) | |
| return BadRequest("No file uploaded"); | |
| if (file.Length > 5 * 1024 * 1024) // 5MB limit | |
| return BadRequest("File size exceeds limit"); | |
| if (!file.ContentType.StartsWith("image/")) | |
| return BadRequest("Only image files are allowed"); | |
| var userId = HttpContext.GetUserId(); | |
| var assetId = AssetId.Avatar(); | |
| var ns = StorageNameSpace.ForUser(userId); | |
| var result = await cdn.CreateAssetAsync(ns, assetId, file); | |
| if (result.HasValue) | |
| return Ok(result); | |
| await grainFactory.GetGrain<IUserGrain>(userId) | |
| .UpdateUser(new UserEditInput(null, null, assetId.ToFileId())); | |
| return Ok(await cdn.GenerateAssetUrl(ns, assetId)); |
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation