From 626e53f5816ffb8894febabbf6f08fcd53a2997d Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Thu, 14 Nov 2024 15:48:43 -0600 Subject: [PATCH 01/39] successfully adds homepage url --- app/controllers/profiles_controller.rb | 2 +- app/controllers/users_controller.rb | 1 + app/models/user.rb | 7 ++++++- app/views/profiles/edit.html.erb | 9 +++++++++ db/migrate/20241114211431_add_homepage_url_to_users.rb | 5 +++++ db/schema.rb | 3 ++- 6 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20241114211431_add_homepage_url_to_users.rb diff --git a/app/controllers/profiles_controller.rb b/app/controllers/profiles_controller.rb index 0ddf1f9184c..c8a0d0a8e33 100644 --- a/app/controllers/profiles_controller.rb +++ b/app/controllers/profiles_controller.rb @@ -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.expect(user: :password)[:password] diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 510094d00a4..5a6ab29b323 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -27,6 +27,7 @@ def create password website twitter_username + homepage_url full_name ].freeze diff --git a/app/models/user.rb b/app/models/user.rb index 3741b4c7003..eb0d60218f9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,6 +83,11 @@ class User < ApplicationRecord message: "can only contain letters, numbers, and underscores" }, allow_nil: true, length: { within: 0..20 } + validates :homepage_url, format: { + with: /\A[a-zA-Z0-9_]*\z/, + message: "can only contain letters, numbers, and underscores" + }, allow_nil: true, length: { within: 0..20 } + validates :password, length: { minimum: 10 }, unpwn: true, @@ -352,7 +357,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") diff --git a/app/views/profiles/edit.html.erb b/app/views/profiles/edit.html.erb index 9328c5ba61d..a4352ccc6dc 100644 --- a/app/views/profiles/edit.html.erb +++ b/app/views/profiles/edit.html.erb @@ -20,6 +20,15 @@ <%= form.text_field :handle, :class => 'form__input' %> +
<%= + link_to( + user.homepage_url + ) + %>
+<%= t('.enter_password') %>
- <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input', required: true %> + <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input' %><%= t('.enter_password') %>
- <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input' %> + <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input', required: true %> diff --git a/test/integration/profile_test.rb b/test/integration/profile_test.rb index 9ca834c44bf..2c1505c5138 100644 --- a/test/integration/profile_test.rb +++ b/test/integration/profile_test.rb @@ -133,19 +133,19 @@ def sign_out assert page.has_link?("@nick1", href: "https://twitter.com/nick1") end - test "adding homepage url" do + 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 "user_homepage_url", with: "www.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") + assert page.has_link?("www.nickisawesome.com") end test "deleting profile" do From 2854947446079a0d2da336570ddb32b8401293b0 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:17:23 -0600 Subject: [PATCH 06/39] passing test --- app/models/user.rb | 2 +- test/integration/profile_test.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/models/user.rb b/app/models/user.rb index 4a8f828e0ca..f17f031a2f8 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,7 +83,7 @@ class User < ApplicationRecord message: "can only contain letters, numbers, and underscores" }, allow_nil: true, length: { within: 0..20 } - validates_formatting_of :homepage_url, using: :url, message: "does not appear to be a valid URL", allow_nil: true + validates_formatting_of :homepage_url, using: :url, allow_nil: true validates :password, length: { minimum: 10 }, diff --git a/test/integration/profile_test.rb b/test/integration/profile_test.rb index 2c1505c5138..b0f228c78b2 100644 --- a/test/integration/profile_test.rb +++ b/test/integration/profile_test.rb @@ -31,7 +31,7 @@ def sign_out fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD click_button "Update" - assert page.has_content? "nick2" + assert_content "nick2" end test "changing to an existing handle" do @@ -138,14 +138,14 @@ def sign_out visit profile_path("nick1") click_link "Edit Profile" - fill_in "user_homepage_url", with: "www.nickisawesome.com" + 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?("www.nickisawesome.com") + assert page.has_link?("https://nickisawesome.com") end test "deleting profile" do From 708956b5216090bd7f1c40fe90a26a3a38ea06fc Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:30:37 -0600 Subject: [PATCH 07/39] tests are failing --- app/views/profiles/edit.html.erb | 2 +- test/integration/profile_test.rb | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/views/profiles/edit.html.erb b/app/views/profiles/edit.html.erb index 38251ff3d9d..e178abe7320 100644 --- a/app/views/profiles/edit.html.erb +++ b/app/views/profiles/edit.html.erb @@ -71,7 +71,7 @@<%= t('.enter_password') %>
- <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input', required: true %> + <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input'%> diff --git a/test/integration/profile_test.rb b/test/integration/profile_test.rb index b0f228c78b2..cef7c5d96bf 100644 --- a/test/integration/profile_test.rb +++ b/test/integration/profile_test.rb @@ -31,7 +31,7 @@ def sign_out fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD click_button "Update" - assert_content "nick2" + assert page.has_content? "nick2" end test "changing to an existing handle" do @@ -130,6 +130,7 @@ def sign_out click_link "Sign out" visit profile_path("nick1") + assert_content("test") assert page.has_link?("@nick1", href: "https://twitter.com/nick1") end From eb0b5b4a50291ce4cb1b1fc4317c47a9211f7afb Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Thu, 14 Nov 2024 17:44:01 -0600 Subject: [PATCH 08/39] undo change --- app/views/profiles/edit.html.erb | 2 +- test/integration/profile_test.rb | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/app/views/profiles/edit.html.erb b/app/views/profiles/edit.html.erb index e178abe7320..e1cc2ba27e8 100644 --- a/app/views/profiles/edit.html.erb +++ b/app/views/profiles/edit.html.erb @@ -71,7 +71,7 @@<%= t('.enter_password') %>
- <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input'%> + <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input' %> diff --git a/test/integration/profile_test.rb b/test/integration/profile_test.rb index cef7c5d96bf..681fa101499 100644 --- a/test/integration/profile_test.rb +++ b/test/integration/profile_test.rb @@ -130,7 +130,6 @@ def sign_out click_link "Sign out" visit profile_path("nick1") - assert_content("test") assert page.has_link?("@nick1", href: "https://twitter.com/nick1") end From d5ad960d314fdd01bf75bdb13c17427ba306fe2d Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Thu, 14 Nov 2024 18:00:19 -0600 Subject: [PATCH 09/39] passing tests --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index f17f031a2f8..f4549ebfe5a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,7 +83,7 @@ class User < ApplicationRecord message: "can only contain letters, numbers, and underscores" }, allow_nil: true, length: { within: 0..20 } - validates_formatting_of :homepage_url, using: :url, allow_nil: true + validates_formatting_of :homepage_url, using: :url, allow_blank: true validates :password, length: { minimum: 10 }, From d3e13e934ed389ae754a6012b76e3f9dd83e15ad Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Thu, 14 Nov 2024 18:15:08 -0600 Subject: [PATCH 10/39] add no follow --- app/views/profiles/show.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index aa34402aa62..de27dfdd431 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -98,6 +98,7 @@ <%= link_to( @user.homepage_url, + rel: "nofollow", class: "profile__header__attribute t-link--black" ) %> From d02cad8e38cbfa508011f23147c1fad5599562e5 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Fri, 15 Nov 2024 11:27:29 -0600 Subject: [PATCH 11/39] fix styling of link --- app/views/profiles/show.html.erb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index de27dfdd431..7980c1cbd7a 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -95,13 +95,15 @@ %> <% end %> <% if @user.homepage_url.present? %> - <%= - link_to( - @user.homepage_url, - rel: "nofollow", - class: "profile__header__attribute t-link--black" - ) - %> ++ <%= + link_to( + @user.homepage_url, + rel: "nofollow", + class: "profile__header__attribute t-link--black" + ) + %> +
<% end %> <% end %> From 14ff9bf362b35c37614f827674aefdc373f91107 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Fri, 15 Nov 2024 11:31:47 -0600 Subject: [PATCH 12/39] update user test --- test/models/user_test.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index add6d52b9a5..a2ab06d750a 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -172,6 +172,12 @@ 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_not allow_value("http://non-secure-site.com").for(:homepage_url) + should_not allow_value("hi").for(:homepage_url) + end + context "password" do should "be between 10 characters and 72 bytes" do user = build(:user, password: "%5a&12ed/") From 691a2360ac9972fba78f80c5fe622a95eebdd015 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Fri, 15 Nov 2024 11:51:45 -0600 Subject: [PATCH 13/39] udpatetest --- test/models/user_test.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index a2ab06d750a..dd09ab9d77d 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -174,7 +174,6 @@ class UserTest < ActiveSupport::TestCase context "homepage url" do should allow_value("https://www.mywebsite.com").for(:homepage_url) - should_not allow_value("http://non-secure-site.com").for(:homepage_url) should_not allow_value("hi").for(:homepage_url) end From 2f815db78e461e0b445fdc883259c53a45452610 Mon Sep 17 00:00:00 2001 From: Martin Emde<%= - link_to( - user.homepage_url - ) - %>
-<%= + link_to( + user.homepage_url, + user.homepage_url, + rel: "nofollow" + ) + %>
+<%= link_to( + @user.homepage_url, @user.homepage_url, rel: "nofollow", class: "profile__header__attribute t-link--black" From 89cc35c60aaf5a14805853a332398ce13f8d28e7 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 23 Nov 2024 20:19:44 -0500 Subject: [PATCH 16/39] new XXS tests for homepage url --- test/models/user_test.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/test/models/user_test.rb b/test/models/user_test.rb index dd09ab9d77d..673104c6148 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -175,6 +175,18 @@ class UserTest < ActiveSupport::TestCase context "homepage url" do should allow_value("https://www.mywebsite.com").for(:homepage_url) should_not allow_value("
hi").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,").for(:homepage_url) + should_not allow_value("data:text/html;base64,SGVsbG8sIFdvcmxkIQ==").for(:homepage_url) + should_not allow_value("data:text/html,").for(:homepage_url) + should_not allow_value("data:text/html,").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 From ff0086bc43abf4182c948e0567a43080493ee146 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 23 Nov 2024 21:38:22 -0500 Subject: [PATCH 17/39] extract httpURL validator into a lib file and update the tests --- app/models/user.rb | 2 +- app/views/profiles/show.html.erb | 2 +- lib/http_url_validator.rb | 10 ++++++++++ test/integration/profile_test.rb | 15 +++++++++++++++ test/models/user_test.rb | 1 + 5 files changed, 28 insertions(+), 2 deletions(-) create mode 100644 lib/http_url_validator.rb diff --git a/app/models/user.rb b/app/models/user.rb index f4549ebfe5a..38cdd54e9f7 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -83,7 +83,7 @@ class User < ApplicationRecord message: "can only contain letters, numbers, and underscores" }, allow_nil: true, length: { within: 0..20 } - validates_formatting_of :homepage_url, using: :url, allow_blank: true + validates :homepage_url, http_url: true, allow_blank: true validates :password, length: { minimum: 10 }, diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index 69eba0041ed..db4fce982ab 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -97,7 +97,7 @@ <% if @user.homepage_url.present? %><%= - link_to( + link_to( @user.homepage_url, @user.homepage_url, rel: "nofollow", diff --git a/lib/http_url_validator.rb b/lib/http_url_validator.rb new file mode 100644 index 00000000000..bf514f457e5 --- /dev/null +++ b/lib/http_url_validator.rb @@ -0,0 +1,10 @@ +class HttpUrlValidator < ActiveModel::EachValidator + 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 diff --git a/test/integration/profile_test.rb b/test/integration/profile_test.rb index 681fa101499..68d2a6009c1 100644 --- a/test/integration/profile_test.rb +++ b/test/integration/profile_test.rb @@ -148,6 +148,21 @@ def sign_out 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") diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 673104c6148..9ad455e367f 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -174,6 +174,7 @@ class UserTest < ActiveSupport::TestCase 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("
hi").for(:homepage_url) should_not allow_value("javascript:alert('hello');").for(:homepage_url) should_not allow_value("file:///etc/passwd").for(:homepage_url) From dd31f9a5d55e29418a067a26613764e59d6d9dd2 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 23 Nov 2024 22:19:54 -0500 Subject: [PATCH 18/39] remove duplication --- lib/http_url_validator.rb | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/http_url_validator.rb b/lib/http_url_validator.rb index bf514f457e5..7ff97e56d5f 100644 --- a/lib/http_url_validator.rb +++ b/lib/http_url_validator.rb @@ -1,10 +1,7 @@ class HttpUrlValidator < ActiveModel::EachValidator 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") + record.errors.add attribute, "is not a valid URL" unless [URI::HTTP, URI::HTTPS].member?(uri.class) + record.errors.add attribute, "is not a valid URL" end end From 3e3fb7bd56b34d5213d491732ce520421a0210ef Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 23 Nov 2024 22:21:31 -0500 Subject: [PATCH 19/39] remove unneeded tests --- test/integration/profile_test.rb | 15 --------------- test/models/user_test.rb | 5 ----- 2 files changed, 20 deletions(-) diff --git a/test/integration/profile_test.rb b/test/integration/profile_test.rb index 68d2a6009c1..681fa101499 100644 --- a/test/integration/profile_test.rb +++ b/test/integration/profile_test.rb @@ -148,21 +148,6 @@ def sign_out 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") diff --git a/test/models/user_test.rb b/test/models/user_test.rb index 9ad455e367f..317782c38aa 100644 --- a/test/models/user_test.rb +++ b/test/models/user_test.rb @@ -183,11 +183,6 @@ class UserTest < ActiveSupport::TestCase should_not allow_value("data:text/html;base64,SGVsbG8sIFdvcmxkIQ==").for(:homepage_url) should_not allow_value("data:text/html,").for(:homepage_url) should_not allow_value("data:text/html,").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 From b05155147af7a0daac4d50441b421855107d8007 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 23 Nov 2024 22:27:37 -0500 Subject: [PATCH 20/39] add rescue --- lib/http_url_validator.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/lib/http_url_validator.rb b/lib/http_url_validator.rb index 7ff97e56d5f..c122927a168 100644 --- a/lib/http_url_validator.rb +++ b/lib/http_url_validator.rb @@ -2,6 +2,7 @@ class HttpUrlValidator < ActiveModel::EachValidator def validate_each(record, attribute, value) uri = URI::DEFAULT_PARSER.parse(value) record.errors.add attribute, "is not a valid URL" unless [URI::HTTP, URI::HTTPS].member?(uri.class) - record.errors.add attribute, "is not a valid URL" + rescue URI::InvalidURIError + record.errors.add attribute, "is not a valid URL" end end From 70feba9b4b566f79aa44d28a400329e81887db61 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Tue, 3 Dec 2024 13:45:34 -0500 Subject: [PATCH 21/39] Fix indentation --- lib/http_url_validator.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/http_url_validator.rb b/lib/http_url_validator.rb index c122927a168..258772a9dee 100644 --- a/lib/http_url_validator.rb +++ b/lib/http_url_validator.rb @@ -3,6 +3,6 @@ def validate_each(record, attribute, value) uri = URI::DEFAULT_PARSER.parse(value) record.errors.add attribute, "is not a valid URL" unless [URI::HTTP, URI::HTTPS].member?(uri.class) rescue URI::InvalidURIError - record.errors.add attribute, "is not a valid URL" + record.errors.add attribute, "is not a valid URL" end end From 64d4a77c35adb29bfb59c474da10b0149a299643 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Tue, 3 Dec 2024 13:55:37 -0500 Subject: [PATCH 22/39] truncate really long homepage urls --- app/views/dashboards/_subject.html.erb | 2 +- app/views/profiles/show.html.erb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/dashboards/_subject.html.erb b/app/views/dashboards/_subject.html.erb index 4f5de533ea9..ed0bdc8a370 100644 --- a/app/views/dashboards/_subject.html.erb +++ b/app/views/dashboards/_subject.html.erb @@ -26,7 +26,7 @@ <%= icon_tag("link", color: :primary, class: "w-6 text-orange mr-3") %><%= link_to( - user.homepage_url, + truncate(user.homepage_url, length: 20), user.homepage_url, rel: "nofollow" ) diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index db4fce982ab..e4d622bf449 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -98,7 +98,7 @@
<%= link_to( - @user.homepage_url, + truncate(@user.homepage_url,length: 20), @user.homepage_url, rel: "nofollow", class: "profile__header__attribute t-link--black" From f970fa018c366228a1f70c08f6372503efac870a Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Tue, 3 Dec 2024 14:21:01 -0500 Subject: [PATCH 23/39] renames test file so it doesn't conflict locally and adds confirmation dialog prior to redirect --- app/views/dashboards/_subject.html.erb | 3 ++- app/views/profiles/show.html.erb | 3 ++- test/system/{profile_test.rb => authorizing_profile_update.rb} | 2 +- 3 files changed, 5 insertions(+), 3 deletions(-) rename test/system/{profile_test.rb => authorizing_profile_update.rb} (95%) diff --git a/app/views/dashboards/_subject.html.erb b/app/views/dashboards/_subject.html.erb index ed0bdc8a370..0ef8bd99f52 100644 --- a/app/views/dashboards/_subject.html.erb +++ b/app/views/dashboards/_subject.html.erb @@ -28,7 +28,8 @@ link_to( truncate(user.homepage_url, length: 20), user.homepage_url, - rel: "nofollow" + rel: "nofollow", + data: { confirm: "Are you sure?" } ) %>
<%= - link_to( - truncate(user.homepage_url, length: 20), - user.homepage_url, - rel: "nofollow", - data: { confirm: "Are you sure?" } - ) - %>
-<%= + link_to( + truncate(user.homepage_url, length: 20), + user.homepage_url, + rel: "nofollow", + data: { confirm: "Are you sure?" } + ) + %>
+<%= link_to( truncate(user.homepage_url, length: 20), - user.homepage_url, + h(user.homepage_url), rel: "nofollow", data: { confirm: "Are you sure?" } ) From 41e72a570d360b79ae531d5c5ec1613c886bc8f6 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 14 Dec 2024 12:31:17 -0500 Subject: [PATCH 28/39] fix failing tests --- app/views/profiles/edit.html.erb | 2 +- test/integration/profile_test.rb | 3 +-- test/system/authorizing_profile_update_test.rb | 3 +-- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/app/views/profiles/edit.html.erb b/app/views/profiles/edit.html.erb index e1cc2ba27e8..38251ff3d9d 100644 --- a/app/views/profiles/edit.html.erb +++ b/app/views/profiles/edit.html.erb @@ -71,7 +71,7 @@
<%= t('.enter_password') %>
- <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input' %> + <%= form.password_field :password, autocomplete: 'current-password', class: 'form__input', required: true %><%= link_to( - truncate(user.homepage_url, length: 20), - h(user.homepage_url), + truncate(append_https(user.homepage_url), length: 20), + h(append_https(user.homepage_url)), rel: "nofollow", - data: { confirm: "You are about to be redirected #{user.homepage_url}" } + data: { confirm: "You are about to be redirected #{h(append_https(user.homepage_url))}" } ) %>
diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index a51f1a62309..e4a16b9f70f 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -98,11 +98,11 @@<%= link_to( - truncate(@user.homepage_url,length: 20), - h(@user.homepage_url), + truncate(h(append_https(@user.homepage_url)),length: 20), + h(append_https(@user.homepage_url)), rel: "nofollow", class: "profile__header__attribute t-link--black", - data: { confirm: "You are about to be redirected #{@user.homepage_url}" } + data: { confirm: "You are about to be redirected #{{h(append_https(user.homepage_url))}}" } ) %>
From 643462f638f2fd9082933578ba26d3266e28e3db Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 14 Dec 2024 23:08:14 -0500 Subject: [PATCH 32/39] add tests for url helper method --- test/unit/helpers/url_helper_test.rb | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 test/unit/helpers/url_helper_test.rb diff --git a/test/unit/helpers/url_helper_test.rb b/test/unit/helpers/url_helper_test.rb new file mode 100644 index 00000000000..24139888c68 --- /dev/null +++ b/test/unit/helpers/url_helper_test.rb @@ -0,0 +1,20 @@ +require "test_helper" + +class UrlHelperTest < ActionView::TestCase + context"append_https" do + should "return url if it begins with https" do + assert_equal "https://www.awesomesite.com", append_https("https://www.awesomesite.com") + end + should "return empty string if url is empty" do + assert_equal "", append_https("") + end + + should "return link with https if it does not begin with https" do + assert_equal "https://javascript:alert('hello');", append_https("javascript:alert('hello');") + end + + should "return empty string if url is nil" do + assert_equal "", append_https(nil) + end + end +end \ No newline at end of file From d4042fdf466a4ae3498344121d2a4b809313ebc5 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 14 Dec 2024 23:12:03 -0500 Subject: [PATCH 33/39] fix rubocop --- app/helpers/url_helper.rb | 8 ++++---- test/unit/helpers/url_helper_test.rb | 6 +++--- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/app/helpers/url_helper.rb b/app/helpers/url_helper.rb index 131a7204973..b6999d83539 100644 --- a/app/helpers/url_helper.rb +++ b/app/helpers/url_helper.rb @@ -1,7 +1,7 @@ -module UrlHelper +module UrlHelper def append_https(url) return "" if url.blank? return url if url.start_with?("https://") - return "https://#{url}" - end -end \ No newline at end of file + "https://#{url}" + end +end diff --git a/test/unit/helpers/url_helper_test.rb b/test/unit/helpers/url_helper_test.rb index 24139888c68..f69add86213 100644 --- a/test/unit/helpers/url_helper_test.rb +++ b/test/unit/helpers/url_helper_test.rb @@ -1,7 +1,7 @@ require "test_helper" class UrlHelperTest < ActionView::TestCase - context"append_https" do + context "append_https" do should "return url if it begins with https" do assert_equal "https://www.awesomesite.com", append_https("https://www.awesomesite.com") end @@ -16,5 +16,5 @@ class UrlHelperTest < ActionView::TestCase should "return empty string if url is nil" do assert_equal "", append_https(nil) end - end -end \ No newline at end of file + end +end From 6ce413ec1d6cc0103aa4110e34b19daeffa6f99b Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 14 Dec 2024 23:15:53 -0500 Subject: [PATCH 34/39] rename method --- app/helpers/url_helper.rb | 2 +- app/views/dashboards/_subject.html.erb | 6 +++--- app/views/profiles/show.html.erb | 6 +++--- test/unit/helpers/url_helper_test.rb | 10 +++++----- 4 files changed, 12 insertions(+), 12 deletions(-) diff --git a/app/helpers/url_helper.rb b/app/helpers/url_helper.rb index b6999d83539..64d79fec3d8 100644 --- a/app/helpers/url_helper.rb +++ b/app/helpers/url_helper.rb @@ -1,5 +1,5 @@ module UrlHelper - def append_https(url) + def prepend_https(url) return "" if url.blank? return url if url.start_with?("https://") "https://#{url}" diff --git a/app/views/dashboards/_subject.html.erb b/app/views/dashboards/_subject.html.erb index 1f32e6a6dcd..f03485bfd47 100644 --- a/app/views/dashboards/_subject.html.erb +++ b/app/views/dashboards/_subject.html.erb @@ -26,10 +26,10 @@ <%= icon_tag("link", color: :primary, class: "w-6 text-orange mr-3") %><%= link_to( - truncate(append_https(user.homepage_url), length: 20), - h(append_https(user.homepage_url)), + truncate(prepend_https(user.homepage_url), length: 20), + h(prepend_https(user.homepage_url)), rel: "nofollow", - data: { confirm: "You are about to be redirected #{h(append_https(user.homepage_url))}" } + data: { confirm: "You are about to be redirected #{h(prepend_https(user.homepage_url))}" } ) %>
diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index e4a16b9f70f..651d883774a 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -98,11 +98,11 @@<%= link_to( - truncate(h(append_https(@user.homepage_url)),length: 20), - h(append_https(@user.homepage_url)), + truncate(h(prepend_https(@user.homepage_url)),length: 20), + h(prepend_https(@user.homepage_url)), rel: "nofollow", class: "profile__header__attribute t-link--black", - data: { confirm: "You are about to be redirected #{{h(append_https(user.homepage_url))}}" } + data: { confirm: "You are about to be redirected #{{h(prepend_https(user.homepage_url))}}" } ) %>
diff --git a/test/unit/helpers/url_helper_test.rb b/test/unit/helpers/url_helper_test.rb index f69add86213..0b0c64989f1 100644 --- a/test/unit/helpers/url_helper_test.rb +++ b/test/unit/helpers/url_helper_test.rb @@ -1,20 +1,20 @@ require "test_helper" class UrlHelperTest < ActionView::TestCase - context "append_https" do + context "prepend_https" do should "return url if it begins with https" do - assert_equal "https://www.awesomesite.com", append_https("https://www.awesomesite.com") + assert_equal "https://www.awesomesite.com", prepend_https("https://www.awesomesite.com") end should "return empty string if url is empty" do - assert_equal "", append_https("") + assert_equal "", prepend_https("") end should "return link with https if it does not begin with https" do - assert_equal "https://javascript:alert('hello');", append_https("javascript:alert('hello');") + assert_equal "https://javascript:alert('hello');", prepend_https("javascript:alert('hello');") end should "return empty string if url is nil" do - assert_equal "", append_https(nil) + assert_equal "", prepend_https(nil) end end end From e3316bbf732f8ea1fabcdda916e3b9411d9fec68 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sun, 15 Dec 2024 11:02:53 -0500 Subject: [PATCH 35/39] fix syntax error --- app/views/profiles/show.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index 651d883774a..9fa0e892d4e 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -102,7 +102,7 @@ h(prepend_https(@user.homepage_url)), rel: "nofollow", class: "profile__header__attribute t-link--black", - data: { confirm: "You are about to be redirected #{{h(prepend_https(user.homepage_url))}}" } + data: { confirm: "You are about to be redirected #{h(prepend_https(@user.homepage_url))} " } ) %> From 0def58ef454091d271b32c6e8b6196387c501df7 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sun, 15 Dec 2024 12:31:10 -0500 Subject: [PATCH 36/39] rename url helper and enhance url to escape html --- app/helpers/url_helper.rb | 6 +++--- app/views/dashboards/_subject.html.erb | 6 +++--- app/views/profiles/show.html.erb | 6 +++--- test/unit/helpers/url_helper_test.rb | 23 ++++++++++++++++++----- 4 files changed, 27 insertions(+), 14 deletions(-) diff --git a/app/helpers/url_helper.rb b/app/helpers/url_helper.rb index 64d79fec3d8..ccbb618ccaf 100644 --- a/app/helpers/url_helper.rb +++ b/app/helpers/url_helper.rb @@ -1,7 +1,7 @@ module UrlHelper - def prepend_https(url) + def display_safe_url(url) return "" if url.blank? - return url if url.start_with?("https://") - "https://#{url}" + return h(url) if url.start_with?("https://") || url.start_with?("http://") + return "https://#{h(url)}" end end diff --git a/app/views/dashboards/_subject.html.erb b/app/views/dashboards/_subject.html.erb index f03485bfd47..c0921ff72e9 100644 --- a/app/views/dashboards/_subject.html.erb +++ b/app/views/dashboards/_subject.html.erb @@ -26,10 +26,10 @@ <%= icon_tag("link", color: :primary, class: "w-6 text-orange mr-3") %><%= link_to( - truncate(prepend_https(user.homepage_url), length: 20), - h(prepend_https(user.homepage_url)), + truncate(display_safe_url(user.homepage_url), length: 20), + display_safe_url(user.homepage_url), rel: "nofollow", - data: { confirm: "You are about to be redirected #{h(prepend_https(user.homepage_url))}" } + data: { confirm: "You are about to be redirected #{display_safe_url(user.homepage_url)}" } ) %>
diff --git a/app/views/profiles/show.html.erb b/app/views/profiles/show.html.erb index 9fa0e892d4e..fbbb36dd0d2 100644 --- a/app/views/profiles/show.html.erb +++ b/app/views/profiles/show.html.erb @@ -98,11 +98,11 @@<%= link_to( - truncate(h(prepend_https(@user.homepage_url)),length: 20), - h(prepend_https(@user.homepage_url)), + truncate(display_safe_url(@user.homepage_url),length: 20), + display_safe_url(@user.homepage_url), rel: "nofollow", class: "profile__header__attribute t-link--black", - data: { confirm: "You are about to be redirected #{h(prepend_https(@user.homepage_url))} " } + data: { confirm: "You are about to be redirected #{display_safe_url(@user.homepage_url)} " } ) %>
diff --git a/test/unit/helpers/url_helper_test.rb b/test/unit/helpers/url_helper_test.rb index 0b0c64989f1..1ec72e82796 100644 --- a/test/unit/helpers/url_helper_test.rb +++ b/test/unit/helpers/url_helper_test.rb @@ -1,20 +1,33 @@ require "test_helper" class UrlHelperTest < ActionView::TestCase - context "prepend_https" do + include ERB::Util + context "display_safe_url" do should "return url if it begins with https" do - assert_equal "https://www.awesomesite.com", prepend_https("https://www.awesomesite.com") + assert_equal "https://www.awesomesite.com", display_safe_url("https://www.awesomesite.com") end should "return empty string if url is empty" do - assert_equal "", prepend_https("") + assert_equal "", display_safe_url("") + end + + should "display a url starting with http" do + assert_equal "http://www.awesomesite.com", display_safe_url("http://www.awesomesite.com") end should "return link with https if it does not begin with https" do - assert_equal "https://javascript:alert('hello');", prepend_https("javascript:alert('hello');") + assert_equal "https://javascript:alert('hello');", display_safe_url("javascript:alert('hello');") end + should "escape html" do + assert_equal "https://<script>alert('hello');</script>https://www", display_safe_url("https://www") + end + + should "prepend https if url does not begin with http or https" do + assert_equal "https://www.awesomesite.com/https://javascript:alert('hello');", display_safe_url("www.awesomesite.com/https://javascript:alert('hello');") + end + should "return empty string if url is nil" do - assert_equal "", prepend_https(nil) + assert_equal "", display_safe_url(nil) end end end From 6e3f2458de70679c4f660d578d5bba1dd4df052c Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sun, 15 Dec 2024 12:35:58 -0500 Subject: [PATCH 37/39] fix syntax --- app/helpers/url_helper.rb | 4 ++-- test/unit/helpers/url_helper_test.rb | 8 ++++---- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/helpers/url_helper.rb b/app/helpers/url_helper.rb index ccbb618ccaf..da941b06848 100644 --- a/app/helpers/url_helper.rb +++ b/app/helpers/url_helper.rb @@ -1,7 +1,7 @@ module UrlHelper def display_safe_url(url) return "" if url.blank? - return h(url) if url.start_with?("https://") || url.start_with?("http://") - return "https://#{h(url)}" + return h(url) if url.start_with?("https://", "http://") + "https://#{h(url)}" end end diff --git a/test/unit/helpers/url_helper_test.rb b/test/unit/helpers/url_helper_test.rb index 1ec72e82796..5b271675323 100644 --- a/test/unit/helpers/url_helper_test.rb +++ b/test/unit/helpers/url_helper_test.rb @@ -18,13 +18,13 @@ class UrlHelperTest < ActionView::TestCase assert_equal "https://javascript:alert('hello');", display_safe_url("javascript:alert('hello');") end - should "escape html" do + should "escape html" do assert_equal "https://<script>alert('hello');</script>https://www", display_safe_url("https://www") - end + end - should "prepend https if url does not begin with http or https" do + should "prepend https if url does not begin with http or https" do assert_equal "https://www.awesomesite.com/https://javascript:alert('hello');", display_safe_url("www.awesomesite.com/https://javascript:alert('hello');") - end + end should "return empty string if url is nil" do assert_equal "", display_safe_url(nil) From 7f3d0d3abc9faa7ad2fe17d8718123c2d9849746 Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 28 Dec 2024 09:41:53 -0500 Subject: [PATCH 38/39] remove test update since it is covered in a different PR --- .../system/authorizing_profile_update_test.rb | 37 ------------------- 1 file changed, 37 deletions(-) delete mode 100644 test/system/authorizing_profile_update_test.rb diff --git a/test/system/authorizing_profile_update_test.rb b/test/system/authorizing_profile_update_test.rb deleted file mode 100644 index 99d5f7f8b66..00000000000 --- a/test/system/authorizing_profile_update_test.rb +++ /dev/null @@ -1,37 +0,0 @@ -require "application_system_test_case" -require "test_helper" - -class AuthorizingProfileUpdateTest < ApplicationSystemTestCase - setup do - @user = create(:user, email: "nick@example.com", password: PasswordHelpers::SECURE_TEST_PASSWORD, handle: "nick1", mail_fails: 1) - end - - def sign_in - visit sign_in_path - fill_in "Email or Username", with: @user.reload.email - fill_in "Password", with: @user.password - click_button "Sign in" - end - - test "adding X(formerly Twitter) username without filling in your password" do - twitter_username = "nick1twitter" - - sign_in - visit profile_path("nick1") - - click_link "Edit Profile" - fill_in "user_twitter_username", with: twitter_username - - assert_equal twitter_username, page.find_by_id("user_twitter_username").value - - click_button "Update" - # Verify that the newly added Twitter username is still on the form so that the user does not need to re-enter it - assert_equal twitter_username, page.find_by_id("user_twitter_username").value - - fill_in "Password", with: @user.password - click_button "Update" - - assert page.has_content? "Your profile was updated." - assert_equal twitter_username, page.find_by_id("user_twitter_username").value - end -end From 43e2dc25c356b3514220336472b40ec3990bddff Mon Sep 17 00:00:00 2001 From: Jacklyn Ma <29336370+jacklynhma@users.noreply.github.com> Date: Sat, 28 Dec 2024 09:43:42 -0500 Subject: [PATCH 39/39] add back profile_test.rb --- test/system/profile_test.rb | 38 +++++++++++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 test/system/profile_test.rb diff --git a/test/system/profile_test.rb b/test/system/profile_test.rb new file mode 100644 index 00000000000..5afb8195384 --- /dev/null +++ b/test/system/profile_test.rb @@ -0,0 +1,38 @@ +require "application_system_test_case" +require "test_helper" + +class ProfileTest < ApplicationSystemTestCase + setup do + @user = create(:user, email: "nick@example.com", password: PasswordHelpers::SECURE_TEST_PASSWORD, handle: "nick1", mail_fails: 1) + end + + def sign_in + visit sign_in_path + fill_in "Email or Username", with: @user.reload.email + fill_in "Password", with: @user.password + click_button "Sign in" + end + + test "adding X(formerly Twitter) username without filling in your password" do + twitter_username = "nick1twitter" + + sign_in + visit profile_path("nick1") + + click_link "Edit Profile" + fill_in "user_twitter_username", with: twitter_username + + assert_equal twitter_username, page.find_by_id("user_twitter_username").value + + click_button "Update" + + # Verify that the newly added Twitter username is still on the form so that the user does not need to re-enter it + assert_equal twitter_username, page.find_by_id("user_twitter_username").value + + fill_in "Password", with: PasswordHelpers::SECURE_TEST_PASSWORD + click_button "Update" + + assert page.has_content? "Your profile was updated." + assert_equal twitter_username, page.find_by_id("user_twitter_username").value + end +end