ENG-1870 Copy Roam-origin assets into Supabase on publish - #1394
ENG-1870 Copy Roam-origin assets into Supabase on publish#1394maparent wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
This pull request has been ignored for the connected project Preview Branches by Supabase. |
d058750 to
14f6b43
Compare
14f6b43 to
6f408af
Compare
6f408af to
d891c94
Compare
d891c94 to
3010d2d
Compare
3010d2d to
7839b4c
Compare
PR size/scope checkThis PR is over our review-size guideline.
Please split this into smaller PRs unless there is a clear reason the changes need to land together. If keeping it as one PR, please add a brief justification covering:
|
![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.
7839b4c to
264ccb5
Compare
|
|
||
| /** Roam uploads land in its own Firebase project, which we do not control. */ | ||
| const ROAM_STORAGE_HOST = "firebasestorage.googleapis.com"; | ||
| const ROAM_STORAGE_BUCKET = "firescript-577a2.appspot.com"; |
There was a problem hiding this comment.
This might not be stable, probably isn't a good idea to rely on it.
There was a problem hiding this comment.
As also said in Linear: I suggest keeping them, but detecting new buckets. Details here.
| * publish. Anything else (an image hotlinked from another site, a link to a paper) is a | ||
| * resource that was never an asset of the node, and is left alone. | ||
| * | ||
| * Matching the bucket rather than the path shape: every Roam upload lands in this one |
There was a problem hiding this comment.
"lands in this one bucket" for now. Not a good idea to rely on this.
264ccb5 to
7e0bb26
Compare
7e0bb26 to
46b1149
Compare
mdroidian
left a comment
mdroidian
left a comment
There was a problem hiding this comment.
Sorry, I'm going to block on storing/relying on Roam's current bucket URL. This is setting us up for failure in the future unncessarily.
Use a regex pattern (that doesn't include their specific bucket name) + roam's API to confirm it's roam's bucket.
|
I see your point. Will work on this. |
|
Investigated. I assumed you meant roamAlphaAPI.file.get. I'm not using it now, and that's a mistake for encrypted graphs; so worth the pushback. BUT it does not distinguish roam buckets from arbitrary firebase buckets, so it does not do what you want (gate on Roam content.) This also has security implications, as noted. |
|
How are you getting the filename? |
|
Why are we not using the roam API to get the files? |
|
Why wouldn't using roamAlphaAPI distinguish roam buckets from arbitrary firebase buckets? If you use roamAlphaAPI on a legitimate file in a roam bucket, you will get a success response. If you use roamAlphaAPI on a non roam bucket, I'm assuming you will get an error response (unauthorized). This would solve the security implication, yes? |
|
Even if the roamAlphaAPI just performs a standard fetch, it isn't clear what the security implication actually is compared to downloading bytes from roam's bucket, or an arbitrary obsidian file. |
9e85cbc to
5e50703
Compare
|
Why not using roam API: I did not think of the encryption case, and Claude chose a common API. Working on reverting that. It also means we'll get the filename and mimetype from the file. However, I checked that the lastModified that we get from file.get is spurious (always now) whereas the one we get from metadata is valid and, more important, stable, so keeping the metadata call for now. I thought it was reliable for size, but again only for unencrypted graphs. There's a shortcut that depended on that, becomes conditional. |
|
BTW making assumptions about encrypted graphs; if you could create one and give me access, I'd love to check some of them. But I doubt it would change anything deeply. |
5e50703 to
df766ec
Compare
df766ec to
cf6023c
Compare
cf6023c to
ab7a424
Compare
From the metadata, in
Corrected. Now identifying roam buckets by URL as agreed. |
ab7a424 to
afdcfb2
Compare
There was a problem hiding this comment.
Devin Review found 3 new potential issues.
3 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| const { data, error } = await client | ||
| .from("my_file_references") | ||
| .select("source_local_id, filepath, filehash, source_path") | ||
| .eq("space_id", spaceId) | ||
| .in("source_local_id", sourceLocalIds); |
There was a problem hiding this comment.
🟡 Asset reconciliation stops at 1,000
When a publish batch has over 1,000 stored references, readExistingReferences silently drops later rows. The configured PostgREST limit truncates this unpaginated query. Omitted stale references survive cleanup, while current references are downloaded again.
Learn more
PostgREST limits one response to 1,000 rows in this repository. This query combines every selected node's references, so a normal row limit applies across the whole batch rather than per node. Rows after the cap never enter existingByNode. The copy loop treats those rows as absent, and stale-reference detection cannot remove them.
Example: Publishing 600 nodes with two existing assets each requests 1,200 rows. If 200 rows are omitted, unchanged assets in those rows are fetched again; stale assets in those rows remain published.
Recommended fix: Read my_file_references through getAllPages with deterministic ordering, and propagate pagination errors before any copying or cleanup.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const { hostname, pathname } = new URL(url); | ||
| if (hostname !== ROAM_STORAGE_HOST) return false; | ||
| const segments = objectSegments(pathname); | ||
| const folder = segments.slice(0, ASSET_FOLDER.length); | ||
| const graphName = segments[ASSET_FOLDER.length]; | ||
| const named = segments.slice(ASSET_FOLDER.length + 1); | ||
| return ( | ||
| folder.join("/") === ASSET_FOLDER.join("/") && | ||
| // Exact, not case-insensitive: Roam preserves the graph name's casing in the URL. | ||
| graphName === window.roamAlphaAPI.graph.name && | ||
| named.some((segment) => segment !== "") |
There was a problem hiding this comment.
🟥 Untrusted Firebase buckets trigger asset copying
A pasted Firebase URL passes isRoamStorageUrl when its path contains the graph name, regardless of bucket. Publishing then fetches attacker-controlled bytes and stores them under the user's Supabase authority.
Was this helpful? React with 👍 or 👎 to provide feedback.
| const contentHash = await addFile({ | ||
| client, | ||
| spaceId, | ||
| sourceLocalId, | ||
| fname: assetUrl, | ||
| sourcePath: asset.filename, | ||
| mimetype: asset.mimetype, | ||
| // Storage timestamps, not `File.lastModified`, which is the time of the call. The | ||
| // node's own dates are the fallback, and are stable for the same reason. | ||
| created: descriptor.createdAt ?? nodeCreated, | ||
| lastModified: descriptor.modifiedAt ?? nodeLastModified, | ||
| content: asset.content, |
There was a problem hiding this comment.
🟨 Shared metadata exposes asset tokens
fname stores the complete Firebase URL, including its download token, in shared file metadata. Group readers can retrieve that reusable token through my_file_references.
Was this helpful? React with 👍 or 👎 to provide feedback.
2 participants
Reviewer brief
This is a large PR, there are a lot of moving pieces.
There are actually five subtasks, in distinct commits, which may be easier to understand:
https://roamresearch.com/#/app/discourse-graphs/page/4o_mub_mn
Verification
There are unit tests.
Loom video
Code walkthrough (long): https://www.loom.com/share/91078efd00b14f959ebf0ba07950a4fe
Note: I mention something missing during the code walkthrough, but upon verification it's fine.
Demo: https://www.loom.com/share/ecdd8cdfaca141cfb26216ecda5cdc08
Scope check
$scope-checkagainst the ENG ticket and final diff.Done When: None.Actually it called me on the change to return parameters of addFile, but I told it that CopiedAsset is the stage's record of what it stored, a stored asset's identity in shared storage is its hash, and the summary was previously conflating references with blobs. addFile's return is what makes that available without re-hashing. It accepted that justification as valid.
It's also pending the issue of whether the size limit should have been split between tickets, but right now it's within 1876.
Local delegated full review
$dg-delegated-full-reviewwhen no other full-review workflow is available.https://linear.app/discourse-graphs/issue/ENG-1870/copy-roam-origin-assets-into-supabase-on-publish