fix(input): Sensitive input sequences are logged in plaintext#659
Conversation
API handlers for keyboard/gamepad input log full parsed input arguments (`keys` / `buttons`). These values can include sensitive sequences (e.g., typed credentials or private control patterns), which then persist in logs and may be exposed through log collection, support bundles, or unauthorized log access. Signed-off-by: tomaioo <203048277+tomaioo@users.noreply.github.com>
📝 WalkthroughWalkthroughAdjusts logging across the codebase to redact or reduce sensitive/request payloads: replaces detailed payload logs with counts or sanitized values, introduces URL redaction, marks certain ZapScript commands as sensitive, and adds tests for sensitivity and URL redaction. Changes
Sequence Diagram(s)(none) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| } | ||
|
|
||
| log.Info().Strs("keys", args).Msg("keyboard input via API") | ||
| log.Info().Int("key_count", len(args)).Msg("keyboard input via API") |
There was a problem hiding this comment.
Bug: The security fix is incomplete. Sensitive keyboard and gamepad inputs are still logged in plaintext via the ZapScript command handlers in pkg/zapscript/input.go.
Severity: HIGH
Suggested Fix
Apply the same logging fix to the ZapScript handlers in pkg/zapscript/input.go. Modify the log.Info().Msgf calls in cmdKeyboard and cmdGamepad to log only the count of arguments, not the raw arguments themselves, similar to the changes made in pkg/api/methods/input.go.
Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: pkg/api/methods/input.go#L56
Potential issue: The pull request aims to prevent logging of sensitive keyboard and
gamepad inputs. While it addresses this in the API handler path
(`pkg/api/methods/input.go`), it fails to fix the same issue in the ZapScript command
handler path. The `cmdKeyboard` and `cmdGamepad` functions in `pkg/zapscript/input.go`
still log the full, potentially sensitive, input arguments in plaintext. This logging
occurs during trusted execution scenarios, such as running a locally stored ZapScript
card, where the `env.Unsafe` flag is false.
Did we get this right? 👍 / 👎 to inform future reviews.
wizzomafizzo
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/api/methods/input.go (1)
56-56: Security fix correctly implemented.The change to log only
key_countinstead of the actual key values effectively prevents sensitive input sequences (credentials, private patterns, etc.) from being persisted in logs. The implementation is correct and the field name is descriptive.Consider adding debug-level logging for troubleshooting.
While the security fix is appropriate for production, having no visibility into actual input sequences makes troubleshooting difficult. As mentioned in the PR objectives, consider implementing a debug-only gated log path that requires explicit opt-in:
🔍 Suggested debug logging enhancement
- log.Info().Int("key_count", len(args)).Msg("keyboard input via API") + logger := log.Info().Int("key_count", len(args)) + if log.Debug().Enabled() { + logger = logger.Strs("keys_debug", args) + } + logger.Msg("keyboard input via API")This would only expose sensitive data when debug logging is explicitly enabled, balancing security with observability.
Consider adding tests for logging behavior.
The existing tests (see
input_test.go:37-83) validate platform method calls but don't verify logging behavior. Adding logging assertions would help ensure this security fix doesn't regress.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/api/methods/input.go` at line 56, Replace the unconditional Info log with a gated debug path: keep log.Info().Int("key_count", len(args)).Msg("keyboard input via API") for normal runs, and add a conditional branch that, when a debug opt-in is enabled (e.g., an env var or a new helper like enableDebugInputLogging()), logs the actual args via log.Debug().Strs("keys", args).Msg("keyboard input (debug)") so sensitive values are only emitted when explicitly enabled; update or add tests in input_test.go to assert that with debug opt-in the logger receives the full args payload and without it only key_count is logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/api/methods/input.go`:
- Line 56: Replace the unconditional Info log with a gated debug path: keep
log.Info().Int("key_count", len(args)).Msg("keyboard input via API") for normal
runs, and add a conditional branch that, when a debug opt-in is enabled (e.g.,
an env var or a new helper like enableDebugInputLogging()), logs the actual args
via log.Debug().Strs("keys", args).Msg("keyboard input (debug)") so sensitive
values are only emitted when explicitly enabled; update or add tests in
input_test.go to assert that with debug opt-in the logger receives the full args
payload and without it only key_count is logged.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
Extend the sensitive input logging fix to cover the rest of the codebase. Strip query params and userinfo from HTTP command URLs in error messages and logs. Redact args for sensitive commands (http.*, input.*, execute) in the command dispatcher. Move NFC tag text and token content from INFO to DEBUG level. Stop logging full request params in the API server. Add tests for redactURL and isSensitiveCommand.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/zapscript/http.go (1)
36-36: Comment should reflect full redaction behavior.Line 36 says query params are stripped, but the function also clears
Userinfo. Please update the comment to match implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zapscript/http.go` at line 36, Update the function comment for redactURL to accurately describe its full behavior: note that it strips query parameters and also clears the URL.User (user info) for safe logging. Edit the comment immediately above the redactURL function (referencing redactURL) to mention both query param removal and user-info redaction so the doc matches the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/zapscript/http.go`:
- Line 36: Update the function comment for redactURL to accurately describe its
full behavior: note that it strips query parameters and also clears the URL.User
(user info) for safe logging. Edit the comment immediately above the redactURL
function (referencing redactURL) to mention both query param removal and
user-info redaction so the doc matches the implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 345d3761-ef2c-4579-9200-593de6ea505e
📒 Files selected for processing (11)
pkg/api/methods/run.gopkg/api/server.gopkg/api/server_logging_test.gopkg/readers/libnfc/libnfc.gopkg/readers/pn532/pn532.gopkg/service/queues.gopkg/zapscript/commands.gopkg/zapscript/commands_test.gopkg/zapscript/http.gopkg/zapscript/http_test.gopkg/zapscript/input.go
✅ Files skipped from review due to trivial changes (1)
- pkg/zapscript/input.go
2 participants
Summary
Security: Sensitive input sequences are logged in plaintext
Problem
Severity:
Medium| File:pkg/api/methods/input.go:L47API handlers for keyboard/gamepad input log full parsed input arguments (
keys/buttons). These values can include sensitive sequences (e.g., typed credentials or private control patterns), which then persist in logs and may be exposed through log collection, support bundles, or unauthorized log access.Solution
Avoid logging raw input payloads. Log only high-level metadata (e.g., action type, argument count, request ID) or redact/hash values. Consider a debug-only gated log path with explicit opt-in.
Changes
pkg/api/methods/input.go(modified)Summary by CodeRabbit
Chores
Tests
Note: No functional or user-facing behavior was changed.