fix(dictation): redact API keys from streaming transcriber errors#710
fix(dictation): redact API keys from streaming transcriber errors#710euxaristia wants to merge 6 commits into
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughDeepgram and OpenAI Realtime transcribers now redact configured API keys from provider errors while preserving ChangesTranscriber error handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/dictation/transcriber_openai_realtime.go (2)
156-161: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHandle the write error instead of discarding it.
If a non-nil
werris pulled fromwriteErrChhere, it is silently discarded. While the subsequentconn.Readwill likely fail and return an error anyway, the original write error context is lost. Return the redacted write error directly to correctly report the failure.🐛 Proposed fix
select { case werr := <-writeErrCh: if werr == nil { committed = true + } else { + return compose(), fmt.Errorf("OpenAI Realtime stream error: %s", providerio.Redact(werr.Error(), o.cfg.APIKey)) } default: }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dictation/transcriber_openai_realtime.go` around lines 156 - 161, Update the writeErrCh handling in the surrounding transcription flow so a non-nil werr is redacted and returned immediately, preserving the original write failure context; retain the committed = true behavior when werr is nil and the existing default path.
77-79: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact API key in session configuration error.
If the server immediately closes the connection with an error message (such as an invalid key rejection) before or during the
writeJSONcall, the returned WebSocket close error will contain the server's reason. Since this error is currently wrapped with%wrather than redacted, the API key could leak here. Applyproviderio.Redactto ensure it is sanitized.🔒️ Proposed fix
- if err := writeJSON(ctx, conn, sessionUpdate); err != nil { - return "", fmt.Errorf("configuring OpenAI Realtime session: %w", err) + if err := writeJSON(ctx, conn, sessionUpdate); err != nil { + return "", fmt.Errorf("configuring OpenAI Realtime session: %s", providerio.Redact(err.Error(), o.cfg.APIKey)) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dictation/transcriber_openai_realtime.go` around lines 77 - 79, Update the writeJSON error handling in the session configuration flow to apply providerio.Redact to the error before wrapping and returning it. Preserve the existing “configuring OpenAI Realtime session” context while ensuring server close reasons cannot expose the API key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/dictation/transcriber_stream_test.go`:
- Around line 233-286: Move TestOpenAIRealtimeStreamTranscribeErrorRedaction
into transcriber_openai_realtime_test.go and move
TestDeepgramStreamTranscribeErrorRedaction into transcriber_deepgram_test.go.
Remove both test functions from transcriber_stream_test.go without changing
their assertions or behavior.
---
Outside diff comments:
In `@internal/dictation/transcriber_openai_realtime.go`:
- Around line 156-161: Update the writeErrCh handling in the surrounding
transcription flow so a non-nil werr is redacted and returned immediately,
preserving the original write failure context; retain the committed = true
behavior when werr is nil and the existing default path.
- Around line 77-79: Update the writeJSON error handling in the session
configuration flow to apply providerio.Redact to the error before wrapping and
returning it. Preserve the existing “configuring OpenAI Realtime session”
context while ensuring server close reasons cannot expose the API key.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 921826ef-803f-4f87-90a7-f4c1744d5429
📒 Files selected for processing (3)
internal/dictation/transcriber_deepgram.gointernal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_stream_test.go
…edaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs Gitlawb#710
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/dictation/transcriber_deepgram_test.go`:
- Around line 12-25: Update the wsTestServer callback in the Deepgram test to
wait for the client’s expected messages or otherwise synchronize on CloseStream
before closing the connection. Ensure StreamTranscribe observes the
API-key-bearing policy-violation close reason instead of failing earlier on a
write error, while preserving the existing invalid-key scenario.
🪄 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: 2948fe8f-3e13-4fe3-9e34-e309792ae886
📒 Files selected for processing (3)
internal/dictation/transcriber_deepgram_test.gointernal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_openai_realtime_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/dictation/transcriber_openai_realtime.go
…edaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs Gitlawb#710
5b9b294 to
b024094
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/dictation/transcriber_openai_realtime.go (1)
63-63: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover every redaction path with regression tests.
The supplied test exercises only a server
realtimeErrorevent. Please add or verify focused tests for connection failures, initial session-write failures, WebSocket read failures, and writer/commit failures so future changes cannot reintroduce API-key leakage in the other newly sanitized paths.Also applies to: 78-78, 117-117, 151-160
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/dictation/transcriber_openai_realtime.go` at line 63, Expand the tests for the OpenAI Realtime transcriber to cover API-key redaction in connection failures, initial session-write failures, WebSocket read failures, and writer/commit failures, alongside the existing realtimeError event case. Exercise each error path and assert the configured API key never appears in the returned or logged error while preserving the relevant failure behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/dictation/transcriber_openai_realtime.go`:
- Line 63: Expand the tests for the OpenAI Realtime transcriber to cover API-key
redaction in connection failures, initial session-write failures, WebSocket read
failures, and writer/commit failures, alongside the existing realtimeError event
case. Exercise each error path and assert the configured API key never appears
in the returned or logged error while preserving the relevant failure behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 025a5a81-3924-4707-9e18-44a20686732b
📒 Files selected for processing (4)
internal/dictation/transcriber_deepgram.gointernal/dictation/transcriber_deepgram_test.gointernal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_openai_realtime_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/dictation/transcriber_openai_realtime_test.go
- internal/dictation/transcriber_deepgram_test.go
- internal/dictation/transcriber_deepgram.go
…close Address CodeRabbit nitpick: the server now drains the client's audio frames and waits for CloseStream before rejecting the connection with an API-key-bearing policy-violation close reason. This ensures StreamTranscribe observes the close reason (and redacts it) instead of failing earlier on a write error. Refs Gitlawb#710
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes — the redaction itself is correct and well-scoped, but the %w → %s switch has a real side effect: it breaks cancellation detection for streaming dictation.
The issue: Esc-abort of streaming dictation now shows a spurious error toast
internal/tui/dictation.go:395 tells a user abort from a real failure with errors.Is(msg.err, context.Canceled). On Esc, cancelDictation() cancels the streaming ctx while StreamTranscribe is blocked in conn.Read(ctx). coder/websocket then returns ctx.Err() — i.e. context.Canceled itself (its finishRead swaps the read error for ctx.Err() on a canceled context).
- Before this PR:
fmt.Errorf("Deepgram stream error: %w", err)kept that in the unwrap chain, soerrors.Is(msg.err, context.Canceled)was true → the abort was treated as a clean interrupt, no toast. - After:
fmt.Errorf("...: %s", providerio.Redact(err.Error(), key))is a flat string"Deepgram stream error: context canceled"with no unwrap chain →errors.Isis false → the handler falls through toappendDictationNotice(...)and shows a "dictation error" toast.cancelDictationalso posts "Dictation cancelled.", so the user sees both notices on one Esc.
The normal stop path (release/stop gesture) is fine — stopDictation() only closes the chunk channel, the server closes cleanly (StatusNormalClosure), and StreamTranscribe returns nil. So this only bites the Esc-abort path — but that's a common gesture, and it regresses behaviour that worked before.
It also makes the providers inconsistent: transcriber_local_stream.go (sherpa) still uses %w, so an Esc-abort on a local streaming session stays clean, while Deepgram / OpenAI Realtime now toast. Same gesture, three providers, two behaviours.
Suggested fix
Short-circuit the cancel signal before redacting — context.Canceled carries no key, so it's safe to return un-redacted:
if websocket.CloseStatus(err) == websocket.StatusNormalClosure {
return compose(), nil
}
select {
case werr := <-writeErrCh:
if werr != nil {
err = werr
}
default:
}
if errors.Is(err, context.Canceled) {
return compose(), ctx.Err()
}
return compose(), fmt.Errorf("Deepgram stream error: %s", providerio.Redact(err.Error(), d.cfg.APIKey))…at both transcriber_deepgram.go:104 and transcriber_openai_realtime.go:117. The single site covers both the Read-error and the writeErrCh-overwrite, since the overwrite happens before the return. That keeps redaction for genuine errors and restores the abort signal so dictation.go:395 matches again. A regression test that cancels the streaming ctx mid-transcribe and asserts errors.Is(err, context.Canceled) on the returned error would lock it in — the two new tests only cover redaction, not the cancel path.
What's good
- Redaction is right and complete for the streaming paths: connect, session-config, read-error, server
errorevent, and thewriteErrChwrite error (the fixup that added theelsearound line 159 closes the last gap). The close-reason vector is real —coder/websocket'sCloseError.Error()is"status = %v and reason = %q", so a server close reason containing the key flows into the error string, andRedactscrubs it. Verified the Deepgram test would fail withoutRedactand isn't flaky as landed (the wait-for-CloseStream-before-close dance in the last fixup is load-bearing). - Switching to
%sis the correct security call beyond the display string: under%w, a caller coulderrors.As(err, &websocket.CloseError{})and pull the rawReason(the key) back out viaUnwrap.%scloses that. The cancel-signal branch above is the only thing the switch cost. - Keys live in headers (
Authorization: Token/Bearer), not the URLs —buildURL's query is encoding/model/language only — so dial errors don't echo the key to begin with;Redactat the dial sites is belt-and-suspenders. - Tests use a fake key, no real secrets, no new outbound headers, no provider-preset or trust-anchor changes. Clean security-only scope.
Nits (non-blocking)
transcriber_cloud.go:76(%s transcription request failed: %w, unredacted transport error) isn't a real leak — the key is in theAuthorizationheader and Go's http transport error echoes the URL (no key), never the header — but redacting it for parity with lines 81/99 would be consistent. Optional.providerio.Redactdoesstrings.Fields/Join, which collapses whitespace — a multi-line server error body gets flattened to one line. Cosmetic; only matters if a caller parses the structure.
CI is green across all three OSes; the only thing in the way is the behavioural side effect above.
|
To be clear on the request-changes above the one thing actually blocking me is the On Esc, Easiest fix: branch |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
- [P1] Preserve cancellation identity after redacting stream errors
internal/dictation/transcriber_deepgram.go:104andinternal/dictation/transcriber_openai_realtime.go:117
These paths now format theconn.Read(ctx)failure with%s, so an Esc cancellation becomes only a string rather than an error matchingcontext.Canceled.cancelDictationcancels that same streaming context, whilehandleDictationTranscribedsuppresses the expected abort only whenerrors.Is(msg.err, context.Canceled)succeeds. As a result, canceling an active Deepgram or OpenAI Realtime dictation shows a spurious provider-failure notice after “Dictation cancelled.” Preserve the cancellation sentinel before rendering the redacted error (or use a redacting wrapper that still unwraps it), and cover the canceled-stream path with a regression test.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/dictation/transcriber_openai_realtime_test.go`:
- Around line 75-80: Update the test around StreamTranscribe and firstFrame so
it waits with a select for either the first-frame signal or an error from errCh.
Fail immediately with the received error if StreamTranscribe exits before
signaling firstFrame, while preserving the existing cancellation and
context.Canceled assertion for the successful startup path.
- Around line 66-68: Add a deferred close for the chunks channel in the
StreamTranscribe test after creating it, preserving the open-channel behavior
during the abort while ensuring the writer goroutine blocked on ranging over
chunks can exit when the test completes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 964956a4-458d-4f8c-b3f4-32a1a2052e3d
📒 Files selected for processing (4)
internal/dictation/transcriber_deepgram.gointernal/dictation/transcriber_deepgram_test.gointernal/dictation/transcriber_openai_realtime.gointernal/dictation/transcriber_openai_realtime_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/dictation/transcriber_deepgram_test.go
- internal/dictation/transcriber_deepgram.go
- internal/dictation/transcriber_openai_realtime.go
|
Pushed 1b0f029 for the cancellation-identity finding: both transcribers now short-circuit before redaction when errors.Is(err, context.Canceled) and return the sentinel itself (it carries no key), so the Esc-abort path matches the TUI's errors.Is check again while genuine failures keep the redacted flat string. Added regression tests that cancel a live stream mid-transcribe and assert the sentinel survives, for both Deepgram and OpenAI Realtime. (The redaction tests were already moved to their per-source test files in an earlier push.) |
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Preserve cancellation identity on all setup/write error paths
internal/dictation/transcriber_deepgram.go:65,internal/dictation/transcriber_openai_realtime.go:64,79,167
The new%sformatting correctly prevents the raw transport error from being unwrapped, but the cancellation special case only runs afterconn.Read. Esc can cancel the streaming context while either WebSocket dial is pending, while OpenAI sendstranscription_session.update, or when its writer error is selected after an event. Those context-aware calls can returncontext.Canceled; flattening them causeserrors.Is(msg.err, context.Canceled)in the TUI to fail, so a normal early abort displays a dictation-error notice after “Dictation cancelled.” Preserve/returnctx.Err()for cancellation before redacting the genuine-error paths, and add startup/write cancellation coverage. -
[P3] Make the new cancellation tests terminate their helper goroutines and fail promptly
internal/dictation/transcriber_deepgram_test.go:69,internal/dictation/transcriber_openai_realtime_test.go:66
Both tests intentionally keepchunksopen but never close it. Once the reader returns aftercancel(), the production writer remains blocked forever in itsfor range chunks, leaking a goroutine per test. They also wait unconditionally onfirstFrame, so any dial/session/write regression that exits before the first frame makes the suite hang until its outer timeout instead of reportingerrCh. Closechunksduring cleanup and select betweenfirstFrameand an early result fromerrCh; this also addresses the currently unresolved CodeRabbit requests for the OpenAI test, and the identical Deepgram test needs the same treatment.
|
Pushed fea74d9 addressing the remaining review feedback. Fixed:
Not fixing, with reasons:
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Re-verified on the current head. The blocking one is fixed: the switch that was swallowing the cancel signal is resolved. Both streaming read paths now return ctx.Err() before the redacted branch, so an Esc-abort no longer fires a spurious dictation-error toast alongside "Dictation cancelled." You went past what I asked and covered the dial, session-update, and post-event paths too, and the two new cancel-path tests would fail without the short-circuit. Redaction still covers every genuine-error path, so nothing leaked back in. CI is green on all three OSes.
My two remaining notes were explicitly non-blocking nits (the out-of-scope non-streaming cloud path, and the whitespace-collapse in Redact), so they don't hold this up. Approving.
|
@jatmn Ready for re-review on head fea74d9. That commit covers the remaining cancel-identity paths (dial, session update, read, writeErrCh) via ctx.Err() before redaction, and the CancelKeepsSentinel tests now close chunks and select on firstFrame vs errCh so they fail promptly without leaking the writer goroutine. |
Streaming dictation (Deepgram, OpenAI Realtime) injected API keys into websocket URLs/headers and echoed unredacted transport errors and server-side error payloads. Wrap those error endpoints with providerio.Redact and switch the connect-error format specifier from %w to %s so the raw error cannot leak via errors.Unwrap(). Adds regression tests asserting the API key is not present in the returned stream error for both transcribers. Co-authored-by: euxaristia <25621994+euxaristia@users.noreply.github.com>
…edaction tests - Redact API key in OpenAI Realtime session configuration error. - Return redacted OpenAI Realtime write error from writeErrCh instead of discarding. - Move TestOpenAIRealtimeStreamTranscribeErrorRedaction and TestDeepgramStreamTranscribeErrorRedaction next to their source files. Refs Gitlawb#710
…close Address CodeRabbit nitpick: the server now drains the client's audio frames and waits for CloseStream before rejecting the connection with an API-key-bearing policy-violation close reason. This ensures StreamTranscribe observes the close reason (and redacts it) instead of failing earlier on a write error. Refs Gitlawb#710
…action Formatting the read error with %s dropped the unwrap chain, so an Esc abort of a Deepgram or OpenAI Realtime dictation no longer matched errors.Is(msg.err, context.Canceled) in the TUI and produced a spurious error toast on top of the cancellation notice. Return the cancellation sentinel itself before redacting — it carries no key — and keep the redacted flat string for genuine failures. Adds regression tests that cancel a live stream and assert the sentinel survives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Key cancellation detection on ctx.Err() instead of errors.Is(err, context.Canceled), which was racing against writer-goroutine transport errors that could overwrite the error value right before the check. Applied consistently across both transcriber implementations. Fix goroutine leaks and a possible hang in the cancellation regression tests.
Add regression tests for Esc-abort during WebSocket dial and right after accept (session update / first write) so those redaction sites keep returning context.Canceled for the TUI errors.Is check.
3a19432
fea74d9 to
3a19432
Compare
|
Ready for re-review on head 3a19432 (rebased onto current main). Addressed jatmn P2/P3 from the 2026-07-19 review: P2 - cancellation identity on all setup/write paths (in af37da2, landed earlier as fea74d9):
P3 - cancellation tests (same commit):
Follow-up in 3a19432 (startup/write cancellation coverage requested in P2):
Verified: |
3 participants
Summary
Port of the Sentinel security finding (originally
🛡️ Sentinel: [CRITICAL] Fix API key leak in streaming dictation) from the fork into upstream.providerio.Redactand switch the connect-error format specifier from%wto%sso the raw error cannot leak viaerrors.Unwrap().Test plan
TestOpenAIRealtimeStreamTranscribeErrorRedactionandTestDeepgramStreamTranscribeErrorRedactionasserting the API key is not present in the returned stream error for both transcribers.go test ./internal/dictation/...passes;go vet ./internal/dictation/...clean.Notes
.jules/sentinel.mdnote; that's a Jules-specific artifact and was intentionally omitted from this upstream PR.Summary by CodeRabbit