Repository navigation
fix(presigned-url): resolve a visible file's bucket through the generated physical-name reader - #1916
Conversation
…h the generated reader downloadUrl and the sync S3 delete read the bucket row directly on the request lane, so a reader who sees a file through file RLS (public file in another entity, path share) but not its bucket row got a null downloadUrl. Both now call the storage module's generated <buckets>_physical_name(bucket_id) reader (SECURITY DEFINER, scoped to the caller's claimed database), on the caller's own request lane.
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
|
|
Review complete. 🟠 1 high · 🟡 1 medium 💬 Inline comments (1)
📍 Findings outside the diff (1) — 🟡 1 medium — defects on lines GitHub can't attach comments to🟡 Medium — Storage fixtures lack the new The plugin now resolves the bucket coordinate exclusively through a generated 🧹 Nitpicks (1) — 🟢 1 low
This PR replaces the plugin's direct read of bucket rows with a dedicated
Reviewed commit: e0e43fc |
![tenki-reviewer[bot]](https://avatars.githubusercontent.com/in/2900641?s=60&v=4){kind=small}
There was a problem hiding this comment.
Resolves physical bucket names through a new SECURITY DEFINER reader in the presigned-URL plugin, with test coverage for buckets, uploads, dedup, and recording.
Key findings
- 🟠 Download resolver now throws where it previously returned null — download-url-field.ts:132
- 🟡 Storage fixtures lack the new
_physical_namereader — schema.sql
| const resolved = config && bucketId | ||
| ? await withRequestPgClient(withPgClient, pgSettings, async (pgClient) => { | ||
| const bucketResult = await pgClient.query({ | ||
| text: `SELECT key, physical_name FROM ${config.bucketsQualifiedName} WHERE id = $1 LIMIT 1`, | ||
| values: [bucketId], | ||
| }); | ||
| const row = bucketResult.rows[0] as { key: string; physical_name?: string | null } | undefined; | ||
| return row ? { config, physicalName: storedPhysicalName(row) } : null; | ||
| }) | ||
| ? { | ||
| config, | ||
| physicalName: await withRequestPgClient( | ||
| withPgClient, | ||
| pgSettings, | ||
| (pgClient) => readBucketPhysicalName(pgClient, config, bucketId), | ||
| ), | ||
| } | ||
| : null; | ||
| // No resolvable bucket row, or no physical bucket was ever | ||
| // provisioned: no object can exist. | ||
| // No physical bucket was ever provisioned: no object can exist. | ||
| if (!resolved || resolved.physicalName === null) return null; |
There was a problem hiding this comment.
🟠 bug · high
Download resolver now throws where it previously returned null
The download resolver replaced a direct bucket-table read that degraded to null with readBucketPhysicalName, which throws STORAGE_BUCKET_READER_MISSING when the module records no private_schema (graphile/graphile-presigned-url-plugin/src/download-url-field.ts:132-143; throw at storage-module-cache.ts:297-299). The Grafast lambda has no error handling, so a missing reader function (legacy module predating the generated reader) or a null private_schema surfaces as a GraphQL field error instead of the previous downloadUrl: null. The delete path has the same regression in milder form: the reader throw is swallowed by the generic catch at plugin.ts:570-573 and logged as a transient S3 failure, silently skipping sync S3 cleanup. Legacy or reader-less storage modules go from working (or cleanly skipping) to hard errors and orphaned S3 objects.
📋 Prompt for AI Agents
In graphile/graphile-presigned-url-plugin/src/storage-module-cache.ts around lines 292-305, change readBucketPhysicalName so that a null config.bucketPhysicalNameQualifiedName (currently throwing STORAGE_BUCKET_READER_MISSING at line 297-298) and a missing/unexecutable reader function (SQLSTATE 42883) fall back to a direct SELECT physical_name FROM ${config.bucketsQualifiedName} WHERE id = $1 LIMIT 1 read under the caller's RLS, returning null when the row is absent — the pre-patch behavior of both download-url-field.ts:132-143 and plugin.ts:560. Optionally distinguish the reader-unavailable case in plugin.ts's catch (lines 570-573) with a distinct log so it is not misreported as a transient S3 failure.
1 participant
Summary
Regression from #1907.
downloadUrl(and the sync S3 delete) moved from the system lane to the request lane, but they still read the bucket row directly:File RLS and bucket RLS are different on purpose. A reader can see a file (a public file in another entity, or a path share) without seeing the bucket row behind it. When that happens the lookup returns 0 rows, and
downloadUrlcomes backnull. This fails constructive-db#4083 (entity-storage-security, scenarios A and B: 33/35 tests pass after the bump, 35/35 before it).Both paths now call the storage module's generated reader,
<private_schema>.<buckets_table>_physical_name(bucket_id), on the caller's own request lane (withRequestPgClient, with the request's claims):bucket_idto its provisioned physical name.storage_module.sql, pushed to the constructive-db#4083 branch). It is SECURITY DEFINER and returns onlyphysical_name, and only when the bucket table's schema belongs tojwt_private.require_database_id(). EXECUTE is revoked from PUBLIC and granted toauthenticated/anonymous.StorageModuleConfig.bucketPhysicalNameQualifiedNamecomes from the module'sprivate_schema. If that's missing,readBucketPhysicalNamethrowsSTORAGE_BUCKET_READER_MISSINGinstead of returning null.Coupling: this needs the constructive-db generator change. Publish this package, then bump it in constructive-db#4083.
Tests
__tests__/bucket-physical-name.test.ts(3 tests): calls the generated function with a bound parameter, never touches the bucket table, returns null when no name is recorded, and throws when the reader is missing.entity-storage-security.integration.test.ts, run against this build plus the regenerated generator: 35/35 pass. On the published bump without this fix it's 33/35, withdownloadUrlnull in scenarios A and B.Link to Devin session: https://app.devin.ai/sessions/6bb9de193ee8459ab6fb71e03fbf4b42
Open in Devin Desktop: https://app.devin.ai/desktop/session/6bb9de193ee8459ab6fb71e03fbf4b42?variant=devin
Requested by: @pyramation