fix!: require auth for restricted products#284
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
alukach
changed the title
|
Claude finished @alukach's task in 2m 41s —— View job PR Review Complete
Review posted. Key findings:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // NOTE: allowed_data_modes is currently unenforced | ||
| allowed_data_modes: z.array(z.nativeEnum(ProductVisibility)), |
There was a problem hiding this comment.
Potential runtime parse failure for existing data
The type of allowed_data_modes changed from RepositoryDataMode (values: open, private, subscription) to ProductVisibility (values: public, unlisted, restricted). Any DataConnection records in the database that still carry old RepositoryDataMode strings will now fail Zod validation when parsed.
Since this field is explicitly unenforced, the practical blast radius is limited — but if any code path calls DataConnectionSchema.parse(...) on a record with stale values, it will throw at runtime.
Options:
- Add a DB migration to update the values (e.g.
open→public,private/subscription→restricted) - Use
z.array(z.string())as a soft type until the data is cleaned up, then tighten it back - Use
.catch([])so stale values degrade gracefully rather than crashing
Also worth a rename follow-up: allowed_data_modes describing ProductVisibility values is confusing. allowed_visibility would be clearer.
| if (!isAuthorized(session, product, Actions.GetRepository)) { | ||
| notFound(); | ||
| } |
There was a problem hiding this comment.
Good catch — this authorization gate was previously missing from the layout. Without it, a user who typed a restricted product URL directly could see the product shell (header, breadcrumbs, upload zone) even though the data API would reject their requests. Now they correctly get a 404 instead.
|
@/home/runner/work/source.coop/source.coop/.claude-review-summary.txt |
|
Claude finished @alukach's task in 3m 17s —— View job Claude finished @alukach's task — View job
Review posted. Key findings:
|
| read_only: z.boolean(), | ||
| allowed_data_modes: z.array(z.nativeEnum(RepositoryDataMode)), | ||
| // NOTE: allowed_data_modes is currently unenforced | ||
| allowed_data_modes: z.array(z.nativeEnum(ProductVisibility)), |
There was a problem hiding this comment.
Production DB records may fail Zod parsing after this change
allowed_data_modes previously held RepositoryDataMode values (open, private, subscription). It now holds ProductVisibility values (public, unlisted, restricted). Any DataConnection documents in the production database that still carry the old string values will throw when parsed through DataConnectionSchema.
Since the enforcement code for this field is commented out (route.ts:147-157), no runtime path today acts on the parsed value — but if any code path calls DataConnectionSchema.parse() on a stale record, it will throw.
Options before merging:
- DB migration: map
open→public,private/subscription→restrictedin existing records. - Soft type: use
z.array(z.string())as a transitional type until the data is clean, then tighten it. - Graceful fallback:
.catch([])so stale values degrade to an empty array instead of crashing.
The fix: conform to product visibility type for allowed_data_modes commit updated the fixtures correctly, but production data is a separate concern.
|
|
||
| "featured": 0 | ||
| }, | ||
| { |
There was a problem hiding this comment.
subscription-org-repo-id fixture is now authz-identical to private-org-repo-id and is unused in tests
With data_mode removed, both subscription-org-repo-id and private-org-repo-id have visibility: "restricted" and no other distinguishing field. A grep across the test suite shows subscription-org-repo-id is not referenced in any test assertion — it exists in the fixture file but exercises no distinct code path.
Consider removing it to reduce confusion, or add a comment explaining why it's kept (e.g., reserved for a future distinction).
|
Claude finished @alukach's task in 3m 36s —— View job Claude finished @alukach's task —
Review posted. Key findings:
|
| read_only: z.boolean(), | ||
| allowed_data_modes: z.array(z.nativeEnum(RepositoryDataMode)), | ||
| // NOTE: allowed_data_modes is currently unenforced | ||
| allowed_data_modes: z.array(z.nativeEnum(ProductVisibility)), |
There was a problem hiding this comment.
Production DB records will fail Zod parsing after this change
allowed_data_modes previously held RepositoryDataMode values (open, private, subscription). It now holds ProductVisibility values (public, unlisted, restricted). Any DataConnection documents in production that still carry old string values will throw when parsed through DataConnectionSchema.
Since no enforcement code runs today against this field, the blast radius is limited to a hard parse failure on any code path that calls DataConnectionSchema.parse() on a stale record.
Options before merging:
- DB migration: map
open→public,private/subscription→restricted - Soft type:
z.array(z.string())until data is clean, then tighten it back - Graceful fallback:
.catch([])so stale values degrade to an empty array instead of throwing
The fix: conform to product visibility type for allowed_data_modes commit updated fixtures correctly, but production data is a separate concern.
| endpoint: z.string().optional(), // For MinIO/Ceph | ||
| }), | ||
| // Mirror-specific settings | ||
| is_primary: z.boolean(), // Is this the primary mirror? |
There was a problem hiding this comment.
config removed from schema but all fixtures and production records still carry it
ProductMirrorSchema no longer includes the config sub-object. Zod uses strip mode by default, so parsing won't throw — the field is silently dropped.
Two things worth tracking:
- All 7 fixtures in
fixtures/products.jsonstill haveconfig: { region, bucket }in their mirror definitions. These fixtures now diverge from the schema — a developer reading them will be confused about what a valid mirror looks like. - Production records have the same field. If any code path reads a product, parses it through
ProductSchema, and writes it back (e.g. an update endpoint), theconfigfield will be silently removed from the DB record.
credentials.ts and page.tsx already read bucket/region from DataConnection.details, not from mirror.config, so there's no runtime regression from the code side. But the fixture cleanup would prevent future confusion. Fix this →
|
Claude finished @alukach's task in 3m 16s —— View job
Review posted. Key findings:
|
|
Claude finished @alukach's task in 3m 46s —— View job
Review posted. Key findings:
|
| return a.every((v, i) => v === b[i]); | ||
| } | ||
|
|
||
| async function backfill(tableName: string, dryRun: boolean) { |
There was a problem hiding this comment.
Deployment ordering: run this before deploying the code change
The new z.nativeEnum(ProductVisibility) in DataConnectionSchema will cause DataConnectionSchema.parse() to throw for any record whose allowed_data_modes still contains old string values (open, private, subscription). This script must be run against the production table successfully before the code change is deployed.
The script itself looks well-written: the mapping is correct, the ?? [] guard handles missing fields, and the Set deduplication correctly collapses ["private", "subscription"] → ["restricted"]. Idempotency is preserved because LEGACY_TO_NEW only maps legacy keys — new values pass through unchanged.
| }) | ||
| .openapi("ProductDataMode"); | ||
|
|
||
| export const ProductVisibilitySchema = z.nativeEnum(ProductVisibility); |
There was a problem hiding this comment.
ProductVisibilitySchema lost its OpenAPI name
The old ProductDataModeSchema used .openapi("ProductDataMode") which registered it as a named component in the generated spec. The new inline z.nativeEnum(ProductVisibility) has no .openapi() call, so it will be inlined everywhere it's referenced rather than appearing as a named $ref. Non-blocking, but if the OpenAPI spec is consumed by external clients a follow-up .openapi("ProductVisibility") would keep the spec cleaner:
| export const ProductVisibilitySchema = z.nativeEnum(ProductVisibility); | |
| export const ProductVisibilitySchema = z.nativeEnum(ProductVisibility).openapi("ProductVisibility"); |
|
@/home/runner/work/source.coop/source.coop/.claude-review-summary.txt |
| read_only: z.boolean(), | ||
| allowed_data_modes: z.array(z.nativeEnum(RepositoryDataMode)), | ||
| // NOTE: allowed_data_modes is currently unenforced | ||
| allowed_data_modes: z.array(z.nativeEnum(ProductVisibility)), |
There was a problem hiding this comment.
Backfill must run before deployment — parse failures will break these API routes
DataConnectionSchema.parse() is called in three production routes that will 500 on any record carrying legacy values:
GET /api/v1/data-connections(route.ts:43)GET /api/v1/data-connections/available(available/route.ts:52)GET /api/v1/data-connections/[id]([data_connection_id]/route.ts)
A record with allowed_data_modes: ["open"] will fail z.nativeEnum(ProductVisibility) and throw in the map, turning the entire endpoint into a 500. Since filter runs before map in those routes, even a single un-migrated record breaks the whole list response.
The backfill script in this PR is the right fix. Run the backfill against production before deploying this code, not after.
Also worth a rename follow-up: now that this field holds ProductVisibility values, allowed_data_modes is a misnomer. allowed_visibility would be clearer — but that's a separate PR.
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
|
Claude finished @alukach's task in 3m 57s —— View job
Review complete. Key findings:
|
| read_only: z.boolean(), | ||
| allowed_data_modes: z.array(z.nativeEnum(RepositoryDataMode)), | ||
| // NOTE: allowed_data_modes is currently unenforced | ||
| allowed_data_modes: z.array(z.nativeEnum(ProductVisibility)), |
There was a problem hiding this comment.
Deployment order: backfill must run before this code ships
dataConnectionsTable.listAll() returns raw DynamoDB items via a TypeScript cast (item as DataConnection) — no Zod parse at the DB layer. The parse happens in the route handlers:
route.ts:39—DataConnectionSchema.omit({authentication: true}).parse(connection)— runs on every connection in the list responseavailable/route.ts:47,52— same pattern[data_connection_id]/route.ts:128— on PUT
Any record still carrying legacy allowed_data_modes values (open, private, subscription) will throw at .parse(), turning the entire list endpoint into a 500 — because the throw happens inside .map() after the filter has already passed.
The backfill script in this PR is the right fix. Run it successfully against production before deploying — not after.
Also: allowed_data_modes is now a misnomer since it holds ProductVisibility values. A follow-up rename to allowed_visibility would help future readers, but that's a separate PR.
|
Claude finished @alukach's task in 2m 28s —— View job PR ReviewGood incremental progress — the last push ( Resolved since prior review cycles ✅
Must address before mergingRun the backfill script before deploying — parse failures will break API routes
The backfill script is well-written (correct mapping, Set deduplication, paginated scan, dry-run support, idempotent). Run it successfully against production before deploying this code. Minor (non-blocking)
All 7 product fixtures carry
The field now holds Positive notes
|
| read_only: z.boolean(), | ||
| allowed_data_modes: z.array(z.nativeEnum(RepositoryDataMode)), | ||
| // NOTE: allowed_data_modes is currently unenforced | ||
| allowed_data_modes: z.array(z.nativeEnum(ProductVisibility)), |
There was a problem hiding this comment.
Deployment order: backfill must run before this code ships
DataConnectionSchema.parse() is called in three production routes that will 500 on any record carrying legacy values:
GET /api/v1/data-connections—route.ts:39,43GET /api/v1/data-connections/available—available/route.ts:47,52PUT /api/v1/data-connections/[id]—[data_connection_id]/route.ts:128
A single record still holding allowed_data_modes: ["open"] will fail z.nativeEnum(ProductVisibility) inside .map(), turning the entire list endpoint into a 500. Run scripts/backfill-allowed-data-modes.ts successfully against production before deploying this code, not after.
Also: allowed_data_modes is now a misnomer since it holds ProductVisibility values. A follow-up rename to allowed_visibility would help future readers.
|
Claude finished @alukach's task in 2m 53s —— View job
Review complete. The latest two commits ( Key findings:
|
| read_only: z.boolean(), | ||
| allowed_data_modes: z.array(z.nativeEnum(RepositoryDataMode)), | ||
| // NOTE: allowed_data_modes is currently unenforced | ||
| allowed_data_modes: z.array(z.nativeEnum(ProductVisibility)), |
There was a problem hiding this comment.
Deployment order: backfill must run before this code ships
DataConnectionSchema.parse() is called in three live route handlers that will 500 on any record carrying legacy values:
GET /api/v1/data-connections→route.ts:37,43GET /api/v1/data-connections/available→available/route.ts:47,52PUT /api/v1/data-connections/[id]→[data_connection_id]/route.ts:128
A single un-migrated record (allowed_data_modes: ["open"]) fails z.nativeEnum(ProductVisibility) inside .map(), turning the entire list endpoint into a 500. Run scripts/backfill-allowed-data-modes.ts against production before deploying this code, not after.
Also worth a follow-up rename: allowed_data_modes is a misnomer now that it holds ProductVisibility values — allowed_visibility would be clearer.
PR ReviewThe latest commit ( Resolved across review cycles ✅
Must address before mergingRun the backfill script before deploying — parse failures will 500 your API routes
The backfill script (
Minor (non-blocking)Commit message typo ( Positive notes
|
|
Claude finished @alukach's task in 2m 16s —— View job
Review complete. The latest commits ( One thing still required before merging: run |
|
Claude finished @alukach's task in 2m 31s —— View job
|
1 participant
What I'm changing
Currently, our app does not enforce Product visibility. As such, anonymous users can view restricted products (e.g. https://staging.source.coop/alukach/alukach-experimentation). This PR cleans up our data model by removing the legacy
data_modefield from the Product and integrates thevisibilityinto our authorization tooling. As such, we will require valid permissions for Products marked asrestricted.How I did it
The Product model currently contains both a legacy
data_modeand a modernvisibilityfield. Thedata_modeshould have been removed when we migrated to our new data model. During this migration, we populated thevisibilityfield from thedata_mode:source.coop/scripts/load-dynamodb-exports.ts
Lines 66 to 72 in 5ad9b41
However, the code base currently erroneously makes use of both
data_modeandvisibilitywhen determining the product authorization:source.coop/src/lib/api/authz.ts
Lines 683 to 686 in 5ad9b41
source.coop/src/lib/api/authz.ts
Lines 726 to 730 in 5ad9b41
source.coop/src/lib/api/authz.ts
Lines 770 to 777 in 5ad9b41
Note that we don't surface
data_modein the Product form, onlyvisibility, as thedata_modefield is deprecated.This PR removes the
data_modefield and only usesvisibilityfor auth permissions. This seems inline with the documentation written during S2 development:source.coop/docs/migration/schema-changes.md
Lines 181 to 186 in 5ad9b41
How you can test it
Compare
Note
Before merging this, we need to determine what to do with the
DataConnection.allowed_data_modesfield. It's currently unenforced.source.coop/src/types/data-connection.ts
Line 164 in 5ad9b41
closes #261
Warning
BREAKING CHANGE: existing data-connections records must be backfilled.
Run: npx tsx scripts/backfill-allowed-data-modes.ts