fix(appkit): trim x-forwarded-user in core OBO path#427
Open
atilafassina wants to merge 2 commits into
Open
Conversation
resolveUserId and asUser now trim the x-forwarded-user header at read time, so surrounding whitespace can't fork user identity or the per-user analytics cache key. Mirrors the existing files-plugin precedent; whitespace-only values resolve to the missing-header path (prod throws, dev falls back). Co-authored-by: Isaac Signed-off-by: Atila Fassina <atila@fassina.eu>
Lock the Phase 1 trim: resolveUserId/asUser normalize a padded x-forwarded-user to the bare id; whitespace-only takes the missing-header path (prod throws, dev falls back); and two OBO analytics requests differing only by header whitespace share one cache key. Co-authored-by: Isaac Signed-off-by: Atila Fassina <atila@fassina.eu>
MarioCadenas
approved these changes
Jun 9, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the core “on-behalf-of user” (OBO) identity resolution by trimming x-forwarded-user at both read sites, preventing whitespace variants from forking user identity and per-user analytics caching/telemetry.
Changes:
- Trim
x-forwarded-userinPlugin.resolveUserId(req)andPlugin.asUser(req). - Add regression tests to pin whitespace normalization behavior (including prod vs dev behavior for whitespace-only values).
- Add an analytics regression test ensuring OBO requests with whitespace-only differences share the same per-user cache key.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/appkit/src/plugin/plugin.ts | Trims x-forwarded-user in core OBO identity resolution (resolveUserId, asUser). |
| packages/appkit/src/plugin/tests/asUser-proxy.test.ts | Adds tests asserting header trimming behavior and whitespace-only handling paths. |
| packages/appkit/src/plugins/analytics/tests/analytics.test.ts | Adds regression test ensuring whitespace variants of x-forwarded-user do not fork OBO cache keys. |
Comments suppressed due to low confidence (1)
packages/appkit/src/plugin/plugin.ts:418
resolveUserIdthrowsAuthenticationError.missingToken(...)but passes a full sentence starting with "Missing ..." as the tokenType, which results in a duplicated/awkward message ("Missing Missing ... in request headers"). This should usemissingUserId()(consistent withasUser) or pass just the header name tomissingToken.
throw AuthenticationError.missingToken(
"Missing x-forwarded-user header. Cannot resolve user ID.",
);
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
432
to
434
| const token = req.header("x-forwarded-access-token"); | ||
| const userId = req.header("x-forwarded-user"); | ||
| const userId = req.header("x-forwarded-user")?.trim(); | ||
| const userEmail = req.header("x-forwarded-email"); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Trims
x-forwarded-userat both core OBO read sites (Plugin.resolveUserId+asUser) so surrounding whitespace can't fork user identity or the per-user analytics cache key; a whitespace-only value takes the missing-header path (prod throws, dev falls back). Mirrors the existing files-plugin precedent.Includes 7 regression tests, notably one asserting that two OBO requests differing only by header whitespace share a single analytics cache key.
This covers a small edge-case where white-space in the user ID would generate a separate cache and telemetry entry.