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

[WIP] Added RequestContextStore, RequestContext and SignedPrivateCookieJar #633

Draft
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

yinho999
Copy link
Contributor

@yinho999 yinho999 commented Jul 2, 2024

Goal

Configurability

  • Provide easy configuration to switch between providers (e.g., in-memory, cookies, and others)
  • Use cookie store as the default for sessions, following Rails' approach

Session

  • Implement a RequestContext to handle session data
  • Change ctx to req in function signatures:
foo_handle(req: RequestContext) -> { .. }

Cookies Store (Cookie)

  • Use both Encrypt and Signed Cookies (PrivateCookieJar) Rails
  • Use app-specific cookie ID (e.g., __loco_myapp_session)

Cookies Security

  • Generate app-specific secret per app, no default
  • Implement SSL in production

Session Store (In Memory or Other Database)

  • Provide a normal session store that looks like a hash and is permanent (e.g., session["foobar"] = 3)
  • Provide options for different backends:
    • In-memory
    • Database
    • Cookie-based

Default Behavior

  • Use cookie store as the default for sessions (following Rails' approach)
  • Ensure this default is secure and follows best practices

Documentation and Tooling

  • Provide clear documentation on configuration options and their security implications
  • Consider developing tools to assist with secure configuration for production environments

Additional Considerations Rails

  • Ensure proper handling of session expiration and invalidation
  • Address size limitations of cookie-based storage - 4KB
  • Implement proper encryption and signing of cookie data
  • Consider the trade-offs between different session storage methods

Flash Messages(Optional)

  • Consider implementing Flash functionality (lives only between actions) Flash

Plan
Drawing 2024-06-30 22 07 52 excalidraw
Template

@yinho999 yinho999 marked this pull request as draft July 2, 2024 20:08
@yinho999
Copy link
Contributor Author

yinho999 commented Jul 2, 2024

@jondot @schungx Here is my current development plan and the key points I've summarized from the discussion. Please don't hesitate to correct me if I've missed anything that might lead to security issues.

src/boot.rs Outdated
@@ -205,6 +206,7 @@ pub async fn create_context<H: Hooks>(environment: &Environment) -> Result<AppCo
queue: connect_redis(&config).await,
storage: Storage::single(storage::drivers::null::new()).into(),
cache: cache::Cache::new(cache::drivers::null::new()).into(),
request_context: create_request_context_store(&config.request_context)?.into(),
Copy link
Contributor

Choose a reason for hiding this comment

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

what happens to request context when there is no request involved? for example tasks, workers ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hmm, that is a good point

@jondot
Copy link
Contributor

jondot commented Jul 3, 2024

Looks like a fantastic plan!
I will keep monitoring this WIP PR

@schungx
Copy link

schungx commented Jul 3, 2024

  • Provide options for different backends:

    In-memory
    Database
    Cookie-based

I wonder if we can do something wrt cookie-based instead of just disallowing it for production. It is just the case that the hash resides on the client instead of on the server.

If the server needs to read the hash, then obviously it'll fail -- not supported.

If the server needs to write to the hash, for example to force-expire it, then it can potentially be queued up (Redis?) and replayed during the next response to be sent to the client (which will include the relevant cookie header to set the hash values).

Would this be possible?

EDIT: Come to think of it ... this may not be very useful. If I can store commands in Redis, I might as well put the session store there. Or in an in memory store.

In this case, why not just make the in-memory store the default and be done with it?

@yinho999
Copy link
Contributor Author

yinho999 commented Jul 3, 2024

  • Provide options for different backends:
    In-memory
    Database
    Cookie-based

I wonder if we can do something wrt cookie-based instead of just disallowing it for production. It is just the case that the hash resides on the client instead of on the server.

If the server needs to read the hash, then obviously it'll fail -- not supported.

If the server needs to write to the hash, for example to force-expire it, then it can potentially be queued up (Redis?) and replayed during the next response to be sent to the client (which will include the relevant cookie header to set the hash values).

Would this be possible?

EDIT: Come to think of it ... this may not be very useful. If I can store commands in Redis, I might as well put the session store there. Or in an in memory store.

In this case, why not just make the in-memory store the default and be done with it?

Cookie sessions can be useful across multiple servers, the pros and cons should be around the same as arguments between JWT tokens and session tokens

Added SignedPrivateCookieJar for encrypting/decrypting and signing data between outcoming/incoming headers.

Added testing for both functionality
@jondot
Copy link
Contributor

jondot commented Jul 16, 2024

I wonder if we can do something wrt cookie-based instead of just disallowing it for production. It is just the case that the hash resides on the client instead of on the server.

I believe if the hash is on the client and is stored on the browser, it is still being sent, and received as-is, so if the server wants, it can access the hash via incoming cookie header.

@yinho999
Copy link
Contributor Author

yinho999 commented Aug 8, 2024

@jondot Sorry for postponing this ticket. I was trying out Ruby on Rails and experimenting it to get a better idea about how the request context being used and the feels in the framework. I am going to continue this ticket this week 👍.

@jondot
Copy link
Contributor

jondot commented Aug 11, 2024

@yinho999 please note that we just pushed a request_id implementation to master (including some improvement in middleware run order), so that takes some effort off from your PR

@yinho999
Copy link
Contributor Author

@yinho999 please note that we just pushed a request_id implementation to master (including some improvement in middleware run order), so that takes some effort off from your PR

No worries, gonna remove my current implementation right now

# Conflicts:
#	src/controller/app_routes.rs
…text

- Updated imports and references to use `LocoRequestId` instead of `RequestId`.
- Removed unused `request_id` module.
- Adjusted `RequestContext` struct and methods to accommodate the new `LocoRequestId`.
- Cleaned up unnecessary imports in `boot.rs`.
…text

- Updated imports and references to use `LocoRequestId` instead of `RequestId`.
- Removed unused `request_id` module.
- Adjusted `RequestContext` struct and methods to accommodate the new `LocoRequestId`.
- Cleaned up unnecessary imports in `boot.rs`.
- Introduced `create_request_context` and updated `get_request_context` in `mysession.rs` for managing request context data.
- Added tests for setting and getting request context data in `mysession.rs`.
- Updated `test.yaml` and `teste2e.yaml` to include request context configuration.
- Minor refactoring and cleanup in `cookie.rs` and other test files.
@jondot jondot added this to the 0.8.0 milestone Aug 12, 2024
- Added `insert`, `get`, `remove`, and `clear` methods to `RequestContext` for session management.
- Introduced `RequestContextError::DriverError` to handle driver-related errors.
- Updated `create_request_context` and `get_request_context` functions in `mysession.rs` to use the new `RequestContext` methods.
# Conflicts:
#	Cargo.toml
#	src/controller/app_routes.rs
…uration

- Introduced `CustomSessionStore` to support custom session management.
- Updated `AppRoutes` to include session store configuration.
- Refactored `RequestContextMiddleware` to handle different session store types.
- Added detailed session configuration options in YAML files.
- Improved documentation and comments for better clarity on session management.
@jondot jondot modified the milestones: 0.8.0, 0.9.0 Sep 1, 2024
# Conflicts:
#	src/controller/app_routes.rs
@yinho999
Copy link
Contributor Author

yinho999 commented Sep 6, 2024

@jondot @schungx I apologize for the long absence without notice. I will continue to work on this PR right away.

@jondot jondot modified the milestones: 0.9.0, 0.10.0 Sep 19, 2024
- Introduced `SessionCookieConfig` to manage cookie attributes such as `name`, `http_only`, `same_site`, `secure`, `path`, `domain`, and `expiry`.
- Updated `SignedPrivateCookieJar` to support configurable cookie attributes.
- Added methods to `SessionCookieConfig` for extracting and applying cookie configurations.
- Modified `RequestContextStore` to include `SessionCookieConfig`.
- Enhanced middleware setup in `app_routes` to utilize `SessionCookieConfig`.
- Updated tests to cover new session cookie configurations and scenarios.
- Improved documentation and comments for better clarity and maintainability.
# Conflicts:
#	examples/demo/Cargo.lock
#	src/config.rs
#	src/controller/app_routes.rs
#	src/tests_cfg/config.rs
@jondot jondot modified the milestones: 0.10.0, backlog Oct 13, 2024
# Conflicts:
#	examples/demo/config/development.yaml
#	src/controller/middleware/cors.rs
#	src/controller/middleware/mod.rs
#	src/controller/middleware/powered_by.rs
#	src/controller/middleware/timeout.rs
# Conflicts:
#	Cargo.toml
#	examples/demo/Cargo.toml
# Conflicts:
#	Cargo.toml
#	examples/demo/Cargo.toml
#	src/scheduler.rs
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