fix(auth): interact redirect #2832

sabineschaller · 2024-08-01T09:51:53Z

Changes proposed in this pull request

Fixes the maxAge is invalid error by changing from hget/hset (saved as redis hashmap) to set/get (string) so that we can use JSON.parse and JSON.stringify. Saving the JSON.stringifyied session preserves the datatypes so that we don't need to deal with manually re-forming the session and can just do JSON.parse on read.
Fixes cookie not expiring in browser by returning null from get when the session is not found.
removes unused session middleware in interaction server ('/grant/:id/:nonce', '/grant/:id/:nonce/:choice')

Context

fixes [BUG] Internal Server Error when redirecting to interaction #2826

Checklist

netlify · 2024-08-01T09:52:09Z

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

Name	Link
🔨 Latest commit	`2e309f9`
🔍 Latest deploy log	https://app.netlify.com/sites/brilliant-pasca-3e80ec/deploys/66b27ff6affa8f00087d4d1d

BlairCurrey · 2024-08-01T19:17:50Z

packages/auth/src/app.ts

+              const s = await redis.hgetall(key)
+              const session = {
+                nonce: s.nonce,
+                _expire: Number(s._expire),
+                _maxAge: Number(s._maxAge)
+              }
+              return session


this would lose anything from s not explicitly set in session. not sure we can assume the only properties set on the session in redis will be these. we could do session = { ...s, _maxAge: Number(s._maxAge), _expire: Number(s._expire)} but that would have the same problem if the other fields are expected not to be strings (just like we saw with maxAge).

I think a more common way of getting/settings is JSON.stringify(object) on set and JSON.parse(string) on get, which which would take care of converting these to numbers without manually reforming the session object. hashes were suggested to me originally when I implemented this but not sure it's the best way to do it now that we're running into this...

BlairCurrey · 2024-08-02T02:53:37Z

When the session is retrieved from redis, the _maxAge and _expire parameters are strings instead of numbers. I don't know why that only messed things up every once in a while but once typecasting them back to Numbers, I didn't see the Internal Server Error anymore.

It does make sense that the string would cause the error. the type for session shows maxAge should be number | "session" | undefined (and unfortunately the return type for get is any). In my testing it happens in every subsequent grant request (with interaction). So I:

make a grant request, visit the link. OK
make another, visit the link. Internal Server Error. Refresh and its OK.
make another, visit the link. Internal Server Error. Refresh and its OK.
...etc

So at least for the initial one it seems obvious that its working because there is no session being retrieved from redis and parsed incorrectly. And then for subsequent requests there are, so it fails. But not sure why the refresh would work...

raducristianpopa · 2024-08-02T04:42:56Z

When the session is retrieved from redis, the _maxAge and _expire parameters are strings instead of numbers. I don't know why that only messed things up every once in a while but once typecasting them back to Numbers, I didn't see the Internal Server Error anymore.

It does make sense that the string would cause the error. the type for session shows maxAge should be number | "session" | undefined (and unfortunately the return type for get is any). In my testing it happens in every subsequent grant request (with interaction). So I:
* make a grant request, visit the link. OK

* make another, visit the link. Internal Server Error. Refresh and its OK.

* make another, visit the link. Internal Server Error. Refresh and its OK.
  ...etc
So at least for the initial one it seems obvious that its working because there is no session being retrieved from redis and parsed incorrectly. And then for subsequent requests there are, so it fails. But not sure why the refresh would work...

I ran some tests on this as well. It looks like the same session is used after the first interaction is complete.

Q:

Is this the intended behaviour?
Whenever we start a new interaction, a new session needs to be created as well or we just have to update the existing one?

Logs:

-------------------
{
  nonce: '53D27796BE7BB9BB',
  _expire: '1722572815903',
  _maxAge: '60000'
}
-------------------
^ logged after accepting the grant (when the user gets redirected to the finish url)
-------------------
{
  nonce: '53D27796BE7BB9BB',
  _expire: '1722572815903',
  _maxAge: '60000'
}
-------------------
^ logged when trying to access the redirect URL for the second grant

  TypeError: option maxAge is invalid
      [redacted]
      
-------------------
{
  nonce: 'D973ED2E15A125A0',
  _expire: '600001722572792015',
  _maxAge: '60000'
}
-------------------
^ logged after refreshing and accepting the second grant (when the user gets redirected to the finish url)

What I found really confusing is that when I am getting redirected to the finish URL (http://localhost:3006/interact/372b14bf-a96d-457a-bc7b-f70b09ddb17c/98F1F12B1BF803C1/finish) and I start refreshing the page (until the session becomes invalid), I am not seeing the maxAge error - the sessions is being logged. The get method is called successfully without throwing (the logs from above are located in the get method)

sabineschaller · 2024-08-02T11:55:38Z

In my testing it happens in every subsequent grant request (with interaction).

It didn't happen to me that often. That is what most confused me.

What I found really confusing is that when I am getting redirected to the finish URL (http://localhost:3006/interact/372b14bf-a96d-457a-bc7b-f70b09ddb17c/98F1F12B1BF803C1/finish) and I start refreshing the page (until the session becomes invalid), I am not seeing the maxAge error - the sessions is being logged. The get method is called successfully without throwing (the logs from above are located in the get method)

Same

BlairCurrey · 2024-08-06T19:24:25Z

Quick recap after going over this with @raducristianpopa.

While the error is being caused by maxAge being parsed from redis incorrectly (koa-session expects a number but its a string), there was also a problem of not expiring sessions correctly. That is, if you completed the interaction and keep refreshing, eventually the session would expire but the browser cookie would not be removed. This PR fixes these two things.

To address Radu's question from above:

Q:

Is this the intended behaviour?

Whenever we start a new interaction, a new session needs to be created as well or we just have to update the existing one?

Basically yes, this is the intended behavior. If you visit the redirect from the same browser you should have the same cookie which will use the same session. For good measure we confirmed that on rafiki.money different users are generating different sessions.

In addition we did an audit of security related cookie settings and found some things that could be improved and captured it here: #2844

BlairCurrey · 2024-08-06T19:33:33Z

packages/auth/src/interaction/routes.ts

@@ -181,7 +181,6 @@ async function startInteraction(

  const trx = await Interaction.startTransaction()
  try {
-    // TODO: also establish session in redis with short expiry


confirmed with Nathan this was just about moving sessions into Redis so its no longer a TODO

BlairCurrey · 2024-08-06T19:34:38Z

packages/auth/src/app.ts

-    koa.use(
-      session(
-        {
-          key: 'sessionId',
-          maxAge: 60 * 1000,
-          signed: true
-        },
-        koa
-      )
-    )
-


not used anywhere. looks like a potential copy/paste artifact from the recent(ish) factoring out of interaction routes into the startInteractionServer ('/grant/:id/:nonce/:choice', '/grant/{id}/{nonce}').

njlie

LGTM

* fix(auth): interact redirect * fix(auth): session cookie not expiring in browser * fix(auth): session expiration time unit --------- Co-authored-by: Blair Currey <12960453+BlairCurrey@users.noreply.github.com>

fix(auth): interact redirect

22e5858

sabineschaller requested a review from BlairCurrey August 1, 2024 09:52

github-actions bot added type: source Changes business logic pkg: auth Changes in the GNAP auth package. labels Aug 1, 2024

sabineschaller requested a review from raducristianpopa August 1, 2024 09:54

BlairCurrey reviewed Aug 1, 2024

View reviewed changes

fix(auth): session cookie not expiring in browser

b1f9248

BlairCurrey reviewed Aug 6, 2024

View reviewed changes

BlairCurrey requested a review from njlie August 6, 2024 19:37

fix(auth): session expiration time unit

2e309f9

njlie approved these changes Aug 6, 2024

View reviewed changes

raducristianpopa approved these changes Aug 7, 2024

View reviewed changes

BlairCurrey merged commit 2c8887d into main Aug 7, 2024
42 checks passed

BlairCurrey deleted the s2/2826/redirect-bug branch August 7, 2024 13:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(auth): interact redirect #2832

fix(auth): interact redirect #2832

sabineschaller commented Aug 1, 2024 •

edited by BlairCurrey

Loading

netlify bot commented Aug 1, 2024 •

edited

Loading

BlairCurrey Aug 1, 2024 •

edited

Loading

BlairCurrey commented Aug 2, 2024

raducristianpopa commented Aug 2, 2024 •

edited

Loading

sabineschaller commented Aug 2, 2024 •

edited

Loading

BlairCurrey commented Aug 6, 2024

BlairCurrey Aug 6, 2024

BlairCurrey Aug 6, 2024 •

edited

Loading

njlie left a comment

fix(auth): interact redirect #2832

fix(auth): interact redirect #2832

Conversation

sabineschaller commented Aug 1, 2024 • edited by BlairCurrey Loading

Changes proposed in this pull request

Context

Checklist

netlify bot commented Aug 1, 2024 • edited Loading

✅ Deploy Preview for brilliant-pasca-3e80ec canceled.

BlairCurrey Aug 1, 2024 • edited Loading

Choose a reason for hiding this comment

BlairCurrey commented Aug 2, 2024

raducristianpopa commented Aug 2, 2024 • edited Loading

sabineschaller commented Aug 2, 2024 • edited Loading

BlairCurrey commented Aug 6, 2024

BlairCurrey Aug 6, 2024

Choose a reason for hiding this comment

BlairCurrey Aug 6, 2024 • edited Loading

Choose a reason for hiding this comment

njlie left a comment

Choose a reason for hiding this comment

sabineschaller commented Aug 1, 2024 •

edited by BlairCurrey

Loading

netlify bot commented Aug 1, 2024 •

edited

Loading

BlairCurrey Aug 1, 2024 •

edited

Loading

raducristianpopa commented Aug 2, 2024 •

edited

Loading

sabineschaller commented Aug 2, 2024 •

edited

Loading

BlairCurrey Aug 6, 2024 •

edited

Loading