Skip to content

fix(tasks): restore cloud artifact uploads - #3858

Merged
trunk-io[bot] merged 5 commits into
mainfrom
posthog-code/fix-sandbox-artifact-oauth-credential
Jul 28, 2026
Merged

fix(tasks): restore cloud artifact uploads#3858
trunk-io[bot] merged 5 commits into
mainfrom
posthog-code/fix-sandbox-artifact-oauth-credential

Conversation

@puemos

@puemos puemos commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Problem

Cloud artifact uploads can report that storage is not configured before making any API or object-storage request. The sandbox credential refresh work in PostHog/posthog#72540 deliberately separated rotating credentials from general /tmp/agent-env metadata:

  • GitHub credentials live in /tmp/agent-github-env
  • the PostHog OAuth credential lives in /tmp/agent-oauth-env
  • an existing but empty credential file is an explicit logout/revocation

The shared PostHog API client resolver still looked for POSTHOG_PERSONAL_API_KEY in /tmp/agent-env or the process-start environment. That misses refreshed credentials in Codex local-tool processes and could resurrect a stale process token after revocation.

Codex exposed the missing-key path after #3844 restored the task run ID passed to its local-tools process: calls then advanced to the storage-configuration guard because no PostHog API client could be constructed.

Why: Agent-created reports and other non-code deliverables need the current managed sandbox identity to prepare and finalize durable task artifact uploads. Credential refresh and revocation must both take effect without restarting the agent.

Changes

  • Read the rotating personal API key exclusively from /tmp/agent-oauth-env
  • Resolve the key from one file snapshot for every request and authentication retry
  • Require the managed OAuth credential file; missing, empty, or unreadable files all fail closed
  • Keep non-secret API URL and project metadata resolution unchanged
  • Share one tested NUL-delimited environment parser across metadata and credential files
  • Add regression coverage for rotation between requests, authenticated retries, explicit revocation, and invalid managed files

How did you test this?

  • pnpm --filter @posthog/agent exec vitest run src/signed-commit-artefacts.test.ts src/posthog-api.test.ts (19 tests)
  • pnpm --filter @posthog/agent typecheck
  • pnpm exec biome check packages/agent/src/signed-commit-artefacts.ts packages/agent/src/signed-commit-artefacts.test.ts

Automatic notifications

  • Publish to changelog?
  • Alert Sales and Marketing teams?

Created with PostHog Code

Read the rotating PostHog API credential from the dedicated sandbox OAuth file while retaining legacy and process-environment fallbacks. Add regression coverage for the split sandbox credential layout.

Generated-By: PostHog Code
Task-Id: 833df099-31c5-4930-addb-305cc5fe2def
@trunk-io

trunk-io Bot commented Jul 28, 2026

Copy link
Copy Markdown

😎 Merged successfully - details.

@github-actions

github-actions Bot commented Jul 28, 2026

Copy link
Copy Markdown

React Doctor found no issues in the changed files. 🎉

Reviewed by React Doctor for commit 77c4658.

Treat the dedicated OAuth credential file as authoritative when present. Empty or unreadable managed files now prevent fallback to stale launch-time credentials, matching the sandbox credential revocation contract.

Generated-By: PostHog Code
Task-Id: 833df099-31c5-4930-addb-305cc5fe2def
@puemos puemos self-assigned this Jul 28, 2026
Remove legacy and process-token fallbacks for sandbox PostHog authentication. Artifact and task metadata clients now require the dedicated OAuth credential file and fail closed when it is missing, empty, or unreadable.

Generated-By: PostHog Code
Task-Id: 833df099-31c5-4930-addb-305cc5fe2def
@puemos

puemos commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@tatoalo i'd love your eyes on that since I'm not sure if that's the right approach here. my trust in the fix isn't 100% since the changes in the env file were merged last week so I wonder why we didn't have issues until now

if (raw.trim() === "") {
return "";
}
const oauthEnv = readSandboxEnvFile(oauthEnvFilePath);

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.

I believe here we are reading thefile a second time after validating the first snapshot, a refresh between those reads could validate one generation and parse another. Could we parse POSTHOG_PERSONAL_API_KEY directly from raw so each resolution uses one consistent snapshot?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 93fc7d0: the resolver now parses the token directly from the single readFileSync snapshot, so validation and parsing cannot observe different file generations.

@tatoalo tatoalo 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.

getApiKey always returns the token captured when the client is created, if the managed OAuth file is rotated or truncated between requests, subsequent requests and the built-in 401/403 retry will reuse the stale token, potentially leaving artifact reporting partially complete which is a bit of a mess for users to end-up in, could we reread the managed file in getApiKey/refreshApiKey and add a test that changes the file between requests?

Read the managed OAuth file from a single snapshot for each credential resolution. Re-resolve credentials for every request and authentication retry so rotation and revocation take effect without restarting the agent.

Generated-By: PostHog Code
Task-Id: 833df099-31c5-4930-addb-305cc5fe2def

puemos commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

@tatoalo Addressed both points in 93fc7d0: getApiKey and refreshApiKey now reread the managed OAuth file for every resolution, and focused tests rotate the real file between ordinary requests and a 401 retry. The parser uses one snapshot per resolution. Would appreciate a re-review when convenient.


Sorry my agent decided to post without asking me :/

Extract the NUL-delimited environment parser and use its parsed object for OAuth token lookup. Add direct coverage for normal, malformed, and equals-containing entries.

Generated-By: PostHog Code
Task-Id: 833df099-31c5-4930-addb-305cc5fe2def

puemos commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Follow-up in 77c4658: extracted a pure, directly tested NUL-delimited environment parser and replaced the key-specific loop with an object lookup. The OAuth file is still read exactly once per credential resolution.

@tatoalo tatoalo 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.

thanks for addressing, looks great!

@puemos
puemos marked this pull request as ready for review July 28, 2026 10:58
oauthEnvFilePath: string = SANDBOX_OAUTH_ENV_FILE,
): SandboxPosthogApi | undefined {
const fileEnv = readSandboxEnvFile(envFilePath);
const oauthToken = readSandboxOauthToken(oauthEnvFilePath);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Medium: Managed OAuth token can be redirected to an untrusted host

The managed token is paired with POSTHOG_API_URL from /tmp/agent-env. A malicious repository can induce the agent to overwrite that general environment file with an attacker-controlled URL and then invoke artifact upload or signed commit reporting, causing PostHogAPIClient to send Authorization: Bearer <token> to that host. Source the API origin from the protected credential configuration or validate it against an explicit HTTPS PostHog-origin allowlist before constructing the client.

@veria-ai

veria-ai Bot commented Jul 28, 2026

Copy link
Copy Markdown

PR overview

This pull request restores cloud artifact uploads and related signed commit artifact reporting through the PostHog API client.

One security issue remains open: a malicious repository can redirect a managed OAuth token to an attacker-controlled host by modifying the agent environment configuration. This enables credential disclosure when artifact uploads or signed commit reporting are invoked, though the impact is limited to the exposed token and its permissions. No issues have yet been addressed.

Open issues (1)

Fixed/addressed: 0 · PR risk: 6/10

@trunk-io
trunk-io Bot merged commit 4961b52 into main Jul 28, 2026
40 checks passed
@trunk-io
trunk-io Bot deleted the posthog-code/fix-sandbox-artifact-oauth-credential branch July 28, 2026 11:12
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.

2 participants