-
Notifications
You must be signed in to change notification settings - Fork 0
feat: privacy mode to suppress logs and tracing #17
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
base: master
Are you sure you want to change the base?
Changes from all commits
118dd47
f86a484
9fa058d
0732c2d
1d7c31f
4bd5c23
f34b25b
54b46e7
6aba6da
b66a9ce
3e2e606
124f837
7e38605
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1 +1,2 @@ | ||
| pub mod guards; | ||
| pub mod privacy_mode; |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,83 @@ | ||||||||||||||||||||||
| use rocket::Request; | ||||||||||||||||||||||
| use rocket::fairing::AdHoc; | ||||||||||||||||||||||
| use tracing::info; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| thread_local! { | ||||||||||||||||||||||
| static PRIVACY_MODE: std::cell::Cell<bool> = const { std::cell::Cell::new(false) }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /// Extract privacy_mode from query parameters or headers | ||||||||||||||||||||||
| fn extract_privacy_mode(req: &Request<'_>) -> bool { | ||||||||||||||||||||||
| // Check query parameters first | ||||||||||||||||||||||
| if let Some(Ok(true)) = req.query_value::<bool>("privacy_mode") { | ||||||||||||||||||||||
| return true; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| // Check headers | ||||||||||||||||||||||
|
Comment on lines
+11
to
+16
|
||||||||||||||||||||||
| // Check query parameters first | |
| if let Some(Ok(true)) = req.query_value::<bool>("privacy_mode") { | |
| return true; | |
| } | |
| // Check headers | |
| // Only check headers for privacy mode |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation on critical limitation: The implementation doesn't reset the privacy flag after request completion, which can cause privacy mode to "leak" into subsequent requests on the same thread. This could result in legitimate requests being silently suppressed from logs/traces.
Additionally, the comment should warn about the thread-safety limitations of the current thread-local approach in Rocket's async environment.
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing cleanup: The privacy mode flag is set but never reset after the request completes. This could cause privacy mode to leak into subsequent requests handled by the same thread.
Add an on_response fairing to reset the privacy mode flag:
pub fn privacy_mode_cleanup_fairing() -> impl rocket::fairing::Fairing {
AdHoc::on_response("Privacy Mode Cleanup", |_, _| {
Box::pin(async move {
PRIVACY_MODE.with(|cell| cell.set(false));
})
})
}Note: This is a partial mitigation. The fundamental concurrency issue with thread-local storage in async contexts still needs to be addressed.
| /// Create a fairing that resets privacy mode state after the response | |
| pub fn privacy_mode_cleanup_fairing() -> impl rocket::fairing::Fairing { | |
| AdHoc::on_response("Privacy Mode Cleanup", |_, _| { | |
| Box::pin(async move { | |
| PRIVACY_MODE.with(|cell| cell.set(false)); | |
| }) | |
| }) | |
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,3 +8,4 @@ pub mod integration_tests; | |
| pub mod lit_actions; | ||
| pub mod session_sigs; | ||
| pub mod shadow; | ||
| pub mod tracing; | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,253 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::common::auth_sig::get_session_sigs_for_auth; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use crate::common::lit_actions::execute_lit_action_session_sigs; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use lit_node_core::request::JsonExecutionRequest; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use lit_node_core::response::{GenericResponse, JsonExecutionResponse}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use lit_node_core::{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Invocation, LitAbility, LitResourceAbilityRequest, LitResourceAbilityRequestResource, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| LitResourcePrefix, NodeSet, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use lit_node_testnet::TestSetupBuilder; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use lit_node_testnet::node_collection::get_identity_pubkeys_from_node_set; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use rand::Rng; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use rand_core::OsRng; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::fs::File; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use std::io::{BufRead, BufReader, Seek}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| use tracing::info; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #[tokio::test] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| pub async fn test_privacy_mode_logging() { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| crate::common::setup_logging(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| info!("Starting privacy mode test"); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Setup testnet | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let (_testnet, validator_collection, end_user) = TestSetupBuilder::default().build().await; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| let (_testnet, validator_collection, end_user) = TestSetupBuilder::default().build().await; | |
| let (testnet, validator_collection, end_user) = TestSetupBuilder::default().build().await; |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded sleep duration could lead to flaky tests. The 1000ms wait may not be sufficient on slower CI systems or under load, potentially causing the test to fail intermittently when logs haven't been fully flushed yet.
Consider either:
- Using a retry loop with a timeout to wait for expected log conditions
- Adding a configurable or environment-based timeout
- Calling an explicit flush method if available on the log readers
| // Wait a bit for logs to be written | |
| tokio::time::sleep(tokio::time::Duration::from_millis(1000)).await; | |
| // Read new logs from all nodes | |
| let logs_no_privacy: Vec<String> = log_readers | |
| .iter_mut() | |
| .enumerate() | |
| .flat_map(|(idx, reader)| { | |
| let start_pos = initial_log_positions.get(idx).copied().unwrap_or(0); | |
| read_logs_from_reader(reader, start_pos) | |
| }) | |
| .collect(); | |
| // Wait for logs to be written, up to a timeout | |
| let logs_no_privacy = { | |
| let mut logs = Vec::new(); | |
| let max_attempts = 50; // 50 * 100ms = 5 seconds | |
| let mut attempts = 0; | |
| loop { | |
| logs = log_readers | |
| .iter_mut() | |
| .enumerate() | |
| .flat_map(|(idx, reader)| { | |
| let start_pos = initial_log_positions.get(idx).copied().unwrap_or(0); | |
| read_logs_from_reader(reader, start_pos) | |
| }) | |
| .collect::<Vec<String>>(); | |
| if !logs.is_empty() { | |
| break logs; | |
| } | |
| if attempts >= max_attempts { | |
| panic!("Timed out waiting for logs to be written after executing lit action without privacy mode"); | |
| } | |
| attempts += 1; | |
| tokio::time::sleep(tokio::time::Duration::from_millis(100)).await; | |
| } | |
| }; |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Duplicate hardcoded sleep with same issue as line 85. Consider extracting this into a helper function to ensure consistency and make it easier to adjust the timing strategy later.
| tokio::time::sleep(tokio::time::Duration::from_millis(1000)).await; | |
| wait_for_logs().await; |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test should verify that the privacy mode log itself doesn't leak sensitive information. The current test verifies that privacy_mode_request logs exist but doesn't check their content to ensure they don't inadvertently include request parameters, user data, or other sensitive information beyond method and path.
Add an assertion to verify the privacy mode log contains only the expected fields (method, path) and no additional sensitive data.
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Potential false negative in privacy mode verification: The test only checks for the absence of specific log patterns like "executing lit action", "POST /web/execute/v2", etc. However, there could be other logs generated during execution that still leak information.
Consider enhancing the test to:
- Count total log lines/bytes generated (already done, which is good)
- Add positive assertions about what logs SHOULD be present (the privacy_mode_request log)
- Use a more comprehensive deny-list of sensitive patterns or check that no logs contain request-specific data like correlation IDs, user addresses, etc.
The current assertion at lines 229-235 comparing content length is a good catch-all, but the specific pattern checks could miss new logging that gets added in the future.
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silent error handling could mask test failures. If seek() fails (e.g., due to file issues), an empty vector is returned without any indication that something went wrong. This could lead to false negatives where the test passes even though log reading failed.
Consider either:
- Propagating the error up:
reader.seek(...)?; - Logging a warning or using
expect()with a descriptive message - At minimum, documenting why silent failure is acceptable here
| // Seek to start position | |
| if reader | |
| .seek(std::io::SeekFrom::Start(start_position)) | |
| .is_err() | |
| { | |
| return Vec::new(); | |
| } | |
| // Seek to start position, fail loudly if it fails | |
| reader | |
| .seek(std::io::SeekFrom::Start(start_position)) | |
| .expect(&format!("Failed to seek to position {} in log file", start_position)); |
Copilot
AI
Dec 9, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Silent error handling with unwrap_or_default() could mask log reading failures. If a log line fails to read (e.g., due to encoding issues or file corruption), it's silently replaced with an empty string, which could affect test accuracy.
Consider using expect() or unwrap() to fail fast on errors, or at least log warnings for failed reads.
| .map(|line| line.unwrap_or_default()) | |
| .map(|line| line.expect("Failed to read log line")) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical Concurrency Issue: Using thread-local storage (
PRIVACY_MODE) for request-scoped state is unsafe in Rocket's async execution model. Async tasks can migrate between threads during.awaitpoints, which means:on_requestmay be on a different thread than where the tracing checks occurSolution: Use Rocket's request-local state instead:
FromRequestto extract privacy mode per requestlocal_cacheor request guardsAlternatively, if thread-local must be used, consider using task-local storage with
tokio::task_local!which is properly scoped to async tasks.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@glitch003 pretty valid point here, second this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, yeah i think this is the main issue with this PR, will fix