fix: strip whitespace from API keys to prevent HTTP header errors

[Kamilbenkirane]

Summary

• Strip whitespace from API keys when converting to SecretStr
• Strip whitespace when extracting secret value for HTTP headers

Fixes LocalProtocolError: Illegal header value when API keys contain trailing newlines (common when pasting from terminals or Colab Secrets).

Test plan

• Verify API keys with trailing whitespace no longer cause HTTP errors

[claude]

Code Review for PR #116

Summary

This PR addresses an important bug where API keys with trailing whitespace (common when copying from terminals or Colab Secrets) cause LocalProtocolError: Illegal header value. The fix strips whitespace in two places in the AuthHeader class.

---

✅ Strengths

1. Clear problem identification: The fix addresses a real usability issue that users commonly encounter when copying API keys from various sources.

2. Minimal, focused changes: The PR appropriately strips whitespace at both the input and output stages without over-engineering.

3. Version bump included: Properly incremented the patch version (0.9.1 → 0.9.2).

---

🔍 Code Quality Observations

Double stripping (Minor inefficiency)

The implementation strips whitespace twice:

• Line 40: SecretStr(v.strip()) when converting string to SecretStr
• Line 45: self.secret.get_secret_value().strip() when generating headers

Analysis:

• If a SecretStr is passed directly (bypassing the validator), line 45's strip ensures it's still cleaned
• If a string is passed, it gets stripped at input (line 40), then stripped again unnecessarily at output (line 45)

Recommendation: This defensive approach is acceptable given:

• strip() is a cheap operation
• It guarantees clean headers regardless of how the SecretStr was created
• The redundancy ensures robustness without meaningful performance impact

---

🧪 Test Coverage Gap

Critical finding: The existing tests in tests/unit_tests/test_auth.py do NOT verify the whitespace stripping behavior.

Current tests:

• test_convert_to_secret_with_string() checks conversion but not stripping
• test_get_headers_returns_correct_format() uses clean inputs

Recommended test additions:

def test_strips_whitespace_from_string_api_key(self) -> None:
    """Test that API keys with whitespace are stripped during conversion."""
    auth = AuthHeader(secret="  test-key\n")  # type: ignore[arg-type]
    assert auth.secret.get_secret_value() == "test-key"

def test_strips_whitespace_from_secretstr_in_headers(self) -> None:
    """Test that get_headers strips whitespace from SecretStr values."""
    # SecretStr created directly, bypassing validator
    secret = SecretStr("  api-key-123\n  ")
    auth = AuthHeader(secret=secret)
    headers = auth.get_headers()
    assert headers == {"Authorization": "Bearer api-key-123"}

def test_handles_api_key_with_only_whitespace(self) -> None:
    """Test that API keys that are only whitespace are handled gracefully."""
    auth = AuthHeader(secret="   \n\t  ")  # type: ignore[arg-type]
    headers = auth.get_headers()
    # Should result in empty secret after stripping
    assert headers == {"Authorization": "Bearer "}

---

🔒 Security Considerations

No security concerns identified:

• strip() is a safe string operation
• SecretStr handling remains secure
• No risk of exposing secrets or injection vulnerabilities

Positive note: The fix actually improves security by preventing header injection attacks that could theoretically exploit newline characters in headers.

---

⚡ Performance Considerations

Impact: Negligible

• str.strip() is O(n) but extremely fast in CPython
• Only called once per authentication header generation
• Headers are typically generated once per request/session

---

🎯 Additional Observations

1. Consistency with codebase: The credentials.py file (line 103) also creates SecretStr from strings without stripping, so this fix improves consistency.

2. Edge case - empty strings: After stripping, an API key like " \n\t " becomes "". This will fail authentication but won't crash - acceptable behavior.

3. SecretStr bypass scenario: Users could theoretically create SecretStr("key\n") and pass it directly to AuthHeader, bypassing the validator. The strip in get_headers() catches this case.

---

📋 Recommendations

Before merge:

1. ✅ Add tests for the whitespace stripping behavior (high priority)
2. ✅ Update test plan checkbox in PR description once tests pass

Nice to have (consider for future):

1. Document this behavior in the AuthHeader docstring
2. Consider validating that API keys are non-empty after stripping (though current behavior is acceptable)

---

Final Verdict

Approve with minor test addition

The implementation is sound and solves a real user pain point. The code changes are minimal and safe. The only gap is test coverage, which should be added before merging to prevent regression.

Impact: This will significantly improve UX for users copying API keys from various sources.