Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add homepage url to user profile #5240

Open
wants to merge 22 commits into
base: master
Choose a base branch
from

Conversation

jacklynhma
Copy link
Contributor

@jacklynhma jacklynhma commented Nov 15, 2024

Objective:

  1. Add form to the edit profile
  2. Update the user profile to display the homepage URL
  3. Display the homepage URL on the dashboard

More context: This PR opened during the Ruby Conf Hack day. After speaking with Martin, it was decided that I add a basic homepage URL that can later be iterated on for future social media links.

How to test part 1: Add form to the edit profile

  • Login
  • On the upper right click the drop-down menu
  • Click Edit Profile
  • Add your homepage with the format https://yourwebsite.com
  • Add your password
  • Submit form
  • You should see the below image
    Note: I was told that the icon will show on production:
Screenshot 2024-11-14 at 17 59 50

How to test part 2: Update the user profile to display the homepage URL

  • Create a user
  • Login as a ruby gem user and navigate to /profiles/new-user-username
  • You should see the new image:
Screenshot 2024-11-15 at 10 41 17

How to test part 3: Navigate to /dashboard

  • You should see the below image with the homepage listed
Screenshot 2024-11-14 at 18 03 36

app/models/user.rb Outdated Show resolved Hide resolved
Copy link

codecov bot commented Nov 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.19%. Comparing base (1143eba) to head (9b2bf91).
Report is 9 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5240      +/-   ##
==========================================
- Coverage   96.85%   94.19%   -2.67%     
==========================================
  Files         456      456              
  Lines        9517     9577      +60     
==========================================
- Hits         9218     9021     -197     
- Misses        299      556     +257     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@segiddins segiddins left a comment

Choose a reason for hiding this comment

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

don't worry about the coverage change

app/views/dashboards/_subject.html.erb Show resolved Hide resolved
<%=
link_to(
@user.homepage_url,
@user.homepage_url,

Check warning

Code scanning / CodeQL

Stored cross-site scripting Medium

Stored cross-site scripting vulnerability due to
stored value
.
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should consider something like HackerOne's "you're about to leave this site for ...". Github appends http:// to urls that don't have either http / https in the front, and they are probably doing more.

@martinemde
Copy link
Member

Blocking temporarily while we make sure we're sanitizing the urls. I suspect that since we already allow urls from gems, this isn't a whole lot worse, but I want to double check.

@jacklynhma
Copy link
Contributor Author

@martinemde Thanks for calling this out.

I could be wrong, but while I was looking at the code, I did not see any sanitizing for the URL.

There are some safeguards though.

Screenshot 2024-11-21 at 16 55 02

which is tied to https://github.com/rubygems/rubygems.org/blob/master/config/initializers/content_security_policy.rb#L33 and which protects against XXS attacks.

However with this validation https://github.com/rubygems/rubygems.org/pull/5240/files#diff-9802ca3c9c4cf89904fd44bc114e35ebdf2c5dd3d5b645491e2b253e1afef29bR357
It looks like the code does prevent javascript:alert('XSS'); from even being submitted
https://github.com/mdespuits/validates_formatting_of/blob/664b7c8b1ae8c9016549944fc833737c74f1d752/lib/validates_formatting_of/method.rb#L19
So something like this will be caught and the below error message will show:
Screenshot 2024-11-21 at 16 33 36

What we can also do is with that sanitize method

      sanitize( link_to(
         user.homepage_url,
         user.homepage_url,
        rel: "nofollow"
      ), tags: %w(a), attributes: %w(href rel))

And then it will remove the href from the link and make it unclickable.
Screenshot 2024-11-21 at 17 18 30

But I understand that true sanitizing would remove everything we don't want in the string. I could also look into this. Please let me know how you would like me to proceed or if I am completely off the mark.

@martinemde
Copy link
Member

I'm happy to see that our Content Security Policy is correctly enforced. We probably agree that we don't want to rely on only that.

Can we write tests that ensure that no data: javascript: file: or similar urls are allowed? That's a good start.

The validation for a link being allowed in a rubygem is this: https://github.com/rubygems/rubygems/blob/master/lib/rubygems/specification_policy.rb#L450-L459

@@ -0,0 +1,10 @@
class HttpUrlValidator < ActiveModel::EachValidator
Copy link
Contributor Author

@jacklynhma jacklynhma Nov 24, 2024

Choose a reason for hiding this comment

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

I read about how the URI validation occurs in the Ruby gem(https://github.com/rubygems/rubygems/blob/master/lib/rubygems/specification_policy.rb#L450-L459) and tried to mimic it since I figured we would want the same validations to happen in both places.

I extracted this validation into a custom validator because I knew that other URLs would eventually be added to the user profile, and we could establish a consistent way of validating the URLs.

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

Successfully merging this pull request may close these issues.

3 participants