feat(webapp): enforce scopes for environment API keys - #4389
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| Name | Type |
|---|---|
| @trigger.dev/core | Patch |
| @trigger.dev/sdk | Patch |
| @trigger.dev/build | Patch |
| trigger.dev | Patch |
| @trigger.dev/python | Patch |
| @trigger.dev/redis-worker | Patch |
| @trigger.dev/schema-to-json | Patch |
| @internal/cache | Patch |
| @internal/clickhouse | Patch |
| @internal/llm-model-catalog | Patch |
| @trigger.dev/rbac | Patch |
| @internal/redis | Patch |
| @internal/replication | Patch |
| @internal/run-engine | Patch |
| @internal/run-store | Patch |
| @internal/schedule-engine | Patch |
| @trigger.dev/sso | Patch |
| @internal/testcontainers | Patch |
| @internal/tracing | Patch |
| @internal/tsql | Patch |
| @internal/dashboard-agent | Patch |
| @internal/sdk-compat-tests | Patch |
| @trigger.dev/react-hooks | Patch |
| @trigger.dev/rsc | Patch |
| @trigger.dev/database | Patch |
| @trigger.dev/otlp-importer | Patch |
Click here to learn what changesets are, and how to add one.
Click here if you're a maintainer who wants to add a changeset to this PR
0e70a0b to
d20a5af
Compare
37de624 to
6b31535
Compare
@trigger.dev/build
trigger.dev
@trigger.dev/core
@trigger.dev/python
@trigger.dev/react-hooks
@trigger.dev/redis-worker
@trigger.dev/rsc
@trigger.dev/schema-to-json
@trigger.dev/sdk
commit: |
6b31535 to
a8e673e
Compare
A number passed to `expirationTime` is a Unix timestamp in seconds, not milliseconds as the JSDoc claimed. Following the old docs produced a token that effectively never expired. Also fail loudly when an additional API key reaches a local self-signing fallback. Those keys are not the environment's JWT signing material, so the token would never verify. Every endpoint that returns a public access token sets `x-trigger-jwt`, so this is unreachable today.
The API-key policy methods are capability extensions, so declare them optional on RoleBaseAccessController and normalize the surface in LazyController rather than requiring every implementation to carry them. An authorization extension is compiled against whichever core commit its base image ships, so a required additive method forces both sides to move together and turns rolling an extension back into a build failure instead of a graceful degradation. Absence fails closed: no preset catalogue, and prepareApiKeyPolicy refuses outright rather than defaulting to full access, so an extension below this contract cannot mint a credential. Keys already issued are unaffected — they authorize from the scopes persisted on their row. loader.create() now returns HostRbacController, the total surface, so host callers neither guard nor invent their own absent-extension default.
36f03ba to
51e1189
Compare
a8e673e to
24771ce
Compare
Add a global, org-locked kill switch (additionalApiKeyLookupEnabled, default off) gating the additional environment API-key lookup, and bounded OTel auth telemetry (api_auth.attempts counter, api_auth.duration_ms histogram, api_auth.rollout_mode gauge) to watch the rollout. Attributes are closed enums only (resolver, credential_kind, result, lookup_path); no credentials, hashes, or tenant identifiers are recorded.
Revert authenticateApiKeyWithScope to rbac.authenticateAuthorizeBearer (the kill switch still applies via the rbac controller), and make the auth telemetry tolerate a missing resolution from test doubles. Also register the v3/apiKeys subpath in core's typesVersions for node10.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| triggerAction: "trigger", | ||
| }); | ||
|
|
||
| const $responseHeaders = await responseHeaders( | ||
| batch, | ||
| authentication.environment, | ||
| triggerClient | ||
| ); | ||
| const $responseHeaders = await publicAccessTokenResponseHeaders({ | ||
| environment: authentication.environment, | ||
| scopes: [`read:batch:${batch.id}`], | ||
| expirationTime: "1h", | ||
| }); |
There was a problem hiding this comment.
🔍 v1 array-batch route lacks the parent-run ownership check its sibling batch routes gained
This PR adds canWriteParentRun(...) guards to the trigger route, api.v1.tasks.$taskId.batch.ts, api.v2.tasks.batch.ts, and api.v3.batches.ts, so that a restricted (task-scoped) additional key cannot link/resume a parent run it does not own. The legacy array-batch route api.v1.tasks.batch.ts was touched in this PR (server-issued token headers at apps/webapp/app/routes/api.v1.tasks.batch.ts:125-129) but was NOT given the same parent-run authorization. Its items use BatchTriggerTaskV2RequestBody, whose per-item options.parentRunId is honored by BatchTriggerV3Service. Because restricted additional keys are new in this PR and this route only fails-closed on task scopes (via everyResource), a task-scoped key that legitimately holds batchTrigger on the batched tasks could still submit items referencing an arbitrary parentRunId it has no write access to. Worth confirming whether this route intentionally omits the check or is a gap. (This is authorization-related and better handled by the dedicated security scan.)
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Summary
Environment API keys backed by the additional-key table can authenticate API requests using their stored effective scopes. Revoked and expired keys are rejected, branch environments retain their existing routing behavior, and last-used timestamps are updated on a throttled best-effort basis.
Design
API route builders receive the resolved ability and reject restricted keys on routes without an authorization declaration. Existing deployment, environment variable, queue, run, task, batch, session, and waitpoint routes declare the resources they access.
Trigger and batch responses return server-signed public access tokens, so additional keys never need access to the environment signing secret. Root-key rotation also keeps public tokens valid for the existing grace window.
Deployment notes
The schema migration must be present before this code is deployed. Because bearer resolution runs on every authenticated request, deploy the resolver with additional-key lookup disabled, verify root-key and public-token parity, then enable lookup before any additional keys can be issued.
The multi-task authorization tightening changes the result for narrowly scoped tokens that request tasks outside their grants. Observe would-deny results before enforcing that check. Request-idempotency keys are also newly isolated by environment and task, so a retry crossing the deployment boundary may execute once more before old cache entries expire.
Follow-ups
Add would-deny observability and an independent enforcement switch for multi-task authorization.Add an independent switch for server-issued batch tokens while root-key parity is verified.