Discourse PR #30 — Live Benchmark

Golden Comments (1/1 found)

#	Comment	Severity	Status	FriendlyReviewer Detail
#1	The `include_website_name` method is missing the required `?` suffix. Rails serializers expect `include_` methods to end with `?` for conditional attribute inclusion, a convention followed by other methods in this serializer. Without it, the `website_name` attribute may not be conditionally included as intended. Additionally, the `'.' << website_host` string concatenation should be replaced with `'.' + website_host` or `'.#{website_host}'` to avoid mutating string literals, which can lead to issues.	MEDIUM	✓ Found	"`include_website_name` is missing the trailing `?`, so AMS does not recognize it as an inclusion guard and `website_name` is unconditionally serialized for every user (including users with no website). The intended `website.present?` check is dead code. Rename to `include_website_name?` and route through the project's conditional-exposure macros." (app/serializers/user_serializer.rb L153) — partial match: the second half of the golden, the `'.' << website_host` concatenation that mutates string literals, is not mentioned by the review.

Supplementary Findings (5 findings)

Finding	File	Severity	Legitimate?
`website_name` is not registered under `untrusted_attributes`, so its host value (e.g. `example.com`) leaks to anonymous/TL0 viewers even when the source `website` field is masked by `restrict_user_fields?`	app/serializers/user_serializer.rb:109	HIGH	✓ Real trust-level bypass: the host leaks to TL0 viewers when the underlying field is masked — arguably more severe than the golden itself
The TL0 anonymous context only asserts the absence of existing attributes and uses a blank website fixture, so the `website_name` leak is invisible to the spec	spec/serializers/user_serializer_spec.rb:11	MEDIUM	✓ Real test gap: the leak passes unseen because the fixture is blank
The label-count string comparison is asymmetric: when the website is a subdomain of the instance (e.g. `www.example.com` vs `example.com`) the path is not shown, while the reverse direction matches; `URI(...).host` also strips the port, treating same host on different ports as identical	app/serializers/user_serializer.rb:137	MEDIUM	✓ Real logic flaw: asymmetric matching plus port-stripping makes same host on different ports look identical
`website_name` returns `nil` for scheme-less websites because `URI("example.com/user").host` is `nil`, while the (once-corrected) guard would still be true — a key with a nil value; the broad `rescue nil` also hides parse failures	app/serializers/user_serializer.rb:138	MEDIUM	✓ Real: scheme-less hosts yield a nil-value key once the guard is fixed, and the rescue masks failures
The spec covers only the exact-match and website-parent-of-instance branches; the middle subdomain branch (e.g. `www.example.com` vs `forum.example.com`, same label count) and the no-path case are untested	spec/serializers/user_serializer_spec.rb:77	LOW	✓ Borderline: coverage gap only, no production defect

Honest Analysis

The single golden is two-part: the missing ? on include_website_name and the '.' << website_host concatenation that mutates string literals. FriendlyReviewer covers the first half thoroughly — the guard is never recognized by AMS, so website_name is serialized unconditionally for every user and the website.present? check is dead code — and pins it to user_serializer.rb L153, the exact line. The second half is absent from the review: no finding mentions the string-literal mutation. The 1/1 recall therefore flatters the result; this is a partial hit, and the Found status is kept only because the dominant defect was caught.

The best supplementary finding is the L109 trust-level leak: website_name not registered under untrusted_attributes exposes the host to anonymous/TL0 viewers even when the source field is masked — arguably more severe than the golden itself. The two spec findings (L11, L77) are coherent: the test suite structurally cannot see either defect. The L137/L138 domain-matching findings are concrete (asymmetric subdomain comparison, port stripping, nil for scheme-less input) and correctly tied to the same serializer.

The weak points are honest to state. The review certifies "critical defects that block merging" partly on a Rails/AMS convention (the include_? suffix) — correct per the golden — but the L109 leak is asserted without showing the masking path it depends on. And the golden's second defect never surfaced: a reviewer who read the diff closely enough to flag dead code at L153 should have seen '.' << website_host a few lines above. Strong on the guard, blind on the second issue — a partial hit that the recall badge alone would overstate.

FriendlyReviewer's full review

As posted on the PR — the complete output from FriendlyReviewer.

friendly-reviewer bot commented now

Hi there 👋

🌥️ Tech	🌤️ Feat
2 high, 3 medium, 1 low	1 mitigated

This MR introduces a website_name attribute to UserSerializer intended to expose the site's host (plus path when it matches the instance domain) as a UX improvement. While the three added specs pass for the exercised branches, the implementation contains critical defects that block merging: the include_website_name guard is misnamed (missing ?) so it never runs and website_name is always serialized for every user; the attribute is not registered under untrusted_attributes, leaking the host to anonymous/TL0 viewers even when the underlying website field is masked; and the domain-matching logic is asymmetric and can fail for subdomain-of-instance cases. Spec coverage also omits the middle subdomain branch, the no-path case, and the TL0 masking path. These issues must be fixed and covered before the change is ready.

These might need a close look

🔴 app/serializers/user_serializer.rb (L153)
include_website_name is missing the trailing ?, so AMS does not recognize it as an inclusion guard and website_name is unconditionally serialized for every user (including users with no website). The intended website.present? check is dead code. Rename to include_website_name? and route through the project's conditional-exposure macros.
🔴 app/serializers/user_serializer.rb (L109)
website_name is not registered under untrusted_attributes, so its host value (e.g. example.com) is exposed to anonymous/TL0 viewers even when the source website field is masked by restrict_user_fields?. This trust-level masking bypass must be fixed by registering website_name as untrusted.

Worth checking

🟡 spec/serializers/user_serializer_spec.rb (L11)
The TL0 anonymous context only asserts the absence of existing attributes and uses a blank website fixture, so the website_name leak is invisible. Add a case where a TL0 user has a website and assert json does not include website_name, and extend the untrusted_attributes list with :website_name.
🟡 app/serializers/user_serializer.rb (L137)
The label-count string comparison is asymmetric and error-prone: when the website is a subdomain of the instance (e.g. www.example.com vs example.com), the path is not shown, while the reverse direction is treated as matching. The else branch can false-positive on single-label hosts, and URI(...).host strips the port, treating same host on different ports as identical. Use a proper suffix/public-suffix comparison and guard label counts; also parse URI(website.to_s) once instead of three times.
🟡 app/serializers/user_serializer.rb (L138)
website_name returns nil for scheme-less websites (e.g. bare example.com/user from import scripts) because URI("example.com/user").host is nil, while the (once-corrected) include_website_name? would still return true, yielding a key with a nil value. The broad rescue nil also hides parse failures. Guard on website.present? and handle scheme-less input before parsing.

Small things (take or leave)

🔵 spec/serializers/user_serializer_spec.rb (L77)
The spec covers only the exact-match and website-parent-of-instance branches; it omits the middle subdomain branch (e.g. www.example.com vs forum.example.com, same label count) and any case where the website has no path. Add coverage for these to lock in the intended contract.

✅ Feature-level checklist

Partially covered

⚠️ UX: show complete URL path if website domain is same as instance domain
The implementation attempts to expose the full host+path when the website domain matches the instance, and works for exact matches and the website-parent-of-instance direction. However, the guard is dead code causing unconditional serialization, the domain-matching logic is asymmetric (fails for subdomain-of-instance cases), and the feature leaks the host to untrusted users. Additionally, the no-path and middle-branch cases are untested. The core requirement is only partially satisfied.

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