Skip to content

Conversation

jsonbailey
Copy link
Contributor

@jsonbailey jsonbailey commented Sep 29, 2025

Fix SDK Key Format Validation to Prevent Logging Invalid Keys

This PR implements SDK key format validation to prevent logging of potentially sensitive invalid SDK keys, aligning the Python SDK with the .NET SDK implementation.

Requirements

  • I have added test coverage for new or changed functionality
  • I have followed the repository's pull request submission guidelines
  • I have validated my changes against all supported platform versions

Related issues

Addresses security concern where invalid SDK keys could be logged, potentially exposing sensitive information. Based on similar validation implemented in dotnet-core PR #163 and PR #162.

Describe the solution you've provided

Key Changes:

  1. New validation function: Added is_valid_sdk_key_format() in ldclient/impl/util.py that validates SDK keys using:

    • Regex pattern ^[-a-zA-Z0-9._]+$ (matching .NET SDK)
    • Maximum length of 8192 characters
    • Allows empty/None keys for offline mode
  2. Silent failure behavior: Modified Config constructor and copy_with_new_sdk_key() to silently set invalid keys to None instead of raising ValueError exceptions

  3. Comprehensive test coverage: Added tests for valid keys, invalid keys, edge cases (empty/None/non-string), and maximum length validation

Security Benefit:

Invalid SDK keys are now prevented from entering the system, eliminating the risk of them being logged elsewhere in the codebase.

Critical Review Points:

  • ⚠️ Behavioral Change: This changes from raising exceptions to silent failure - verify this won't break existing error handling
  • ⚠️ Regex Pattern: Confirm ^[-a-zA-Z0-9._]+$ exactly matches the .NET SDK implementation
  • ⚠️ None Handling: Ensure all SDK code paths properly handle None SDK keys

Describe alternatives you've considered

  • Continue raising exceptions: Rejected because it doesn't prevent logging and breaks the goal of silent validation
  • Log validation errors: Rejected because it would still potentially log the invalid key value
  • Use HTTP header validation regex: Rejected to maintain consistency with .NET SDK pattern

Additional context


Note

Add strict SDK key validation (length/characters) and integrate into Config; standardize constants; add comprehensive tests.

  • SDK key validation:
    • Add validate_sdk_key_format in ldclient/impl/util.py enforcing allowed characters and max length (8192), returning '' on invalid and logging warnings.
    • Use validation in Config (ldclient/config.py) to set __sdk_key; update copy_with_new_sdk_key behavior accordingly.
  • Logging/validation tweaks:
    • Adjust _validate in Config to warn on missing/blank SDK key with updated message.
  • Refactors:
    • Standardize constants in impl/util.py (e.g., _retryable_statuses_RETRYABLE_STATUSES) and update references.
  • Tests:
    • Add ldclient/testing/impl/test_util.py covering valid/invalid/non-string/empty/None/max-length SDK keys.
    • Expand ldclient/testing/test_config.py to verify SDK key validation, copy behavior, and max length handling.

Written by Cursor Bugbot for commit 6ec6770. This will update automatically on new commits. Configure here.

BEGIN_COMMIT_OVERRIDE
fix: Add validation format check for SDK key
END_COMMIT_OVERRIDE

- Add validate_sdk_key function to prevent logging invalid SDK keys
- Validate SDK key format in Config constructor
- Add comprehensive tests for SDK key validation
- Follow existing validation patterns in codebase

Prevents logging of invalid SDK keys by validating that they contain
only visible ASCII characters suitable for HTTP headers. Invalid keys
trigger a ValueError with a generic message that doesn't expose the
actual key value.

Co-Authored-By: jbailey@launchdarkly.com <accounts@sidewaysgravity.com>
@jsonbailey jsonbailey requested a review from a team as a code owner September 29, 2025 13:56
Copy link
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

cursor[bot]

This comment was marked as outdated.

- Update test_validate_sdk_key_non_string to actually test non-string inputs
- Test various non-string types: int, None, object, list, dict
- Verify proper error message is logged for each invalid type

Co-Authored-By: jbailey@launchdarkly.com <accounts@sidewaysgravity.com>
cursor[bot]

This comment was marked as outdated.

devin-ai-integration bot and others added 2 commits September 29, 2025 15:07
- Rename validate_sdk_key to is_valid_sdk_key_format
- Use dotnet regex pattern ^[-a-zA-Z0-9._]+$ instead of HTTP header validation
- Add maximum length validation of 8192 characters
- Change from raising ValueError to silently not setting invalid keys
- Update all tests to expect silent failure instead of exceptions
- Update documentation to reflect new behavior

Co-Authored-By: jbailey@launchdarkly.com <accounts@sidewaysgravity.com>
- Address GitHub comment about missing docstring format
- Follow Python conventions for function documentation

Co-Authored-By: jbailey@launchdarkly.com <accounts@sidewaysgravity.com>
cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

Copy link
Contributor

GitHub Response to jsonbailey's comment

Hi @jsonbailey,

I believe there may be some confusion about the current implementation. The diff you're referencing shows the old approach that raised a ValueError, but my current implementation actually follows the dotnet pattern with silent failure.

The current code in ldclient/config.py (lines 264-267) is:

if is_valid_sdk_key_format(sdk_key):
    self.__sdk_key = sdk_key
else:
    self.__sdk_key = None

This matches the dotnet SDK behavior where invalid keys are silently set to None rather than raising exceptions. The validation method is also correctly named is_valid_sdk_key_format and uses the same regex pattern as the dotnet implementation: ^[-a-zA-Z0-9._]+$.

Could you clarify what specific issue you're seeing with the current implementation? I want to make sure I address any concerns properly.

cursor[bot]

This comment was marked as outdated.

Copy link
Contributor

Devin is archived and cannot be woken up. Please unarchive Devin if you want to continue using it.

@jsonbailey jsonbailey merged commit 4c8d4ae into main Sep 30, 2025
14 checks passed
@jsonbailey jsonbailey deleted the devin/1759153910-sdk-1460-add-sdk-key-validation branch September 30, 2025 16:32
jsonbailey added a commit that referenced this pull request Sep 30, 2025
🤖 I have created a release *beep* *boop*
---


##
[9.12.1](9.12.0...9.12.1)
(2025-09-30)


### Bug Fixes

* Add validation format check for SDK key
([#351](#351))

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> Release 9.12.1 with SDK key format validation bug fix and
version/provenance updates.
> 
> - **Release 9.12.1**:
> - Bump version to `9.12.1` in `pyproject.toml`, `ldclient/version.py`,
and `.release-please-manifest.json`.
>   - Update `CHANGELOG.md` with bug fix: add SDK key format validation.
>   - Update `PROVENANCE.md` example `SDK_VERSION` to `9.12.1`.
> 
> <sup>Written by [Cursor
Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit
7da6750. This will update automatically
on new commits. Configure
[here](https://cursor.com/dashboard?tab=bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

---------

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Jason Bailey <jbailey@launchdarkly.com>
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.

2 participants