Skip to content

fix: require authenticated principal for task management tools#1139

Merged
KonstantinMirin merged 2 commits intoprebid:mainfrom
numarasSigmaSoftware:AuditFixPR02
Mar 23, 2026
Merged

fix: require authenticated principal for task management tools#1139
KonstantinMirin merged 2 commits intoprebid:mainfrom
numarasSigmaSoftware:AuditFixPR02

Conversation

@numarasSigmaSoftware
Copy link
Copy Markdown
Collaborator

Closes #1132 (F-03)

Summary

list_tasks, get_task, and complete_task checked identity.tenant but not identity.principal_id. Because the localhost tenant fallback resolves a tenant even for requests with no auth token, the existing guard was silently bypassed — an unauthenticated caller could read task state and advance or complete workflow steps on behalf of any tenant.

Changes

src/core/tools/task_management.py

Added identity.is_authenticated check immediately after the existing tenant check in all three functions, preserving the existing error order: tenant resolution fails first, then authentication, then DB access.
Unauthenticated callers now receive AdCPAuthenticationError before any DB access occurs.
Scope is intentionally limited to these three task-mutating operations. The localhost tenant fallback for read-only tools such as get_products is unchanged.
tests/unit/test_task_management_auth.py (new)

6 rejection cases: unauthenticated callers (no principal_id, no identity) across all three tools.
3 regression cases confirming authenticated callers pass through normally.

Note on architecture compliance

While reviewing the auth fix I noticed a pre-existing violation worth flagging: task_management.py imports Context from fastmcp.server.context at module level and accepts it as a parameter in all three functions. This is the pattern the transport-boundary guards are designed to prevent — business logic functions should accept ResolvedIdentity, not Context.

The structural guard in test_transport_agnostic_impl.py does not catch this because it only scans functions whose names end in _impl. These three functions are named list_tasks, get_task, and complete_task, so the AST sweep never inspects them.

This is not introduced by this PR. I'm not fixing it here to keep the scope of the auth change narrow, but I think it warrants a follow-up issue so it doesn't get lost. The fix would be:

Rename to _list_tasks_impl, _get_task_impl, _complete_task_impl
Remove the context: Context | None parameter from each
Move identity resolution from context into the MCP/A2A wrapper layer, consistent with every other tool in src/core/tools/
Happy to open that issue separately if useful.

Summary

Closes prebid#1132 (F-03).

list_tasks
,

get_task
, and

complete_task
 checked identity.tenant but not identity.principal_id. Because the localhost tenant fallback resolves a tenant even for requests with no auth token, the existing guard was silently bypassed — an unauthenticated caller could read and mutate workflow task state.

Changes

src/core/tools/task_management.py

Added identity.is_authenticated check immediately after the existing tenant check in all three functions.
Unauthenticated callers now receive AdCPAuthenticationError before any DB access occurs.
No change to the localhost fallback for other tools (get_products, etc.) — the scope is intentionally limited to task-mutating operations.

tests/unit/test_task_management_auth.py
 (new)

6 rejection cases: no principal_id and no identity × 3 tools
3 regression cases confirming authenticated callers pass through normally
@KonstantinMirin
Copy link
Copy Markdown
Collaborator

I think this a great catch regarding the architecture compliance - I suggest adding them ti the structural guard and allowlist and file a follow-up refactoring issue.

@numarasSigmaSoftware
Copy link
Copy Markdown
Collaborator Author

Thanks! I will do it for sure!

Copy link
Copy Markdown
Collaborator

@KonstantinMirin KonstantinMirin left a comment

Choose a reason for hiding this comment

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

Good work. The is_authenticated check is the right fix for the right gap.

One outstanding item: you mentioned you'd file a follow-up issue for the task_management.py architecture refactor (functions accept Context directly instead of using the _impl + transport wrapper pattern). Please file that before we merge so it's tracked. The debt is pre-existing, but we want it in the issue tracker.

@KonstantinMirin KonstantinMirin merged commit ff1006d into prebid:main Mar 23, 2026
12 checks passed
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.

security: Currency Integrity Drift on Update (Implicit USD)

2 participants