Skip to content

[ENG-8317] Fix GV session/cookie issue which breaks Zotero/OAuth1 addons#311

Merged
cslzchen merged 7 commits into
feature/verified-resource-linkingfrom
for-the-sake-of-sanity
Jul 4, 2025
Merged

[ENG-8317] Fix GV session/cookie issue which breaks Zotero/OAuth1 addons#311
cslzchen merged 7 commits into
feature/verified-resource-linkingfrom
for-the-sake-of-sanity

Conversation

@adlius

@adlius adlius commented Jul 2, 2025

Copy link
Copy Markdown
Collaborator

No description provided.

@cslzchen cslzchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions:

  • Is this port from the same Django version we are using? 5.2.x IIRC?
  • I don't see any settings added to GV e.g. SESSION_COOKIE_SECURE, SESSION_COOKIE_HTTPONLY, SESSION_COOKIE_SAMESITE, etc.
  • Finally, my main concern is why Zotero needs to modify OSF session?
    • Before our session optimization, I don't think GV has access to OSF session, then how can it need to modify it? This makes me guess Zotero may just need a GV session.
    • If I am wrong, can you point out where Zotero modify OSF session and what value in the session does it modify or add? Is this extra piece of info used by OSF too?
    • Or maybe Zotero used to use GV session, but as a side-effect of our session optimization, it has to use the OSF session? If so, we need some kind of documentation on both side so further developers knows GV can update OSF session. This may also be a security fail point if others write an Addon in GV to manipulate user's OSF session.

@adlius

adlius commented Jul 2, 2025

Copy link
Copy Markdown
Collaborator Author

A few questions:

  • Is this port from the same Django version we are using? 5.2.x IIRC?

This is ported from the latest Django version SessionMiddleware, which has been unchanged for about 3 years. The version that we use (4.2.x) also has the same code.

  • I don't see any settings added to GV e.g. SESSION_COOKIE_SECURE, SESSION_COOKIE_HTTPONLY, SESSION_COOKIE_SAMESITE, etc.

Me neither. I believe there are some default Django settings.

  • Finally, my main concern is why Zotero needs to modify OSF session?

    • Before our session optimization, I don't think GV has access to OSF session, then how can it need to modify it? This makes me guess Zotero may just need a GV session.

GV session and OSF session is now the same thing

  • If I am wrong, can you point out where Zotero modify OSF session and what value in the session does it modify or add? Is this extra piece of info used by OSF too?

It's oauth1a_account_id field on the session that the Zotero oauth workflow is trying to update.

  • Or maybe Zotero used to use GV session, but as a side-effect of our session optimization, it has to use the OSF session? If so, we need some kind of documentation on both side so further developers knows GV can update OSF session. This may also be a security fail point if others write an Addon in GV to manipulate user's OSF session.

Correct. It was using a GV session. And because we have now united GV and OSF session, it now uses an OSF session. Yes we should add some documentation somewhere, but I don't know what is a good place to put them. As for the security concern, I suppose there is always some security downside to this in the sense that either OSF or GV can read/modify the same session, especially if we store sensitive data in the session itself. This is the inherent risk if we choose to share session between GV and OSF because there is nothing that stops GV/OSF from modifying/reading what it's not supposed to. I also don't think Django currently supports "read-only" session out of the box, so either we'll have to customize the SessionStore class, or we'll just have to use extra caution when dealing with sessions, not only in GV but also in OSF, as there is now some sort of coupling between these two services. Some potential improvement we could do in the future to separate the namespaces of OSF and GV in the same redis record.

@cslzchen cslzchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the explanation and the it all make sense to me now. The only thing is we need to add a few settings.

Comment thread app/middleware.py Outdated
@cslzchen cslzchen changed the title [ENG-8317] Fix zotero issues. [ENG-8317] Fix GV session/cookie issue which breaks Zotero/OAuth1 addons Jul 2, 2025
@cslzchen
cslzchen force-pushed the for-the-sake-of-sanity branch from bf4f51d to a77a22f Compare July 2, 2025 20:15

@cslzchen cslzchen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a list of changes for the full fix:

  • Override SessionMiddleware's process_response() in UnsignCookieSessionMiddleware:
    • Sign cookie value using server secret.
    • Don't delete cookie.
    • Don't set Max-Age or Expires.
  • Update session/cookie settings to sync with OSF
  • Fix tests

Verified locally with Zotero that everything works as expected and that GV's OSF session access keeps session/cookie intact

Screenshot 2025-07-04 at 10 30 39

Screenshot 2025-07-04 at 10 32 25

In addition:

  • Updated .gitignore

@cslzchen
cslzchen merged commit 4b4ad9e into feature/verified-resource-linking Jul 4, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants