Skip to content

fix(ai): send actor user id in OpenAI-style user identifiers - #3856

Open
rasadregmi wants to merge 1 commit into
HeyPuter:mainfrom
rasadregmi:fix/ai-user-identifier
Open

fix(ai): send actor user id in OpenAI-style user identifiers#3856
rasadregmi wants to merge 1 commit into
HeyPuter:mainfrom
rasadregmi:fix/ai-user-identifier

Conversation

@rasadregmi

@rasadregmi rasadregmi commented Sep 12, 2026

Copy link
Copy Markdown

A precedence bug in the AI providers' identifier expression made every request send user: ':undefined' (the ternary bound to the whole actor?.user.id + actor?.app?.uid ? … : … instead of just the app-uid suffix). Rather than patching each provider, this PR replaces the inline builder in all eight OpenAI-/Azure-/xAI-style providers with one shared helper.

What changed

  • New aiUserIdentifier() in src/backend/drivers/util/aiUserIdentifier.ts, used by OpenAI chat (completions + responses), Azure chat (completions + responses), OpenAI/xAI image, and now also Meta and ZAI (which carried the same flawed pattern and now use the helper too).
  • The identifier is puter-[-] — the user's random UUID, never the sequential id (which leaked signup order/count to vendors), capped at 64 chars (ZAI keeps 128; Meta/ZAI still honor a caller-supplied safety_identifier/user_id override).
  • App attribution reads actor.effectiveApp ?? actor.app, so requests authenticated via an app's access token name the issuing app instead of looking like the user directly.
  • System actors send no identifier.

Verification

  • Per-provider tests cover user-only, app, access-token (via effectiveApp), and system-actor cases; the two Azure safety_identifier assertions that passed vacuously under the system actor now run under a real user actor; 8 new access-token cases, one per provider.
  • 9 affected suites: 203 tests pass.
  • npm run typecheck: no new errors. npm run test:backend: green for everything touched; remaining failures are pre-existing env-dependent flakes (Postgres integration timeout, share-email) that pass in isolation.

@CLAassistant

CLAassistant commented Sep 12, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@ProgrammerIn-wonderland

Copy link
Copy Markdown
Collaborator

@404oops can you review?

@404oops 404oops self-assigned this Sep 12, 2026

@404oops 404oops left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this is ready to merge. The fix creates several new problems, mostly because it reuses the same flawed pattern in multiple places instead of centralizing how the safety identifier is built.

When a request is authenticated with an access token instead of a direct user session, the actor record does not put the issuing app in actor.app, but in actor.effectiveApp.

The current code reads actor.app?.uid, so for access-token requests, the provider still gets the user ID, but no app ID. This means you can't tell whether abuse is coming from a third-party app, or from the user directly.

It also leaks our internal sequential user IDs to external AI vendors. That lets them correlate Puter users across different services, and estimate rough signup order and total user count.

This is especially bad, because the old code accidentally avoided sending the raw ID at all due to a precedence bug. Azure and OpenAI previously sent :undefined; the others sent an empty string because numeric ID plus undefined became NaN.

With this PR, OpenAI, Azure, and xAI start sending the raw integer for the first time. Meta and ZAI already receive it as puter-<id>. We should send a UUID or a hash instead, but that should be up to us to decide.

The PR description says it mirrors MetaProvider, but that's inaccurate. MetaProvider and ZAIProvider have the same actor.app?.uid bug and the same raw integer problem, so copying them just spreads it across 8 places, instead of fixing it.

Also, Meta and ZAI use the format puter-<id>-<appUid>, while this code uses <id>:<appUid>. This should've been one shared helper that reads actor.effectiveApp ?? actor.app and returns a non-sequential identifier.

The ?? actor.app fallback is safe because makeActor derives effectiveApp from app, so a null effectiveApp always means app is null too. The fallback is only needed because the new tests construct actor objects directly without using makeActor, leaving effectiveApp undefined. Either keep the fallback or update the tests to use makeActor and only read actor.effectiveApp.

Test gaps

  • The existing Azure test titled "sends safety_identifier for OpenAI deployments" now runs under the system actor. With the new code, both relevant fields are undefined, and the assertion passes only because { safety_identifier: undefined } still creates an undefined value. The test title no longer matches what it actually verifies. It should either run under a user actor or be renamed to assert that the value is undefined. The same applies to the safety_identifier assertion at the end of the Azure Responses request-shape test, which assigns undefined directly and likewise passes for the wrong reason.

  • The two Azure tests and the two image tests do not include a system-actor case, but the OpenAI chat tests do.

  • There is no access-token test anywhere. Adding one that expects 42:app-abc would fail against the current code.

  • The Grok "strip safety_identifier" test is still valid, because the identifier is removed before the request goes out.

To be clear, the precedence fix itself is correct, and the actor?.user?.id chaining removes the old TypeError. That part is fine. But the rest of the change is a quick fix that introduces bigger problems than it solves.

@rasadregmi
rasadregmi force-pushed the fix/ai-user-identifier branch from 3376353 to 012ef65 Compare September 13, 2026 06:27
@rasadregmi

Copy link
Copy Markdown
Author

@404oops
Thanks for the thorough review, I've reworked the change along those lines.

One shared helper. All eight providers now build their identifier via src/backend/drivers/util/aiUserIdentifier.ts, and Meta/ZAI use it too:

export const aiUserIdentifier = (actor, maxLength = 64) => {
if (!actor || isSystemActor(actor)) return undefined;
const userUuid = actor.user?.uuid;
if (!userUuid) return undefined;
const appUid = actor.effectiveApp?.uid ?? actor.app?.uid;
const identifier = appUid ? puter-${userUuid}-${appUid} : puter-${userUuid};
return identifier.slice(0, maxLength);
};

  • Reads effectiveApp ?? app, so access-token requests name the issuing app (your makeActor note is exactly why the fallback is safe to keep).
  • Sends the user's random UUID, never the sequential id, no raw integer leaks, and the old :undefined/NaN noise is gone entirely.
  • Returns undefined for the system actor and when there's no UUID.
  • Dash separators, matching the existing puter-<...>- format Meta/ZAI already used.
  • Same caps as before: 64 everywhere, ZAI keeps 128. Meta/ZAI still honor a caller-supplied safety_identifier/user_id override.

Test updates.

  • Access-token cases added to every provider suite, a token issued by an app now sends puter-u42-app-abc via effectiveApp, which fails against the old actor.app-only read.
  • System-actor cases added to the Azure chat, Azure responses, and both image suites (asserted undefined).
  • Vacuous Azure assertions fixed: sends safety_identifier for OpenAI deployments and the Azure Responses check now run under a real user actor instead of passing on { safety_identifier: undefined }.

Verification. Nine affected suites pass (203 tests); npm run typecheck has no new errors. Full npm run test:backend is green for everything touched by this change, the only intermittent failures are pre-existing env-dependent ones (PostgresDatabaseClient.integration 180s timeouts and the share-email suite: they pass in isolation and flake run-to-run regardless of this diff, and this run had a different subset fail than the last).

@rasadregmi
rasadregmi requested a review from 404oops September 13, 2026 06:35

@404oops 404oops left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I have more notes on this.

  1. src/backend/drivers/util/aiUserIdentifier.ts at line 48: The identifier this produces for a real user and a real app does not survive the cap on this line intact. The tests do not catch it.

  2. src/backend/drivers/ai-image/providers/xai/XAIImageProvider.ts at line 183: The edit path does not do what the generation path at line 130 does.

  3. src/backend/drivers/ai-chat/providers/azure/AzureChatProvider.test.ts at line 468: This test would still pass if user were removed from the Grok branch it covers.

  4. OpenAiChatCompletionsProvider.ts at line 184 and AzureChatProvider.ts at line 211: Switching user from per-app to per-user has a side effect on OpenAI's end that the PR does not address or mention.

  5. aiUserIdentifier.ts at lines from 23 to 39: The exported constant, its comment, and the maxLength parameter do not hold up when checked against their callers and against the OpenAI SDK docs.

  6. aiUserIdentifier.ts at line 44: The fallback on this line is not reachable by any actor the codebase actually builds, and the test that covers it shows why.

  7. ZAIProvider.test.ts at line 409: Comment is stale.

  8. The commit subject contradicts the commit body.

  9. OpenAiImageProvider.test.ts at line 254 is in the wrong describe block.

  10. The four-actor test matrix is pasted into six files.

Please, take a look at the code, attack it from all sides and try to do extensive reviews. I think your agent did a shallow analysis, which lead to these problems and holes.

I'm not saying this PR is bad at all, this is a good PR, but some things are simply overlooked and that's a BIG issue when you're attempting to fix a problem.

Do fix those issues, and if you (or your agent) find more within the scope of what you were trying to solve, don't hesitate to fix them as well.

A precedence bug in the AI providers' identifier expression made every
request send 'user: :undefined' (the ternary bound the app-uid suffix to
the whole actor.user.id + actor.app?.uid sum instead of just the suffix),
or read actor.user.id on a missing user. Rather than patching one
expression, all eight OpenAI-, Azure- and xAI-style providers share a
single builder:

- puter-<user-uuid>[-<app-token>] — the random UUID, never the
  sequential id that leaked signup order and let vendors correlate
  accounts
- the full UUID is always preserved; only the app token is truncated to
  fit each vendor's cap (64 for OpenAI/Meta 'safety_identifier', 128 for
  ZAI 'user_id'), and dropped entirely when it has no budget
- app attribution comes from effectiveApp, so access-token requests name
  the issuing app instead of looking like direct user traffic
- nothing is sent for the system actor

Meta and ZAI carried the same flawed pattern (raw id, app ignored under
access tokens); they now use the helper too, still honoring a
caller-supplied override.

Tests: a shared four-actor matrix (user / app / access-token / system)
across the six OpenAI-style suites; xAI image edit requests carry the
identifier like generation does; the Azure Grok test now runs under a
real user actor so it can't pass vacuously; realistic production-length
UUID/app IDs exercise the truncation boundary exactly. 9 suites pass,
typecheck has no new errors.
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.

4 participants