Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions backend/app/email_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
# Utility to normalize emails for account merging


def normalize_email(email: str) -> str:
"""
Normalize email by removing any "+..." before the @, and lowercasing.
E.g. user+alias@example.com -> user@example.com
"""
if not email:
return email
if "@" not in email:
return email
Comment on lines +8 to +12
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The early return when '@' is not present returns the original email unchanged. This could silently allow malformed emails to pass through. Consider either raising an exception for invalid email formats or documenting this behavior explicitly in the docstring, as it may lead to unexpected account merging issues.

Suggested change
"""
if not email:
return email
if "@" not in email:
return email
Raises:
ValueError: If the email does not contain an '@' character.
"""
if not email:
return email
if "@" not in email:
raise ValueError(f"Invalid email format: '{email}' (missing '@')")

Copilot uses AI. Check for mistakes.
local, sep, domain = email.partition("@")
plus_index = local.find("+")
if plus_index != -1:
local = local[:plus_index]
return f"{local.lower()}@{domain.lower()}"
19 changes: 13 additions & 6 deletions backend/app/logins.py
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

from . import apps, config, models
from .database import get_db
from .email_utils import normalize_email
from .emails import EmailCategory
from .login_info import (
LoginInformation,
Expand Down Expand Up @@ -822,12 +823,19 @@ def continue_oauth_flow(
)
# We now have a logged in user, so let's do our best to do something useful
provider_data = token_to_data(login_result)
# Do we have a provider's user noted with this ID already?

# Try to find an existing account by provider ID
account = account_model.by_provider_id(db, provider_data.id)
normalized_email = (
normalize_email(provider_data.email) if provider_data.email else None
)
merged_user = None
if normalized_email:
merged_user = models.FlathubUser.by_normalized_email(db, normalized_email)
Comment on lines +829 to +834
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Email normalization is being used for automatic account merging, which could lead to a security issue. If a user controls user+alias@example.com and another user has user@example.com, this normalization could allow the first user to gain access to the second user's account. Email alias normalization should only be applied within the same email provider/domain and with user consent, not automatically for account merging.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback


if account is None:
# We've never seen this provider's user before, if we're not already logged
# in then create a user
user = login.user
# If merging, use the merged user, else current login user, else new user
user = merged_user or login.user
if user is None:
user = models.FlathubUser(display_name=provider_data.name)
db.add(user)
Expand Down Expand Up @@ -855,8 +863,7 @@ def continue_oauth_flow(
# The provider's user has been seen before, if we're logged in already and
# things don't match then abort now
user = login.user
if user is not None:
# Eventually we might do user-merge here?
if user is not None and (merged_user is None or user.id != merged_user.id):
Copy link

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This condition allows login when user is logged in and merged_user exists with the same ID, but doesn't handle the case where the account's existing user_entity differs from merged_user. If an account already exists and is linked to a different user, but the normalized email matches yet another user, this could create inconsistent state. Should check if account.user_entity.id != merged_user.id before allowing the merge.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

db.commit()
return JSONResponse(
{"status": "error", "error": "error-already-logged-in"},
Expand Down
44 changes: 44 additions & 0 deletions backend/app/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
desc,
false,
func,
literal,
or_,
text,
)
Expand Down Expand Up @@ -426,6 +427,49 @@ def by_permission(db, permission_name: str):

return result

@staticmethod
def by_normalized_email(db, normalized_email: str) -> Optional["FlathubUser"]:
"""
Find a FlathubUser by normalized email across all connected account types.

Returns:
FlathubUser if a user is found, otherwise None.
"""
account_classes = [
GithubAccount,
GitlabAccount,
GnomeAccount,
GoogleAccount,
KdeAccount,
]
for acc_cls in account_classes:
# Normalize email: lowercase, remove anything after '+' in local part, keep domain
norm_expr = func.lower(
func.concat(
func.substring(
acc_cls.email,
1,
func.nullif(func.position(literal("+"), acc_cls.email) - 1, -1),
)
if func.position(literal("+"), acc_cls.email) > 0
else func.substring(
acc_cls.email, 1, func.position(literal("@"), acc_cls.email) - 1
),
literal("@"),
func.substring(
acc_cls.email, func.position(literal("@"), acc_cls.email) + 1
),
)
)
Comment on lines +447 to +463
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This SQL-based email normalization duplicates the logic in normalize_email() from email_utils.py, making the code harder to maintain. If the normalization logic changes, both places need to be updated. Consider using a database-side generated column or creating a normalized_email field that's populated using the Python function to ensure consistency.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot open a new pull request to apply changes based on this feedback

match = (
db.session.query(acc_cls)
.filter(acc_cls.email.isnot(None), norm_expr == normalized_email)
.first()
)
if match:
return match.user_entity
return None


class flathubuser_role(Base):
__tablename__ = "flathubuser_role"
Expand Down
22 changes: 22 additions & 0 deletions backend/tests/test_email_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
from app.email_utils import normalize_email


def test_normalize_email_removes_plus():
assert normalize_email("user+alias@example.com") == "user@example.com"
assert normalize_email("USER+foo@Example.com") == "user@example.com"


def test_normalize_email_no_plus():
assert normalize_email("user@example.com") == "user@example.com"


def test_normalize_email_multiple_plus():
assert normalize_email("user+foo+bar@example.com") == "user@example.com"


def test_normalize_email_empty():
assert normalize_email("") == ""


def test_normalize_email_case_insensitive():
assert normalize_email("User+Test@Example.COM") == "user@example.com"
31 changes: 31 additions & 0 deletions backend/tests/test_logins.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,31 @@
from fastapi.testclient import TestClient

from app.main import app

client = TestClient(app)


def test_login_methods():
response = client.get("/auth/login")
assert response.status_code == 200
data = response.json()
assert any(m["method"] == "github" for m in data)
assert any(m["method"] == "gitlab" for m in data)
assert any(m["method"] == "gnome" for m in data)
assert any(m["method"] == "kde" for m in data)


def test_userinfo_not_logged_in():
response = client.get("/auth/userinfo")
assert response.status_code == 204


def test_deleteuser_not_logged_in():
response = client.get("/auth/deleteuser")
Copy link

Copilot AI Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The HTTP method used here should be DELETE instead of GET for a delete operation. Using GET for a destructive operation violates REST API best practices and could lead to security issues (e.g., accidental deletion via link prefetching).

Suggested change
response = client.get("/auth/deleteuser")
response = client.delete("/auth/deleteuser")

Copilot uses AI. Check for mistakes.
assert response.status_code == 403


def test_logout_not_logged_in():
response = client.post("/auth/logout")
assert response.status_code == 200
assert response.json() == {}