Skip to content

Use a structured SecretCache key instead of a concatenated string - #72201

Merged
potiuk merged 1 commit into
apache:mainfrom
potiuk:fix-secretcache-injective-key
Aug 28, 2026
Merged

Use a structured SecretCache key instead of a concatenated string#72201
potiuk merged 1 commit into
apache:mainfrom
potiuk:fix-secretcache-injective-key

Conversation

@potiuk

@potiuk potiuk commented Aug 28, 2026

Copy link
Copy Markdown
Member

SecretCache built its key by concatenating three parts:

_TEAM_PATTERN = "_{}_"
team = cls._TEAM_PATTERN.format(team_name) if team_name else ""
cls._cache.get(f"{prefix}{team}{key}")

That mapping is not injective while the team segment is optional. Team analytics with key DB_PASSWORD composes to __v__analytics_DB_PASSWORD, and so does no team with key _analytics_DB_PASSWORD — the two entries share a cache slot.

Reads, writes and invalidations all resolve through the same composed string, so entries that collide could read, overwrite or evict one another.

The three parts are now kept as a tuple, which is injective by construction and needs no escaping or length-prefixing:

@staticmethod
def _key(prefix: str, team_name: str | None, key: str) -> tuple[str, str | None, str]:
    return (prefix, team_name, key)

_VARIABLE_PREFIX, _CONNECTION_PREFIX and _TEAM_PATTERN are private to this module and had no callers outside it, so nothing depended on the previous key shape. _TEAM_PATTERN is now gone.

Tests. Added coverage for entries that shared a slot under the old scheme, on each of the three paths — read, write (the colliding write must not clobber the other entry) and invalidate — plus team names that share a prefix. Reverting the source change fails four of them; they pass with it.

Local: 17 passed in the touched file, 1070 across task-sdk/tests/task_sdk/execution_time/ (11 pre-existing TriggerDagRunOperator failures reproduce identically on a clean tree), 18 in airflow-core/tests/unit/always/test_secrets.py. ruff and mypy clean.

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This pull request fixes a key-collision bug in the Task SDK’s SecretCache by replacing the previous string-concatenated cache key (which was not injective when team_name was optional) with a structured key. This prevents teamless reads/writes/invalidations from accidentally (or maliciously) colliding with team-scoped entries.

Changes:

  • Introduce a structured _CacheKey (NamedTuple) and use it for all cache get/set/pop operations.
  • Remove the previous _TEAM_PATTERN + string composition scheme that could produce collisions.
  • Add regression tests covering the previously-colliding read/write/invalidate paths and team-name prefix cases.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
task-sdk/src/airflow/sdk/execution_time/cache.py Switch SecretCache internal keying from concatenated strings to a structured _CacheKey to eliminate collisions.
task-sdk/tests/task_sdk/execution_time/test_cache.py Add regression tests ensuring colliding keys no longer allow cross-team access, overwrite, or eviction.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@vincbeck vincbeck left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Indeed, simpler and more robust

The cache key was built by concatenating the prefix, an optional '_{team}_'
segment and the entry key. That mapping is not injective while the team
segment is optional: for a team 'analytics' and key 'DB_PASSWORD' the composed
string is identical to the one produced with no team and key
'_analytics_DB_PASSWORD', so the two entries share a slot. Reads, writes and
invalidations all resolve through the same composed string, so distinct
entries could read, overwrite or evict one another.

The parts are now carried in a _CacheKey NamedTuple, which is injective by
construction and needs no escaping, and names each part at the call site. It
is declared at module level so it pickles by qualified name across the
multiprocessing manager the cache is stored in. The prefixes are private to
this module, so no caller depends on the previous key shape.

Added coverage for entries that shared a slot under the old scheme, on the
read, write and invalidate paths, plus team names that share a prefix.
@potiuk
potiuk force-pushed the fix-secretcache-injective-key branch from 78853a2 to 36b440e Compare August 28, 2026 14:34
@potiuk
potiuk merged commit b908c63 into apache:main Aug 28, 2026
107 checks passed
@potiuk
potiuk deleted the fix-secretcache-injective-key branch August 28, 2026 20:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants