Skip to content

fix: ensure requestTimeout and maxRetries are integers#394

Open
tgarciaalv wants to merge 3 commits intookta:masterfrom
tgarciaalv:patch-1
Open

fix: ensure requestTimeout and maxRetries are integers#394
tgarciaalv wants to merge 3 commits intookta:masterfrom
tgarciaalv:patch-1

Conversation

@tgarciaalv
Copy link
Copy Markdown

This is failing when environment variables are used for configuration. Env vars are strings but are never converted to int before using them in arithmetic operations.

My configuration:

              "OKTA_CLIENT_CONNECTIONTIMEOUT": "30",
               "OKTA_CLIENT_ORGURL": "https://dev-89428916.okta.com",
               "OKTA_CLIENT_AUTHORIZATIONMODE": "PrivateKey",
               "OKTA_CLIENT_CLIENTID": "${env:OKTA_CLIENT_CLIENTID}",
               "OKTA_CLIENT_SCOPES": "okta.users.read,okta.groups.read",
               "OKTA_CLIENT_PRIVATEKEY": "${env:OKTA_CLIENT_PRIVATEKEY}",
               "OKTA_CLIENT_REQUESTTIMEOUT": "0",
               "OKTA_CLIENT_RATELIMIT_MAXRETRIES": "4",
               "OKTA_CLIENT_LOGGING_ENABLED": "true",
               "OKTA_CLIENT_LOGGING_LEVEL": "INFO",

My error:

        # Raise Value Error if numerical inputs are invalid (< 0)
        self._request_timeout = config["client"].get('requestTimeout', 0)
>       if self._request_timeout < 0:
E       TypeError: '<' not supported between instances of 'str' and 'int'

/opt/homebrew/lib/python3.11/site-packages/okta/request_executor.py:35: TypeError

The os.environ dictionary in Python expects its values to be strings.

@bryanapellanes-okta
Copy link
Copy Markdown
Contributor

@tgarciaalv I apologize for the delayed response and thank you for your submission. To ensure integrity, please provide a unit test that fails prior to your proposed change and passes with your change in place.

We appreciate your engagement and thanks for using Okta!

@BinoyOza-okta
Copy link
Copy Markdown
Contributor

Hi @tgarciaalv,
Thanks for your submission. As per the last comment, please update the PR with the unit tests and resolve the merge conflicts.

This is failing when environment variables are used for configuration. Env vars are strings but are never converted to int before using them in arithmetic operations.

        # Raise Value Error if numerical inputs are invalid (< 0)
        self._request_timeout = config["client"].get('requestTimeout', 0)
>       if self._request_timeout < 0:
E       TypeError: '<' not supported between instances of 'str' and 'int'

/opt/homebrew/lib/python3.11/site-packages/okta/request_executor.py:35: TypeError
@tgarciaalv
Copy link
Copy Markdown
Author

tgarciaalv commented Feb 18, 2026

Hi @bryanapellanes-okta @BinoyOza-okta,

Thanks for the feedback. I've rebased onto master, resolved the merge conflicts, and added unit tests. I also fixed the CI failure on Python 3.13.

Changes

1. int() casts for env var config values (okta/request_executor.py)

  • requestTimeout and maxRetries are now cast to int before arithmetic comparison
  • Without this, env vars (always strings) cause TypeError: '<' not supported between instances of 'str' and 'int'

2. Unit tests (tests/unit/test_request_executor.py)

  • 8 tests covering: string values, int values, zero strings, and negative string validation
  • All fail before the fix (TypeError), all pass after

3. Python 3.13 CI fix (requirements.txt, setup.py, okta/config/config_setter.py)

  • Replaced flatdict==4.0.1 (abandoned, last release 2020) with flatdict2==4.0.4 (maintained fork, identical API)
  • flatdict fails to build on Python 3.13 because it uses pkg_resources (removed from stdlib)
  • This was a pre-existing issue on master unrelated to my original fix

How to verify

pytest tests/unit/test_request_executor.py::TestRequestExecutorStringConfig -v

Let me know if anything else is needed.

flatdict==4.0.1 (last release 2020) uses pkg_resources in its setup.py,
which was removed from Python 3.13 stdlib. flatdict2 is a maintained
fork with identical API and pre-built wheels.
@BinoyOza-okta
Copy link
Copy Markdown
Contributor

Hi @tgarciaalv,

Thanks for the continued effort and for rebasing onto master. I appreciate the thorough testing and the CI fix.

However, since your original PR was opened, we've made several significant changes to the SDK in our recent releases that affect the areas you've touched:

1. flatdict removal is already done
We've already removed the flatdict dependency entirely and replaced it with our own utility functions (flatten_dict, unflatten_dict) in okta/utils.py. So the flatdictflatdict2 migration in your PR is no longer needed — that part can be dropped.

2. Type coercion should happen in ConfigValidator, not RequestExecutor
Since your PR was opened, we introduced DPoP support which included a pattern for handling exactly this kind of env-var string coercion. Rather than casting to int() inside RequestExecutor.__init__(), we'd like you to follow the established pattern in ConfigValidator._validate_dpop_config().

Take a look at how dpopKeyRotationInterval is handled there — it:

  • Checks if the value is a str and coerces it to int with proper error handling for non-numeric strings
  • Writes the coerced value back into the client dict so downstream code (like RequestExecutor) receives the correct Python type
  • Appends a clear validation error if coercion fails

We'd like you to add a similar method (e.g. _validate_rate_limit_config) in ConfigValidator that coerces and validates both requestTimeout and rateLimit.maxRetries before they ever reach RequestExecutor. This keeps RequestExecutor clean and ensures all type coercion + validation is centralized in one place (ConfigValidator).

Here's the general shape we're looking for:

# In ConfigValidator.validate_config(), add before the auth mode checks:
errors += self._validate_rate_limit_config(client)

And the method should:

  • Coerce requestTimeout from string → int (if string), write back into client, error on non-numeric
  • Coerce rateLimit.maxRetries from string → int (if string), write back into client["rateLimit"], error on non-numeric
  • Validate both are non-negative (consistent with RequestExecutor's existing checks)

3. Unit tests
Your 8 unit tests sound great — please keep them but adjust to validate the new coercion path through ConfigValidator as well. Tests that confirm ConfigValidator properly coerces string env-var values and raises ValueError for invalid inputs would be ideal.

Could you please rebase again onto the current master and restructure the fix along these lines? Happy to answer any questions about the current codebase structure.

Thanks again for your contribution!!

@BinoyOza-okta BinoyOza-okta self-requested a review April 22, 2026 06:57
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.

3 participants