Improve shop domain sanitization

[stidges]

After launching our first app, we noticed a couple of weird shop domains trying to install our app (stuff like asfdasfd.asfdasfd.addf). I found that the shop domain sanization doesn't work fully in this case. I compared this to how the Shopify PHP package does this and added a few lines to improve it.

Let me know if you think throwing an exception is too much here, but I figured if someone tries to install an app with an invalid domain we don't want to process it in any case.

[Kyon147]

@stidges sounds like a good idea to me - I've not noticed any incorrect domains trying to be installed on my apps but better security and sanitization is always welcomed!

Throwing the exception might potentially bog down peoples logs, if bots decide to just spam an app on the install route. What are your thoughts?

[stidges]

Yes that's true.. My reason for adding the exception was because I think we wouldn't want to process a request with an invalid shop domain, and also because otherwise we'd have to update all the other code to check if the shop domain is empty/valid before using it

[Kyon147]

@stidges yeah that makes sense, an exception seems like the correct approach as people can always bind it in the Exception handler.

        $this->renderable(function(MissingShopDomainException $e){
            // Do something
        });

[Kyon147]

LGTYVM

[ClaraLeigh]

This comment is more for anyone searching the repo, this PR is where the InvalidShopDomainException was created. For me it created about 100+ errors in my application as domains will now throw an error when incorrect, ie via a mocked relationship that doesn't use the myshopify domain.

Hope this saves someone else the 2hrs I spent trying to work out why a composer update broke everything.

[Kyon147]

@ClaraLeigh that is a great point - it might be worth adding in a whitelist for unit testing etc, so that we don't need to use myshopfiy in tests, but it might also be better to force tests to be closer to prod? Thoughts?

[ClaraLeigh]

@Kyon147 I think it's probably fine as is. It just meant I had to create a new state for shopify versions of a factory and fake a domain name with myshopify attached. It probably should have been that way to begin with for data integrity but in my usecase I mock all the api calls, so the issue was just hidden from view until this change

[Kyon147]

@ClaraLeigh i've been there! Glad you got it sorted.