fix: stop disclosing user e-mail (username) outside the project members list by Anty0 · Pull Request #3823 · tolgee/tolgee-platform

Anty0 · 2026-07-24T14:03:06Z

What

Strip a user's username (which is their e-mail address) from every API response
except the surfaces where disclosure is legitimate. After this change the only place one user
can see another user's username is where both of these hold by construction:

the caller holds MEMBERS_VIEW, and
the target user is a current member of the project.

Everywhere else — activity feed, translation suggestions/comments/history, tasks, batch-job
author, notifications, key-trash "deleted by", branches, API-key owner, the XLSX task report —
username is now always empty and the field is marked deprecated (kept on the wire so existing
clients don't break).

Why the previous masking was not enough

SecurityService.maskedMemberField gated only on the caller having MEMBERS_VIEW. It could
not satisfy the rule above:

It never checked whether the target is a member. On a public project a project admin
(who has MEMBERS_VIEW) opening the suggestions list / activity feed / comment thread / history
saw the raw e-mail of community contributors and ex-members who authored those rows — exactly
the disclosure we want to prevent.
It failed open. shouldExposeMemberInfo() returns true when there is no request scope or no
project in scope, so non-project-scoped endpoints (Notifications originatingUser,
/v2/user-tasks co-assignees) leaked usernames to anyone.

A membership-aware masking fix would need a per-author "is currently a member" check on ~11
surfaces — expensive, N+1-prone, and re-opened by every new author-bearing surface. Stripping the
field instead makes the rule hold structurally and cannot drift.

Kept (legitimate disclosure — unchanged)

Project members list (UserAccountInProjectModel) — the one intended surface.
Org members list (UserAccountWithOrganizationRoleModel) — org-scoped, kept as an exemption.
Self / "me" (PrivateUserAccountModel) — your own e-mail.
Instance-admin user management (UserAccountModel, super-auth gated).
Project export ZIP (UserRef(username)) — admin-only, and import re-links authors by
username, so it must stay.

Changes

SimpleUserAccountModel.username → deprecated constant "" (removed from the constructor so no
assembler can repopulate it). This one model backs ~16 assemblers (suggestions, comments, tasks,
batch, notifications, key-trash, branches, PAT-with-user, invitations), so it closes most surfaces
— including the two fail-open leaks — at once.
ProjectActivityAuthorModel.username and ApiKeyModel.username → deprecated, always "".
TaskReportHelper (XLSX task report) no longer embeds the e-mail — name only.
Deleted the now-unused SecurityService.maskedMemberField (kept shouldExposeMemberInfo, still
used for the members-count gate).
Regenerated the frontend API schema (adds @deprecated on the three username fields; field type
unchanged, so the webapp still compiles — display code already prefers name).
Updated backend tests that asserted a non-empty username on the now-stripped surfaces.

Notes

The contributor list endpoint already added on the integration branch needs no change — its model
never carried a username.
Frontend needs no code change: name (+ avatar) is already preferred for identity everywhere off
the members/org/admin surfaces, which keep username.

coderabbitai · 2026-07-24T14:03:14Z

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 2192e5ed-f9ee-4315-b3c8-452c62add8ea

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch jirikuchynka/strip-username-disclosure

_{Comment @coderabbitai help to get the list of available commands.}

…rs list A user's `username` is their e-mail address. It was exposed on many API surfaces (activity feed, suggestions, comments, translation history, tasks, batch-job author, notifications, key-trash "deleted by", branches, API-key owner, XLSX task report) behind `maskedMemberField`, which only gated on the caller holding MEMBERS_VIEW and never on the target being a member — so a public-project admin saw community contributors' and ex-members' e-mails, and non-project-scoped endpoints leaked to anyone (fail-open). Strip `username` from every response model except the project members list, org members list, self ("me"), instance-admin user management, and the admin-only project export (which re-links authors by username on import). The field stays on the wire, deprecated and always empty. Remove the now-unused maskedMemberField. Regenerate the frontend schema and update affected tests.

Follow-up from review of the username-disclosure change: - Centralize the deprecation/schema text into a single `USERNAME_FIELD_DEPRECATION` constant so the five annotation sites can't drift, and correct the wording (username is also returned on org-member/self/admin surfaces, not "only the members list"). - Make ApiKeyModel enforce the empty-username invariant structurally like the other two models: move username out of the constructor to a body `val ""`, and make IApiKeyModel.username read-only. - Document the disclosure policy on UserAccount.username (the allowlisted surfaces). - TaskReportHelper.formatUserName falls back to `#<id>` when name is blank, so the XLSX report never has an empty/ambiguous cell and never the e-mail. - Reword the now-stale shouldExposeMemberInfo comment (its websocket caller went away with maskedMemberField). - Lock the "kept" side: assert the project members list still exposes username; add a TaskReportHelper unit test. - Fix the soft-delete-keys e2e: the deleter filter identifies users by name now that username is stripped, so give the test user a name and assert on it.

- UserName (useUserName): fall back to the "Unnamed user" label after name/username, so name-less authors on stripped surfaces (comments, history, trash) no longer render a blank label. Username stays "", so no e-mail is shown. - Add UsernameDisclosureGuardTest: classpath-scans every HATEOAS response model and fails if a non-allowlisted one accepts `username` via its constructor. The allowlist lives in the test; a vacuity guard ensures the scan actually finds the models. UserAccount.username KDoc now points at the test instead of enumerating api-module class names. - Rename shouldExposeMemberInfo -> shouldExposeMemberCount and scope its comment: it gates only the non-sensitive member count (where the fail-open default is intentional), not any member field. Rename its test to ShouldExposeMemberCountTest (the masking concept is gone). - Assert the instance-admin user list keeps exposing username (the last untested allowlist surface).

Round-3 review: the guard only proved `username` was absent from the primary constructor, a weaker property than "cannot be set". Tighten it so a non-allowlisted response model is flagged when username is settable via ANY constructor OR is a mutable (`var`) property an assembler could assign post-construction; the immutable `val username = ""` pattern still passes. Scan a single `io.tolgee` root so every hateoas package (including a mispackaged one) is covered without a prefix list. Anchor the vacuity check on an EE model too, and fail loud on reflection errors instead of treating an un-inspectable model as clean.

Round-4 review: - The four models that intentionally expose username (the e-mail) are allowlisted by class, but nothing bound each to the gated endpoint it may be served from. Document that binding on each model (endpoint + scope, "do not return from a less-gated endpoint") and mirror the scopes in the guard test's allowlist. The guard KDoc now states it does NOT prove endpoint gating. - Note the guard's coverage boundary (RepresentationModel response types only) and remove the dead UserResponseDTO (a plain data class with a `var username`, referenced nowhere), which was the one un-scanned response-shaped type carrying the e-mail. - Strengthen the two positive allowlist assertions from "non-empty" to the exact e-mail, matching the org-members and self tests, so a wrong-field wiring (e.g. name in place of e-mail) is caught.

The disclosure guard lives in the EE test module (it must, to scan EE response models too), so a core-only build that runs without EE does not run it. Document that limitation on the guard and on UserAccount.username so a core-only fork knows to re-add an equivalent core-scoped scan.

The guard lived only in :ee-test, a conditionally-built module — an EE-less build (OSS fork, or the CWD drift that drops :ee-app) shipped green with zero enforcement of what is a core invariant. Extract the allowlist and the scan-and-assert into a single shared helper (io.tolgee.testing.security.UsernameDisclosureGuard in :testing), keyed on string class names so :testing needs no compile dependency on :api or spring-hateoas. Add a core entrypoint in :server-app — built and tested in every build — anchored on the core allowlisted models, so core response models stay guarded even when EE is absent. The existing EE test becomes a thin entrypoint that additionally anchors an EE model. One allowlist, no duplication.

…ones The core entrypoint's anti-vacuous anchor was the allowlisted (exempt) model set — the models the offender loop skips — so it only proved the scan reached models it ignores, not the stripped models it polices. A scan that returned just the allowlisted subset would pass green on the always-built entrypoint. Add corePolicedModelNames (SimpleUserAccountModel, ApiKeyModel, ProjectActivityAuthorModel, across three hateoas packages) and anchor both entrypoints on it, so the vacuity check proves the offender loop actually sees stripped models.

…-email fields Round-8 review (full panel): - Consolidate the changeset's comments: keep the invariant in one place (UsernameDisclosureGuard), reduce the model/entity/test KDocs to one-line pointers, trim the guard class doc to the load- bearing blind spots, and cut inline comments that restate the code or an assertion message. - Extract the offender rule into UsernameDisclosureGuard.hasSettableUsername and unit-test it directly (UsernameDisclosureDetectorTest) with plain non-RepresentationModel fixtures, so the detector's positive path is covered — the entrypoint scans only ever see stripped models. - Add a self-endpoint retention test (GET /v2/user keeps username), the one allowlisted surface that lacked one. - Remove the now-dead authorEmail/authorUsername view fields and the query columns that populated them (translation history, project activity, suggestions) — unread since the assemblers stopped reading them, and a standing re-leak footgun. - TaskReportHelper: carry the user id ("name #id") so same-named users stay distinguishable in the XLSX report without the e-mail.

Per the project's comment rule (default: no comment; keep only where a reader would burn themselves). Drop the one-line model KDocs (allowlist membership is the record), the two entrypoint KDocs, the test-assertion narration, the detector-test rationale, and the guard property/predicate docs. Trim the entity field doc to the one non-obvious fact (username is the e-mail) and the guard class doc to the RepresentationModel-only blind spot. Keep the SecurityService fail-open note, the blind-spot warning, and the per-entry endpoint+gate comments on the allowlist.

They restated endpoint/scope wiring that lives in the controllers and would rot silently against it; the model class names already identify each surface and the guard's error message states the criterion an entry must meet.

The fail-open comment on shouldExposeMemberCount named a remote endpoint (rot-prone) and stood in for a missing test on the projectless branch. Add that test (mirroring the off-request one) so both fail-open branches are checked, and remove the comment — a reader who "tightens" return true to false now fails a test instead of relying on prose.

Round-12 review (full panel converged on the guard's core predicate): - Flag any non-sanctioned RepresentationModel that DECLARES a `username` property, not just a settable one — a computed immutable `val username get() = email` no longer slips through. The five username-bearing models (including the two IApiKeyModel delegators) are listed in strippedModelNames; anything else declaring username must be sanctioned deliberately. - Add UsernameDisclosureGuardValueTest asserting each stripped model actually emits "", so a getter that returns the e-mail fails a test even though the scan (which reads declaration, not value) stays green. Document that the guard keys on the literal name `username` and reads declaration not value. - Add a scanned-count tripwire so a whole model-bearing module dropping off the classpath fails loudly; fold the guard-owned anchor sets into assertNoLeak; order the entry point above its helper. - Remove the last dead deletedByUserUsername (trash queries) symmetrically, keeping the positional KeyWithTranslationsView offsets aligned.

Update KeyWithTranslationsView.of()'s layout comment from 6 to 5 optional trashed columns after deletedByUserUsername was removed, and restore a one-line note on shouldExposeMemberCount that its request-scope guard must run before projectHolder.projectOrNull (which throws off-request).

… search KeySearchSearchResultModel delegates KeySearchResultView (`by view`), so removing deletedByUserUsername from that view also removed it from the model's serialized shape. That field carried the deleter's raw username (their e-mail) under a non-`username` name on the KEYS_VIEW-gated key-search/trash response — a disclosure the SimpleUserAccountModel-based analysis had missed. Regenerate the frontend schema to match.

@Schema

…he allowlist bindings Round-14 review: - Give RevealedApiKeyModel and ApiKeyWithLanguagesModel an explicit empty username override so the @Deprecated/@Schema reaches the generated schema (Kotlin interface delegation does not propagate it); regenerate the schema so all five stripped models advertise the deprecation. - Cover the guard's declaresUsername predicate directly (positive and negative) now that it replaced the settable check, and assert the value-tested set stays in sync with strippedModelNames. - Record each allowlisted model's endpoint + required scope next to its allowlist entry, so a future author reusing an e-mail-bearing model on a new endpoint has a local signal. - Trim the guard/value-test/view docs to the load-bearing lines.

Drop the value-test narration (the sync rationale moves into the assertion's failure message), the duplicated "explicit override" comments on the two api-key models (the schema-check job already fails if the override is dropped), and the guard header's self-restating first sentence. Rename the EE entrypoint UsernameDisclosureGuardTest -> UsernameDisclosureGuardEeTest so it reads as the twin of UsernameDisclosureGuardCoreTest.

…rs list (#3823) Strip a user's `username` (which is their e-mail) from every API response except the surfaces where disclosure is legitimate: after this change one user can see another's `username` only where the caller holds `MEMBERS_VIEW` AND the target is a current member (plus org-members list, self, instance-admin, and the admin-only export ZIP). `SecurityService.maskedMemberField` gated only on the caller's `MEMBERS_VIEW`, never on target membership, and failed open off-request/off-project — so project admins on public projects saw community contributors' and ex-members' raw e-mails, and non-project-scoped endpoints leaked to anyone. Stripping the field makes the rule hold structurally instead of re-checking membership on ~11 author-bearing surfaces. `username` is now a deprecated always-empty field on the stripped models (kept on the wire so clients don't break); a classpath-scanning `UsernameDisclosureGuard` test fails CI if any non-sanctioned response model reintroduces it. Frontend already prefers name + avatar for identity off the members/org/admin surfaces.

Anty0 force-pushed the jirikuchynka/community-contributors branch 2 times, most recently from 70a1fe0 to 37ab36d Compare July 25, 2026 15:58

Anty0 added 17 commits July 26, 2026 03:36

docs: drop the allowlist per-entry auth-wiring comments

94c0522

They restated endpoint/scope wiring that lives in the controllers and would rot silently against it; the model class names already identify each surface and the guard's error message states the criterion an entry must meet.

Anty0 force-pushed the jirikuchynka/strip-username-disclosure branch from e9aeaf0 to c0467c1 Compare July 26, 2026 01:50

Anty0 merged commit 34a4bce into jirikuchynka/community-contributors Jul 26, 2026
72 of 74 checks passed

Anty0 deleted the jirikuchynka/strip-username-disclosure branch July 26, 2026 11:02

This was referenced Jul 26, 2026

feat: Community translation v1.1 — Contributors (pitch #3806) #3819

Open

Community translation - v1.1 (Contributors) #3806

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix: stop disclosing user e-mail (username) outside the project members list#3823

fix: stop disclosing user e-mail (username) outside the project members list#3823
Anty0 merged 17 commits into
jirikuchynka/community-contributorsfrom
jirikuchynka/strip-username-disclosure

Anty0 commented Jul 24, 2026

Uh oh!

coderabbitai Bot commented Jul 24, 2026 •

edited

Loading

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

Anty0 commented Jul 24, 2026

What

Why the previous masking was not enough

Kept (legitimate disclosure — unchanged)

Changes

Notes

Uh oh!

coderabbitai Bot commented Jul 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jul 24, 2026 •

edited

Loading