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
Open
Show file tree
Hide file tree
Changes from 19 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/controllers/profiles_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ def params_user
end
end

PERMITTED_PROFILE_PARAMS = %i[handle twitter_username unconfirmed_email public_email full_name].freeze
PERMITTED_PROFILE_PARAMS = %i[handle twitter_username unconfirmed_email homepage_url public_email full_name].freeze

def verify_password
password = params.permit(user: :password).require(:user)[:password]
Expand Down
1 change: 1 addition & 0 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ def create
password
website
twitter_username
homepage_url
full_name
].freeze

Expand Down
4 changes: 3 additions & 1 deletion app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ class User < ApplicationRecord
message: "can only contain letters, numbers, and underscores"
}, allow_nil: true, length: { within: 0..20 }

validates :homepage_url, http_url: true, allow_blank: true

validates :password,
length: { minimum: 10 },
unpwn: true,
Expand Down Expand Up @@ -352,7 +354,7 @@ def clear_personal_attributes
handle: nil, email_confirmed: false,
unconfirmed_email: nil, blocked_email: nil,
api_key: nil, confirmation_token: nil, remember_token: nil,
twitter_username: nil, webauthn_id: nil, full_name: nil,
twitter_username: nil, webauthn_id: nil, full_name: nil, homepage_url: nil,
totp_seed: nil, mfa_hashed_recovery_codes: nil,
mfa_level: :disabled,
password: SecureRandom.hex(20).encode("UTF-8")
Expand Down
13 changes: 13 additions & 0 deletions app/views/dashboards/_subject.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,19 @@
</div>
<% end %>

<% if user.homepage_url.present? %>
<div class="flex items-center mb-4 text-b3 lg:text-b2">
<%= icon_tag("link", color: :primary, class: "w-6 text-orange mr-3") %>
<p class="text-neutral-800 dark:text-white"><%=
link_to(
jacklynhma marked this conversation as resolved.
Show resolved Hide resolved
user.homepage_url,
user.homepage_url,
rel: "nofollow"
jacklynhma marked this conversation as resolved.
Show resolved Hide resolved
)
%></p>
</div>
<% end %>

<% if user.twitter_username.present? %>
<div class="flex items-center mb-4 text-b3 lg:text-b2">
<%= icon_tag("x-twitter", color: :primary, class: "w-6 text-orange mr-3") %>
Expand Down
5 changes: 5 additions & 0 deletions app/views/profiles/edit.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@
<%= form.text_field :handle, :class => 'form__input' %>
</div>

<div class="text_field">
<%= form.label :homepage_url, class: 'form__label' %>
<%= form.text_field(:homepage_url, class: 'form__input') %>
</div>

<div class="text_field">
<%= form.label :twitter_username, class: 'form__label form__label__icon-container' do %>
<%=
Expand Down
12 changes: 12 additions & 0 deletions app/views/profiles/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,18 @@
)
%>
<% end %>
<% if @user.homepage_url.present? %>
<p id="homepage-url">
<%=
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.

rel: "nofollow",
class: "profile__header__attribute t-link--black"
)
%>
</p>
<% end %>
<% end %>
</div>

Expand Down
5 changes: 5 additions & 0 deletions db/migrate/20241114211431_add_homepage_url_to_users.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
class AddHomepageUrlToUsers < ActiveRecord::Migration[7.2]
def change
add_column :users, :homepage_url, :string
end
end
3 changes: 2 additions & 1 deletion db/schema.rb
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
#
# It's strongly recommended that you check this file into your version control system.

ActiveRecord::Schema[7.2].define(version: 2024_11_04_065953) do
ActiveRecord::Schema[7.2].define(version: 2024_11_14_211431) do
# These are extensions that must be enabled in order to support this database
enable_extension "hstore"
enable_extension "pgcrypto"
Expand Down Expand Up @@ -556,6 +556,7 @@
t.string "mfa_hashed_recovery_codes", default: [], array: true
t.boolean "public_email", default: false, null: false
t.datetime "deleted_at"
t.string "homepage_url"
t.index "lower((email)::text) varchar_pattern_ops", name: "index_users_on_lower_email"
t.index ["email"], name: "index_users_on_email"
t.index ["handle"], name: "index_users_on_handle"
Expand Down
10 changes: 10 additions & 0 deletions lib/http_url_validator.rb
Original file line number Diff line number Diff line change
@@ -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.

def validate_each(record, attribute, value)
validUriPattern = %r{\Ahttps?://([^\s:@]+:[^\s:@]*@)?[A-Za-z\d\-]+(\.[A-Za-z\d\-]+)+\.?(:\d{1,5})?([\/?]\S*)?\z} # :nodoc:

uri = URI::DEFAULT_PARSER.parse(value)
record.errors.add attribute, "is not a valid URL" unless [URI::HTTP, URI::HTTPS].member?(uri.class) && validUriPattern.match?(value)
rescue URI::InvalidURIError
record.errors.add attribute, ("is not a valid URL")
end
end
30 changes: 30 additions & 0 deletions test/integration/profile_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,36 @@ def sign_out
assert page.has_link?("@nick1", href: "https://twitter.com/nick1")
end

test "adding homepage url" do
sign_in
visit profile_path("nick1")

click_link "Edit Profile"
fill_in "user_homepage_url", with: "https://nickisawesome.com"
fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD
click_button "Update"

click_link "Sign out"
visit profile_path("nick1")

assert page.has_link?("https://nickisawesome.com")
end

test "adding malicious homepage url" do
sign_in
visit profile_path("nick1")

click_link "Edit Profile"
fill_in "user_homepage_url", with: "http://www.site.com/redirect?url=http://www.malicious-site.com"
fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD
click_button "Update"

click_link "Sign out"
visit profile_path("nick1")

assert page.has_link?("http://www.site.com/redirect?url=http://www.malicious")
end

test "deleting profile" do
sign_in
visit profile_path("nick1")
Expand Down
18 changes: 18 additions & 0 deletions test/models/user_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,24 @@ class UserTest < ActiveSupport::TestCase
should_not allow_value("012345678901234567890").for(:twitter_username)
end

context "homepage url" do
should allow_value("https://www.mywebsite.com").for(:homepage_url)
should allow_value("http://www.mywebsite.com").for(:homepage_url)
should_not allow_value("<html><body>hi</body><html>").for(:homepage_url)
should_not allow_value("javascript:alert('hello');").for(:homepage_url)
should_not allow_value("file:///etc/passwd").for(:homepage_url)
should_not allow_value("file://C:/Windows/System32/cmd.exe").for(:homepage_url)
should_not allow_value("data:text/html,<script>alert('XSS')</script>").for(:homepage_url)
should_not allow_value("data:text/html;base64,SGVsbG8sIFdvcmxkIQ==").for(:homepage_url)
should_not allow_value("data:text/html,<img src='x' onerror='alert(1)'>").for(:homepage_url)
should_not allow_value("data:text/html,<iframe src='javascript:alert(1)'></iframe>").for(:homepage_url)
# should_not allow_value("http://www.site.site?#redirect=www.fake-target.site").for(:homepage_url)
# should_not allow_value("http://www.site.com/redirect?url=http://www.malicious-site.com").for(:homepage_url)
# should_not allow_value("http://www.site.com/?next=http://www.malicious-site.com").for(:homepage_url)
# should_not allow_value("http://www.site.com/?url=http://www.malicious-site.com").for(:homepage_url)
# should_not allow_value("http://www.site.com/?redirect=http://www.malicious-site.com").for(:homepage_url)
end

context "password" do
should "be between 10 characters and 72 bytes" do
user = build(:user, password: "%5a&12ed/")
Expand Down