Hi there 👏
The UserSerializer implementation for website_name is functionally correct for the three main domain comparison scenarios and has adequate test coverage for those cases. However, the review identified a critical data leak: website_name is not listed in untrusted_attributes, which means it could expose the website hostname/path to TL0 users even when website itself is hidden. Additionally, there is a naming inconsistency in the conditional include method (include_website_name without ? vs. include_website_name? pattern used elsewhere) that may cause issues depending on the ActiveModelSerializers version. Test coverage is incomplete: no test for nil website (key should be absent), no tests for staff-only attributes, private attributes, or nested relationships. These gaps should be addressed to ensure robustness and security.
These might need a close look
- 🔴 app/serializers/user_serializer.rb (L98)
website_name is not declared in untrusted_attributes. Since website is untrusted (hidden for TL0 users viewed anonymously), website_name — which exposes the website hostname and path — must also be untrusted to prevent data leakage.
- 🔴 spec/serializers/user_serializer_spec.rb (L16)
The TL0 untrusted attributes test does not include website_name in its checked list. Add :website_name to the untrusted_attributes list and assert that the key is absent for TL0 anonymous viewers.
Worth checking
- 🟡 app/serializers/user_serializer.rb (L165)
Method named include_website_name without a trailing ?, while all other conditional include methods use include_<attr>? pattern. Rename to include_website_name? for consistency and reliable behavior across ActiveModelSerializers versions.
- 🟡 spec/serializers/user_serializer_spec.rb (L91)
Missing test case: when user.user_profile.website is nil, include_website_name returns false, so the website_name key should NOT appear in the JSON output.
- 🟡 spec/serializers/user_serializer_spec.rb (L91)
Missing test coverage for staff attributes (post_count, can_be_deleted, etc.)
- 🟡 spec/serializers/user_serializer_spec.rb (L91)
Missing test coverage for private attributes (locale, email_digests, etc.)
- 🟡 spec/serializers/user_serializer_spec.rb (L91)
Missing test coverage for nested relationships (invited_by, custom_groups, etc.)
FriendlyReviewer found golden comment #1 partially — it detected the missing
?suffix oninclude_website_namebut did not catch the string literal mutation ('.' << website_hostusing<<instead of interpolation). The string mutation is a subtle Ruby-specific bug that requires understanding that string literals are frozen by default in newer Ruby versions, which FriendlyReviewer did not infer.The most significant finding is not in the golden set at all: FriendlyReviewer discovered that
website_nameis not listed inuntrusted_attributes, creating a real security data leak. Sincewebsiteitself is an untrusted attribute (hidden for TL0 anonymous viewers), its derivativewebsite_namemust also be untrusted — otherwise the website hostname and path leak through. This is a genuine security bug, and it is the most important finding of this review.The four YELLOW test coverage findings (missing nil test, staff attributes, private attributes, nested relationships) are legitimate but generic — they would apply to virtually any serializer change. They are not wrong, but they lack specificity to this particular PR's logic. FriendlyReviewer tends to produce generic test-coverage findings when the actual code change is small and well-scoped, which is acceptable: the coverage gaps are real, even if formulaic.
For a 5-file, +44/-12 PR, the analysis is correct but not exhaustive. FriendlyReviewer did not miss any bugs that the golden set considers high-impact — the untrusted attributes finding compensates for the one missed sub-bug. The review output is concise and focused, with no noise findings.