Repository navigation
fix(secrets): key the single-secret cache on every request parameter - #64
RishavTiwari25 wants to merge 1 commit into
Conversation
get_secret_by_name built its cache key from project_id, environment, path and name only. With project_slug (project_id None), the same secret name in two projects shared one entry, so the second project got the first project's value. version, view_secret_value, expand_secret_references and include_imports were also ignored: a masked value or an old version was returned for later default reads. The key now uses the request parameters, like list_secrets. Because a secret can be cached under several keys (id or slug, read options), create, update and delete now drop all cached single secrets along with the lists; before, update/delete by project_slug left the entry cached by project_id in place.
📝 Contributor License Agreement requiredBefore this PR can merge, every contributor must sign the Infisical CLA. Still needs to sign: Once everyone has signed, the check updates automatically — no need to close and reopen the PR. |
[Medium impact] The change reaches secret reads and writes made through this SDK. The PR appears safe to merge, with non-blocking gaps in test coverage and CI execution. Findings
|
| ({"project_id": "p1", "include_imports": False}, {"project_id": "p1"}), | ||
| ], | ||
| ) | ||
| def test_get_secret_by_name_does_not_share_cache_across_requests(cache, first, second): |
| assert requests.get.call_count == 1 | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("mutation", ["update_secret_by_name", "delete_secret_by_name"]) |
There was a problem hiding this comment.
These tests cover only update_secret_by_name and delete_secret_by_name, although create_secret_by_name also changes from filling the cache to clearing it. Add a case that caches one secret, creates another, and checks that rereading the first hits the API. Removing the new create invalidation call would currently leave every test green.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Problem
With caching on (the default,
cache_ttl=60),secrets.get_secret_by_namecan return the wrong secret.get_secret_by_namekeyed its cache entry onproject_id,environment_slug,secret_pathandsecret_nameonly:project_slug=...the key hasproject_id=None, soAPI_KEYindev/of project A and of project B is one cache entry. The second call returns project A's value for project B.version,view_secret_value,expand_secret_referencesandinclude_importsare not in the key. After readingversion="1"orview_secret_value=False, a normal read within the TTL returns the old version or the masked value.project_id. A secret read withproject_idand then updated or deleted withproject_slugstays cached with its old value.Fix
get_secret_by_namebuilds its key from the secret name plus the full requestparams, likelist_secretsalready does.create_secret_by_name,update_secret_by_nameanddelete_secret_by_namedrop every cached single secret (invalidate_operation(CACHE_KEY_SINGLE_SECRET)) together with the cached lists, which they already dropped.create_secret_by_nameno longer pre-fills the cache with the create response; the next read fetches it once.Tests
The repo had no unit tests, so this adds
tests/test_secrets_cache.py, which runsV3RawSecretsagainst a realSecretsCacheand a mockedInfisicalRequests:view_secret_value,expand_secret_referencesorinclude_importsboth hit the API and get their own value (5 cases);project_sluginvalidate a secret cached byproject_id.7 of the 8 tests fail on
mainand all pass with the fix (pytest tests).flake8 . --select=E9,F63,F7,F82(the CI check) is clean, and so is plainflake8 tests.