UX: show complete URL path if website domain is same as instance domain#5
UX: show complete URL path if website domain is same as instance domain#5ShashankFC wants to merge 1 commit into
Conversation
|
@CodeAnt-AI: review |
|
CodeAnt AI is running the review. |
Nitpicks 🔍
|
| website_host = URI(website.to_s).host rescue nil | ||
| discourse_host = Discourse.current_hostname | ||
| return if website_host.nil? | ||
| if website_host == discourse_host | ||
| # example.com == example.com | ||
| website_host + URI(website.to_s).path | ||
| elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2 | ||
| # www.example.com == forum.example.com | ||
| website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host | ||
| else | ||
| # example.com == forum.example.com | ||
| discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host | ||
| end | ||
| end | ||
|
|
||
| def include_website_name | ||
| website.present? |
There was a problem hiding this comment.
Suggestion: The website_name attribute is always considered for serialization, even when the user's website field is supposed to be hidden (e.g. TL0 users viewed anonymously) or blank, because the include hook is misnamed (include_website_name instead of include_website_name?) and does not respect scope.restrict_user_fields?, which can leak website information and produce inconsistent JSON; update the guard to check both website.present? and !scope.restrict_user_fields?(object) and use the correct include_... ? naming so the serializer honours it. [security]
Severity Level: Critical 🚨
- ❌ TL0 anonymous user website hidden but website_name exposed.
- ❌ /users/:username.json leaks restricted website URL data.
- ⚠️ HTML profile preloaded JSON also exposes website_name.| website_host = URI(website.to_s).host rescue nil | |
| discourse_host = Discourse.current_hostname | |
| return if website_host.nil? | |
| if website_host == discourse_host | |
| # example.com == example.com | |
| website_host + URI(website.to_s).path | |
| elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2 | |
| # www.example.com == forum.example.com | |
| website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host | |
| else | |
| # example.com == forum.example.com | |
| discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host | |
| end | |
| end | |
| def include_website_name | |
| website.present? | |
| return if website.blank? || scope.restrict_user_fields?(object) | |
| website_host = URI(website.to_s).host rescue nil | |
| discourse_host = Discourse.current_hostname | |
| return if website_host.nil? | |
| if website_host == discourse_host | |
| # example.com == example.com | |
| website_host + URI(website.to_s).path | |
| elsif (website_host.split('.').length == discourse_host.split('.').length) && discourse_host.split('.').length > 2 | |
| # www.example.com == forum.example.com | |
| website_host.split('.')[1..-1].join('.') == discourse_host.split('.')[1..-1].join('.') ? website_host + URI(website.to_s).path : website_host | |
| else | |
| # example.com == forum.example.com | |
| discourse_host.ends_with?("." << website_host) ? website_host + URI(website.to_s).path : website_host | |
| end | |
| end | |
| def include_website_name? | |
| website.present? && !scope.restrict_user_fields?(object) |
Steps of Reproduction ✅
1. In the Rails app, create or use a TL0 user with a non-blank website, matching the setup
from `spec/serializers/user_serializer_spec.rb:6-12` (user with `trust_level: 0` and a
`user_profile`), but also set `user.user_profile.website = 'http://example.com/user'` as
in the spec context at `spec/serializers/user_serializer_spec.rb:67-70`.
2. Serialize this user as anonymous using the same pattern as the specs: `serializer =
UserSerializer.new(user, scope: Guardian.new, root: false); json = serializer.as_json`
(see `spec/serializers/user_serializer_spec.rb:8-9` where `scope: Guardian.new` represents
an anonymous guardian).
3. Because the user is TL0 and the scope is anonymous, `Guardian#restrict_user_fields?` at
`lib/guardian/user_guardian.rb:58-60` returns true, so the dynamically defined
`include_website?` method (created by `untrusted_attributes` at
`app/serializers/user_serializer.rb:25-33,105-111`) evaluates `return false if
scope.restrict_user_fields?(object)` and suppresses the `website` attribute; this behavior
is asserted for `:website` in the TL0 spec at
`spec/serializers/user_serializer_spec.rb:11-15`.
4. Despite `website` being omitted, the `website_name` attribute is still serialized
because (a) it is listed in `attributes` at `app/serializers/user_serializer.rb:35-43`,
(b) its include hook is misnamed `include_website_name` without a `?` at
`app/serializers/user_serializer.rb:153-155`, so ActiveModel::Serializer does not treat it
as an `include_... ?` predicate, and (c) the `website_name` method at
`app/serializers/user_serializer.rb:137-151` has no `restrict_user_fields?` guard; as a
result, `json[:website]` is absent while `json[:website_name]` still contains
`'example.com/user'`, leaking a field that was intentionally hidden for TL0 anonymous
views.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** app/serializers/user_serializer.rb
**Line:** 138:154
**Comment:**
*Security: The `website_name` attribute is always considered for serialization, even when the user's website field is supposed to be hidden (e.g. TL0 users viewed anonymously) or blank, because the include hook is misnamed (`include_website_name` instead of `include_website_name?`) and does not respect `scope.restrict_user_fields?`, which can leak website information and produce inconsistent JSON; update the guard to check both `website.present?` and `!scope.restrict_user_fields?(object)` and use the correct `include_... ?` naming so the serializer honours it.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.|
CodeAnt AI finished running the review. |
User description
Test 6
Summary by CodeRabbit
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.
Replicated from ai-code-review-evaluation/discourse-coderabbit#6
CodeAnt-AI Description
Show full website path on user profiles when website domain matches forum domain
What Changed
Impact
✅ Clearer website links on user profiles✅ Accurate display when forum and website domains match✅ Fewer client-side inconsistencies for profile website text💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.