Skip to content
Draft
Show file tree
Hide file tree
Changes from 11 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
2 changes: 1 addition & 1 deletion rust/lit-node/lit-node/src/git_info.rs
Original file line number Diff line number Diff line change
@@ -1 +1 @@
pub const GIT_COMMIT_HASH: &str = "5caf4cb4b70f0ec3b5094e71785ce78421027805";
pub const GIT_COMMIT_HASH: &str = "9fa058d074c49194725a75b99f23a40cbd072474";
6 changes: 6 additions & 0 deletions rust/lit-node/lit-node/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -409,6 +409,7 @@ pub fn main() {
})
}))
.attach(metrics_fairings)
.attach(crate::utils::rocket::privacy_mode::privacy_mode_fairing())
.manage(cfg.clone())
.manage(resolver)
.manage(tss_state.peer_state.clone())
Expand Down Expand Up @@ -491,6 +492,11 @@ async fn init_observability(
.await
.map_err(|e| unexpected_err(e, Some("failed to create OTEL providers: {:?}".into())))?;

// Add privacy mode layer to disable tracing when privacy_mode is enabled
// The privacy mode layer checks thread-local state set by the fairing
use tracing_subscriber::layer::SubscriberExt;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this should be at the top with the other imports

let subscriber = subscriber.with(crate::utils::rocket::privacy_mode::PrivacyModeLayer);

// Set globals
global::set_text_map_propagator(TraceContextPropagator::new());
global::set_tracer_provider(tracing_provider);
Expand Down
1 change: 1 addition & 0 deletions rust/lit-node/lit-node/src/utils/rocket/mod.rs
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
pub mod guards;
pub mod privacy_mode;
83 changes: 83 additions & 0 deletions rust/lit-node/lit-node/src/utils/rocket/privacy_mode.rs
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) };
}
Comment on lines +5 to +7
Copy link

Copilot AI Dec 9, 2025

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 .await points, which means:

  1. The privacy mode flag set in on_request may be on a different thread than where the tracing checks occur
  2. Multiple concurrent requests on the same thread could interfere with each other's privacy settings
  3. The flag may not be reset after the request completes, affecting subsequent requests on the same thread

Solution: Use Rocket's request-local state instead:

  • Implement FromRequest to extract privacy mode per request
  • Store privacy mode in request-local state using local_cache or request guards
  • Pass privacy mode context through the tracing span/context rather than thread-local state

Alternatively, if thread-local must be used, consider using task-local storage with tokio::task_local! which is properly scoped to async tasks.

Copilot uses AI. Check for mistakes.
Copy link
Contributor

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

Copy link
Contributor Author

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


/// 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
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

Potential security concern: Privacy mode can be enabled via query parameters (?privacy_mode=true), which may be logged by web servers, proxies, or browser history, defeating the purpose of privacy mode. Query parameters are typically logged in access logs and appear in browser history/bookmarks.

Consider:

  1. Removing query parameter support and only allowing the X-Privacy-Mode header
  2. If query parameter support is required, document this limitation clearly
  3. Add a warning log when privacy mode is enabled via query parameters (though this somewhat defeats the purpose)
Suggested change
// 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 uses AI. Check for mistakes.
if let Some(privacy_mode_header) = req.headers().get_one("X-Privacy-Mode") {
if privacy_mode_header.eq_ignore_ascii_case("true") || privacy_mode_header == "1" {
return true;
}
}

false
}

/// Check if privacy mode is enabled for the current request
pub fn is_privacy_mode_enabled() -> bool {
PRIVACY_MODE.with(|cell| cell.get())
}
Comment on lines +26 to +29
Copy link

Copilot AI Dec 9, 2025

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 uses AI. Check for mistakes.

/// Create a fairing that sets privacy mode state for the request
pub fn privacy_mode_fairing() -> impl rocket::fairing::Fairing {
AdHoc::on_request("Privacy Mode", |req, _| {
Box::pin(async move {
let privacy_enabled = extract_privacy_mode(req);

// If privacy mode is enabled, log just the endpoint path for metrics
// This must happen before we set privacy mode, otherwise the log will be filtered
if privacy_enabled {
let method = req.method().as_str();
let path = req.uri().path().as_str();
info!(method = method, path = path, "privacy_mode_request");
}

// Store in thread-local state so the tracing layer can check it
PRIVACY_MODE.with(|cell| cell.set(privacy_enabled));
})
})
}

Copy link

Copilot AI Dec 9, 2025

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.

Suggested change
/// 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));
})
})
}

Copilot uses AI. Check for mistakes.
/// A tracing layer that filters out all events and spans when privacy mode is enabled
pub struct PrivacyModeLayer;

impl<S> tracing_subscriber::Layer<S> for PrivacyModeLayer
where
S: tracing::Subscriber,
{
fn enabled(
&self,
_metadata: &tracing::Metadata<'_>,
_ctx: tracing_subscriber::layer::Context<'_, S>,
) -> bool {
// Disable all tracing when privacy mode is enabled
!is_privacy_mode_enabled()
}

fn on_event(
&self,
_event: &tracing::Event<'_>,
_ctx: tracing_subscriber::layer::Context<'_, S>,
) {
// Events are filtered by enabled() above
}

fn on_new_span(
&self,
_attrs: &tracing::span::Attributes<'_>,
_id: &tracing::span::Id,
_ctx: tracing_subscriber::layer::Context<'_, S>,
) {
// Spans are filtered by enabled() above
}
}
1 change: 1 addition & 0 deletions rust/lit-node/lit-node/tests/integration/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@ pub mod integration_tests;
pub mod lit_actions;
pub mod session_sigs;
pub mod shadow;
pub mod tracing;
253 changes: 253 additions & 0 deletions rust/lit-node/lit-node/tests/integration/tracing.rs
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;
Copy link

Copilot AI Dec 9, 2025

Choose a reason for hiding this comment

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

[nitpick] The variable _testnet is prefixed with an underscore but is actually used (it keeps the testnet alive through its lifetime). This is correct usage, but the underscore prefix is misleading.

Consider removing the underscore prefix:

let (testnet, validator_collection, end_user) = TestSetupBuilder::default().build().await;

Or add a comment explaining why the underscore is present if it's intentional to suppress warnings while still using the variable for its Drop behavior.

Suggested change
let (_testnet, validator_collection, end_user) = TestSetupBuilder::default().build().await;
let (testnet, validator_collection, end_user) = TestSetupBuilder::default().build().await;

Copilot uses AI. Check for mistakes.

let node_set = validator_collection.random_threshold_nodeset().await;
let node_set_map = get_identity_pubkeys_from_node_set(&node_set).await;

let realm_id = ethers::types::U256::from(1);
let epoch = validator_collection
.actions()
.get_current_epoch(realm_id)
.await
.as_u64();

// Read the lit action script
let lit_action_code =
std::fs::read_to_string("./tests/lit_action_scripts/broadcast_and_collect.js")
.expect("failed to read broadcast_and_collect.js");
let lit_action_code = data_encoding::BASE64.encode(lit_action_code.as_bytes());
let js_params = serde_json::Map::new();

// Generate session sigs
let session_sigs_and_node_set = get_session_sigs_for_auth(
&node_set_map,
vec![LitResourceAbilityRequest {
resource: LitResourceAbilityRequestResource {
resource: "*".to_string(),
resource_prefix: LitResourcePrefix::LA.to_string(),
},
ability: LitAbility::LitActionExecution.to_string(),
}],
Some(end_user.wallet.clone()),
None,
None,
);

// Get log readers for the nodes
let mut log_readers = validator_collection.log_readers();

// Test 1: Execute without privacy mode
info!("=== TEST 1: Executing lit action WITHOUT privacy mode ===");

// Get initial log positions
let initial_log_positions: Vec<u64> = log_readers
.iter_mut()
.map(|reader| reader.stream_position().unwrap_or(0))
.collect();

let execute_resp_no_privacy = execute_lit_action_session_sigs(
Some(lit_action_code.clone()),
None,
Some(serde_json::Value::Object(js_params.clone())),
None,
&session_sigs_and_node_set,
epoch,
)
.await
.expect("failed to execute lit action without privacy mode");

assert!(!execute_resp_no_privacy.is_empty());
assert!(execute_resp_no_privacy[0].ok);

// 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();

Comment on lines +84 to +96
Copy link

Copilot AI Dec 9, 2025

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:

  1. Using a retry loop with a timeout to wait for expected log conditions
  2. Adding a configurable or environment-based timeout
  3. Calling an explicit flush method if available on the log readers
Suggested change
// 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 uses AI. Check for mistakes.
info!("Logs without privacy mode: {} lines", logs_no_privacy.len());

// Verify we have some logs
assert!(
!logs_no_privacy.is_empty(),
"Should have logs without privacy mode"
);

// Check for some expected log content (like endpoint paths, execution info, etc.)
let has_endpoint_log = logs_no_privacy
.iter()
.any(|line| line.contains("/web/execute") || line.contains("POST /web/execute"));
assert!(has_endpoint_log, "Should have endpoint logs");

// Test 2: Execute WITH privacy mode using header
info!("=== TEST 2: Executing lit action WITH privacy mode (header) ===");

// Get log positions before privacy mode request
let log_positions_before_privacy: Vec<u64> = log_readers
.iter_mut()
.map(|reader| reader.stream_position().unwrap_or(0))
.collect();

// Execute with privacy mode using custom header
// We need to manually construct the request with the privacy mode header
let nodes: Vec<NodeSet> = session_sigs_and_node_set
.iter()
.map(|sig_and_nodeset| sig_and_nodeset.node.clone())
.collect();

let my_private_key = OsRng.r#gen();
let execute_request = lit_sdk::ExecuteFunctionRequest::new()
.url_prefix(lit_sdk::UrlPrefix::Http)
.add_custom_header("X-Privacy-Mode", "true")
.node_set(
session_sigs_and_node_set
.iter()
.map(|sig_and_nodeset| {
let execute_request = JsonExecutionRequest {
auth_sig: lit_node_core::AuthSigItem::Single(
sig_and_nodeset.session_sig.clone(),
),
code: Some(lit_action_code.clone()),
ipfs_id: None,
js_params: Some(serde_json::Value::Object(js_params.clone())),
auth_methods: None,
epoch,
node_set: nodes.clone(),
invocation: Invocation::Sync,
};
lit_sdk::EndpointRequest {
node_set: sig_and_nodeset.node.clone(),
identity_key: sig_and_nodeset.identity_key,
body: execute_request,
}
})
.collect::<Vec<_>>(),
)
.build()
.expect("failed to build execute request");

let execute_resp_privacy = execute_request
.send(&my_private_key)
.await
.expect("failed to execute lit action with privacy mode");

let results: Vec<GenericResponse<JsonExecutionResponse>> =
execute_resp_privacy.results().to_owned();
assert!(!results.is_empty());
assert!(results[0].ok);

// Wait a bit for logs to be written
tokio::time::sleep(tokio::time::Duration::from_millis(1000)).await;
Copy link

Copilot AI Dec 9, 2025

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.

Suggested change
tokio::time::sleep(tokio::time::Duration::from_millis(1000)).await;
wait_for_logs().await;

Copilot uses AI. Check for mistakes.

// Read new logs from all nodes
let logs_with_privacy: Vec<String> = log_readers
.iter_mut()
.enumerate()
.flat_map(|(idx, reader)| {
let start_pos = log_positions_before_privacy.get(idx).copied().unwrap_or(0);
read_logs_from_reader(reader, start_pos)
})
.collect();

info!("Logs with privacy mode: {} lines", logs_with_privacy.len());

// Verify we have the privacy mode endpoint log
let has_privacy_endpoint_log = logs_with_privacy.iter().any(|line| {
line.contains("privacy_mode_request")
|| (line.contains("method") && line.contains("path") && line.contains("POST"))
});
assert!(
has_privacy_endpoint_log,
"Should have privacy_mode_request log"
);
Comment on lines +183 to +191
Copy link

Copilot AI Dec 9, 2025

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 uses AI. Check for mistakes.

// Verify that detailed logs are filtered out
// Check that we don't have detailed execution logs that were present without privacy mode
let has_detailed_execution_logs = logs_with_privacy.iter().any(|line| {
// Look for logs that would contain sensitive execution details
// Exclude the privacy_mode_request log from this check
!line.contains("privacy_mode_request")
&& (line.contains("executing lit action")
|| line.contains("POST /web/execute/v2")
|| (line.contains("execute") && (line.contains("debug") || line.contains("trace"))))
});

// We should NOT have detailed execution logs when privacy mode is on
assert!(
!has_detailed_execution_logs,
"Should not have detailed execution logs when privacy mode is enabled"
);
Comment on lines +193 to +208
Copy link

Copilot AI Dec 9, 2025

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:

  1. Count total log lines/bytes generated (already done, which is good)
  2. Add positive assertions about what logs SHOULD be present (the privacy_mode_request log)
  3. 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 uses AI. Check for mistakes.

// Filter out privacy_mode_request logs for content comparison
let non_privacy_logs: Vec<_> = logs_with_privacy
.iter()
.filter(|line| !line.contains("privacy_mode_request"))
.collect();

info!("Non-privacy logs count: {}", non_privacy_logs.len());

// Compare: logs without privacy should have more content than logs with privacy
// (excluding the privacy_mode_request log)
let no_privacy_content: usize = logs_no_privacy.iter().map(|l| l.len()).sum();
let with_privacy_content: usize = non_privacy_logs.iter().map(|l| l.len()).sum();

info!("Content length without privacy: {}", no_privacy_content);
info!(
"Content length with privacy (excluding privacy_mode_request): {}",
with_privacy_content
);

assert!(
no_privacy_content > with_privacy_content,
"Logs with privacy mode should have less content than without privacy mode. \
No privacy: {}, With privacy: {}",
no_privacy_content,
with_privacy_content
);

info!("Privacy mode test completed successfully");
}

fn read_logs_from_reader(reader: &mut BufReader<File>, start_position: u64) -> Vec<String> {
// Seek to start position
if reader
.seek(std::io::SeekFrom::Start(start_position))
.is_err()
{
return Vec::new();
}
Comment on lines +241 to +247
Copy link

Copilot AI Dec 9, 2025

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:

  1. Propagating the error up: reader.seek(...)?;
  2. Logging a warning or using expect() with a descriptive message
  3. At minimum, documenting why silent failure is acceptable here
Suggested change
// 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 uses AI. Check for mistakes.

reader
.lines()
.map(|line| line.unwrap_or_default())
Copy link

Copilot AI Dec 9, 2025

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.

Suggested change
.map(|line| line.unwrap_or_default())
.map(|line| line.expect("Failed to read log line"))

Copilot uses AI. Check for mistakes.
.collect()
}
Loading