-
Notifications
You must be signed in to change notification settings - Fork 45
fix: Add validation format check for SDK key #351
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
133b3db
6b0920b
5891f34
b7eec96
e514784
b4f8ab6
6959036
a19e5bf
6ec6770
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,8 +27,13 @@ def timedelta_millis(delta: timedelta) -> float: | |
|
|
||
| __BASE_TYPES__ = (str, float, int, bool) | ||
|
|
||
| # Maximum length for SDK keys | ||
| _MAX_SDK_KEY_LENGTH = 8192 | ||
|
|
||
| _retryable_statuses = [400, 408, 429] | ||
| _RETRYABLE_STATUSES = [400, 408, 429] | ||
|
|
||
| # Compiled regex pattern for valid characters in application values and SDK keys | ||
| _VALID_CHARACTERS_REGEX = re.compile(r"[^a-zA-Z0-9._-]") | ||
|
|
||
|
|
||
| def validate_application_info(application: dict, logger: logging.Logger) -> dict: | ||
|
|
@@ -46,13 +51,35 @@ def validate_application_value(value: Any, name: str, logger: logging.Logger) -> | |
| logger.warning('Value of application[%s] was longer than 64 characters and was discarded' % name) | ||
| return "" | ||
|
|
||
| if re.search(r"[^a-zA-Z0-9._-]", value): | ||
| if _VALID_CHARACTERS_REGEX.search(value): | ||
| logger.warning('Value of application[%s] contained invalid characters and was discarded' % name) | ||
| return "" | ||
|
|
||
| return value | ||
|
|
||
|
|
||
| def validate_sdk_key_format(sdk_key: str, logger: logging.Logger) -> str: | ||
| """ | ||
| Validates that an SDK key does not contain invalid characters and is not too long for our systems. | ||
|
|
||
| :param sdk_key: the SDK key to validate | ||
| :param logger: the logger to use for logging warnings | ||
| :return: the validated SDK key, or empty string if the SDK key is invalid | ||
| """ | ||
| if sdk_key is None or sdk_key == '': | ||
| return "" | ||
|
|
||
| if not isinstance(sdk_key, str): | ||
| return "" | ||
| if len(sdk_key) > _MAX_SDK_KEY_LENGTH: | ||
| logger.warning('SDK key was longer than %d characters and was discarded' % _MAX_SDK_KEY_LENGTH) | ||
| return "" | ||
| if _VALID_CHARACTERS_REGEX.search(sdk_key): | ||
| logger.warning('SDK key contained invalid characters and was discarded') | ||
| return "" | ||
| return sdk_key | ||
jsonbailey marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: SDK Key Validation Breaks Offline ModeThe new Additional Locations (2)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I updated the config validation logic to remove the None check.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Both None and "" are valid in offline mode. The validation method on the config didn't prevent anything, it only logged a warning if a blank or missing sdk key was provided. |
||
|
|
||
|
|
||
| def _headers(config): | ||
| base_headers = _base_headers(config) | ||
| base_headers.update({'Content-Type': "application/json"}) | ||
|
|
@@ -106,7 +133,7 @@ def throw_if_unsuccessful_response(resp): | |
|
|
||
| def is_http_error_recoverable(status): | ||
| if status >= 400 and status < 500: | ||
| return status in _retryable_statuses # all other 4xx besides these are unrecoverable | ||
| return status in _RETRYABLE_STATUSES # all other 4xx besides these are unrecoverable | ||
| return True # all other errors are recoverable | ||
|
|
||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,65 @@ | ||
| import logging | ||
|
|
||
| from ldclient.impl.util import validate_sdk_key_format | ||
|
|
||
|
|
||
| def test_validate_sdk_key_format_valid(): | ||
| """Test validation of valid SDK keys""" | ||
| logger = logging.getLogger('test') | ||
| valid_keys = [ | ||
| "sdk-12345678-1234-1234-1234-123456789012", | ||
| "valid-sdk-key-123", | ||
| "VALID_SDK_KEY_456", | ||
| "test.key_with.dots", | ||
| "test-key-with-hyphens" | ||
| ] | ||
|
|
||
| for key in valid_keys: | ||
| result = validate_sdk_key_format(key, logger) | ||
| assert result == key # Should return the same key if valid | ||
|
|
||
|
|
||
| def test_validate_sdk_key_format_invalid(): | ||
| """Test validation of invalid SDK keys""" | ||
| logger = logging.getLogger('test') | ||
| invalid_keys = [ | ||
| "sdk-key-with-\x00-null", | ||
| "sdk-key-with-\n-newline", | ||
| "sdk-key-with-\t-tab", | ||
| "sdk key with spaces", | ||
| "sdk@key#with$special%chars", | ||
| "sdk/key\\with/slashes" | ||
| ] | ||
|
|
||
| for key in invalid_keys: | ||
| result = validate_sdk_key_format(key, logger) | ||
| assert result == '' # Should return empty string for invalid keys | ||
|
|
||
|
|
||
| def test_validate_sdk_key_format_non_string(): | ||
| """Test validation of non-string SDK keys""" | ||
| logger = logging.getLogger('test') | ||
| non_string_values = [123, object(), [], {}] | ||
|
|
||
| for value in non_string_values: | ||
| result = validate_sdk_key_format(value, logger) | ||
| assert result == '' # Should return empty string for non-string values | ||
|
|
||
|
|
||
| def test_validate_sdk_key_format_empty_and_none(): | ||
| """Test validation of empty and None SDK keys""" | ||
| logger = logging.getLogger('test') | ||
| assert validate_sdk_key_format("", logger) == '' # Empty string should return empty string | ||
| assert validate_sdk_key_format(None, logger) == '' # None should return empty string | ||
|
|
||
|
|
||
| def test_validate_sdk_key_format_max_length(): | ||
| """Test validation of SDK key maximum length""" | ||
| logger = logging.getLogger('test') | ||
| valid_key = "a" * 8192 | ||
| result = validate_sdk_key_format(valid_key, logger) | ||
| assert result == valid_key # Should return the same key if valid | ||
|
|
||
| invalid_key = "a" * 8193 | ||
| result = validate_sdk_key_format(invalid_key, logger) | ||
| assert result == '' # Should return empty string for keys that are too long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: Config Copy Method Incomplete and Misleading
The
copy_with_new_sdk_keymethod doesn't fully replicate the originalConfiginstance. It omits several parameters (e.g.,application,hooks,plugins), causing them to reset to defaults in the newConfig. Also, its docstring is misleading: if a new SDK key is invalid, a newConfigis created with an empty SDK key, rather than preserving the original key as implied.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is addressed in a separate PR to keep this one focused.