Skip to content

fix: empty/whitespace credentials should raise MissingCredentialsError#190

Merged
Kamilbenkirane merged 3 commits intomainfrom
fix/empty-whitespace-credentials
Feb 25, 2026
Merged

fix: empty/whitespace credentials should raise MissingCredentialsError#190
Kamilbenkirane merged 3 commits intomainfrom
fix/empty-whitespace-credentials

Conversation

@Kamilbenkirane
Copy link
Copy Markdown
Member

Summary

Closes #184

  • get_credentials() now rejects empty/whitespace SecretStr values with MissingCredentialsError instead of passing them through to produce invalid HTTP headers
  • has_credential() now returns False for empty/whitespace credentials, which also fixes list_available_providers()
  • Inverted test_whitespace_credentials to assert the new behavior

Test plan

  • test_whitespace_credentials_are_treated_as_missing passes for "", " ", "\t\n"
  • All 478 unit tests pass
  • All pre-commit hooks pass (ruff, mypy, bandit)

@claude
Copy link
Copy Markdown

claude bot commented Feb 25, 2026

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

@Kamilbenkirane
Copy link
Copy Markdown
Member Author

Code review

Found 3 issues:

  1. override_key whitespace bypass: create_client(api_key=" ") flows into get_credentials(override_key=" "). Since " " is truthy in Python, it bypasses the new whitespace guard at line 119 and returns SecretStr(" ") directly, producing a malformed Authorization: Bearer header. This is the same bug class the PR fixes for the env-var path, but the override_key path is left unguarded.

"""
if override_key:
if isinstance(override_key, str):
return SecretStr(override_key)
return override_key

  1. get_credentials docstring incomplete: The Raises section says "If provider requires credentials but none are configured" but the implementation now also raises for empty/whitespace-only values. Should mention the new condition, e.g. "or the configured value is empty or whitespace-only."

Raises:
MissingCredentialsError: If provider requires credentials but none are configured.
"""

  1. has_credential docstring incomplete: The one-liner "Check if a specific provider has credentials configured" doesn't reflect the behavior change -- whitespace/empty values are now treated as "not configured." A clarifying sentence would make the new semantics discoverable.

def has_credential(self, provider: Provider) -> bool:
"""Check if a specific provider has credentials configured."""

🤖 Generated with Claude Code

- If this code review was useful, please react with 👍. Otherwise, react with 👎.

Also updates get_credentials and has_credential docstrings to reflect
the new empty/whitespace-only rejection behavior.
`if override_key:` was falsy for "" and SecretStr(""), silently falling
through to env-var lookup instead of raising MissingCredentialsError.
Also adds SecretStr coverage and a fallthrough regression test.
@Kamilbenkirane Kamilbenkirane merged commit 3ef3f9d into main Feb 25, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix: empty/whitespace credentials should raise MissingCredentialsError

1 participant