feat(avatar): validate user and org avatar fields#1734
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR adds an avatar validation package for base64 JPEG/PNG data URLs, integrates validation into organization and user services and their Connect API handlers (mapping validation failures to CodeInvalidArgument), threads avatar configuration through server config and wiring, and adds UI security headers middleware. ChangesAvatar Validation Feature
UI Security Headers
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
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 |
Avatars must now be base64 JPEG or PNG data URLs within a configurable size cap (app.avatar.max_size_bytes, default 1MB). Validation runs in the user and organization services, and the admin UI server now sets a Content-Security-Policy header for images. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4a3b4c5 to
a677dbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
pkg/server/security_headers.go (1)
7-9: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider expanding beyond
img-srcfor defense-in-depth.Only
img-srcis constrained; other CSP directives fall back to permissive defaults, so scripts/frames/etc. remain unrestricted. Since the map-based design already makes this trivial to extend, consider also adding common hardening headers likeX-Content-Type-Options: nosniffandReferrer-Policy.♻️ Optional expansion
var uiSecurityHeaders = map[string]string{ "Content-Security-Policy": "img-src 'self' data: https:", + "X-Content-Type-Options": "nosniff", + "Referrer-Policy": "strict-origin-when-cross-origin", }internal/api/v1beta1connect/user_test.go (1)
127-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd matching coverage for
UpdateUser/UpdateCurrentUseravatar mapping.Only
CreateUsergets a test for the newavatar.ErrInvalid→CodeInvalidArgumentmapping, even though the samecase errors.Is(err, avatar.ErrInvalid)branch was also added toUpdateUserandUpdateCurrentUserin this file. A regression in either of those switches wouldn't be caught by this file's tests.Do you want me to generate the two additional test cases mirroring this one for
UpdateUserandUpdateCurrentUser?
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 384eacbe-427f-4ccb-ae1f-0263bac38e32
📒 Files selected for processing (15)
cmd/serve.gocore/avatar/avatar.gocore/avatar/avatar_test.gocore/organization/service.gocore/organization/service_test.gocore/user/service.gocore/user/service_test.gointernal/api/v1beta1connect/organization.gointernal/api/v1beta1connect/organization_test.gointernal/api/v1beta1connect/user.gointernal/api/v1beta1connect/user_test.gopkg/server/config.gopkg/server/security_headers.gopkg/server/security_headers_test.gopkg/server/server.go
| mockUserSrv := new(mocks.UserService) | ||
| mockUserSrv.EXPECT().Create(mock.Anything, mock.Anything).Return(user.User{}, avatar.ErrInvalid) | ||
| h := &ConnectHandler{userService: mockUserSrv} |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether mocks.NewUserService(t) exists and is the prevailing pattern in this package
rg -n 'func NewUserService' internal/api/v1beta1connect/mocks/user_service.go
rg -n 'mocks\.NewUserService\(t\)|new\(mocks\.UserService\)' internal/api/v1beta1connectRepository: raystack/frontier
Length of output: 2032
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '744,790p' internal/api/v1beta1connect/mocks/user_service.go
printf '\n----\n'
sed -n '100,150p' internal/api/v1beta1connect/user_test.goRepository: raystack/frontier
Length of output: 1864
Use mocks.NewUserService(t) here. new(mocks.UserService) skips the t.Cleanup(func() { mock.AssertExpectations(t) }) wiring, so a missing Create call can let this test pass.
Coverage Report for CI Build 28654358183Coverage increased (+0.08%) to 44.943%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Summary
The avatar field on users and organizations accepted any string with no format or size checks. This PR validates it on all create and update paths.
Changes
core/avatarpackage:avatar.Validateaccepts onlydata:image/jpeg;base64,/data:image/png;base64,values, checks the size cap before decoding, decodes the base64, and confirms the bytes start with the matching JPEG/PNG file signature. Empty values stay allowed.user.ServiceCreate/Update andorganization.ServiceCreate/Update/AdminCreate run the validation before persisting.app.avatar.max_size_bytes(default 1MB, measured on the encoded string); wired throughNewServiceinbuildAPIDependencies.avatar.ErrInvalidtoCodeInvalidArgument.Content-Security-Policy: img-src 'self' data: https:on its responses.Technical Details
avatar.Configlives incore/avatarand is embedded inserver.Config; a zero or negative limit falls back toavatar.DefaultMaxBytes.pkg/server/security_headers.goadds a header map wrapper; the UI server passesuiSecurityHeadersto it.Test Plan
core/avatarvalidator table tests, service rejection tests for user/org, handler mapping tests, security header test.make lint(0 issues) andmake test(race detector) pass.SQL Safety (if your PR touches
*_repository.goorgoqu.*)Not applicable — no repository or query changes.
🤖 Generated with Claude Code