Skip to content

Conversation

@glitch003
Copy link
Contributor

@glitch003 glitch003 commented Nov 25, 2025

starting on a privacy_mode param that would suppress logs and tracing for just that single request. requested by Polaris / Osmosis

How it works

We added a "X-Privacy-Mode" header in https://github.com/LIT-Protocol/lit-peer/pull/17/files#diff-444d506d6b348808f56db7bb8fb7c677ee5f67b68bf3457a8c04c7c3630f6c04 which sets a variable to suppress logging and tracing for the duration of that rocket request, and only for that request.

If privacy mode is enabled, we don't get any metrics, not even on endpoint usage. So, if privacy mode is enabled, we also log the request method and path before turning off logging, so that we can at least track those basic metrics of how many times an endpoint was hit.

To enable this, we added a SubscriberExt layer let subscriber = subscriber.with(crate::utils::rocket::privacy_mode::PrivacyModeLayer); that will disable logging and tracing when privacy mode is set.

Tests

We have a test of this new header, but it only checks for logs. We probably need to manually check that the traces are excluded too.

@glitch003 glitch003 marked this pull request as ready for review November 26, 2025 00:49
@github-actions
Copy link

github-actions bot commented Nov 26, 2025

PASS [ 43.814s] (3/3) lit_node::test toxiproxy::perf_tests::load_with_no_latency
PASS [ 43.988s] (2/3) lit_node::test toxiproxy::perf_tests::load_with_50ms_latency_single_link
PASS [ 94.443s] (1/3) lit_node::test toxiproxy::perf_tests::load_with_50ms_latency_all_links


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

Copilot AI review requested due to automatic review settings December 9, 2025 18:11
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR implements a privacy mode feature that suppresses logs and tracing for individual requests when the X-Privacy-Mode: true header is provided. The feature is designed for privacy-sensitive use cases like Polaris/Osmosis. A basic privacy mode log is still emitted to track endpoint usage metrics (method and path only).

Key changes:

  • Added PrivacyModeLayer to filter tracing events/spans based on thread-local privacy flag
  • Implemented request fairing to detect privacy mode from headers/query params
  • Added integration test to verify log suppression

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 11 comments.

Show a summary per file
File Description
rust/lit-node/lit-node/src/utils/rocket/privacy_mode.rs New module implementing privacy mode via thread-local storage and tracing layer filter
rust/lit-node/lit-node/src/utils/rocket/mod.rs Exports the new privacy_mode module
rust/lit-node/lit-node/src/main.rs Integrates privacy mode fairing and tracing layer into the Rocket application
rust/lit-node/lit-node/tests/integration/tracing.rs New integration test verifying privacy mode suppresses logs while preserving basic metrics
rust/lit-node/lit-node/tests/integration/mod.rs Registers the new tracing test module

⚠️ Critical Issues Found: The implementation has serious concurrency bugs due to using thread-local storage in Rocket's async environment. Async tasks can migrate between threads, causing privacy mode flags to apply to wrong requests or persist incorrectly. Additionally, the privacy flag is never reset after request completion, causing state leakage between requests.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +241 to +247
// Seek to start position
if reader
.seek(std::io::SeekFrom::Start(start_position))
.is_err()
{
return Vec::new();
}
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.
Comment on lines +183 to +191
// 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"
);
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.
})
})
}

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.
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.

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.
Comment on lines +5 to +7
thread_local! {
static PRIVACY_MODE: std::cell::Cell<bool> = const { std::cell::Cell::new(false) };
}
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

Comment on lines +193 to +208
// 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"
);
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.
Comment on lines +26 to +29
/// Check if privacy mode is enabled for the current request
pub fn is_privacy_mode_enabled() -> bool {
PRIVACY_MODE.with(|cell| cell.get())
}
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.
Comment on lines +84 to +96
// 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();

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.
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.
@kapoorabhishek24 kapoorabhishek24 marked this pull request as draft December 31, 2025 12:18
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