-
Notifications
You must be signed in to change notification settings - Fork 480
Add shop validation #1443
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
base: main
Are you sure you want to change the base?
Add shop validation #1443
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| # typed: strict | ||
| # frozen_string_literal: true | ||
|
|
||
| module ShopifyAPI | ||
| module Errors | ||
| class InvalidShopError < StandardError | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,31 @@ | ||
| # typed: strict | ||
| # frozen_string_literal: true | ||
|
|
||
| module ShopifyAPI | ||
| module Utils | ||
| class ShopValidator | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we need to modify the ShopValidator check to protect against URLs like "attacker.com/.myshopify.com". This passes validation since it ends with .myshopify.com, but URI.parse resolves the host to
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could probably be module similar to hmac
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's also a similar sanitize method from shopify_app gem, I wonder if the logic can be extracted so we don't have to maintain 2 packages for validating Shopify URLS |
||
| extend T::Sig | ||
|
|
||
| SHOPIFY_OWNED_SUFFIXES = T.let([ | ||
| ".myshopify.com", | ||
| ".myshopify.io", | ||
| ].freeze, T::Array[String]) | ||
|
|
||
| class << self | ||
| extend T::Sig | ||
|
|
||
| sig { params(shop: String).returns(String) } | ||
| def validate!(shop) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| cleaned = shop.to_s.strip.downcase.gsub(%r{\A(https?://)?}, "").gsub(%r{/\z}, "") | ||
|
|
||
| if cleaned.empty? || !SHOPIFY_OWNED_SUFFIXES.any? { |suffix| cleaned.end_with?(suffix) } | ||
| raise Errors::InvalidShopError, | ||
| "shop must end with one of #{SHOPIFY_OWNED_SUFFIXES.join(", ")}, got: #{shop.inspect}" | ||
| end | ||
|
|
||
| cleaned | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,69 @@ | ||
| # typed: false | ||
| # frozen_string_literal: true | ||
|
|
||
| require_relative "../test_helper" | ||
|
|
||
| module ShopifyAPITest | ||
| module Utils | ||
| class ShopValidatorTest < Test::Unit::TestCase | ||
| def test_accepts_valid_myshopify_com_domain | ||
| assert_equal("test-shop.myshopify.com", ShopifyAPI::Utils::ShopValidator.validate!("test-shop.myshopify.com")) | ||
| end | ||
|
|
||
| def test_accepts_valid_myshopify_io_domain | ||
| assert_equal("test-shop.myshopify.io", ShopifyAPI::Utils::ShopValidator.validate!("test-shop.myshopify.io")) | ||
| end | ||
|
|
||
| def test_strips_https_scheme | ||
| assert_equal("test-shop.myshopify.com", ShopifyAPI::Utils::ShopValidator.validate!("https://test-shop.myshopify.com")) | ||
| end | ||
|
|
||
| def test_strips_http_scheme | ||
| assert_equal("test-shop.myshopify.com", ShopifyAPI::Utils::ShopValidator.validate!("http://test-shop.myshopify.com")) | ||
| end | ||
|
|
||
| def test_strips_trailing_slash | ||
| assert_equal("test-shop.myshopify.com", ShopifyAPI::Utils::ShopValidator.validate!("test-shop.myshopify.com/")) | ||
| end | ||
|
|
||
| def test_normalizes_to_lowercase | ||
| assert_equal("test-shop.myshopify.com", ShopifyAPI::Utils::ShopValidator.validate!("Test-Shop.MyShopify.com")) | ||
| end | ||
|
|
||
| def test_strips_whitespace | ||
| result = ShopifyAPI::Utils::ShopValidator.validate!(" test-shop.myshopify.com ") | ||
| assert_equal("test-shop.myshopify.com", result) | ||
| end | ||
|
|
||
| def test_rejects_attacker_controlled_domain | ||
| assert_raises(ShopifyAPI::Errors::InvalidShopError) do | ||
| ShopifyAPI::Utils::ShopValidator.validate!("attacker.example") | ||
| end | ||
| end | ||
|
|
||
| def test_rejects_empty_string | ||
| assert_raises(ShopifyAPI::Errors::InvalidShopError) do | ||
| ShopifyAPI::Utils::ShopValidator.validate!("") | ||
| end | ||
| end | ||
|
|
||
| def test_rejects_non_shopify_domain | ||
| assert_raises(ShopifyAPI::Errors::InvalidShopError) do | ||
| ShopifyAPI::Utils::ShopValidator.validate!("evil.com") | ||
| end | ||
| end | ||
|
|
||
| def test_rejects_shopify_suffix_as_subdomain_of_attacker | ||
| assert_raises(ShopifyAPI::Errors::InvalidShopError) do | ||
| ShopifyAPI::Utils::ShopValidator.validate!("myshopify.com.evil.com") | ||
| end | ||
| end | ||
|
|
||
| def test_rejects_similar_looking_domain | ||
| assert_raises(ShopifyAPI::Errors::InvalidShopError) do | ||
| ShopifyAPI::Utils::ShopValidator.validate!("test-shop.notmyshopify.com") | ||
| end | ||
| end | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we are no longer using the shop param. We could deprecate it, so it can be removed in the next major version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree! We should remove
shopfrom this method's arguments