Skip to content

UX: show complete URL path if website domain is same as instance domain#5

Open
zaibkhan wants to merge 1 commit into
url-handling-prefrom
url-handling-post
Open

UX: show complete URL path if website domain is same as instance domain#5
zaibkhan wants to merge 1 commit into
url-handling-prefrom
url-handling-post

Conversation

@zaibkhan

Copy link
Copy Markdown

No description provided.

@codoki-pr-intelligence

codoki-pr-intelligence Bot commented Sep 10, 2025

Copy link
Copy Markdown

Codoki PR Review

Summary: Show full website path, fix privacy guard
What’s good: Nice UX improvement to display the full website path when the user’s site matches the instance domain; the Ember template and tests reflect this intent.
Review Status: ❌ Requires changes
Overall Priority: High

Issues (Critical & High only)

Severity Issue Why it matters
High Security — website_name include hook misnamed and bypasses untrusted... …/serializers/user_serializer.rb
The include hook is misnamed (missing the ?) so it won't be used by the serializer, and it doesn't incorporate the untrusted-field guard. As a result, website_name may be serialized when it should be hidden for TL0 users viewed anonymously. Either (a) define the proper include hook with the same guard logic as other untrusted attributes, or (b) better, add :website_name to untrusted_attributes and remove this method entirely to centralize behavior.

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

Key Feedback (click to expand)
  • Needs improvement: The serializer’s include hook is misnamed and currently bypasses existing untrusted field guards, potentially exposing website_name for TL0 users to anonymous viewers.
  • Testing: Add a spec to ensure website_name is not serialized for a TL0 user viewed by anonymous, mirroring the existing test for website; this will prevent privacy regressions.
  • Security: The current implementation may expose website_name for untrusted (TL0) users to anonymous viewers because the include hook is misnamed and not tied into the untrusted attribute guard, creating a potential privacy leak.
  • Open questions: Should website_name also be hidden whenever website is hidden (e.g., TL0/anonymous)? If yes, do we also want symmetry for subdomain cases (blog.example.com vs example.com)?

Confidence: 3/5 — Needs work before merge (1 high · status: Requires changes)

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

discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host
end
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ High: The include hook is misnamed (missing the ?) so it won't be used by the serializer, and it doesn't incorporate the untrusted-field guard. As a result, website_name may be serialized when it should be hidden for TL0 users viewed anonymously. Either (a) define the proper include hook with the same guard logic as other untrusted attributes, or (b) better, add :website_name to untrusted_attributes and remove this method entirely to centralize behavior.

Suggested change
def include_website_name?
return false if scope.restrict_user_fields?(object)
website.present?
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants