Skip to content

fix: redact Authorization header in request timeout error cause - #2229

Merged
Strift merged 3 commits into
meilisearch:mainfrom
soroush5:fix/redact-apikey-in-timeout-error
Sep 9, 2026
Merged

fix: redact Authorization header in request timeout error cause#2229
Strift merged 3 commits into
meilisearch:mainfrom
soroush5:fix/redact-apikey-in-timeout-error

Conversation

@soroush5

@soroush5 soroush5 commented Sep 6, 2026 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

Request timeouts embedded the full RequestInit — including Authorization: Bearer <key> — in error.cause, leaking the API key into logs and error trackers. This change stores a copy with the Authorization header redacted, keeping timeout/method/headers for debugging. Adds tests/request-timeout-redaction.test.ts (3 tests, green).

Summary by CodeRabbit

  • Bug Fixes
    • Request timeout errors now redact authorization credentials from diagnostic details.
    • Other request information, such as the HTTP method and timeout, remains available for troubleshooting.
    • Errors continue to provide diagnostic context even when no API key is supplied.

@soroush5
fix: redact Authorization header in request timeout error cause
280a66a 
The timeout error embedded the full RequestInit (including
'Authorization: Bearer <apiKey>') in error.cause, leaking the API key
to logs and error trackers. Store a copy with Authorization redacted.
@coderabbitai

coderabbitai Bot commented Sep 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: d77c961e-05c0-4433-858d-9030545abacb

📥 Commits

Reviewing files that changed from the base of the PR and between 6c5c653 and 558dd6e.

📒 Files selected for processing (1)
  • tests/errors.test.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

The timeout error now stores a sanitized request cause. Tests verify API-key redaction, retained request metadata, and unauthenticated behavior.

Changes

Timeout Error Redaction

Layer / File(s) Summary
Sanitize timeout request causes
src/errors/meilisearch-request-timeout-error.ts		 The constructor clones request headers, replaces Authorization with <redacted>, and stores the sanitized request init in cause.
Validate redaction behavior
tests/errors.test.ts		 Tests verify API-key removal, preserved request metadata, timeout reporting, and behavior without an API key.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 558dd

Timeout errors now retain useful request diagnostics while redacting Authorization values, with coverage for authenticated, metadata-preservation, and unauthenticated cases. The change is ready to merge.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: redacting the Authorization header in the request timeout error cause.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 3 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

A rabbit reads each line,
The patch grows clear beneath the moon,
Small changes hop in place,
Tests guard the garden path,
Reviews bloom before the dawn.

Comment @coderabbitai help to get the list of available commands.

@Strift Strift added the security Address a security vulnerability label Sep 9, 2026
@codecov

codecov Bot commented Sep 9, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.14%. Comparing base (a4e0707) to head (558dd6e).
⚠️ Report is 9 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #2229      +/-   ##
==========================================
+ Coverage   98.11%   98.14%   +0.03%     
==========================================
  Files          14       14              
  Lines         688      700      +12     
  Branches      109      110       +1     
==========================================
+ Hits          675      687      +12     
  Misses         12       12              
  Partials        1        1

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@Strift
test: move timeout redaction cases into errors.test.ts
558dd6e 
Keep error tests next to the other error-class coverage and exercise the constructor directly instead of a hanging fetch.
Strift
Strift approved these changes Sep 9, 2026
View reviewed changes

@Strift Strift left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Hey @soroush5, thanks for this 🙌

@Strift
Strift enabled auto-merge September 9, 2026 05:31
@Strift
Strift added this pull request to the merge queue Sep 9, 2026
Merged via the queue into meilisearch:main with commit b1159ea Sep 9, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Strift Strift Strift approved these changes

Assignees

No one assigned

Labels

security Address a security vulnerability

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@soroush5 @Strift