Annotations: BE

[apata]

Changes

Adds the BE for annotations. It's heavily based on segments and in this PR, it piggybacks on SiteSegments billing feature for brevity. Accessible only with feature flag "annotations".

Tests

• Automated tests have been added

Changelog

• This PR does not make a user-facing change

Documentation

• This change does not need a documentation update

Dark mode

• This PR does not change the UI

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch annotations/be

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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

[zoldar]

LGTM, aside from the tests that probably need a slight update after the last round of changes.

[zoldar]

Pipelines are defined at the router root anyway (behind a macro). I'd either group it with other pipelines or - given it's now used only by a single controller - move the plug directly to the controller.

[zoldar]

Side note - even for error cases, I'd recommend shaping the changeset and arguments in a way where all the other data are correct (for instance note and type aren't missing), so that the error case is tested in isolation from other errors.

[apata]

Thanks! This valid comment led me down a rabbit hole of adding and refactoring tests. Thankfully, it also exposed some missing functionality that I fixed in 579c601

[github-actions]

🔎 deepsec reviewed 25634e82271696251438896b035f7a4c2453548d · findings below are truncated — full findings artifact

🔎 deepsec found 9 findings

🟠 HIGH ×1 · 🟡 MEDIUM ×3 · 🟣 BUG ×5

_{scope: git-diff:origin/master · run 20260708132043-560166195c046bc8}

🟠 HIGH · lib/plausible/segments/segments.ex:L20-L34

get_all_for_site/2 exposes all members' personal segments to public/shared-link/low-privilege viewers
_{slug: acl-check · confidence: high}

get_all_for_site/2 returns every segment for a site with only where: segment.site_id == ^site.id — it never filters segment.type == :site nor owner_id == <current user>. This contradicts the rest of the module (personal segments are owner-private: do_get_one/3 at L376-386 enforces segment.type == :site or segment.owner_id == ^user_id) and the sibling Annotations module (Annotations.get_all_for_site/4 filters annotation.type == :site or (type == :personal and owner_id == ^user.id)).

This function is the dashboard's segment source. In StatsController.stats/2 it is called as `…

Recommendation: Scope personal segments to the requester like the annotations module does. Pass the current user id into get_all_for_site and, for the :public branch, return only where: segment.type == :site; for the member branch, return where: segment.type == :site or (segment.type == :personal and segment.owner_id == ^user_id). Update both call sites in StatsController to pass the authenticated user (…

---

🟡 MEDIUM · lib/plausible/annotations/annotations.ex:L318-L334

Shared per-site annotation limit exhaustible by a low-privilege member
_{slug: other-quota-exhaustion · confidence: low}

can_insert_one?/3 (lines 316-332) enforces count_annotations(site.id) >= @max_annotations (500) where count_annotations (lines 334-339) counts ALL annotations for the site regardless of type or owner. A member holding only personal-annotation rights (e.g. viewer or billing role, per roles_with_personal_annotations) is permitted to create personal annotations (line 325-326). Such a low-privilege member can create up to 500 personal annotations and exhaust the shared per-site budget, after which every subsequent insert — including SITE annotations by editors/owners and other users' personal a…

Recommendation: Track quota per-type and/or per-owner (e.g. cap personal annotations per user separately from the site-wide site-annotation cap), or exclude personal annotations from the shared cap. Consider rate-limiting annotation creation.

---

🟡 MEDIUM · lib/plausible/segments/segments.ex:L59-L66

get_many/3 resolves any site segment (incl. other users' personal ones) in query filters without ownership check
_{slug: cross-tenant-id · confidence: medium}

get_many/3 selects segments with where: segment.site_id == ^site.id and where: segment.id in ^segment_ids, but no type/owner_id restriction. It is used by Plausible.Segments.Filters.preload_needed_segments/2, which is called from the query builders (query_builder.ex:80, legacy_query_builder.ex:60) to expand ["is", "segment", [id]] filters into the segment's stored filter tree. Neither preload_needed_segments nor get_many receives or checks the requesting user. Consequently a :viewer/:billing member — or an anonymous :public caller of the internal query endpoint `POST /…

Recommendation: Thread the requesting user's identity/role into preload_needed_segments/get_many and restrict resolvable segments to type == :site or owner_id == <user> (mirroring do_get_one/3). Reject queries that reference segment IDs the caller cannot access instead of silently resolving them.

---

🟡 MEDIUM · lib/plausible/segments/segments.ex:L349-L359

get_related_shared_links/3 leaks shared-link names for segments the caller doesn't own (IDOR)
_{slug: cross-tenant-id · confidence: medium}

get_related_shared_links/3 (reached via GET /api/:domain/segments/:segment_id/shared-links, SegmentsController.get_related_shared_links) only checks site_role in roles_with_personal_segments() (which includes :viewer and :billing) and then queries shared links by segment_id and site_id. Unlike update_one/delete_one, it does NOT call do_get_one/3, so it never verifies that segment_id is a segment the caller may access. An authenticated low-privilege member can therefore enumerate segment IDs and learn the names of shared links associated with any segment on the site — inc…

Recommendation: Before querying related shared links, resolve the segment through the same ownership-aware path used by update/delete (get_one/do_get_one) so that personal segments belonging to other users return :segment_not_found/:not_enough_permissions. Also consider restricting this endpoint to roles that can actually manage the segment.

---

🟣 BUG · lib/plausible/annotations/annotation.ex:L98-L100

Unhandled DateTime.from_naive/2 error branch can raise CaseClauseError
_{slug: other-unhandled-error · confidence: low}

maybe_coerce_naive_datetime/2 (lines 92-108) matches only {:ok, _}, {:ambiguous, _, _} and {:gap, _, _} from DateTime.from_naive(naive_dt, timezone) (lines 98-102). DateTime.from_naive/2 can also return {:error, :time_zone_not_found} / {:error, :utc_only_time_zone_database} when the timezone is not resolvable by the tz database. That error case is not handled and is inside the do block of the with, so it is NOT caught by the else clause — it would raise CaseClauseError and 500 the request for every annotation create/update that supplies a naive datetime string. In practice this is mitig…

Recommendation: Add an explicit fallthrough for the {:error, _} result of DateTime.from_naive/2 (e.g. return the changeset unchanged or add a changeset error) instead of relying on an exhaustive case over only the success shapes.

---

🟣 BUG · lib/plausible/annotations/annotations.ex:L62-L69

Dangling site annotations hidden from authenticated members but shown to public
_{slug: other-logic-bug · confidence: high}

In get_all_for_site/4 the authenticated branch (lines 58-72) uses inner_join: owner in assoc(annotation, :owner) (line 62). The schema (annotation.ex L40-41) and the cleanup functions after_user_removed_from_team/after_user_removed_from_site/user_removed (lines 175-243) explicitly set owner_id: nil on SITE annotations when their creator leaves/is removed, producing 'dangling' site annotations. Because the authenticated query INNER-joins the owner, any site annotation with owner_id == nil is silently dropped from the result set for every logged-in role (viewer, editor, admin, owner). The p…

Recommendation: Use a left join (or drop the owner join and preload owner separately) in the authenticated branch so site annotations with owner_id == nil are returned, matching the public branch. E.g. left_join: owner in assoc(annotation, :owner) and handle nil owner in preloading/encoding.

---

🟣 BUG · lib/plausible/teams/memberships.ex:L131

update_role/5 crashes with 500 on unknown role string instead of graceful rejection
_{slug: other-unhandled-exception · confidence: low}

In update_role/5, new_role = String.to_existing_atom(new_role_str) (line 131) executes on the raw, user-supplied new_role_str parameter BEFORE any validation. If an authenticated owner/admin submits a new_role value that does not correspond to an already-existing atom (e.g. new_role=superowner), String.to_existing_atom/1 raises ArgumentError, producing an unhandled 500 Internal Server Error. The controller (MembershipController.update_role_by_user) clearly intends malformed/disallowed roles to be handled gracefully — its {:error, _} branch renders a friendly flash: "You are not…

Recommendation: Validate new_role_str against the known set of grantable roles before converting, or wrap the conversion, e.g. case Enum.find([:editor, :viewer, :owner, :admin, :guest], &(Atom.to_string(&1) == new_role_str)) do nil -> {:error, :not_allowed}; role -> ... end, so unknown roles flow into the existing {:error, :not_allowed} path instead of raising.

---

🟣 BUG · lib/plausible/teams/memberships/leave.ex:L13-L53

TOCTOU race in last-owner check can leave a team with zero owners
_{slug: other-race-condition · confidence: medium}

check_owner_can_leave/2 (lines 52-58) guards the 'last owner cannot leave' invariant by reading Memberships.owners_count(team) > 1. This count is read in the with chain (line 14) OUTSIDE and before the Repo.transaction (lines 17-30) that deletes the membership, and owners_count issues a plain aggregate with no row lock / SELECT ... FOR UPDATE and no serializable isolation. Two owners of a 2-owner team who leave concurrently (or one owner leaving while another owner is concurrently removed via Remove.remove) will BOTH read count == 2, BOTH pass the check, and BOTH delete their membership, le…

Recommendation: Perform the owner-count check and the delete atomically inside a single transaction with a locking read (e.g. SELECT ... FOR UPDATE on the team's owner memberships, or lock the team row) so concurrent removals serialize; alternatively add a DB constraint/trigger enforcing at least one owner per team.

---

🟣 BUG · lib/plausible/teams/memberships/remove.ex:L15-L58

TOCTOU race in last-owner check can leave a team with zero owners
_{slug: other-race-condition · confidence: medium}

check_owner_can_get_removed/2 (lines 57-63) enforces the 'the last owner cannot be removed' invariant via Memberships.owners_count(team) > 1, read in the with chain (line 15) BEFORE the Repo.transaction (lines 18-31) that deletes the membership. owners_count uses an unlocked aggregate and the check is not atomic with the delete, so two concurrent owner-removals (e.g. two owners each removing the other, or a Remove.remove concurrent with a Leave.leave) both observe count == 2 and both proceed, deleting both owner memberships and leaving the team with zero owners. Only owners can remove owner…

Recommendation: Wrap the owner-count check and the membership delete in one transaction using a locking read (SELECT ... FOR UPDATE on owner memberships or a team-row lock) so concurrent removals serialize, and/or add a DB constraint enforcing >=1 owner per team.

---

_{🤖 generated by deepsec}

[apata]

🔎 deepsec reviewed 25634e82271696251438896b035f7a4c2453548d · findings below are truncated — full findings artifact

🔎 deepsec found 9 findings

🟠 HIGH ×1 · 🟡 MEDIUM ×3 · 🟣 BUG ×5

scope: git-diff:origin/master · run 20260708132043-560166195c046bc8

🟠 HIGH · lib/plausible/segments/segments.ex:L20-L34

get_all_for_site/2 exposes all members' personal segments to public/shared-link/low-privilege viewers slug: acl-check · confidence: high

get_all_for_site/2 returns every segment for a site with only where: segment.site_id == ^site.id — it never filters segment.type == :site nor owner_id == <current user>. This contradicts the rest of the module (personal segments are owner-private: do_get_one/3 at L376-386 enforces segment.type == :site or segment.owner_id == ^user_id) and the sibling Annotations module (Annotations.get_all_for_site/4 filters annotation.type == :site or (type == :personal and owner_id == ^user.id)).

This function is the dashboard's segment source. In StatsController.stats/2 it is called as `…

Recommendation: Scope personal segments to the requester like the annotations module does. Pass the current user id into get_all_for_site and, for the :public branch, return only where: segment.type == :site; for the member branch, return where: segment.type == :site or (segment.type == :personal and segment.owner_id == ^user_id). Update both call sites in StatsController to pass the authenticated user (…

Personal segments aren't private by design. They were initially supposed to be, but we reevaluated and wanted to support sharing links to dashboards filtered by a particular segment and having these work consistently even if the segment type changes over time. Need to add code comments so this wouldn't keep coming up.

🟡 MEDIUM · lib/plausible/annotations/annotations.ex:L318-L334

Shared per-site annotation limit exhaustible by a low-privilege member slug: other-quota-exhaustion · confidence: low

can_insert_one?/3 (lines 316-332) enforces count_annotations(site.id) >= @max_annotations (500) where count_annotations (lines 334-339) counts ALL annotations for the site regardless of type or owner. A member holding only personal-annotation rights (e.g. viewer or billing role, per roles_with_personal_annotations) is permitted to create personal annotations (line 325-326). Such a low-privilege member can create up to 500 personal annotations and exhaust the shared per-site budget, after which every subsequent insert — including SITE annotations by editors/owners and other users' personal a…

Recommendation: Track quota per-type and/or per-owner (e.g. cap personal annotations per user separately from the site-wide site-annotation cap), or exclude personal annotations from the shared cap. Consider rate-limiting annotation creation.

What if there's just one user on the site and they want to add 500 notes? This is a matter the site team owner can sort out with the user exhausting the shared cap.

🟡 MEDIUM · lib/plausible/segments/segments.ex:L59-L66

get_many/3 resolves any site segment (incl. other users' personal ones) in query filters without ownership check slug: cross-tenant-id · confidence: medium

get_many/3 selects segments with where: segment.site_id == ^site.id and where: segment.id in ^segment_ids, but no type/owner_id restriction. It is used by Plausible.Segments.Filters.preload_needed_segments/2, which is called from the query builders (query_builder.ex:80, legacy_query_builder.ex:60) to expand ["is", "segment", [id]] filters into the segment's stored filter tree. Neither preload_needed_segments nor get_many receives or checks the requesting user. Consequently a :viewer/:billing member — or an anonymous :public caller of the internal query endpoint `POST /…

Recommendation: Thread the requesting user's identity/role into preload_needed_segments/get_many and restrict resolvable segments to type == :site or owner_id == <user> (mirroring do_get_one/3). Reject queries that reference segment IDs the caller cannot access instead of silently resolving them.

See above why personal segments aren't private.

🟡 MEDIUM · lib/plausible/segments/segments.ex:L349-L359

get_related_shared_links/3 leaks shared-link names for segments the caller doesn't own (IDOR) slug: cross-tenant-id · confidence: medium

get_related_shared_links/3 (reached via GET /api/:domain/segments/:segment_id/shared-links, SegmentsController.get_related_shared_links) only checks site_role in roles_with_personal_segments() (which includes :viewer and :billing) and then queries shared links by segment_id and site_id. Unlike update_one/delete_one, it does NOT call do_get_one/3, so it never verifies that segment_id is a segment the caller may access. An authenticated low-privilege member can therefore enumerate segment IDs and learn the names of shared links associated with any segment on the site — inc…

Recommendation: Before querying related shared links, resolve the segment through the same ownership-aware path used by update/delete (get_one/do_get_one) so that personal segments belonging to other users return :segment_not_found/:not_enough_permissions. Also consider restricting this endpoint to roles that can actually manage the segment.

Need to look into this separately.

🟣 BUG · lib/plausible/annotations/annotation.ex:L98-L100

Unhandled DateTime.from_naive/2 error branch can raise CaseClauseError slug: other-unhandled-error · confidence: low

maybe_coerce_naive_datetime/2 (lines 92-108) matches only {:ok, _}, {:ambiguous, _, _} and {:gap, _, _} from DateTime.from_naive(naive_dt, timezone) (lines 98-102). DateTime.from_naive/2 can also return {:error, :time_zone_not_found} / {:error, :utc_only_time_zone_database} when the timezone is not resolvable by the tz database. That error case is not handled and is inside the do block of the with, so it is NOT caught by the else clause — it would raise CaseClauseError and 500 the request for every annotation create/update that supplies a naive datetime string. In practice this is mitig…

Recommendation: Add an explicit fallthrough for the {:error, _} result of DateTime.from_naive/2 (e.g. return the changeset unchanged or add a changeset error) instead of relying on an exhaustive case over only the success shapes.

7542f6e

🟣 BUG · lib/plausible/annotations/annotations.ex:L62-L69

Dangling site annotations hidden from authenticated members but shown to public slug: other-logic-bug · confidence: high

In get_all_for_site/4 the authenticated branch (lines 58-72) uses inner_join: owner in assoc(annotation, :owner) (line 62). The schema (annotation.ex L40-41) and the cleanup functions after_user_removed_from_team/after_user_removed_from_site/user_removed (lines 175-243) explicitly set owner_id: nil on SITE annotations when their creator leaves/is removed, producing 'dangling' site annotations. Because the authenticated query INNER-joins the owner, any site annotation with owner_id == nil is silently dropped from the result set for every logged-in role (viewer, editor, admin, owner). The p…

Recommendation: Use a left join (or drop the owner join and preload owner separately) in the authenticated branch so site annotations with owner_id == nil are returned, matching the public branch. E.g. left_join: owner in assoc(annotation, :owner) and handle nil owner in preloading/encoding.

fcd5c4e

🟣 BUG · lib/plausible/teams/memberships.ex:L131

update_role/5 crashes with 500 on unknown role string instead of graceful rejection slug: other-unhandled-exception · confidence: low

In update_role/5, new_role = String.to_existing_atom(new_role_str) (line 131) executes on the raw, user-supplied new_role_str parameter BEFORE any validation. If an authenticated owner/admin submits a new_role value that does not correspond to an already-existing atom (e.g. new_role=superowner), String.to_existing_atom/1 raises ArgumentError, producing an unhandled 500 Internal Server Error. The controller (MembershipController.update_role_by_user) clearly intends malformed/disallowed roles to be handled gracefully — its {:error, _} branch renders a friendly flash: "You are not…

Recommendation: Validate new_role_str against the known set of grantable roles before converting, or wrap the conversion, e.g. case Enum.find([:editor, :viewer, :owner, :admin, :guest], &(Atom.to_string(&1) == new_role_str)) do nil -> {:error, :not_allowed}; role -> ... end, so unknown roles flow into the existing {:error, :not_allowed} path instead of raising.

Separate issue

🟣 BUG · lib/plausible/teams/memberships/leave.ex:L13-L53

TOCTOU race in last-owner check can leave a team with zero owners slug: other-race-condition · confidence: medium

check_owner_can_leave/2 (lines 52-58) guards the 'last owner cannot leave' invariant by reading Memberships.owners_count(team) > 1. This count is read in the with chain (line 14) OUTSIDE and before the Repo.transaction (lines 17-30) that deletes the membership, and owners_count issues a plain aggregate with no row lock / SELECT ... FOR UPDATE and no serializable isolation. Two owners of a 2-owner team who leave concurrently (or one owner leaving while another owner is concurrently removed via Remove.remove) will BOTH read count == 2, BOTH pass the check, and BOTH delete their membership, le…

Recommendation: Perform the owner-count check and the delete atomically inside a single transaction with a locking read (e.g. SELECT ... FOR UPDATE on the team's owner memberships, or lock the team row) so concurrent removals serialize; alternatively add a DB constraint/trigger enforcing at least one owner per team.

Separate issue

🟣 BUG · lib/plausible/teams/memberships/remove.ex:L15-L58

TOCTOU race in last-owner check can leave a team with zero owners slug: other-race-condition · confidence: medium

check_owner_can_get_removed/2 (lines 57-63) enforces the 'the last owner cannot be removed' invariant via Memberships.owners_count(team) > 1, read in the with chain (line 15) BEFORE the Repo.transaction (lines 18-31) that deletes the membership. owners_count uses an unlocked aggregate and the check is not atomic with the delete, so two concurrent owner-removals (e.g. two owners each removing the other, or a Remove.remove concurrent with a Leave.leave) both observe count == 2 and both proceed, deleting both owner memberships and leaving the team with zero owners. Only owners can remove owner…

Recommendation: Wrap the owner-count check and the membership delete in one transaction using a locking read (SELECT ... FOR UPDATE on owner memberships or a team-row lock) so concurrent removals serialize, and/or add a DB constraint enforcing >=1 owner per team.

Separate issue

[zoldar]

About 7542f6e

1. chances of this happening are pretty much zero because the timezone is always provided from site.timezone, which is validated on every change.
2. there's no clear way for user to act upon on this error, should it ever happen - it's a system error

[aerosol]

About 7542f6e

1. chances of this happening are pretty much zero because the timezone is always provided from site.timezone, which is validated on every change.

2. there's no clear way for user to act upon on this error, should it ever happen - it's a system error

+1 can we stop the madness please?

[apata]

About 7542f6e

1. chances of this happening are pretty much zero because the timezone is always provided from site.timezone, which is validated on every change.
2. there's no clear way for user to act upon on this error, should it ever happen - it's a system error

Thanks, @zoldar! With the UI implemented, a status 400 error message would be shown to user, but a status 500 would be shown as "something went wrong". The user's path to salvation would be the same, reaching out for help. In the 400 case, their screenshot to us would be more informative. In the 500 case, there's also a chance that we pick it up from our logging platform without the user needing to reach out. That has a slight advantage, so I will revert it to be internal server error in #6481.