fix: SAST Scan Findings - Security Improvements (#295)#296
Open
Muneerali199 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Open
fix: SAST Scan Findings - Security Improvements (#295)#296Muneerali199 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Muneerali199 wants to merge 2 commits intoAOSSIE-Org:mainfrom
Conversation
- Create GET/PUT /v1/users/me endpoints for profile management - Add database migration with users table and auto-sync trigger - Rewrite ProfilePage with real data integration - Make email field read-only - Add loading states and error handling - Include toast notifications for UX feedback BREAKING CHANGE: Requires database migration (02_create_users_table.sql)
Contributor
📝 WalkthroughWalkthroughIntroduces a complete user profile feature with backend API endpoints (GET/PUT /v1/users/me), database migration with RLS policies and auto-sync triggers, and frontend components to display and edit user profiles with email field read-only protection. Changes
Sequence DiagramsequenceDiagram
participant User as User
participant FE as Frontend<br/>(ProfilePage)
participant API as Frontend API<br/>(apiClient)
participant BE as Backend<br/>(FastAPI)
participant DB as Database<br/>(PostgreSQL)
rect rgba(100, 150, 200, 0.5)
Note over User,DB: Profile Loading Flow
User->>FE: Open Profile Page
FE->>FE: useEffect: loadProfile()
FE->>API: getUserProfile()
API->>BE: GET /v1/users/me
BE->>DB: Query public.users
DB-->>BE: User Profile Data
BE-->>API: UserProfileResponse
API-->>FE: UserProfile Object
FE->>FE: setState(profile, editedProfile)
FE->>User: Display Profile (read-only view)
end
rect rgba(150, 200, 100, 0.5)
Note over User,DB: Profile Update Flow
User->>FE: Click Edit Profile
FE->>FE: setState(isEditing=true)
FE->>User: Display editable form
User->>FE: Modify fields (display_name, bio, location)
FE->>FE: setState(editedProfile)
User->>FE: Click Save Changes
FE->>API: updateUserProfile(editedProfile)
API->>BE: PUT /v1/users/me
BE->>DB: Update public.users
DB-->>BE: Updated Profile
BE-->>API: UserProfileResponse
API-->>FE: Updated UserProfile
FE->>FE: setState(profile, isEditing=false)
FE->>User: Display success toast & updated profile
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/app/api/v1/users.py`:
- Around line 43-47: The handlers in users.py currently map any None from the
management helpers to a 404, which hides backend errors; update the code so you
only raise HTTP 404 when the helper explicitly indicates a "not found" result
(e.g., a NotFound sentinel/exception or an explicit return type), and let
unexpected errors bubble up (or re-raise them) so 5xx handlers run. Concretely,
change the calls that currently do "if not user: raise HTTPException(...)" (the
checks at the top-level handler around the user lookup and the similar checks at
the other two sites) to either (A) have the management helper throw a
NotFoundError for missing profiles and catch only that to convert to HTTP 404,
re-raising other exceptions, or (B) have the helper return a discriminated
result (e.g., {found: True, user} vs {found: False}) and only map the
found=False case to 404; ensure you reference the specific helper function names
used in those handlers when making the change so you don't inadvertently swallow
real errors.
In `@backend/database/02_create_users_table.sql`:
- Around line 64-68: The "Users can update their own profile" RLS policy on
public.users currently lets owners update the entire row; change it to only
allow the intended editable profile fields (e.g., display_name, bio, avatar_url
— replace with your actual allowed columns) by adding column-level restrictions:
update the policy's WITH CHECK to assert auth.uid() = id AND that every
sensitive column (email, email_verified, stats, linked_platform_ids, etc.)
remains equal to its current value (thus preventing changes), or alternatively
implement a BEFORE UPDATE trigger that rejects changes to those protected
columns; update the policy name/definition accordingly (refer to "Users can
update their own profile", public.users, auth.uid()).
- Around line 71-90: This migration only creates the trigger
(public.handle_new_user / on_auth_user_created) for future auth.users inserts,
so backfill existing auth.users now: after creating the trigger, run an INSERT
... SELECT from auth.users into public.users that reproduces the same column
logic as handle_new_user (id, email, display_name using
COALESCE(raw_user_meta_data->>'display_name', split_part(email,'@',1), 'User'),
avatar_url) and use ON CONFLICT DO NOTHING (or equivalent) to avoid
duplicate-key errors; include this backfill step in the migration so current
accounts get public.users rows immediately.
In `@frontend/src/components/pages/ProfilePage.tsx`:
- Around line 159-163: The img tag in ProfilePage.tsx currently leaks
profile.display_name to a third-party avatar service when profile.avatar_url is
falsy; replace that external fallback by using a local avatar solution: either
render a local initials SVG/placeholder component (e.g., create/use an Avatar or
InitialsAvatar component that takes profile.display_name and profile.id and
returns an inline SVG or styled div) and use that when profile.avatar_url is
missing, or proxy the fallback through your backend (e.g., point to
/api/avatar?userId={profile.id} which returns a generated image) so no PII is
sent to external services; update the img/src usage to prefer profile.avatar_url
and fall back to the local Avatar component or backend proxy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e9f6c1ca-e909-4dbb-a900-32ac0411b153
⛔ Files ignored due to path filters (2)
frontend/package-lock.jsonis excluded by!**/package-lock.jsonlanding/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
IMPLEMENTATION_SUMMARY.mdPR_SUMMARY.mdPULL_REQUEST_TEMPLATE.mdbackend/app/api/router.pybackend/app/api/v1/users.pybackend/database/02_create_users_table.sqlfrontend/src/components/pages/ProfilePage.tsxfrontend/src/lib/api.ts
Comment on lines
+43
to
+47
| if not user: | ||
| raise HTTPException( | ||
| status_code=status.HTTP_404_NOT_FOUND, | ||
| detail="User profile not found" | ||
| ) |
Contributor
There was a problem hiding this comment.
Preserve 404 vs. 5xx semantics.
Both management helpers currently return None for “not found” and for caught backend errors, so these branches will misreport operational failures as missing profiles and the 500 handlers here will never run for those cases. Please let the service raise on query failures, or return an explicit result/error type before mapping None to 404.
Also applies to: 94-98, 114-118
🧰 Tools
🪛 Ruff (0.15.4)
[warning] 44-47: Abstract raise to an inner function
(TRY301)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/app/api/v1/users.py` around lines 43 - 47, The handlers in users.py
currently map any None from the management helpers to a 404, which hides backend
errors; update the code so you only raise HTTP 404 when the helper explicitly
indicates a "not found" result (e.g., a NotFound sentinel/exception or an
explicit return type), and let unexpected errors bubble up (or re-raise them) so
5xx handlers run. Concretely, change the calls that currently do "if not user:
raise HTTPException(...)" (the checks at the top-level handler around the user
lookup and the similar checks at the other two sites) to either (A) have the
management helper throw a NotFoundError for missing profiles and catch only that
to convert to HTTP 404, re-raising other exceptions, or (B) have the helper
return a discriminated result (e.g., {found: True, user} vs {found: False}) and
only map the found=False case to 404; ensure you reference the specific helper
function names used in those handlers when making the change so you don't
inadvertently swallow real errors.
Comment on lines
+64
to
+68
| CREATE POLICY "Users can update their own profile" | ||
| ON public.users | ||
| FOR UPDATE | ||
| USING (auth.uid() = id) | ||
| WITH CHECK (auth.uid() = id); |
Contributor
There was a problem hiding this comment.
Restrict writable columns at the database boundary.
The backend only intends a small profile subset to be editable, but this policy authorizes the row owner to update the entire public.users record. That leaves email, verification fields, stats, and linked-platform identifiers writable unless another layer blocks them.
🛡️ Suggested column-level restriction
-- Users can update their own profile
CREATE POLICY "Users can update their own profile"
ON public.users
FOR UPDATE
USING (auth.uid() = id)
WITH CHECK (auth.uid() = id);
+
+REVOKE UPDATE ON public.users FROM authenticated;
+GRANT UPDATE (display_name, avatar_url, bio, location, preferred_languages)
+ ON public.users TO authenticated;📝 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.
Suggested change
| CREATE POLICY "Users can update their own profile" | |
| ON public.users | |
| FOR UPDATE | |
| USING (auth.uid() = id) | |
| WITH CHECK (auth.uid() = id); | |
| CREATE POLICY "Users can update their own profile" | |
| ON public.users | |
| FOR UPDATE | |
| USING (auth.uid() = id) | |
| WITH CHECK (auth.uid() = id); | |
| REVOKE UPDATE ON public.users FROM authenticated; | |
| GRANT UPDATE (display_name, avatar_url, bio, location, preferred_languages) | |
| ON public.users TO authenticated; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/database/02_create_users_table.sql` around lines 64 - 68, The "Users
can update their own profile" RLS policy on public.users currently lets owners
update the entire row; change it to only allow the intended editable profile
fields (e.g., display_name, bio, avatar_url — replace with your actual allowed
columns) by adding column-level restrictions: update the policy's WITH CHECK to
assert auth.uid() = id AND that every sensitive column (email, email_verified,
stats, linked_platform_ids, etc.) remains equal to its current value (thus
preventing changes), or alternatively implement a BEFORE UPDATE trigger that
rejects changes to those protected columns; update the policy name/definition
accordingly (refer to "Users can update their own profile", public.users,
auth.uid()).
Comment on lines
+71
to
+90
| CREATE OR REPLACE FUNCTION public.handle_new_user() | ||
| RETURNS TRIGGER AS $$ | ||
| BEGIN | ||
| INSERT INTO public.users (id, email, display_name, avatar_url) | ||
| VALUES ( | ||
| NEW.id, | ||
| NEW.email, | ||
| COALESCE(NEW.raw_user_meta_data->>'display_name', split_part(NEW.email, '@', 1), 'User'), | ||
| NEW.raw_user_meta_data->>'avatar_url' | ||
| ); | ||
| RETURN NEW; | ||
| END; | ||
| $$ LANGUAGE plpgsql SECURITY DEFINER; | ||
|
|
||
| -- Trigger to create user profile when auth user is created | ||
| DROP TRIGGER IF EXISTS on_auth_user_created ON auth.users; | ||
| CREATE TRIGGER on_auth_user_created | ||
| AFTER INSERT ON auth.users | ||
| FOR EACH ROW | ||
| EXECUTE FUNCTION public.handle_new_user(); |
Contributor
There was a problem hiding this comment.
Backfill current auth users before shipping this migration.
This trigger only provisions rows for future auth.users inserts. Accounts that already exist when this migration runs will still have no public.users record, so /v1/users/me will 404 for them until somebody backfills the table manually.
🧩 Suggested backfill
CREATE OR REPLACE FUNCTION public.handle_new_user()
RETURNS TRIGGER AS $$
BEGIN
INSERT INTO public.users (id, email, display_name, avatar_url)
VALUES (
NEW.id,
NEW.email,
- COALESCE(NEW.raw_user_meta_data->>'display_name', split_part(NEW.email, '@', 1), 'User'),
+ COALESCE(NULLIF(TRIM(NEW.raw_user_meta_data->>'display_name'), ''), split_part(NEW.email, '@', 1), 'User'),
NEW.raw_user_meta_data->>'avatar_url'
);
RETURN NEW;
END;
$$ LANGUAGE plpgsql SECURITY DEFINER;
+
+INSERT INTO public.users (id, email, display_name, avatar_url)
+SELECT
+ au.id,
+ au.email,
+ COALESCE(NULLIF(TRIM(au.raw_user_meta_data->>'display_name'), ''), split_part(au.email, '@', 1), 'User'),
+ au.raw_user_meta_data->>'avatar_url'
+FROM auth.users AS au
+ON CONFLICT (id) DO NOTHING;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@backend/database/02_create_users_table.sql` around lines 71 - 90, This
migration only creates the trigger (public.handle_new_user /
on_auth_user_created) for future auth.users inserts, so backfill existing
auth.users now: after creating the trigger, run an INSERT ... SELECT from
auth.users into public.users that reproduces the same column logic as
handle_new_user (id, email, display_name using
COALESCE(raw_user_meta_data->>'display_name', split_part(email,'@',1), 'User'),
avatar_url) and use ON CONFLICT DO NOTHING (or equivalent) to avoid
duplicate-key errors; include this backfill step in the migration so current
accounts get public.users rows immediately.
Comment on lines
159
to
163
| <img | ||
| src="https://images.unsplash.com/photo-1494790108377-be9c29b29330?w=200" | ||
| src={profile.avatar_url || `https://ui-avatars.com/api/?name=${encodeURIComponent(profile.display_name)}&size=200&background=10b981&color=fff`} | ||
| alt="Profile" | ||
| className="w-24 h-24 rounded-xl border-4 border-gray-900" | ||
| className="w-24 h-24 rounded-xl border-4 border-gray-900 object-cover" | ||
| /> |
Contributor
There was a problem hiding this comment.
Avoid leaking profile names to a third-party avatar fallback.
When avatar_url is empty, this embeds profile.display_name in an off-site image request. That sends user profile data to an external service on every profile view. Prefer a local initials/avatar placeholder, or proxy the fallback through your backend.
🔒 Suggested local fallback
- <img
- src={profile.avatar_url || `https://ui-avatars.com/api/?name=${encodeURIComponent(profile.display_name)}&size=200&background=10b981&color=fff`}
- alt="Profile"
- className="w-24 h-24 rounded-xl border-4 border-gray-900 object-cover"
- />
+ {profile.avatar_url ? (
+ <img
+ src={profile.avatar_url}
+ alt="Profile"
+ className="w-24 h-24 rounded-xl border-4 border-gray-900 object-cover"
+ />
+ ) : (
+ <div className="w-24 h-24 rounded-xl border-4 border-gray-900 bg-green-500 flex items-center justify-center text-2xl font-semibold text-white">
+ {(profile.display_name.trim()[0] || 'U').toUpperCase()}
+ </div>
+ )}📝 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.
Suggested change
| <img | |
| src="https://images.unsplash.com/photo-1494790108377-be9c29b29330?w=200" | |
| src={profile.avatar_url || `https://ui-avatars.com/api/?name=${encodeURIComponent(profile.display_name)}&size=200&background=10b981&color=fff`} | |
| alt="Profile" | |
| className="w-24 h-24 rounded-xl border-4 border-gray-900" | |
| className="w-24 h-24 rounded-xl border-4 border-gray-900 object-cover" | |
| /> | |
| {profile.avatar_url ? ( | |
| <img | |
| src={profile.avatar_url} | |
| alt="Profile" | |
| className="w-24 h-24 rounded-xl border-4 border-gray-900 object-cover" | |
| /> | |
| ) : ( | |
| <div className="w-24 h-24 rounded-xl border-4 border-gray-900 bg-green-500 flex items-center justify-center text-2xl font-semibold text-white"> | |
| {(profile.display_name.trim()[0] || 'U').toUpperCase()} | |
| </div> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@frontend/src/components/pages/ProfilePage.tsx` around lines 159 - 163, The
img tag in ProfilePage.tsx currently leaks profile.display_name to a third-party
avatar service when profile.avatar_url is falsy; replace that external fallback
by using a local avatar solution: either render a local initials SVG/placeholder
component (e.g., create/use an Avatar or InitialsAvatar component that takes
profile.display_name and profile.id and returns an inline SVG or styled div) and
use that when profile.avatar_url is missing, or proxy the fallback through your
backend (e.g., point to /api/avatar?userId={profile.id} which returns a
generated image) so no PII is sent to external services; update the img/src
usage to prefer profile.avatar_url and fall back to the local Avatar component
or backend proxy.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses PR 1 from the issue checklist.
Summary by CodeRabbit
Release Notes
New Features