Commit 7fd671a
fix(webapp): schedule & env-var write scoping (reject cross-project/env IDs) (#46)
* fix(webapp): scope schedule lookups by project and environment (TRI-9865)
Closes a cluster of cross-tenant IDORs in the schedule routes and services
by scoping every `findFirst({friendlyId})` to the caller's projectId, and
by gating the env-scoped schedule API on env-visibility so a low-trust
key (e.g. dev) can't see or mutate a schedule whose instances live in a
different env. Foreign environment IDs passed to CheckScheduleService now
fail closed instead of being silently filtered.
Three observable API changes are documented in
.server-changes/tri-9865-tenant-isolation-schedules.md.
Closes TRI-9865, TRI-10040, TRI-10041 (P0); TRI-9854, TRI-9869, TRI-9960,
TRI-9963, TRI-9997 (P1); TRI-9859 (P2).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* refactor(webapp): consolidate schedule env-visibility into tri-state helper
Address review feedback on the TRI-9865 batch:
- Promote findScheduleScopedToEnvironment to a tri-state helper
getScheduleEnvVisibility returning { status: 'visible' | 'hidden' |
'missing' }. PUT can disambiguate hidden (refuse, 404) from missing
(fall through to upsert's create path) without re-implementing the
visibility logic inline.
- All three call sites (GET / PUT / DELETE in api.v1.schedules.$id)
now share the single helper.
- Add test coverage for the deduplicationKey branch of
scheduleWhereClause (covered the sched_-prefix branch only before).
- Add DeleteTaskScheduleService test asserting DECLARATIVE schedules
cannot be deleted (surfaces as a service-layer error after the
visibility check passes).
- Rename the misleading "devEnv" test fixture to "stagingEnv" to match
its actual RuntimeEnvironment type.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* test(webapp): regression tests for schedule environment scoping
Scopes schedule read/update/delete/activate to the caller's environment via a
shared getScheduleEnvVisibility (visible/hidden/missing) helper. Real-Postgres
tests (testcontainers) cover checkSchedule, delete, setActive, and PUT upsert.
Verified RED on the shared visibility guard (cross-env detection flips); GREEN
13/13. Bundles the streamBatchItems timeout bump.
* fix(webapp): reject foreign environment IDs when upserting a schedule
CheckScheduleService.call previously intersected `project.environments`
with the caller-supplied `environmentIds` via `.filter()`, silently
dropping any ID that didn't belong to the authorized project. Downstream
UpsertTaskScheduleService.#createNewSchedule then iterated the raw input
and created TaskScheduleInstance rows pairing the validated projectId
with whichever environmentId the caller sent — including another
tenant's RuntimeEnvironment. When the schedule fires, the engine
triggers against the *referenced* environment, enabling cross-tenant
task execution if the victim env id is known.
Replace the silent intersection with an explicit rejection: build a
map of project.environments by id and throw `ServiceValidationError` on
the first foreign id, so #createNewSchedule and #updateExistingSchedule
never see an environmentId outside the authorized project.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(repro): remove reproducer from PR branch
* test(webapp): regression test for cross-project schedule env scoping
Extracts the env-scoping into a pure resolveProjectScopedEnvironments (returns a
foreign-id flag or the mapped envs) and unit-tests it: all-valid resolves;
foreign id rejected; foreign mixed with valid still rejected (not dropped);
empty ok. Verified RED against the pre-fix silent filter-drop, GREEN with the
rejection. Bundles the streamBatchItems timeout bump.
* fix(webapp): enforce per-env access when creating env vars
The env-vars `new` action passed user-supplied `environmentIds[]`
straight to `repository.create` after a project-membership check.
DEV environments are per-user (`RuntimeEnvironment.orgMember.userId`),
and the dashboard loader filters other members' DEV envs out of the
UI — but the action did not enforce the same filter. A project
member could submit `environmentIds=[my_dev, victim_dev]` plus a
key/value, and the value was injected into the victim's next task
run via `resolveVariablesForEnvironment` (whose secret-store key
prefix is `environmentvariable:<projectId>:<envId>:`).
Now query `runtimeEnvironment.findMany` with the same OR clause
`findEnvironmentBySlug` uses — non-DEV envs in the project, or DEV
envs owned by the requesting user — and refuse the submission if
any submitted ID is missing from the result.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* chore(repro): remove reproducer from PR branch
* test(webapp): regression test for env-var DEV-environment ownership
Extracts the per-env writability rule into a pure findUnauthorizedEnvironmentId
(shared env types writable by any member; DEV only by its owner; unknown id
rejected) and unit-tests it. Route now fetches the candidate envs and applies
the helper. Verified RED against the pre-fix no-check behaviour, GREEN with it.
Bundles the streamBatchItems timeout bump.
* fix(webapp): reject mixed valid/foreign environmentIds in env-var create/edit
The create()/edit() guard used `environmentIds.every((v) => !inProject(v))`, which
only errors when EVERY id is foreign — a single in-project id short-circuited it, so
a mixed array passed and the write loop stored values/secrets against another tenant's
environment. Switch to `.some` so any foreign id rejects the whole request.
Adds RED→GREEN cross-tenant regression tests and bumps the streamBatchItems CI timeout.
* test(webapp): isolate schedule scoping tests from the global prisma singleton
The checkSchedule/deleteTaskSchedule/setActiveOnTaskSchedule regression tests
imported the full services, which pull in ~/db.server; its eager global-prisma
$connect() becomes an unhandled rejection in a unit-test job with no reachable
global database, failing vitest even though every test passes.
Make schedules.server a leaf (type-only Prisma imports) and rewrite the three
tests to exercise the project/environment scoping primitives directly against the
container DB (scheduleWhereClause, scheduleUniqWhereClause,
resolveProjectScopedEnvironments) instead of importing the services.
* fix(webapp): enforce DEV-env ownership on env-var edit and delete
The per-user DEV-env write check was only on the create route; the edit/delete
value actions passed a user-controlled environmentId straight to the repository,
which only checks project membership — letting a member overwrite or delete an
env-var value in another member's DEV environment. Gate both actions with the
same findUnauthorizedEnvironmentId check the create route uses.
* format
* chore: consolidate server-change and tighten comments
* fix(webapp): use .some for schedule env visibility and gate activate/deactivate
A schedule can be bound to several environments at once, and the schedule
list surfaces a schedule for any environment it has an instance in. The
per-schedule visibility check required every instance to be in the caller's
environment, so a multi-environment schedule was listed but returned 404 on
GET/PUT/DELETE for the same key. Match the list's semantics: a schedule is
visible when it has at least one instance in the caller's environment.
Also apply the same environment-visibility gate to the activate and
deactivate endpoints, which previously scoped only by project and let a key
scoped to one environment enable/disable a schedule that runs only in another
environment of the same project.
* fix(webapp): reject empty-string foreign environment id instead of dropping it
The foreign-id guard used a truthiness check, so an empty-string environment
id (a foreign id) was falsy and fell through to the accepted path, where it
was silently dropped rather than rejected. Use an explicit undefined check so
any foreign id, including the empty string, is reported and rejected.
---------
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Chris Arderne <chris@trigger.dev>1 parent 677ce4c commit 7fd671a
21 files changed
Lines changed: 962 additions & 38 deletions
File tree
- .server-changes
- apps/webapp
- app
- models
- routes
- _app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables.new
- _app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.environment-variables
- _app.orgs.$organizationSlug.projects.$projectParam.env.$envParam.schedules.$scheduleParam
- v3
- environmentVariables
- services
- test
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | | - | |
| 1 | + | |
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
| |||
35 | 35 | | |
36 | 36 | | |
37 | 37 | | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
Lines changed: 26 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
55 | 55 | | |
56 | 56 | | |
57 | 57 | | |
| 58 | + | |
58 | 59 | | |
59 | 60 | | |
60 | 61 | | |
| |||
164 | 165 | | |
165 | 166 | | |
166 | 167 | | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
167 | 193 | | |
168 | 194 | | |
169 | 195 | | |
| |||
Lines changed: 19 additions & 0 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
78 | 78 | | |
79 | 79 | | |
80 | 80 | | |
| 81 | + | |
81 | 82 | | |
82 | 83 | | |
83 | 84 | | |
| |||
267 | 268 | | |
268 | 269 | | |
269 | 270 | | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
| 284 | + | |
| 285 | + | |
| 286 | + | |
| 287 | + | |
| 288 | + | |
270 | 289 | | |
271 | 290 | | |
272 | 291 | | |
| |||
Lines changed: 1 addition & 6 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
9 | | - | |
10 | 9 | | |
11 | 10 | | |
12 | 11 | | |
| |||
78 | 77 | | |
79 | 78 | | |
80 | 79 | | |
81 | | - | |
82 | | - | |
83 | | - | |
84 | | - | |
85 | | - | |
| 80 | + | |
86 | 81 | | |
87 | 82 | | |
88 | 83 | | |
| |||
Lines changed: 12 additions & 9 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
5 | | - | |
| 5 | + | |
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| |||
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
45 | 48 | | |
46 | 49 | | |
47 | 50 | | |
| |||
Lines changed: 12 additions & 9 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2 | 2 | | |
3 | 3 | | |
4 | 4 | | |
5 | | - | |
| 5 | + | |
6 | 6 | | |
7 | 7 | | |
8 | 8 | | |
| |||
34 | 34 | | |
35 | 35 | | |
36 | 36 | | |
37 | | - | |
38 | | - | |
39 | | - | |
40 | | - | |
41 | | - | |
42 | | - | |
43 | | - | |
44 | | - | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
45 | 48 | | |
46 | 49 | | |
47 | 50 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
5 | 5 | | |
6 | 6 | | |
7 | 7 | | |
8 | | - | |
| 8 | + | |
9 | 9 | | |
10 | 10 | | |
11 | 11 | | |
| |||
38 | 38 | | |
39 | 39 | | |
40 | 40 | | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
41 | 51 | | |
42 | 52 | | |
43 | 53 | | |
| |||
76 | 86 | | |
77 | 87 | | |
78 | 88 | | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
79 | 102 | | |
80 | 103 | | |
81 | 104 | | |
| |||
137 | 160 | | |
138 | 161 | | |
139 | 162 | | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
140 | 173 | | |
141 | 174 | | |
142 | 175 | | |
| |||
Lines changed: 7 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
82 | 82 | | |
83 | 83 | | |
84 | 84 | | |
85 | | - | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
86 | 89 | | |
87 | 90 | | |
88 | 91 | | |
| |||
291 | 294 | | |
292 | 295 | | |
293 | 296 | | |
294 | | - | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
295 | 300 | | |
296 | 301 | | |
297 | 302 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1 | 1 | | |
2 | 2 | | |
3 | 3 | | |
| 4 | + | |
4 | 5 | | |
5 | 6 | | |
6 | 7 | | |
| |||
82 | 83 | | |
83 | 84 | | |
84 | 85 | | |
85 | | - | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
86 | 99 | | |
87 | 100 | | |
88 | 101 | | |
| |||
0 commit comments