made cleaner app prompt handling and placed in JWT Key#2983
made cleaner app prompt handling and placed in JWT Key#2983tim-inkeep wants to merge 8 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 1e43335 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
TL;DR — Fixes a security vulnerability where Key changes
Summary ｜ 10 files ｜ 8 commits ｜ base:
|
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid security fix. This PR closes a real vulnerability where an attacker with a regular API key could inject an arbitrary appId via the x-inkeep-app-id header, which would then influence prompt resolution. The fix correctly moves appId propagation into the signed JWT service token and resolves appPrompt server-side with tenant scoping. Two minor items — one inline, one here:
Stale comment in runAuth.ts:899-900 (outside the diff, so noting here): The comment says "The appId is forwarded for context only" but the header-based appId forwarding has been removed by this PR. The sub-agent now gets appId from the verified JWT payload instead. Consider updating to: "When subAgentId is set, this is an internal A2A call — skip app credential auth. The sub-agent authenticates via its own service token (which carries appId in the JWT)."
Claude Opus ｜ 𝕏
| // Parent agents already have appPrompt set at auth time. | ||
| if (!config.executionContext.metadata?.appPrompt && config.executionContext.metadata?.appId) { | ||
| // Resolve appPrompt from DB using tenant-scoped lookup. | ||
| if (config.executionContext.metadata?.appId) { |
There was a problem hiding this comment.
This now re-fetches the app from the database on every task handler invocation, even when appPrompt was already resolved earlier in the delegation chain. Correct from a security standpoint (ensures freshness + tenant scoping), but worth noting the added DB query per handler invocation. If this becomes a hot path, a short-lived in-memory cache keyed by (tenantId, appId) could help — not a blocker, just flagging.
|
Claude finished @tim-inkeep's task in 1m 10s —— View job PR Review: App Prompt Security Vulnerability Fix
SummaryThis is a well-executed security fix that closes a real vulnerability. The core problem — Security Analysis: Looks GoodWhat was vulnerable:
What the fix does correctly:
Issues Found1. Stale comment at The comment now reads: This is actually already updated in this PR — the old comment "The appId is forwarded for context only" was replaced. This looks correct now. 2. At const appId = c.req.header('x-inkeep-app-id');And at if (reqData.appId && !subAgentId) {This is correct behavior — the header is still needed for the initial app credential auth flow (where a web client sends 3. Missing changeset The changeset bot flagged this PR has no changeset. Since this modifies pnpm bump patch --pkg agents-core --pkg agents-api "Fix appId spoofing vulnerability by propagating appId via signed JWT instead of headers"4. Minor: At const appId = typeof payload.appId === 'string' ? payload.appId : undefined;Good defensive check — consistent with how Test Coverage: SolidThe tests comprehensively cover the new behavior:
VerdictApprove. This is a clean, well-scoped security fix with good test coverage. The only action item is adding a changeset before merge. | Branch: |
There was a problem hiding this comment.
Solid security hardening. This PR closes two real attack vectors: (1) an untrusted x-inkeep-app-id header being blindly copied into metadata.appId for non-app-credential auth paths, and (2) a client-supplied appPrompt being trusted instead of always re-resolving from DB. The appId propagation via signed JWT is the right pattern.
Two minor items flagged below — one potential issue with the tryAppCredentialAuth lookup and one nit about redundant header reading.
Claude Opus ｜ 𝕏
|
|
||
| // When subAgentId is set, this is an internal A2A call — skip app credential auth. | ||
| // The appId is forwarded for context only; the sub-agent authenticates via its own token. | ||
| // The sub-agent authenticates via its own service token (which carries appId in the JWT). |
There was a problem hiding this comment.
The x-inkeep-app-id header is still read into reqData.appId (line 83) and still used here to gate entry into tryAppCredentialAuth. For the A2A path (subAgentId is set), this correctly skips app credential auth and relies on the JWT. However, an external caller could send x-inkeep-app-id + x-inkeep-sub-agent-id headers together with an API key to bypass the tryAppCredentialAuth path and still have reqData.appId floating around in the request data.
This is safe now because reqData.appId is no longer copied into metadata anywhere — but you could tighten this further by clearing reqData.appId when it doesn't come from a verified source (i.e., when it's just a header value and the auth strategy is not app-credential). Not blocking, just defense-in-depth.
| // Parent agents already have appPrompt set at auth time. | ||
| if (!config.executionContext.metadata?.appPrompt && config.executionContext.metadata?.appId) { | ||
| // Resolve appPrompt from DB using tenant-scoped lookup. | ||
| if (config.executionContext.metadata?.appId) { |
There was a problem hiding this comment.
Previously this had !config.executionContext.metadata?.appPrompt && as a guard, meaning it would skip the DB lookup if appPrompt was already set. Removing that guard is the right call for security — it ensures the prompt always comes from the DB, not from anything an upstream caller could have injected. Worth noting this adds one DB query per task handler invocation when appId is set, even when the prompt hasn't changed. If this becomes a hot path concern, consider a short TTL cache keyed on (appId, tenantId) — but that's a future optimization, not a blocker.
| originAgentId: agentId, | ||
| targetAgentId: currentAgentId, | ||
| initiatedBy, | ||
| appId: executionContext.metadata?.appId, |
There was a problem hiding this comment.
Clean. appId is now propagated through the signed JWT claim rather than the x-inkeep-app-id header, and the header forwarding (...(appId ? { 'x-inkeep-app-id': appId } : {})) is removed at line 315. This ensures sub-agents can only receive an appId that was cryptographically verified by the parent's service token.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This PR correctly fixes a security vulnerability where appId (and by extension appPrompt) could be spoofed via the x-inkeep-app-id header during A2A forwarding. The fix is comprehensive and well-implemented.
✅ Security Assessment
The security fix is complete and correctly implemented:
appIdis now cryptographically protected — Embedded in signed JWT service tokens using HMAC-SHA256, eliminating header-based spoofing- Header forwarding removed — The
x-inkeep-app-idheader is no longer propagated in A2A delegation calls - Tenant-scoped lookup enforced —
appPromptresolution usesgetAppByIdForTenantwhich requires matchingtenantId, preventing cross-tenant prompt injection - Fresh resolution on every request — Changed from
if (!appPrompt && appId)toif (appId), ensuring stale/malicious prompts in metadata are always overwritten
💭 Consider
💭 1) runAuth-appPrompt.test.ts Add explicit cross-tenant attack prevention test
Issue: The test suite verifies header-based appId is blocked, but lacks an explicit test verifying that a service token JWT with an appId from a different tenant results in the tenant-scoped lookup returning null.
Why: While the code correctly uses getAppByIdForTenant with tenant scoping, the tests mock the DB layer and don't explicitly exercise this security property. An explicit test would catch regressions if tenant scoping is accidentally bypassed.
Fix: Consider adding a test that mocks getAppByIdForTenant to return undefined for cross-tenant lookups and verifies the handler continues without appPrompt:
it('should continue without appPrompt when app belongs to different tenant', async () => {
const mockGetApp = vi.fn().mockResolvedValue(undefined);
(getAppByIdForTenant as any).mockReturnValue(mockGetApp);
// ... verify appPrompt remains undefined
});Refs: generateTaskHandler.test.ts:1134
🕐 Pending Recommendations (1)
- 🟡
generateTaskHandler.ts:92Performance consideration: DB query per handler invocation (flagged by prior review)
✅ APPROVE
Summary: This is a solid, well-implemented security fix. The vulnerability is correctly addressed by moving appId into the signed JWT service token and enforcing tenant-scoped lookups. The code is clean, tests are comprehensive for the core security scenarios, and the prior review feedback was addressed. Ship it! 🚀
Discarded (0)
No findings discarded.
Reviewers (3)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
pr-review-security-iam |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-standards |
0 | 0 | 0 | 0 | 0 | 0 | 0 |
pr-review-tests |
4 | 0 | 1 | 0 | 0 | 0 | 3 |
| Total | 4 | 0 | 1 | 0 | 0 | 0 | 3 |
Note: 3 test coverage suggestions from pr-review-tests were assessed as LOW confidence / low-signal and discarded (edge case validation, integration test coverage, appId format validation).
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This re-review is scoped to the delta since the last automated review. The only change is a merge from main (commit 544b60f3f) which brought in PR #2964 (copilot auth routing refactor). No new PR-specific code was introduced — the security fix remains exactly as previously approved.
✅ Delta Verified
The merge from main cleanly integrated with this PR:
- Removed
tryTempJwtAuthfunction (moved to dedicated app credential path in #2964) - Removed
verifyTempTokenimport andisCopilotAgentutility - Auth flow order adjusted accordingly
These changes are orthogonal to the security fix. The core security properties remain intact:
appIdcryptographically protected — Embedded in signed JWT service tokens- JWT-based agent identity —
agentIdfrom JWTsub,subAgentIdfrom JWTaud - Tenant-scoped lookup —
getAppByIdForTenantrequires matchingtenantId - Header forwarding removed —
x-inkeep-app-idno longer propagated
🕐 Pending Recommendations (1)
- 🟡
generateTaskHandler.ts:92Performance consideration: DB query per handler invocation whenappIdis set (future optimization, not blocking)
✅ APPROVE
Summary: The merge from main integrated cleanly. All security properties from the previous approved review remain intact. Ready to ship! 🚀
Discarded (0)
No new findings to discard.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched for this delta — the only change was a merge from main with no new PR-specific code.
There was a problem hiding this comment.
PR Review Summary
(1) Total Issues | Risk: Low
This re-review is scoped to the delta since the last automated review (2 commits, 3 files, +32/-17 lines). The changes strengthen the security fix.
✅ Delta Changes Verified
The delta changes are correct and improve security:
-
Project-scoped lookup — Changed from
getAppByIdForTenanttogetAppByIdForProject. This is a stricter security boundary — apps are now validated against bothtenantIdANDprojectId, preventing cross-project prompt injection within the same tenant. -
Tests updated — Test assertions now verify the project-scoped lookup receives both
tenantIdandprojectIdin thescopesparameter. -
Changeset added — Properly documents the security fix for release notes (minor typo flagged as inline comment).
🟡 Minor (1) 🟡
Inline Comments:
- 🟡 Minor:
.changeset/nice-candles-wonder.md:6Typo: "Vulerability" → "Vulnerability"
🕐 Pending Recommendations (1)
- 🟡
generateTaskHandler.ts:92Performance consideration: DB query per handler invocation whenappIdis set (future optimization, not blocking)
✅ APPROVE
Summary: The delta strengthens the security fix by adding project-level scoping on top of tenant-level scoping. This is a strictly better security boundary. The only issue is a typo in the changeset message. Ship it! 🚀
Discarded (0)
No findings discarded.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched — the delta was small (3 files, +32/-17 lines) and only strengthened the existing security fix with stricter scoping.
| "@inkeep/agents-api": patch | ||
| --- | ||
|
|
||
| Bugfix App Prompt Security Vulerability |
There was a problem hiding this comment.
🟡 Minor: Typo in changeset message
Issue: "Vulerability" should be "Vulnerability".
Why: Changeset messages appear in release notes and should be spelled correctly.
Fix: (1-click apply)
| Bugfix App Prompt Security Vulerability | |
| Bugfix App Prompt Security Vulnerability |
|
@pullfrog, tell me do any other security vulnerabilities exist in the code i changed? |
There was a problem hiding this comment.
Security Audit: Residual Vulnerabilities in Changed Code
This PR does a solid job closing the primary attack vectors — moving agentId/subAgentId derivation to the cryptographically verified JWT, scoping the appPrompt lookup to getAppByIdForProject, and propagating appId through the service token instead of headers. Three residual issues remain in the changed code, ordered by severity.
Severity: 1 medium, 2 low. None are critical, but the medium one is worth addressing before merge.
Claude Opus ｜ 𝕏
| @@ -108,6 +112,7 @@ export async function verifyServiceToken(token: string): Promise<VerifyServiceTo | |||
| } | |||
|
|
|||
| const initiatedBy = payload.initiatedBy as { type: 'user' | 'api_key'; id: string } | undefined; | |||
There was a problem hiding this comment.
[Medium — trust-boundary gap] initiatedBy is blindly cast without validating its shape.
The appId field on the next line correctly uses typeof ... === 'string' to validate, but initiatedBy is just cast via as. A malformed service token with initiatedBy: { type: "admin", id: "..."} or initiatedBy: "garbage" passes through unchecked.
Since initiatedBy.type flows into runAsUserId logic (executionHandler.ts:301-306 — only type === 'user' is accepted) and audit trails, a crafted token could inject unexpected values into metadata. The service token is signed so this requires a compromised signing key, but defense-in-depth says validate at the boundary.
Suggested fix — validate shape before accepting, same pattern as appId:
const initiatedBy =
payload.initiatedBy &&
typeof payload.initiatedBy === 'object' &&
(payload.initiatedBy.type === 'user' || payload.initiatedBy.type === 'api_key') &&
typeof payload.initiatedBy.id === 'string'
? (payload.initiatedBy as { type: 'user' | 'api_key'; id: string })
: undefined;| // When subAgentId is set, this is an internal A2A call — skip app credential auth. | ||
| // The appId is forwarded for context only; the sub-agent authenticates via its own token. | ||
| // The sub-agent authenticates via its own service token (which carries appId in the JWT). | ||
| if (reqData.appId && !subAgentId) { |
There was a problem hiding this comment.
[Low — auth routing confusion] x-inkeep-app-id header can force auth strategy selection.
An external caller with a valid database API key can attach x-inkeep-app-id: <any-real-app-id> (without x-inkeep-sub-agent-id) to force the request into tryAppCredentialAuth. The API key is then treated as a bearer token for JWT verification, which fails — the request is rejected rather than falling through to tryApiKeyAuth.
This is pre-existing (not introduced by this PR) and low severity (DoS of your own request, not privilege escalation), but worth noting since the auth-routing code was touched. A future hardening pass could attempt remaining strategies if tryAppCredentialAuth returns authResult: null.
| apiKeyId: authResult.apiKeyId, | ||
| baseUrl: reqData.baseUrl, | ||
| subAgentId: reqData.subAgentId, | ||
| subAgentId, |
There was a problem hiding this comment.
[Low — observation] subAgentId is no longer set from reqData.subAgentId for non-team-delegation paths.
Before this PR, subAgentId was always set from reqData.subAgentId (the x-inkeep-sub-agent-id header). Now it's only set in the teamDelegation branch from the verified JWT. This is a security improvement — external callers can no longer inject a subAgentId into their execution context by sending the header.
However, confirm that no legitimate non-team-delegation code path relies on subAgentId being set in the execution context. The header is still read and used for auth routing (authenticateRequest at line 814, tryTeamAgentAuth at line 848), but it no longer flows into the execution context for non-team paths. This looks intentional and correct — flagging for visibility.
There was a problem hiding this comment.
PR Review Summary
(0) Total Issues | Risk: Low
This re-review is scoped to the delta since the last automated review (1 commit, 1 file, +9/-1 lines). The change directly addresses pullfrog's Medium severity finding.
✅ Delta Verified
The single change in service-token-auth.ts:114-122 is correct and complete:
Before:
const initiatedBy = payload.initiatedBy as { type: 'user' | 'api_key'; id: string } | undefined;After:
const initiatedBy =
payload.initiatedBy &&
typeof payload.initiatedBy === 'object' &&
'type' in payload.initiatedBy &&
'id' in payload.initiatedBy &&
(payload.initiatedBy.type === 'user' || payload.initiatedBy.type === 'api_key') &&
typeof payload.initiatedBy.id === 'string'
? (payload.initiatedBy as { type: 'user' | 'api_key'; id: string })
: undefined;Security assessment:
- ✅ Addresses pullfrog's Medium finding —
initiatedByis no longer blindly cast; it's now properly validated for shape - ✅ Defense-in-depth — Even though the service token is signed (requiring a compromised key to exploit), validating at the boundary is correct practice
- ✅ Consistent pattern — Uses the same validation pattern as
appIdon line 123 - ✅ Correct type narrowing — Validates object type, key presence, enum values, and string
id - ✅ Graceful fallback — Returns
undefinedif validation fails, maintaining backward compatibility
🕐 Pending Recommendations (2)
- 🟡
.changeset/nice-candles-wonder.md:6Typo: "Vulerability" → "Vulnerability" - 🟡
generateTaskHandler.ts:92Performance consideration: DB query per handler invocation whenappIdis set (future optimization, not blocking)
✅ APPROVE
Summary: The delta correctly addresses the Medium severity finding from pullfrog's security audit. The initiatedBy field is now properly validated instead of blindly cast, closing the trust-boundary gap. All prior security properties remain intact. The only outstanding items are a typo in the changeset message and a future performance optimization — neither blocking. Ship it! 🎉
Discarded (0)
No findings discarded.
Reviewers (0)
| Reviewer | Returned | Main Findings | Consider | While You're Here | Inline Comments | Pending Recs | Discarded |
|---|---|---|---|---|---|---|---|
| Total | 0 | 0 | 0 | 0 | 0 | 0 | 0 |
Note: No sub-reviewers dispatched — the delta was minimal (1 file, +9/-1 lines) and directly addressed a prior finding.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
1 participant
No description provided.