Skip to content

Move the session cookie to $XDG_RUNTIME_DIR #387

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

knedlsepp
Copy link

Before this commit the session cookie and its corresponding lock file
would be placed in the user's home directory. Sadly this breaks all
setups that configure the user's home to live on a NFS mount. File
locking on NFS mounts is something that doesn't work reliably and thus
causes some users to experience issues like this:

Failed to save cookie file: bad file descriptor

To counteract, this commit instead stores this data in the
$XDG_RUNTIME_DIR which is a directory that is usually located on the
local disk or even located in runtime memory.

This commit will most likely slightly change the behaviour of how
persistent the session data will be stored since often $XDG_RUNTIME_DIR
will be cleared on logout of the user.

Fixes: #290

Before this commit the session cookie and its corresponding lock file
would be placed in the user's home directory. Sadly this breaks all
setups that configure the user's home to live on a NFS mount. File
locking on NFS mounts is something that doesn't work reliably and thus
causes some users to experience issues like this:

    Failed to save cookie file: bad file descriptor

To counteract, this commit instead stores this data in the
$XDG_RUNTIME_DIR which is a directory that is usually located on the
local disk or even located in runtime memory.

This commit will most likely slightly change the behaviour of how
persistent the session data will be stored since often $XDG_RUNTIME_DIR
will be cleared on logout of the user.

Fixes: go-jira#290
@hrivera-ntap
Copy link

Any chance this could get reviewed and merged?

@knedlsepp
Copy link
Author

Still need this patch locally. Would appreciate if somebody takes a look. :-)

@georgettica
Copy link
Collaborator

the code looks OK, but I cannot approve as I am not farmilliar with XDG_RUNTIME_DIR. I hope some of the maintainers do have the experience with it and can weigh in

@georgettica
Copy link
Collaborator

so checked a bit and it seems that any system that has the X-Server can use it (not clearly compatible with macos / windows)
I checked online and this seems like something that is done in the wild and no package that has fancy checks around it.

@knedlsepp
Copy link
Author

Thanks for the review. In fact I did not consider macOS or Windows when writing this. This PR should keep the existing behavior of writing to the user's home directory in case that environment variable is unset. Another alternative would be to rely on some other package to provide this functionality. How about depending on https://pkg.go.dev/github.com/zchee/go-xdgbasedir#section-readme for this?

@georgettica
Copy link
Collaborator

depending on a third party does not solve the issue. as MacOS and Windows won't have the XDG.
I think the other maintainers will help as their main driver is linux and have more experience with go-jira

@knedlsepp
Copy link
Author

knedlsepp commented Nov 14, 2022

depending on a third party does not solve the issue. as MacOS and Windows won't have the XDG

This library would provide a function RuntimeDir() that returns the platform specific directories:

  • C:\Users\%USER% on Windows,
  • ~/Library/Application Support on macOS
  • /run/user/$UID on Linux

@georgettica
Copy link
Collaborator

seems ok, but adding the library is not something I feel confident approving. so again deferring it to the others :/

@nyarly
Copy link
Collaborator

nyarly commented May 7, 2025

@knedlsepp Sorry to be returning to this after so long - are you still interested in this issue?

@knedlsepp
Copy link
Author

@knedlsepp Sorry to be returning to this after so long - are you still interested in this issue?

I'm still locally applying this patch for my use case, so one could say I'm still interested. I don't have much experience in go, so probably won't find time to give the alternative I mentioned in #387 (comment) a shot.

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.

bad file descriptor
4 participants