FEATURE: per-topic unsubscribe option in emails

[zaibkhan]

No description provided.

[codoki-pr-intelligence]

Codoki PR Review

Summary: Add per-topic unsubscribe, ensure secure tokenization
What’s good: - Solid UX addition: per-topic unsubscribe link in emails and UI route; List-Unsubscribe header path wired into MessageBuilder spec.

• Minor robustness improvement in dropdown-button to avoid rendering an empty/undefined title.
  Review Status: ❌ Requires changes
  Overall Priority: Critical
  Review Update:
  • Coverage: Reviewed all 18 files across 2 batches

Issues (Critical & High only)

Severity	Issue	Why it matters
Critical	Security — Unsubscribe URL lacks signed, user-scoped token	…/models/topic.rb Security: The per-topic unsubscribe URL currently contains no user-scoped, signed token, which allows unintended or unauthorized unsubscribes if the endpoint changes state on GET (e.g., by link scanners or third parties). Ensure the URL carries a cryptographically signed, user-specific token and that the server verifies it before changing notification state; alternatively, make GET show a confirmation page and require a tokenized POST to apply changes.
High	Correctness — Nil TopicUser crash and bypass of TopicUser.change side e...	…/controllers/topics_controller.rb Possible NoMethodError if no topic_users row exists and inconsistent state updates. Directly mutating and saving bypasses notifications_changed_at, notifications_reason_id, and MessageBus publish that TopicUser.change handles. Use TopicUser.get with a safe fallback and TopicUser.change to atomically update and keep side effects consistent.

Showing top 2 issues. Critical: 1, High: 1. See inline suggestions for more.

Key Feedback (click to expand)

• Needs improvement: - The unsubscribe URL lacks a user-scoped, signed token; this risks unauthorized or drive-by unsubscriptions and violates secure email action patterns.
• Prefer generating canonical absolute URLs via existing helpers to avoid edge cases (double slashes, base URL changes) and ensure RFC-compliant List-Unsubscribe headers.
• Testing: - Add a request spec covering the unsubscribe flow via the emailed link, ensuring it requires a valid, user-scoped token and does not change state without it.
• Include a spec asserting the List-Unsubscribe URL is absolute and contains the token, and that invalid/expired tokens are rejected without changing TopicUser.notification_level.
• Documentation: - Document the unsubscribe flow briefly (token generation, validity window, and link behavior) to aid future maintainers and email template authors.
• Compatibility: - If mail clients or link scanners prefetch links, ensure the unsubscribe endpoint does not change state on a HEAD/OPTIONS/GET without a valid token; consider returning a lightweight confirmation page for GET without token.
• Security: - Unsubscribe links in emails should include a signed, user-specific token and verify it server-side; avoid state changes based solely on path parameters or session to prevent CSRF, link-scanner, and cross-user unsubscription.
• Open questions: - What guards does the server route for /t/:slug/:id/unsubscribe enforce? Is it behind auth, or does it accept a signed token for unauthenticated users?
• Do we want GET to perform state-changing unsubscribe, or should the link land on a confirmation page (GET) with a subsequent token-validated POST?

Confidence: 2/5 — Not ready to merge (1 critical · 1 high · status: Requires changes)

Sequence Diagram

sequenceDiagram
    participant UserNotifications
    participant Topic
    participant EmailMessageBuilder
    UserNotifications->>Topic: unsubscribe_url
    Topic-->>UserNotifications: url + "/unsubscribe"
    UserNotifications->>EmailMessageBuilder: build(body, add_unsubscribe_link, unsubscribe_url)
    EmailMessageBuilder-->>UserNotifications: message with List-Unsubscribe header

Loading

React with 👍 or 👎 if you found this review useful.

[codoki-pr-intelligence]

⚠️ High: Possible NoMethodError if no topic_users row exists and inconsistent state updates. Directly mutating and saving bypasses notifications_changed_at, notifications_reason_id, and MessageBus publish that TopicUser.change handles. Use TopicUser.get with a safe fallback and TopicUser.change to atomically update and keep side effects consistent.

Suggested change

	```suggestion
	tu = TopicUser.get(@topic_view.topic, current_user)
	current_level = tu ? tu.notification_level : TopicUser.notification_levels[:regular]
	desired_level = current_level > TopicUser.notification_levels[:regular] ? TopicUser.notification_levels[:regular] : TopicUser.notification_levels[:muted]
	TopicUser.change(current_user.id, params[:topic_id], notification_level: desired_level)

[codoki-pr-intelligence]

💡 Low: Typo in property name ('Notificiations'). It works because the template uses the same name, but it invites future bugs and makes the code harder to read.

Suggested change

	stopNotificiationsText: function() {
	```suggestion
	stopNotificationsText: function() {

[codoki-pr-intelligence]

🛑 Critical: Security: The per-topic unsubscribe URL currently contains no user-scoped, signed token, which allows unintended or unauthorized unsubscribes if the endpoint changes state on GET (e.g., by link scanners or third parties). Ensure the URL carries a cryptographically signed, user-specific token and that the server verifies it before changing notification state; alternatively, make GET show a confirmation page and require a tokenized POST to apply changes.