Skip to content
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

issue#348. Fixed CSRF tokens not sessioned when using scope and memory_sessions #349

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/dream.ml
Original file line number Diff line number Diff line change
Expand Up @@ -260,9 +260,9 @@ let drop_session_field = Session.drop_session_field
let all_session_values = Session.all_session_values
let all_session_fields = all_session_values
let invalidate_session = Session.invalidate_session
let memory_sessions = Session.memory_sessions
let cookie_sessions = Session.cookie_sessions
let sql_sessions = Sql_session.sql_sessions
let memory_sessions = Session.memory_sessions ()
let cookie_sessions = Session.cookie_sessions ()
let sql_sessions = Sql_session.sql_sessions ()
Copy link
Owner

Choose a reason for hiding this comment

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

At first glance, this looks like whatever is delayed by the extra unit parameter, will be triggered once per process, rather than once per server (or once per scope). With this change, can there be two concurrent uses of two different memory_session middlewares in one process?

Copy link
Collaborator Author

@alxtuz alxtuz Nov 10, 2024

Choose a reason for hiding this comment

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

Thanks for your questions. Right after you answered them, I realised that my solution is wrong.

With this change, can there be two concurrent uses of two different memory_session middlewares in one process?

No, there can't be.
I suppose that, a correct fix is to change the way how we use session middlewares. I mean like this Dream.memory_sessions (). But in this case we would need to change examples and documentation. I haven't dare to make such global changes without discussion.

Copy link
Owner

Choose a reason for hiding this comment

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

Could you give some consideration to whether there is an alterative way to fix this without complicating usage in this way? And if not, then it's fine to add a () parameter to the session middlewares. Thank you!

let session_id = Session.session_id
let session_label = Session.session_label
let session_expires_at = Session.session_expires_at
Expand Down
6 changes: 3 additions & 3 deletions src/dream.mli
Original file line number Diff line number Diff line change
Expand Up @@ -1583,15 +1583,15 @@ val invalidate_session : request -> unit promise

(** {2 Back ends} *)

val memory_sessions : ?lifetime:float -> middleware
val memory_sessions : middleware
Copy link
Owner

Choose a reason for hiding this comment

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

What happened to the ?lifetime parameter?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I was forced to eliminate it for saving existing way of usage session middlewares. In other way every usage of session middleware would look like Dream.memory_sessions ().

Copy link
Owner

Choose a reason for hiding this comment

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

I think this parameter has to be kept, and, in light of the other comment, can be kept.

(** Stores sessions in server memory. Passes session IDs to clients in cookies.
Session data is lost when the server process exits. *)

val cookie_sessions : ?lifetime:float -> middleware
val cookie_sessions : middleware
(** Stores sessions in encrypted cookies. Use {!Dream.set_secret} to be able to
decrypt cookies from previous server runs. *)

val sql_sessions : ?lifetime:float -> middleware
val sql_sessions : middleware
(** Stores sessions in an SQL database. Passes session IDs to clients in
cookies. Must be used under {!Dream.sql_pool}. Expects a table

Expand Down
21 changes: 8 additions & 13 deletions src/server/router.ml
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ type token =
| Param of string
| Wildcard of string

let log =
Log.sub_log "dream.router"

let rec validate route = function
| (Param "")::_ ->
Printf.ksprintf failwith "Empty path parameter name in '%s'" route
Expand Down Expand Up @@ -141,28 +144,24 @@ let any pattern handler =
let no_route =
[]

let rec apply middlewares routes =
let rec compose handler = function
| [] -> handler
| middleware::more -> middleware @@ compose handler more
in
let rec apply pipeline routes =
routes
|> List.flatten
|> List.map (fun (pattern, node) ->
let node =
match node with
| Handler (method_, handler) ->
Handler (method_, compose handler middlewares)
| Scope route -> Scope (apply middlewares [route])
Handler (method_, pipeline handler)
| Scope route -> Scope (apply pipeline [route])
in
pattern, node)

let under prefix routes =
[strip_empty_trailing_token (parse prefix), Scope (List.flatten routes)]

let scope prefix middlewares routes =
under prefix [apply middlewares routes]

let pipeline = Message.pipeline middlewares in
under prefix [apply pipeline routes]


let path_field : string list Message.field =
Expand Down Expand Up @@ -213,10 +212,6 @@ let params_field : (string * string) list Message.field =
()



let log =
Log.sub_log "dream.router"

let missing_param request name =
let message = Printf.sprintf "Dream.param: missing path parameter %S" name in
log.error (fun log -> log ~request "%s" message);
Expand Down
4 changes: 2 additions & 2 deletions src/server/session.ml
Original file line number Diff line number Diff line change
Expand Up @@ -341,10 +341,10 @@ let two_weeks =
module Make (Pclock : Mirage_clock.PCLOCK) = struct
let now () = Ptime.to_float_s (Ptime.v (Pclock.now_d_ps ()))

let memory_sessions ?(lifetime = two_weeks) =
let memory_sessions ?(lifetime = two_weeks) () =
middleware (Memory.back_end ~now lifetime)

let cookie_sessions ?(lifetime = two_weeks) =
let cookie_sessions ?(lifetime = two_weeks) () =
middleware (Cookie.back_end ~now lifetime)
end

Expand Down
2 changes: 1 addition & 1 deletion src/sql/session.ml
Original file line number Diff line number Diff line change
Expand Up @@ -211,5 +211,5 @@ let back_end lifetime = {
send;
}

let sql_sessions ?(lifetime = Session.two_weeks) =
let sql_sessions ?(lifetime = Session.two_weeks) () =
Session.middleware (back_end lifetime)