search

oxidizing / sihl

A modular functional web framework
https://v3.ocaml.org/p/sihl
MIT License
359 stars 13 forks source link

Add cookie attributes in `Web.Session.set` #566

Open Favo02 opened 6 months ago

Favo02 commented 6 months ago

Hello, thanks for the incredibly useful library!

I am developing a REST API, the authorization is managed using signed cookies with the Web.Sessions module.

The set cookie function accepts as optional parameters only ?cookie_key and ?secret. I noticed that the underlying Opium.Response.add_cookie_or_replace offers a bigger API, with ?⁠expires, ?⁠same_site, ?⁠secure and ?⁠http_only parameters, which are all very useful (and important for security reasons) while building apps that uses cookies. Is there any particular reason for "hiding" them?

I ended up using directly Opium.Response.add_cookie_or_replace to set cookies and Web.Sessions.find to read them, which is inconvenient. I think that a few more optional parameters to the Web.Sessions.set function (which would be passed directly to the Opium call) would be really helpful, if needed I can create a PR.

joseferben commented 6 months ago

hey @Favo02!

exposing those parameters would make sense! the main reason that we haven't done that yet, is that the defaults worked for our uses cases. if you want to give it a shot, we'd appreciate a pr!

Favo02 commented 6 months ago

The PR needs to wait a bit because the function Opium.Response.add_cookie_or_replace seems to not work with same_site attribute. I opened an issue (opium issue #290) and waiting an answer, I keep this issue updated.