feat: add profile picture upload support with S3 presigned URLs#222
feat: add profile picture upload support with S3 presigned URLs#222sasidaran-99 wants to merge 3 commits into
Conversation
|
Thank you for your contribution! Before we can merge this PR, we need you to sign our Contributor License Agreement. I have read the CLA and agree to its terms. You can retrigger this bot by commenting recheck in this Pull Request. Posted by the CLA Assistant Lite bot. |
|
Warning Review limit reached
More reviews will be available in 15 minutes and 58 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds AWS S3-backed profile image upload support: a new ChangesS3 Profile Image Upload
Sequence Diagram(s)sequenceDiagram
participant Client
participant AuthHandler
participant StorageService
participant AWSS3
Client->>AuthHandler: POST /api/auth/profile/upload-url {fileName}
AuthHandler->>AuthHandler: Extract userID from Gin context
AuthHandler->>StorageService: GenerateUploadURL(userID, fileName)
StorageService->>AWSS3: PresignPutObject(users/{userID}/{fileName}, 15min)
AWSS3-->>StorageService: presignedURL
StorageService-->>AuthHandler: presignedURL, publicFileURL
AuthHandler-->>Client: 200 {uploadUrl, fileUrl}
Client->>AWSS3: PUT presignedURL (file bytes directly)
AWSS3-->>Client: 200 OK
Client->>AuthHandler: PATCH /api/auth/profile {profileImage: fileUrl}
AuthHandler->>AuthHandler: UpdateProfile(req with profileImage)
AuthHandler-->>Client: 200 updated profile
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/config/config.go`:
- Around line 206-211: The StorageConfig initialization in LoadConfig() loads
AWS_S3_BUCKET and AWS_REGION from environment variables but does not validate
that these required fields are set. Add validation after loading these fields to
ensure they are not empty strings, using log.Fatal to fail at startup with a
descriptive error message, following the same validation pattern already
implemented for JWT secrets and encryption key validation elsewhere in the
LoadConfig() function. Extract the bucket and region values into variables, then
add conditional checks that call log.Fatal with appropriate error messages if
either field is empty.
In `@internal/handler/auth_handler.go`:
- Around line 199-233: The GenerateProfileUploadURL method in the AuthHandler
struct is missing Swagger annotations that document the API endpoint. Add
Swagger documentation comments above the GenerateProfileUploadURL method to
describe the endpoint, including request parameters, response types, possible
error codes, and any security requirements. After adding the annotations, run
the `make swagger` command to regenerate the API documentation files.
- Around line 202-220: The error responses in this handler are using
inconsistent utility functions (UnauthorizedResponse, ValidationErrorResponse,
ErrorResponse) that do not conform to the required JSON error schema. Replace
all error response calls with direct JSON structures that return the required
format with "error" and "code" fields. For the 401 Unauthorized response, use an
appropriate error code like UNAUTHORIZED; for the 400 Bad Request validation
error, use VALIDATION_ERROR or similar; and for the 500 Internal Server Error
when GenerateUploadURL fails, use an appropriate code like
UPLOAD_URL_GENERATION_FAILED. Ensure all three error response branches (around
StatusUnauthorized, StatusBadRequest, and StatusInternalServerError) follow the
consistent {"error": "message", "code": "ERROR_CODE"} format.
- Around line 200-215: The code checks if userID exists in the context but fails
to validate its type before assertion. After verifying that userID exists using
c.Get("userID"), add a type assertion with the comma-ok idiom to safely check if
userID is a string type. If the type assertion fails, return an appropriate
error response such as http.StatusUnauthorized before attempting to use userID
in the GenerateUploadURL method call, preventing a panic if the context value is
not a string.
In `@internal/routes/routes.go`:
- Around line 78-88: The SetupRoutes function calls
service.NewStorageService(...) unconditionally, which panics when AWS config
loading fails, causing a process crash during startup. Modify the
NewStorageService call to handle potential errors by checking the return value
for an error (the constructor likely returns both a service instance and an
error). Return or propagate this error from SetupRoutes instead of allowing the
panic to crash the application, ensuring graceful error handling rather than
panic-driven startup failures.
In `@internal/service/auth_service.go`:
- Around line 222-224: The profile image validation in the update block accepts
any non-empty string for req.ProfileImage without verifying it originates from
the configured S3 bucket/region, allowing clients to persist arbitrary external
URLs and bypass the presigned-upload mechanism. Before adding profile_image to
the updates map, validate that req.ProfileImage is a trusted S3 URL matching
your configured bucket and region (and user prefix if applicable) to ensure only
authorized presigned URLs are persisted.
In `@internal/service/storage_service.go`:
- Around line 37-59: The GenerateUploadURL method uses context.TODO() for the S3
presign operation, which decouples it from the HTTP request lifecycle and
prevents proper cancellation and timeout handling. Update the GenerateUploadURL
method signature to accept a context parameter as the first argument (after the
receiver), replace the context.TODO() call in the presign operation with the
passed context parameter, and then update the handler in
internal/handler/auth_handler.go to pass the request context when calling
GenerateUploadURL.
- Around line 37-46: The GenerateUploadURL method accepts fileName directly
without validation, creating security risks for path traversal, empty filenames,
and invalid characters. Add validation at the beginning of the GenerateUploadURL
function to check that fileName is not empty, does not contain path traversal
patterns like "../" or "./", does not contain forward slashes that would break
the intended key structure, and is within acceptable length limits. Return an
appropriate error if any validation fails. Add the "strings" package to the
imports if it is not already present to support these validation checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0536e886-5701-480f-8d4b-929cd425788c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
.env.examplego.modinternal/config/config.gointernal/dto/auth_dto.gointernal/dto/upload.gointernal/handler/auth_handler.gointernal/handler/auth_handler_protected_test.gointernal/handler/auth_handler_test.gointernal/routes/routes.gointernal/service/auth_service.gointernal/service/storage_service.go
| Storage: StorageConfig{ | ||
| Bucket: getEnv("AWS_S3_BUCKET", ""), | ||
| Region: getEnv("AWS_REGION", ""), | ||
| AccessKeyID: getEnv("AWS_ACCESS_KEY_ID", ""), | ||
| SecretAccessKey: getEnv("AWS_SECRET_ACCESS_KEY", ""), | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Validate required AWS configuration fields before runtime errors occur.
The AWS_S3_BUCKET and AWS_REGION fields are not validated in LoadConfig(), unlike the JWT and encryption key validation at lines 135–140. An empty AWS_S3_BUCKET will silently reach the service layer and fail with a confusing S3 API error during presign requests, instead of failing loudly at startup.
Add validation to ensure these fields are set, mirroring the pattern used for JWT secrets:
bucket := getEnv("AWS_S3_BUCKET", "")
region := getEnv("AWS_REGION", "")
if bucket == "" {
log.Fatal("AWS_S3_BUCKET must be set")
}
if region == "" {
log.Fatal("AWS_REGION must be set")
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/config/config.go` around lines 206 - 211, The StorageConfig
initialization in LoadConfig() loads AWS_S3_BUCKET and AWS_REGION from
environment variables but does not validate that these required fields are set.
Add validation after loading these fields to ensure they are not empty strings,
using log.Fatal to fail at startup with a descriptive error message, following
the same validation pattern already implemented for JWT secrets and encryption
key validation elsewhere in the LoadConfig() function. Extract the bucket and
region values into variables, then add conditional checks that call log.Fatal
with appropriate error messages if either field is empty.
| c.JSON(http.StatusUnauthorized, utils.UnauthorizedResponse("Unauthorized")) | ||
| return | ||
| } | ||
|
|
||
| var req dto.UploadURLRequest | ||
|
|
||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| c.JSON(http.StatusBadRequest, utils.ValidationErrorResponse(err.Error())) | ||
| return | ||
| } | ||
|
|
||
| uploadURL, fileURL, err := h.storageService.GenerateUploadURL( | ||
| userID.(string), | ||
| req.FileName, | ||
| ) | ||
|
|
||
| if err != nil { | ||
| c.JSON(http.StatusInternalServerError, | ||
| utils.ErrorResponse("Failed to generate upload URL", err)) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Return handler errors using the required {error, code} JSON shape.
This endpoint currently emits helper responses with different schemas across 401/400/500 branches, which violates the handler error contract.
As per coding guidelines, internal/handler/**/*.go: Return JSON error responses in format {"error": "message", "code": "ERROR_CODE"} from HTTP handlers.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/handler/auth_handler.go` around lines 202 - 220, The error responses
in this handler are using inconsistent utility functions (UnauthorizedResponse,
ValidationErrorResponse, ErrorResponse) that do not conform to the required JSON
error schema. Replace all error response calls with direct JSON structures that
return the required format with "error" and "code" fields. For the 401
Unauthorized response, use an appropriate error code like UNAUTHORIZED; for the
400 Bad Request validation error, use VALIDATION_ERROR or similar; and for the
500 Internal Server Error when GenerateUploadURL fails, use an appropriate code
like UPLOAD_URL_GENERATION_FAILED. Ensure all three error response branches
(around StatusUnauthorized, StatusBadRequest, and StatusInternalServerError)
follow the consistent {"error": "message", "code": "ERROR_CODE"} format.
Source: Coding guidelines
| storageService := service.NewStorageService( | ||
| cfg.Storage.Bucket, | ||
| cfg.Storage.Region, | ||
| ) | ||
|
|
||
| authHandler := handler.NewAuthHandler( | ||
| authService, | ||
| oauthService, | ||
| oauthProviderService, | ||
| storageService, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Avoid panic-driven startup failure from storage initialization.
SetupRoutes now unconditionally calls service.NewStorageService(...), and the provided constructor panics on AWS config load errors. A bad/missing storage config will crash the process during route setup.
Suggested direction
-// service/storage_service.go
-func NewStorageService(bucket, region string) *StorageService {
+func NewStorageService(bucket, region string) (*StorageService, error) {
cfg, err := awsconfig.LoadDefaultConfig(
context.TODO(),
awsconfig.WithRegion(region),
)
if err != nil {
- panic(err)
+ return nil, err
}
client := s3.NewFromConfig(cfg)
- return &StorageService{bucket: bucket, region: region, client: client}
+ return &StorageService{bucket: bucket, region: region, client: client}, nil
}
-// routes.go
-storageService := service.NewStorageService(cfg.Storage.Bucket, cfg.Storage.Region)
+storageService, err := service.NewStorageService(cfg.Storage.Bucket, cfg.Storage.Region)
+if err != nil {
+ log.Fatalf("failed to initialize storage service: %v", err)
+}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/routes/routes.go` around lines 78 - 88, The SetupRoutes function
calls service.NewStorageService(...) unconditionally, which panics when AWS
config loading fails, causing a process crash during startup. Modify the
NewStorageService call to handle potential errors by checking the return value
for an error (the constructor likely returns both a service instance and an
error). Return or propagate this error from SetupRoutes instead of allowing the
panic to crash the application, ensuring graceful error handling rather than
panic-driven startup failures.
| if req.ProfileImage != "" { | ||
| updates["profile_image"] = req.ProfileImage | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Restrict profileImage to trusted S3 URLs before persisting.
This branch stores any non-empty string, so clients can bypass the presigned-upload path and persist arbitrary external URLs as profile images. Validate against your configured bucket/region (and user prefix) before writing profile_image.
Suggested fix
+import (
+ "fmt"
+ "strings"
+)
...
if req.ProfileImage != "" {
+ expectedPrefix := fmt.Sprintf(
+ "https://%s.s3.%s.amazonaws.com/users/%s/",
+ s.config.Storage.Bucket,
+ s.config.Storage.Region,
+ userID,
+ )
+ if !strings.HasPrefix(req.ProfileImage, expectedPrefix) {
+ return nil, errors.New("invalid profile image url")
+ }
updates["profile_image"] = req.ProfileImage
}📝 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.
| if req.ProfileImage != "" { | |
| updates["profile_image"] = req.ProfileImage | |
| } | |
| import ( | |
| "fmt" | |
| "strings" | |
| ) | |
| ... | |
| if req.ProfileImage != "" { | |
| expectedPrefix := fmt.Sprintf( | |
| "https://%s.s3.%s.amazonaws.com/users/%s/", | |
| s.config.Storage.Bucket, | |
| s.config.Storage.Region, | |
| userID, | |
| ) | |
| if !strings.HasPrefix(req.ProfileImage, expectedPrefix) { | |
| return nil, errors.New("invalid profile image url") | |
| } | |
| updates["profile_image"] = req.ProfileImage | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/auth_service.go` around lines 222 - 224, The profile image
validation in the update block accepts any non-empty string for req.ProfileImage
without verifying it originates from the configured S3 bucket/region, allowing
clients to persist arbitrary external URLs and bypass the presigned-upload
mechanism. Before adding profile_image to the updates map, validate that
req.ProfileImage is a trusted S3 URL matching your configured bucket and region
(and user prefix if applicable) to ensure only authorized presigned URLs are
persisted.
| func (s *StorageService) GenerateUploadURL( | ||
| userID string, | ||
| fileName string, | ||
| ) (string, string, error) { | ||
|
|
||
| objectKey := fmt.Sprintf( | ||
| "users/%s/%s", | ||
| userID, | ||
| fileName, | ||
| ) | ||
|
|
||
| presignClient := s3.NewPresignClient(s.client) | ||
|
|
||
| req, err := presignClient.PresignPutObject( | ||
| context.TODO(), | ||
| &s3.PutObjectInput{ | ||
| Bucket: &s.bucket, | ||
| Key: &objectKey, | ||
| }, | ||
| func(opts *s3.PresignOptions) { | ||
| opts.Expires = 15 * time.Minute | ||
| }, | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Pass request context from handler to service for proper lifecycle management.
GenerateUploadURL uses context.TODO() for the S3 presign operation (line 51), which decouples the presign request from the HTTP request lifecycle. This means:
- HTTP request cancellation does not cancel the S3 operation (resource leak).
- There is no timeout mechanism for the S3 call (potential hangs).
- The service violates Go best practices for passing request context through layers.
Update the method signature to accept a context parameter:
💡 Proposed fix
func (s *StorageService) GenerateUploadURL(
+ ctx context.Context,
userID string,
fileName string,
) (string, string, error) {
// ...
req, err := presignClient.PresignPutObject(
- context.TODO(),
+ ctx,
&s3.PutObjectInput{And update the handler call (in internal/handler/auth_handler.go) to pass the request context:
- uploadURL, fileURL, err := h.storageService.GenerateUploadURL(
+ uploadURL, fileURL, err := h.storageService.GenerateUploadURL(
+ c.Request.Context(),
userID.(string),
req.FileName,
)📝 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.
| func (s *StorageService) GenerateUploadURL( | |
| userID string, | |
| fileName string, | |
| ) (string, string, error) { | |
| objectKey := fmt.Sprintf( | |
| "users/%s/%s", | |
| userID, | |
| fileName, | |
| ) | |
| presignClient := s3.NewPresignClient(s.client) | |
| req, err := presignClient.PresignPutObject( | |
| context.TODO(), | |
| &s3.PutObjectInput{ | |
| Bucket: &s.bucket, | |
| Key: &objectKey, | |
| }, | |
| func(opts *s3.PresignOptions) { | |
| opts.Expires = 15 * time.Minute | |
| }, | |
| ) | |
| func (s *StorageService) GenerateUploadURL( | |
| ctx context.Context, | |
| userID string, | |
| fileName string, | |
| ) (string, string, error) { | |
| objectKey := fmt.Sprintf( | |
| "users/%s/%s", | |
| userID, | |
| fileName, | |
| ) | |
| presignClient := s3.NewPresignClient(s.client) | |
| req, err := presignClient.PresignPutObject( | |
| ctx, | |
| &s3.PutObjectInput{ | |
| Bucket: &s.bucket, | |
| Key: &objectKey, | |
| }, | |
| func(opts *s3.PresignOptions) { | |
| opts.Expires = 15 * time.Minute | |
| }, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/service/storage_service.go` around lines 37 - 59, The
GenerateUploadURL method uses context.TODO() for the S3 presign operation, which
decouples it from the HTTP request lifecycle and prevents proper cancellation
and timeout handling. Update the GenerateUploadURL method signature to accept a
context parameter as the first argument (after the receiver), replace the
context.TODO() call in the presign operation with the passed context parameter,
and then update the handler in internal/handler/auth_handler.go to pass the
request context when calling GenerateUploadURL.
|
I have read the CLA and agree to its terms |
|
|
@roshankumar0036singh
I have also signed the CLA by posting the required agreement comment. The CLA Assistant workflow appears to be awaiting maintainer approval to run. Could someone please approve and rerun it when convenient? |
|
@sasidaran-99 kindly do the following tasks to get your pr merged:
|
2 participants
Summary
This PR adds profile picture support by enabling secure direct-to-cloud uploads using AWS S3 presigned URLs.
Changes Made
StorageServicefor generating AWS S3 presigned upload URLs.How It Works
Acceptance Criteria
Testing
go build ./... go test ./...Both commands complete successfully.
Notes
This implementation follows a direct-to-cloud upload approach, reducing backend load and improving scalability while maintaining secure uploads through time-limited presigned URLs.
Summary by CodeRabbit
Release Notes