Conversation
gonzaloriestra
force-pushed
the
river/use-jwt-dest-for-token-exchange
branch
from
aaf2cc7 to
7992177
Compare
| dest_shop = jwt_payload.shop | ||
|
|
||
| shop_session = ShopifyAPI::Auth::Session.new(shop: shop) | ||
| shop_session = ShopifyAPI::Auth::Session.new(shop: dest_shop) |
There was a problem hiding this comment.
If we are no longer using the shop param. We could deprecate it, so it can be removed in the next major version.
jwt_payload = ShopifyAPI::Auth::JwtPayload.new(session_token)
dest_shop = jwt_payload.shop
if shop != dest_shop
ShopifyAPI::Context.logger.warn(
"shop parameter (#{shop}) does not match session token dest claim (#{dest_shop}). " \
"The dest claim will be used. The shop parameter is deprecated for exchange_token " \
"and will be removed in a future major version."
)
end
There was a problem hiding this comment.
I also agree! We should remove shop from this method's arguments
|
|
||
| module ShopifyAPI | ||
| module Utils | ||
| class ShopValidator |
There was a problem hiding this comment.
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
attacker.com
|
|
||
| module ShopifyAPI | ||
| module Utils | ||
| class ShopValidator |
|
|
||
| module ShopifyAPI | ||
| module Utils | ||
| class ShopValidator |
There was a problem hiding this comment.
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
| dest_shop = jwt_payload.shop | ||
|
|
||
| shop_session = ShopifyAPI::Auth::Session.new(shop: shop) | ||
| shop_session = ShopifyAPI::Auth::Session.new(shop: dest_shop) |
There was a problem hiding this comment.
I also agree! We should remove shop from this method's arguments
| extend T::Sig | ||
|
|
||
| sig { params(shop: String).returns(String) } | ||
| def validate!(shop) |
There was a problem hiding this comment.
validate might be a misleading method name since it does more than validation and returns the sanitized shop name
3 participants
Description
https://docs.google.com/document/d/1c0iXhKBpm-yhvff0iSgq1XwBSPL39fTBVajIJ3svaxU/edit?usp=sharing
Checklist: