Skip to content

fix: sanitize environment document cache payload to prevent internal …#7014

Open
SahilJat wants to merge 2 commits intoFlagsmith:mainfrom
SahilJat:fix/environment-cache-leak
Open

fix: sanitize environment document cache payload to prevent internal …#7014
SahilJat wants to merge 2 commits intoFlagsmith:mainfrom
SahilJat:fix/environment-cache-leak

Conversation

@SahilJat
Copy link

Thanks for submitting a PR! Please check the boxes below:

  • [ x ] I have read the Contributing Guide.
  • [ x ] I have added information to docs/ if required so people know about the feature.
  • [ x ] I have filled in the "Changes" section below.
  • [ x ] I have filled in the "How did you test this code" section below.

Changes

Closes #6944

Please describe.
Since V1EnvironmentDocumentResponse is a TypedDict, I implemented an explicit dictionary extraction right before environment_document_cache.set_many(). This guarantees that internal DynamoDB configs (like compress_dynamo_documents) do not leak into the Redis cache consumed by the API.

I also added a mocked unit test to verify the payload is stripped and perfectly validates against the V1EnvironmentDocumentResponse schema.

How did you test this code?

Please describe.
1.Mocked the Cache: Used @mock.patch to intercept the payload right before it hits environment_document_cache.set_many().

2.Forced Persistent Mode: Set CACHE_ENVIRONMENT_DOCUMENT_MODE to PERSISTENT to trigger the exact code path causing the leak.

2.Asserted Data Stripping: Verified that internal configuration fields (like compress_dynamo_documents and use_v2_feature_versioning) are successfully stripped from the payload.

4.Schema Validation: Explicitly proved that the intercepted dictionary perfectly aligns with the V1EnvironmentDocumentResponse TypedDict, guaranteeing that the public API view will not break.

@SahilJat SahilJat requested a review from a team as a code owner March 23, 2026 05:14
@SahilJat SahilJat requested review from khvn26 and removed request for a team March 23, 2026 05:14
Copy link

@claude claude bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Code review skipped — your organization's overage spend limit has been reached.

Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit in Settings → Usage.

Once credits are available, reopen this pull request to trigger a review.

@vercel
Copy link

vercel bot commented Mar 23, 2026

@SahilJat is attempting to deploy a commit to the Flagsmith Team on Vercel.

A member of the Team first needs to authorize it.

@github-actions github-actions bot added the api Issue related to the REST API label Mar 23, 2026
Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 3 potential issues.

Fix All in Cursor

Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

"feature_states": doc_dict.get("feature_states", []),
"identity_overrides": doc_dict.get("identity_overrides", []),
"name": doc_dict.get("name"),
"project": doc_dict.get("project"),
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanitization strips essential fields like id from cached document

High Severity

The sanitization reduces the cached environment document to only 5 keys (api_key, feature_states, identity_overrides, name, project), but the flag engine's EnvironmentModel requires id: int with no default, and SDKs depend on additional fields like allow_client_traits, updated_at, hide_sensitive_data, hide_disabled_flags, use_identity_composite_key_for_hashing, and use_identity_overrides_in_local_eval. When this stripped document is served from cache via the SDK endpoint, consuming SDKs will fail to parse it. The DB fallback path (_get_environment_document_from_db) returns a full document including all these fields, creating an inconsistency.

Fix in Cursor Fix in Web

# Just in case Flagsmith has a legacy custom class here
doc_dict = fat_doc.dict()
else:
doc_dict = fat_doc
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dead code: unnecessary Pydantic model conversion checks

Low Severity

map_environment_to_environment_document always returns a plain dict (type Document), so the hasattr(fat_doc, "model_dump") and hasattr(fat_doc, "dict") branches are unreachable dead code. Plain dicts have neither a model_dump nor a dict attribute. The code always falls through to doc_dict = fat_doc. This unnecessary indirection obscures the actual logic and suggests a misunderstanding of the return type.

Fix in Cursor Fix in Web


# 4. ASSERT: The internal/dangerous fields MUST NOT be in the payload
assert "compress_dynamo_documents" not in cached_document
assert "use_v2_feature_versioning" not in cached_document
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Test asserts fields that were never present anyway

Low Severity

The test asserts that compress_dynamo_documents and use_v2_feature_versioning are absent from the cached document, but these fields were never part of the output of map_environment_to_environment_document — that function iterates over EnvironmentModel fields, which doesn't include either of these keys. The assertions are vacuously true and provide no meaningful validation, giving a false sense of correctness. The "schema validation" at step 5 is also a no-op at runtime since TypedDict annotations are not enforced by Python.

Fix in Cursor Fix in Web

@SahilJat SahilJat force-pushed the fix/environment-cache-leak branch from 9c36c69 to 251b885 Compare March 23, 2026 05:42
@cursor
Copy link

cursor bot commented Mar 23, 2026

You have used all of your free Bugbot PR reviews.

To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

@SahilJat
Copy link
Author

hi @khvn26 all the precommit has been passed the only test failing require authorization , please let me know if its good to go or do i have to make some changes also I noticed that compress_dynamo_documents was temporarily disabled in api/integrations/flagsmith/data/environment.json to mitigate this cache poisoning. Once this PR is merged and deployed, you should be completely safe to flip that flag back to true!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api Issue related to the REST API

Projects

None yet

Development

Successfully merging this pull request may close these issues.

compress_dynamo_documents breaks permanent environment cache

1 participant