Skip to content

feat(gfs-controller): validate the drive name and tag principal denials - #1496

Open
zach88 wants to merge 3 commits into
devfrom
feat/gfs-event-subscriptions-gfsc
Open

zach88 wants to merge 3 commits into
devfrom
feat/gfs-event-subscriptions-gfsc

Conversation

@zach88

@zach88 zach88 commented Nov 14, 2026 •

Copy link
Copy Markdown
Contributor

What

  • gfs-controller fails at boot when GFS_DRIVE_NAME is outside ^[a-z0-9-]{1,63}$.
  • The 403 returned when the principal itself cannot be resolved now carries reason: "principal_denied". The code stays forbidden.

Why

The live GFS watch (rest of this stack) identifies every resource by a canonical key gfs://<drive>/<rid>, and that key format only holds if the drive name stays inside that alphabet. Today nothing checks it.

The watch also needs to tell "this principal cannot read anything" apart from "this principal cannot read this resource" without parsing the error message. The first one ends the whole stream; the second one only rejects a key.

How

  • config.ts validates the drive name next to the rest of the boot config, with the same error style.
  • serve.ts adds the field to the existing principal-denied branch only. Additive: no current client reads it.

Testing

  • config.test.ts: default main when unset or empty, valid names, and rejected ones (uppercase, _, ., /, surrounding spaces, non-ASCII, trailing newline, 64 characters).
  • serve.test.ts: the principal-denied 403 carries the new reason, a per-resource 403 does not.
  • At this head: tsc --noEmit clean, vitest run 47 files / 675 tests passed.

Risks

A deployment with a drive name outside the alphabet now fails at boot instead of running. No deploy manifest in this repo sets the variable, so the default main applies and passes.

The pattern is duplicated in other services by this stack; consolidating it is tracked in #1507.

Stack

Live GFS subscriptions ship as a stack. The server half and the desktop half must reach dev together: the server PRs retire the route the current desktop uses, and the desktop PR needs the new watch route.

# PR Base Scope
1 #1496 dev gfs-controller: drive name validation, principal_denied reason
2 #1497 dev host-context-controller: drive name validation (merge after 1)
3 #1498 dev control-api: shared building blocks, per-resource change trigger
4 #1499 3 control-api: watch registry, listener, process runtime
5 #1500 4 control-api: GET/PUT /external/gfs/watch
6 #1501 5 external-rest-api: GET/PUT /api/v1/me/gfs/watch
7 #1502 6 control-api + ERA: authorization and session push, retire the per-user entity-change stream
8 #1503 7 desktop-app: watch client, live previews and folders

1 and 2 are independent of the chain and safe to merge on their own (1 before 2, since the controller check in 2 mirrors the one in 1).

3 to 8 are one unit and should not be deployed partially:

  • 3 and 4 add groundwork; the only new runtime behavior is the gfs_resource_changed NOTIFY trigger, which nothing listens to yet.
  • 5 and 6 expose the route, but until 7 lands an authorization or session change does not cut the changed events of a stream that is idle (no op in flight). Do not ship 5/6 without 7.
  • 7 removes the per-user entity-change stream (ERA /api/v1/entity-changes/stream and control-api /external/entity-changes/stream), which the current desktop still uses. Do not ship 7 without 8.

zach88 and others added 3 commits November 11, 2026 22:18
Fail at boot when GFS_DRIVE_NAME is outside ^[a-z0-9-]{1,63}$, the
alphabet canonical gfs://<drive>/<rid> URIs and live-watch keys rely on.

The 403 returned when the principal itself cannot be resolved now carries
reason "principal_denied" (code stays "forbidden"), so callers can tell a
principal denial apart from a per-resource denial without parsing the
message. Additive: no current client reads the field.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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.

1 participant