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

[everettbu]

Test 6

[github-actions]

This pull request has been automatically marked as stale because it has been open for 60 days with no activity. To keep it open, remove the stale tag, push code, or add a comment. Otherwise, it will be closed in 14 days.

[mfeuerstein]

PR Review — approved

Reviewed 5 files. 0 high-severity issues found. Verdict: approved.

app/assets/javascripts/discourse/templates/user/user.hbs (low)

• Reviewed app/assets/javascripts/discourse/templates/user/user.hbs — looks good

app/serializers/user_serializer.rb (low)

• Reviewed app/serializers/user_serializer.rb — looks good

app/assets/javascripts/discourse/models/user.js.es6 (low)

• Reviewed app/assets/javascripts/discourse/models/user.js.es6 — looks good

spec/serializers/user_serializer_spec.rb (low)

• Reviewed spec/serializers/user_serializer_spec.rb — looks good

app/assets/javascripts/discourse/controllers/user.js.es6 (low)

• Reviewed app/assets/javascripts/discourse/controllers/user.js.es6 — looks good

[zach-source]

Serializer inclusion predicate naming bug — see inline.

[ron-x5labs]

Code Review: UX: show complete URL path if website domain is same as instance domain

Problem

This PR moves the user profile "website name" display computation from the Ember frontend (`website.split("/")[2]`) to a Rails serializer attribute `website_name`, so the server can append the URL path when the website domain matches the Discourse instance domain — showing `example.com/user` instead of just `example.com` for same-instance links.

Solution Reviewed

`UserSerializer` gains a `website_name` attribute that uses `URI(website).host` and `Discourse.current_hostname` to decide whether to append the URL path. The Ember controller`s `websiteName` computed property is removed; the template now binds `model.website_name`. A JSDoc `@property` tag is corrected. Three spec cases cover exact-match, parent-domain, and unrelated-domain scenarios.

Summary

The intent is sound and the frontend migration is clean, but the serializer has two blocking defects: the inclusion predicate is misnamed (dead code), and `website_name` bypasses the `untrusted_attributes` gate that protects anonymous viewers of TL0 users — leaking the host/path of a website that `website` itself suppresses. Both are fixable in one move by registering `:website_name` in `untrusted_attributes` and deleting the hand-written predicate.

Files Reviewed

• `app/serializers/user_serializer.rb` — deeply reviewed
• `spec/serializers/user_serializer_spec.rb` — deeply reviewed
• `app/assets/javascripts/discourse/controllers/user.js.es6` — deeply reviewed (removal is clean; no remaining `websiteName` consumers)
• `app/assets/javascripts/discourse/models/user.js.es6` — lightly reviewed (doc-only fix, correct)
• `app/assets/javascripts/discourse/templates/user/user.hbs` — deeply reviewed (binding migration is correct)

Verification

• Grep-verified AMS 0.8.3 inclusion convention: every `include_` predicate in the repo ends with `?`; the `untrusted_attributes` macro (user_serializer.rb:25) defines `"include_#{attr}?"`. `website_name` is NOT in `untrusted_attributes` (line 105) — confirmed by direct read.
• Grep-verified no remaining `websiteName` references in the worktree (frontend migration complete).
• Grep-verified `RestModel` is `Ember.Object`-based with no `attr()` declarations — `model.website_name` is available without model changes.
• Ruby/Rails test suite: not run (no Ruby runtime available in this environment).

Issues Found

See inline comments for details. Summary:

Blocking

• user_serializer.rb:153 — `include_website_name` missing `?` -> dead predicate, `website_name` always serialized (including `null` for blank websites)
• user_serializer.rb:43 — `website_name` not in `untrusted_attributes` -> leaks host/path for TL0 users viewed anonymously, bypassing the `website` suppression gate

Non-blocking

• user_serializer.rb:138 — `rescue nil` swallows URI errors -> silent link-vanish for malformed website URLs
• user_serializer.rb:141 — case-sensitive host comparison (`URI.host` lowercases, `current_hostname` does not)

Suggestions

• spec:78 — missing test for the `elsif` sibling-subdomain branch and edge-case inputs (no path, trailing slash, blank website)
• user_serializer.rb:138 — `URI(website.to_s)` is parsed 2-3 times per call; extract once for clarity and minor perf

Verdict

Recommend changes before merge — two blocking defects in the serializer (dead inclusion predicate + TL0/anonymous data leak) are fixable together by adding `:website_name` to `untrusted_attributes` and deleting the manual predicate. The non-blocking issues should be addressed but are not merge-blockers on their own.

[ron-x5labs]

The inclusion predicate `include_website_name` is missing the required `?` suffix. ActiveModel::Serializers 0.8.3 (per Gemfile.lock) dispatches attribute inclusion via `include_?` — with the question mark. Every other predicate in this file follows that convention (`include_email?`, `include_stats?`, `include_card_image_badge_id?`, etc.), and the `untrusted_attributes`/`staff_attributes`/`private_attributes` macros at the top of this file all `define_method("include_#{attr}?")` with the `?`.

Without it, AMS never calls this method, so the `website.present?` gate is dead code and `website_name` is unconditionally serialized — emitting `website_name: null` for users with no website. No test covers the blank-website case that would expose this.

Fix: rename to `include_website_name?`, or better, register via `untrusted_attributes` (see the related comment on line 43) which auto-generates the correctly-named predicate and deletes the need for this hand-written method.

[ron-x5labs]

`:website_name` is declared in the `attributes` list here but is NOT registered in `untrusted_attributes` (line 105), unlike its sibling `:website` (line 109). The `untrusted_attributes` macro generates an `include_?` predicate that returns `false` when `scope.restrict_user_fields?(object)` is true — i.e., for TL0 users viewed by anonymous viewers. Because `website_name` is omitted, an anonymous viewer of a TL0 user gets `website` suppressed but still sees `website_name` (host + path), leaking exactly the information `website` is designed to hide.

Clean fix for both this and the dead-predicate issue on line 153: add `:website_name` to the `untrusted_attributes` call (auto-generates the correct gated `include_website_name?`) and delete the hand-written `include_website_name` method.

[ron-x5labs]

The bare `rescue nil` swallows `URI::InvalidURIError` (and all `StandardError`) for malformed-but-present website values. `UserUpdater#format_url` only prepends `http://`, it does not validate the URL, and `UserProfile` has no website validator — so values like `http://not a url` can persist.

When parsing fails, `website_host` becomes nil, `website_name` returns nil, and the `{{#if model.website_name}}` template guard hides the entire website block. Pre-PR, the Ember `website.split("/")[2]` still rendered something for many such inputs, so a previously-visible (if janky) link silently disappears with no error or log.

Consider falling back to the raw `website` when URI parsing fails (mirroring prior behavior), and narrowing the rescue to `URI::InvalidURIError` with a `Rails.logger.warn`.

[ron-x5labs]

`URI(website.to_s).host` lowercases the host, but `Discourse.current_hostname` (lib/discourse.rb:178) returns the raw `force_hostname` or multisite hostname without normalization. If `force_hostname` is set with uppercase letters, the `website_host == discourse_host` comparison here (and the `ends_with?` check on line 149) will fail to match, silently degrading to host-only display.

Consider normalizing both sides with `.downcase`.

[ron-x5labs]

The `elsif` sibling-subdomain branch of `website_name` (user_serializer.rb:144) is not tested. That branch fires when `website_host` and `discourse_host` have equal part-count > 2 and share the same parent domain (e.g. `www.example.com` vs `forum.example.com`). It contains nontrivial `split`/`join` comparison logic that could regress silently.

Consider adding a case for it, plus edge-case inputs: no path (`http://example.com\` -> empty path), trailing slash (`http://example.com/\`), and a blank-website assertion that `website_name` is absent from the JSON.

[ron-x5labs]

Code Review: UX: show complete URL path if website domain is same as instance domain

Problem

This PR moves the user-profile "website name" display label from the Ember frontend (website.split("/"[2]) into a Rails serializer attribute website_name, so the server appends the URL path when the website's domain matches the instance domain (or a parent/sibling) and shows just the host otherwise.

Solution Reviewed

The frontend websiteName computed property and its JSDoc are removed; the template renders model.website_name. UserSerializer adds a website_name attribute computed via URI host/path logic against Discourse.current_hostname, plus an include_website_name predicate. Specs cover three matching scenarios.

Summary

The server-side approach is sound, but the inclusion predicate is misnamed for the pinned AMS 0.8.3 API, so it is never invoked — website_name is always serialized and, because it skips untrusted_attributes, leaks the website host/path to anonymous/TL0-restricted viewers who are explicitly denied website. That is a privacy regression that should block merge. Two smaller edge-case/coverage gaps noted inline.

Files Reviewed

• app/serializers/user_serializer.rb — deeply reviewed (high risk: privacy-adjacent)
• spec/serializers/user_serializer_spec.rb — deeply reviewed
• app/assets/javascripts/discourse/templates/user/user.hbs — lightly reviewed (escape-safe template change)
• app/assets/javascripts/discourse/controllers/user.js.es6 — lightly reviewed (dead-code removal)
• app/assets/javascripts/discourse/models/user.js.es6 — lightly reviewed (doc-comment fix)

Verification

• Bundler/Rails test suite — skipped: gems not installed in the review env and a full Rails run is too expensive here. Findings are grounded in the AMS 0.8.3 source (define_include_method/include? register and call include_<name>?) and the repo's own untrusted_attributes / restrict_user_fields? definitions, not inference.

Issues Found

See inline comments. Summary:

• 🔴 Blocking: include_website_name is missing the ? suffix → never invoked by AMS 0.8.3 → website_name always serialized; and :website_name is not in untrusted_attributes → bypasses the TL0 restrict_user_fields? privacy gate that hides website from anonymous viewers.
• 🟡 Non-blocking: host comparison is case-sensitive (URI#host lowercases; Discourse.current_hostname is not normalized).
• 🟡 Non-blocking: the elsif sibling-subdomain branch (user_serializer.rb:144) is untested.
• 💡 Suggestion: narrow the bare rescue nil to URI::InvalidURIError.

Verdict

Recommend changes before merge — the inclusion-predicate bug silently regresses the existing website-visibility privacy gate; the fix is a one-line untrusted_attributes registration plus deleting the custom predicate.

[ron-x5labs]

🔴 Blocking: inclusion predicate is never invoked — website_name is always serialized, leaking to restricted viewers.

ActiveModel::Serializers 0.8.3 (Gemfile.lock) dispatches attribute inclusion via include_<attr>? with the ? suffix: define_include_method registers include_website_name? in INCLUDE_METHODS, and include?(name) does send INCLUDE_METHODS[name]. A bare include_website_name (no ?) is not that method, so AMS uses its auto-generated default include_website_name? → always true.

Net effect: website_name is emitted for every user regardless of website.present?. And because :website_name was not added to untrusted_attributes (line 105) like its sibling :website, it bypasses the restrict_user_fields? gate (UserGuardian#restrict_user_fields? = TL0 user viewed by anonymous). An anonymous viewer who is explicitly denied website still receives website_name, leaking the website host (and the full path on same-domain links).

Fix: register :website_name in untrusted_attributes (so AMS defines the proper include_website_name? with the TL0 gate) and delete this method:

  untrusted_attributes :bio_raw,
                       :bio_cooked,
                       :bio_excerpt,
                       :location,
                       :website,
                       :website_name,
                       :profile_background,
                       :card_background

# then remove:
  def include_website_name
    website.present?
  end

[ron-x5labs]

🟡 Non-blocking: host comparison is case-sensitive.

URI(website.to_s).host lowercases the host, but Discourse.current_hostname (lib/discourse.rb:178) returns the raw force_hostname / multisite hostname without normalization. If force_hostname (or the multisite DB hostname) is stored with uppercase letters (e.g. Example.com), the website_host == discourse_host check on line 141 misses and the sibling/parent branches fall through to host-only display, silently degrading the same-domain path feature. Compare normalized values:

discourse_host = Discourse.current_hostname.to_s.downcase

[ron-x5labs]

🟡 Non-blocking: the elsif sibling-subdomain branch (user_serializer.rb:144) is untested.

The three specs cover cross-domain (→ host only), exact-match (→ host+path), and website-is-parent-of-instance (→ host+path via the else branch). The elsif branch — equal part-count > 2 with a shared parent, e.g. website http://www.example.com/page on instance forum.example.com — is never exercised. Add a case so that branch is covered:

it "returns complete website path when website is a sibling subdomain of instance domain" do
  user.user_profile.website = 'http://www.example.com/page'
  Discourse.stubs(:current_hostname).returns('forum.example.com')
  expect(json[:website_name]).to eq 'www.example.com/page'
end

[ron-x5labs]

💡 Suggestion: narrow the rescue.

rescue nil swallows every StandardError (e.g. an unexpected NoMethodError if website ever isn't a string), masking a real bug as a silent nil return. UserUpdater#format_url only prepends http://, so the only realistic failure here is a malformed URI — catch that specifically:

website_host =
  begin
    URI(website.to_s).host
  rescue URI::InvalidURIError
    nil
  end

[ron-x5labs]

Code Review: UX: show complete URL path if website domain is same as instance domain

Problem

Move the user-profile "website name" display label from the Ember frontend (website.split("/")[2]) into a Rails serializer attribute website_name, so the server appends the URL path when the website's domain matches (or is a parent/sibling of) the Discourse instance domain, and shows just the host otherwise. (PR body is "Test 6"; intent inferred from the title and diff.)

Solution Reviewed

Frontend websiteName computed property removed; templates/user/user.hbs renders model.website_name; JSDoc @property tag corrected to profileBackground. UserSerializer adds a website_name attribute using URI host/path logic against Discourse.current_hostname, plus an include_website_name predicate. Three specs added for the matching scenarios.

Summary

The same-domain path feature works, but the privacy/inclusion gate is broken in two compounding ways that together leak a TL0 user's website host (and path) to anonymous viewers — a regression of the exact field :website is designed to hide. Needs changes before merge.

Files Reviewed

• app/serializers/user_serializer.rb — deeply reviewed (high risk: serializer/privacy boundary).
• spec/serializers/user_serializer_spec.rb — deeply reviewed (medium risk).
• app/assets/javascripts/discourse/templates/user/user.hbs — lightly reviewed (low risk: field rename).
• app/assets/javascripts/discourse/controllers/user.js.es6 — lightly reviewed (low risk: removed computed prop; repo grep confirmed no stale websiteName consumers).
• app/assets/javascripts/discourse/models/user.js.es6 — lightly reviewed (low risk: JSDoc tag fix only).

Verification

• AMS 0.8.3 inclusion dispatch — verified from the gem source (define_include_method registers include_<attr>? with the ?; include?(name) sends INCLUDE_METHODS[name]): confirms include_website_name (no ?) is never invoked. Ground truth, not inference.
• Threat model — verified restrict_user_fields? at lib/guardian/user_guardian.rb:58 = trust_level == 0 && anonymous?.
• Hostname normalization — verified Discourse.current_hostname (lib/discourse.rb:178) has no .downcase. Case-sensitivity was not flagged: it's pre-existing convention (session_controller.rb:93, topic_link.rb:120 already do case-sensitive parsed.host == Discourse.current_hostname); ends_with? is the ActiveSupport alias used in 3 other files.
• No stale JS consumers — grep for websiteName in app/assets/javascripts returns no matches outside the changed template.
• RSpec suite / typecheck — skipped: no Ruby toolchain in the review environment.

Verdict

Recommend changes before merge — fix the privacy/inclusion gate (register :website_name in untrusted_attributes and delete the dead include_website_name predicate) so a TL0 user's website host isn't leaked to anonymous viewers, and add the missing elsif/edge-case coverage.

[ron-x5labs]

🔴 Blocking: website_name bypasses the TL0/anonymous privacy gate that hides website.

:website_name is declared in attributes (here) but is NOT registered in untrusted_attributes (line 105) — the macro that generates include_<attr>? returning false when scope.restrict_user_fields?(object) is true (TL0 user viewed anonymously; see lib/guardian/user_guardian.rb:58). :website IS registered, so it's suppressed for those viewers, but :website_name (host, +path when same-domain) is emitted anyway — leaking exactly the field :website is designed to hide. Pre-PR the client derived websiteName from the suppressed model.website, so nothing leaked; moving extraction server-side past the gate regresses privacy.

Fix — register it alongside :website and drop the dead predicate (see next comment):

# line 105
untrusted_attributes :bio_raw, :bio_cooked, :bio_excerpt, :location,
                     :website, :website_name, :profile_background, :card_background

Also add :website_name to the untrusted_attributes list in spec/serializers/user_serializer_spec.rb:11 so the existing TL0-anon suppression test catches this leak.

[ron-x5labs]

🔴 Blocking: inclusion predicate is misnamed — AMS 0.8.3 never calls it.

This method is include_website_name (no ?). AMS 0.8.3 dispatches attribute inclusion via include_<attr>? with the ?: define_include_method registers include_website_name? in INCLUDE_METHODS, and include?(:website_name) sends that. So this method is never invoked; AMS falls back to its auto-defined include_website_name? → true, the website.present? gate is dead code, and website_name is serialized unconditionally (emitting website_name: null for users with no website). Every sibling predicate (include_email?, include_card_image_badge_id?) and all three macros use the ? suffix.

Don't just rename it — register :website_name in untrusted_attributes (line 105), which auto-generates the correct include_website_name? carrying both the restrict_user_fields? privacy gate and the present? gate, then delete this method.

[ron-x5labs]

🟡 Non-blocking: bare rescue nil swallows URI errors and regresses malformed-URL display.

UserProfile has no website validator and UserUpdater#format_url only prepends http:// without URI validation, so values like http://foo bar can persist. For such input URI(website.to_s).host raises URI::InvalidURIError, rescue nil returns nil, website_name returns nil, and the template guard {{#if model.website_name}} hides the entire website block — whereas the removed Ember website.split("/")[2] rendered a value for many of these inputs, so the link silently vanishes with no log.

Narrow the rescue and cover it with a spec:

website_host =
  begin
    URI(website.to_s).host
  rescue URI::InvalidURIError
    nil
  end

[ron-x5labs]

💡 Suggestion: trailing-slash root URL renders a dangling /; also parse URI once.

For a same-domain website like http://example.com/, URI(...).path is "/", so website_host + path yields example.com/ — the old Ember website.split("/")[2] returned example.com (no slash). Strip a bare root path, and parse URI(website.to_s) once into a local to avoid re-parsing 2-3x per call across the branches:

uri = URI(website.to_s)
website_host = uri.host
return if website_host.nil?
path = uri.path == "/" ? "" : uri.path
# ... then use website_host + path in each branch

[ron-x5labs]

🟡 Non-blocking: the elsif sibling-subdomain branch and path edge inputs are untested.

The three specs cover cross-domain (→ host only), exact same-domain (→ host+path), and website-is-parent-of-instance (→ host+path via the else branch). The elsif branch — equal part-count >2 with a shared parent, e.g. website http://www.example.com/page on instance forum.example.com — is never exercised, and neither are its non-matching-parent outcome, no-path, trailing-slash, or blank-website inputs. Add:

it "returns host+path when website is a sibling subdomain of the instance" do
  user.user_profile.website = 'http://www.example.com/page'
  Discourse.stubs(:current_hostname).returns('forum.example.com')
  expect(json[:website_name]).to eq 'www.example.com/page'
end

it "returns host only when sibling subdomains differ in parent" do
  user.user_profile.website = 'http://www.example.com/page'
  Discourse.stubs(:current_hostname).returns('forum.other.com')
  expect(json[:website_name]).to eq 'www.example.com'
end

it "omits website_name when website is blank" do
  user.user_profile.website = nil
  expect(json).not_to have_key(:website_name)
end

(The last case currently fails until the dead-predicate bug above is fixed — that's the point.)