fix(logging): sanitize message strings and migrate printToConsole to os.Logger#492
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughLogging now sanitizes message text and metadata via regex redaction, constructs LogEntry from sanitized content, and emits level-specific output through os.Logger; logging still occurs when registered destinations exist even if local and Sentry flags are false. Tests validate redaction and regression cases. Changes
Sequence Diagram(s)sequenceDiagram
participant App as "App / Caller"
participant SDKLogger as "SDKLogger"
participant Dest as "Registered Destinations"
participant OS as "os.Logger"
App->>SDKLogger: log(level, message, metadata)
SDKLogger->>SDKLogger: sanitizeMessage(message)\nsanitizeMetadata(metadata)
alt currentDestinations not empty
SDKLogger->>Dest: deliver(LogEntry(sanitized))
end
SDKLogger->>OS: emit(level, formattedOutput, privacy: .public)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swift (2)
299-307: Asymmetry between metadata and message keyword lists.
sensitivePatterns(line 299) flags metadata keys containing"auth"or"credential"as a substring, but the message regex only matchesauth[_\-]?keyandcredentialas a whole word. As a result, a field likemetadata["authorization"]is redacted, while the same substring interpolated into a message ("authorization=...") is not. Consider extracting a single source of truth (e.g.Bearer|Basic|api[_\-]?key|apikey|secret|password|token|auth\w*|credential\w*) so the two paths stay in sync as new keywords are added.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swift` around lines 299 - 307, The message-redaction regex and the metadata keyword list are out of sync: update the implementation so both use a single source of truth (e.g., derive the message regex from the sensitivePatterns array or extract a shared constant) and expand the message pattern to cover substring variants like auth\w* and credential\w* as well as HTTP schemes (Bearer|Basic); specifically, change the creation of sensitiveMessagePattern to build its alternation from the sensitivePatterns symbols (or a shared keywords constant) ensuring tokens like "authorization" and "credentialX" match the regex in SDKLogger (referencing sensitivePatterns and sensitiveMessagePattern).
284-294: LGTM — privacy marker is sound given upstream sanitization.Routing through
os.Loggerwithprivacy: .publicis acceptable here because bothsanitizeMessageandsanitizeMetadatarun before theoutputstring is built. Worth noting in passing that thesubsystemis a hard-coded"com.runanywhere.sdk"string — if you already expose a bundle identifier or SDK constant, reusing it would avoid drift. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swift` around lines 284 - 294, Replace the hard-coded subsystem string "com.runanywhere.sdk" used when constructing the Logger in SDKLogger.swift with a shared bundle identifier or SDK constant (e.g., use your existing SDK constant or Bundle.main.bundleIdentifier) so the subsystem stays in sync; update the Logger instantiation in the logging path that builds `osLog` (where `osLog = Logger(subsystem: "com.runanywhere.sdk", category: entry.category)`) to reference that constant instead, leaving the rest of the sanitized output handling (sanitizeMessage/sanitizeMetadata and the switch over entry.level) unchanged.sdk/runanywhere-swift/Tests/RunAnywhereTests/SecurityLoggingTests.swift (1)
102-111: Missing regression coverage for false positives.These two cases verify that messages with no sensitive keywords pass through untouched, but they don't exercise messages that contain a sensitive keyword followed by a benign word (e.g.
"password required","rotate your token soon","Basic authentication disabled"). Given the current regex behavior flagged onSDKLogger.swiftlines 303–307, such messages will be over-redacted. Adding a test here would lock in the intended behavior once the regex is tightened.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-swift/Tests/RunAnywhereTests/SecurityLoggingTests.swift` around lines 102 - 111, Add regression tests that verify false positives aren't over-redacted by the current regex: in SecurityLoggingTests.swift add a test (e.g., testSensitiveKeywordsInBenignContextPassThrough) that calls Logging.shared.log(level: .info, category: "Test", message: ...) with examples like "password required", "rotate your token soon", and "Basic authentication disabled" and assert destination.captured.first?.message equals the original string; this ensures SDKLogger.swift's redaction logic (lines around the regex) doesn't redact these benign contexts.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swift`:
- Around line 303-318: The regex in sensitiveMessagePattern is over-broad and
treats whitespace-only separators as credential separators, causing false
redactions; update the NSRegularExpression in sensitiveMessagePattern to use two
alternations: one that matches generic keywords
(api[_\-]?key|apikey|secret|password|token|auth[_\-]?key|credential) only when
followed by an explicit separator like '=' or ':' (e.g., (\s*[=:]\s*)(\S+)), and
a second alternation that matches Bearer|Basic followed by whitespace and a
token (e.g., (?i)(Bearer|Basic)\s+(\S+)); keep sanitizeMessage using the same
replacement but ensure templates target the correct capture groups, and add
regression tests validating benign phrases like "password required", "Please
rotate your token soon", "Basic authentication disabled", and "Check the secret
menu" remain unredacted while credential forms like "password=secret" and
"Bearer abc123" are redacted.
In `@sdk/runanywhere-swift/Tests/RunAnywhereTests/SecurityLoggingTests.swift`:
- Around line 35-50: Snapshot Logging.shared.configuration at the start of
setUp, then apply the test-specific config and add the CapturingDestination as
you already do; in tearDown, remove the destination and restore the previously
saved configuration back onto Logging.shared (i.e., call
Logging.shared.configure(originalConfig)) so the process-wide singleton is
returned to its prior state; keep using the existing
destination/CapturingDestination and ensure restoration happens before calling
super.tearDown().
---
Nitpick comments:
In
`@sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swift`:
- Around line 299-307: The message-redaction regex and the metadata keyword list
are out of sync: update the implementation so both use a single source of truth
(e.g., derive the message regex from the sensitivePatterns array or extract a
shared constant) and expand the message pattern to cover substring variants like
auth\w* and credential\w* as well as HTTP schemes (Bearer|Basic); specifically,
change the creation of sensitiveMessagePattern to build its alternation from the
sensitivePatterns symbols (or a shared keywords constant) ensuring tokens like
"authorization" and "credentialX" match the regex in SDKLogger (referencing
sensitivePatterns and sensitiveMessagePattern).
- Around line 284-294: Replace the hard-coded subsystem string
"com.runanywhere.sdk" used when constructing the Logger in SDKLogger.swift with
a shared bundle identifier or SDK constant (e.g., use your existing SDK constant
or Bundle.main.bundleIdentifier) so the subsystem stays in sync; update the
Logger instantiation in the logging path that builds `osLog` (where `osLog =
Logger(subsystem: "com.runanywhere.sdk", category: entry.category)`) to
reference that constant instead, leaving the rest of the sanitized output
handling (sanitizeMessage/sanitizeMetadata and the switch over entry.level)
unchanged.
In `@sdk/runanywhere-swift/Tests/RunAnywhereTests/SecurityLoggingTests.swift`:
- Around line 102-111: Add regression tests that verify false positives aren't
over-redacted by the current regex: in SecurityLoggingTests.swift add a test
(e.g., testSensitiveKeywordsInBenignContextPassThrough) that calls
Logging.shared.log(level: .info, category: "Test", message: ...) with examples
like "password required", "rotate your token soon", and "Basic authentication
disabled" and assert destination.captured.first?.message equals the original
string; this ensures SDKLogger.swift's redaction logic (lines around the regex)
doesn't redact these benign contexts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82c16e8c-087e-45d0-b6ab-24f50c6d7db1
📒 Files selected for processing (2)
sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swiftsdk/runanywhere-swift/Tests/RunAnywhereTests/SecurityLoggingTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@sdk/runanywhere-swift/Tests/RunAnywhereTests/SecurityLoggingTests.swift`:
- Around line 124-145: The three tests testTokenCountPhraseIsNotRedacted,
testPasswordRequiredPhraseIsNotRedacted, and testBasicEnglishPhraseIsNotRedacted
currently use a nil-coalescing boolean check that silently passes if no log was
captured; change each assertion to assert the captured message equals the
original msg (e.g., use XCTAssertEqual(destination.captured.first?.message,
msg)) so the test both verifies a log entry was captured and that its content
was not redacted; update the assertions in those test methods accordingly
referencing destination.captured and the local msg variable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d05eaf45-5b3d-4624-961e-ff4ece5ddf7b
📒 Files selected for processing (2)
sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swiftsdk/runanywhere-swift/Tests/RunAnywhereTests/SecurityLoggingTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swift
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
Bugbot Autofix is ON, but it could not run because the branch was deleted or merged before autofix could start.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 19b12e2. Configure here.
…r false-positive tests, add authorization keyword
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swift`:
- Around line 310-326: The sanitization patterns in SDKLogger.swift are ordered
so the generic key=value regex (containing "authorization") runs before the
Bearer-specific regex, causing "Bearer" to be consumed and the token left
unredacted; fix by reordering sensitiveMessagePatterns so the Bearer pattern
(the regex matching "(?i)(bearer)(\s+)([A-Za-z0-9+/=._\-]{8,})") comes before
the generic key/value pattern (the one containing "authorization") or
alternatively remove "authorization" from the first pattern; update tests (e.g.,
testBearerTokenInMessageIsRedacted) to include cases "Authorization: Bearer …"
and mixed-case scenarios to prevent regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 19e0fab2-f68a-4970-b983-740a302202e0
📒 Files selected for processing (2)
sdk/runanywhere-swift/Sources/RunAnywhere/Infrastructure/Logging/SDKLogger.swiftsdk/runanywhere-swift/Tests/RunAnywhereTests/SecurityLoggingTests.swift
|
@Siddhesh2377 - please have a look when you get time |
|
@sanchitmonga22 - please review when you get chance and let me know if any changes needed. |
|
hey @ManthanNimodiya we are in the middle of a Repo-Refactor, I will review Your changes shortly and merge them once we are done with the refactor okay |
2 participants
Summary
The logging system was sanitizing metadata keys (e.g.
["apiKey": "..."]) butletting the
messagestring through raw. So any call like:would print the key straight to stdout. GitHub Advanced Security flagged this
exact taint path (
message→print()), and it's been open since December.Also noticed
import oswas already at the top of the file butos.Loggerwas never actually used — we were just calling
print(). Fixed that too.Root Cause
Logging.log()calledsanitizeMetadata()on the metadata dict but passedmessageintoLogEntryuntouched.printToConsole()then wrote it outverbatim via
print(). ThelogError()helper had the same gap — it buildsa message string from
error.localizedDescriptionandadditionalInfo, bothof which could carry credential values.
Changes
SDKLogger.swift— AddedsanitizeMessage(_:)that usesNSRegularExpressionto redact values after credential-like keywords(
apiKey=,password:,Bearer <token>, etc.). Hooked it intolog()right alongside the existing
sanitizeMetadata()call, so every path —console and Sentry destinations — gets a clean entry. Swapped
print()foros.Loggerwith per-category subsystems; output is marked.publicbecausethe message is already sanitized before it gets there.
SecurityLoggingTests.swift— 12 new tests:key=valueandkey: valuepatterns, Bearer token scheme, metadata regression,
logErrorwith sensitiveadditionalInfo, and a check that normal messages aren't touched.Testing
All 12 new cases pass.
AudioCaptureManagerTestsunaffected.Checklist
SecurityLoggingTests.swift, 12 cases)sanitizeMetadatapattern, just extended to messagesNotes
instead of string interpolation — that's a broader refactor worth a separate PR.
Summary by CodeRabbit
Security Improvements
Logging
Tests
Greptile Summary
This PR adds
sanitizeMessage(_:)to redact credential values embedded in log message strings, complementing the existingsanitizeMetadata(), and migratesprintToConsolefromprint()toos.Logger. The approach is sound, but there is a critical test infrastructure bug and a regex false-positive issue that need to be addressed before merging.setUp, bothenableLocalLoggingandenableSentryLoggingare set tofalse, triggering an earlyreturninlog()before any destination is written. AllXCTAssertTrue(msg.contains(\"[REDACTED]\"))assertions operate on an empty string and would fail; the 12 tests don't actually verify sanitization works.[=:\\s]accepts a bare space, so\"token count: 5\"→\"token [REDACTED] 5\". Thebasickeyword exacerbates this with common English prose.Confidence Score: 4/5
Not safe to merge as-is — the test suite has a structural bug that means none of the 12 new sanitization tests actually verify the fix works.
The production sanitization logic is correctly wired and the os.Logger migration is clean. However, the P1 test guard bypass means the PR's stated test coverage is illusory — merging leaves the security fix unverified and masks future regressions.
SecurityLoggingTests.swift — all 12 tests structurally broken by the early-return guard. SDKLogger.swift regex pattern needs narrowing to avoid false-positive redaction.
Important Files Changed
sanitizeMessage(_:)using NSRegularExpression for credential pattern redaction and migratesprintToConsolefromprint()toos.Logger. The regex separator pattern[=:\s]is overly broad andbasicis too generic a keyword, causing false-positive redaction.CapturingDestination, but all are structurally broken: setting bothenableLocalLogging = falseandenableSentryLogging = falsein setUp causeslog()to early-return before writing to any destination, so sanitization assertions never actually fire.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A["log(level:category:message:metadata:)"] --> B{level >= minLogLevel?} B -- No --> Z[return] B -- Yes --> C{enableLocalLogging\nOR enableSentryLogging?} C -- No --> Z2["return ⚠️ CapturingDestination\nnever reached in tests"] C -- Yes --> D["sanitizeMessage(message)\n+ sanitizeMetadata(metadata)"] D --> E[Create LogEntry] E --> F{enableLocalLogging?} F -- Yes --> G["printToConsole(entry)\nos.Logger .public"] F -- No --> H G --> H[Write to registered destinations\nCapturingDestination / SentryDestination]Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix(logging): sanitize message strings a..." | Re-trigger Greptile
Note
Medium Risk
Touches core logging behavior and redaction logic; mistakes could either leak sensitive data or over-redact/alter log output, and console logging now depends on
os.Loggerbehavior.Overview
Prevents credential leaks via log messages.
Logging.log()now sanitizes themessagestring (in addition to existing metadata sanitization) by redacting common secret patterns (e.g.,apiKey=...,password: ...,Bearer ...).Changes console logging implementation.
printToConsole()is migrated fromprint()toos.Loggerwith a fixed subsystem and per-category logging, andlog()will now proceed when custom destinations are registered even if both local and Sentry logging are disabled.Adds regression coverage. Introduces
SecurityLoggingTeststo verify message/metadata redaction, avoid false positives, and ensureSDKLogger.logError(...additionalInfo:)also gets sanitized.Reviewed by Cursor Bugbot for commit 19b12e2. Configure here.