Shopify / shopify_app

A Rails Engine for building Shopify Apps
MIT License
1.76k stars 683 forks source link

Breaking change in 22.3.0 - TypeError related to JWT Payload #1908

Closed jagthedrummer closed 3 weeks ago

jagthedrummer commented 3 weeks ago

Issue summary

Before opening this issue, I have:

I updated to 22.4.0 and ran my test suite and there are a bunch of errors that look like this:

  1) DashboardController GET #index returns http success
     Failure/Error: get :index

     TypeError:
       T.let: Expected type Integer, got type String with value "1726161888"
       Caller: /Users/jgreen/.asdf/installs/ruby/3.3.4/lib/ruby/gems/3.3.0/gems/shopify_api-14.5.0/lib/shopify_api/auth/jwt_payload.rb:34
     # ./spec/controllers/dashboard_controller_spec.rb:12:in `block (3 levels) in <top (required)>'

Expected behavior

It should not throw errors. Breaking changes should not be introduced in a minor point release.

Actual behavior

It throws errors.

Steps to reproduce the problem

Upgrading to 22.3.0 or 22.4.0 reliably reproduces this error in all of my apps.

Debug logs

$ git diff Gemfile
diff --git a/Gemfile b/Gemfile
index 4b8f7cb..3d954ed 100644
--- a/Gemfile
+++ b/Gemfile
@@ -32,7 +32,7 @@ gem 'jquery-rails'
 # Use Capistrano for deployment
 # gem 'capistrano-rails', group: :development

-gem 'shopify_app', '22.2.1'
+gem 'shopify_app', '22.4.0'
 gem 'shopify_api', '14.5.0'

 gem 'sidekiq', '< 8'
$ git diff Gemfile.lock
diff --git a/Gemfile.lock b/Gemfile.lock
index 2a13a2f..16445a7 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -71,7 +71,7 @@ GEM
     activerecord (7.0.8.4)
       activemodel (= 7.0.8.4)
       activesupport (= 7.0.8.4)
-    activeresource (6.1.0)
+    activeresource (6.1.1)
       activemodel (>= 6.0)
       activemodel-serializers-xml (~> 1.0)
       activesupport (>= 6.0)
@@ -419,7 +419,7 @@ GEM
       securerandom
       sorbet-runtime
       zeitwerk (~> 2.5)
-    shopify_app (22.2.1)
+    shopify_app (22.4.0)
       activeresource
       addressable (~> 2.7)
       jwt (>= 2.2.3)
@@ -528,7 +528,7 @@ DEPENDENCIES
   rspec-rails
   sass-rails (~> 6.0)
   shopify_api (= 14.5.0)
-  shopify_app (= 22.2.1)
+  shopify_app (= 22.4.0)
   sidekiq (< 8)
   sidekiq-unique-jobs (< 9)
   simplecov
jagthedrummer commented 3 weeks ago

In trying to figure this out I've ended up updating every single gem in the project that is updateable, and I still run into this error when the only gem changing is shopify_app. Unfortunately the error doesn't show a regular stack trace, so I can't tell what the call chain looks like.

$ git diff
diff --git a/Gemfile b/Gemfile
index 9dc13c6..19d5c6f 100644
--- a/Gemfile
+++ b/Gemfile
@@ -34,7 +34,7 @@ gem 'jquery-rails'

 # Currently pinned to an old version because of this issue:
 # https://github.com/Shopify/shopify_app/issues/1908
-gem 'shopify_app', '22.2.1'
+gem 'shopify_app', '22.4.0'
 gem 'shopify_api', '14.5.0'

 gem 'sidekiq', '< 8'
diff --git a/Gemfile.lock b/Gemfile.lock
index 6ed14f7..4588758 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -416,7 +416,7 @@ GEM
       securerandom
       sorbet-runtime
       zeitwerk (~> 2.5)
-    shopify_app (22.2.1)
+    shopify_app (22.4.0)
       activeresource
       addressable (~> 2.7)
       jwt (>= 2.2.3)
@@ -525,7 +525,7 @@ DEPENDENCIES
   rspec-rails
   sass-rails (~> 6.0)
   shopify_api (= 14.5.0)
-  shopify_app (= 22.2.1)
+  shopify_app (= 22.4.0)
   sidekiq (< 8)
   sidekiq-unique-jobs (< 9)
   simplecov
jagthedrummer commented 3 weeks ago

Just in case it's useful, here are the outdated gems in one of the projects with this problem:

$ bundle outdated
Fetching gem metadata from https://gem.fury.io/jagthedrummer/..
Fetching gem metadata from https://rubygems.org/...........
Resolving dependencies...

Gem                Current  Latest  Requested  Groups
cucumber-core      13.0.3   14.0.0
cucumber-gherkin   27.0.0   29.0.0
cucumber-messages  22.0.0   26.0.0
moneta             1.0.0    1.6.0
shopify_app        22.2.1   22.4.0  = 22.2.1   default
zzooeeyy commented 3 weeks ago

Hey @jagthedrummer, I'm sorry that you're encountering this TypeError.

We replaced the JWT decoder in the JwtMiddleware to use ShopifyAPI::JwtPayload instead of ShopifyApp::JWT in version 22.3.0. The decoding logic for these 2 are the exact same, except the typing is more strict for the ShopifyApi::JwtPayload decoder.

ShopifyApi::JwtPayload is expecting exp to have integer type, this is the expected behaviour since the session token we decoded from Shopify contains integers for those fields. (example payload in shopify.dev)

Image

My guess for why your tests are failing is maybe due to test setup? Perhaps your tests are encoding a JWT payload with a string value instead and that's failing the type check. Here is an example of a JWT payload in our test for your reference

I hope this helps!

jagthedrummer commented 3 weeks ago

Thanks, @zzooeeyy, that was it. I had been doing this:

exp: Time.now.to_i + 5.minutes

And the problem went away after I changed it to this:

exp: (Time.now + 5.minutes).to_i
zzooeeyy commented 3 weeks ago

Glad that helped! I'll close this issue :)