nils/photos
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix code-review findings on multi-tenant branch

Browse Source 
- images.rs: scope /api/img and /api/photos/{id}/original by album owner —
  close the cross-tenant original/thumbnail leak (tenancy test now covers
  these routes)
- migration 0003: refuse to run when albums exist and users != 1 instead of
  silently reassigning every album to the oldest user
- Gallery: callback-ref ResizeObserver so a gallery mounted empty still
  lays out once photos arrive (was permanently blank)
- upload dedup: re-uploading identical content whose photo is in 'error'
  resets and re-enqueues it instead of returning the broken row
- client hashing: skip (and fall back to plain upload) above 512MB to avoid
  whole-file arrayBuffer OOM / the ~2GiB cap
- zip: always spool each entry (no unread prefetched S3 body held across a
  slow client stream) and backfill BOTH sha256 and crc32 for legacy photos
- tests/auth: share one session_payload builder instead of re-implementing
  the cookie format in the test and mint_session
This commit is contained in:
nils
2026-07-17 15:16:25 +02:00
parent 40f7f2fb5e
commit 84323ab637
9 changed files with 149 additions and 78 deletions
Download Patch File Download Diff File Expand all files Collapse all files
frontend/src/api.js
+8 -2
View File
@@ -73,9 +73,15 @@ export function postDownload(url, ids = '') {
form.remove()
}
// SHA-256 of a File, matching the server's content hash — used to skip
// uploading bytes the album already has.
// Above this, hashing whole-file in memory (WebCrypto has no streaming digest)
// risks OOM / the ~2GiB ArrayBuffer cap, so we skip the client dedup check and
// just upload — the server still dedups on arrival.
const CLIENT_HASH_LIMIT = 512 * 1024 * 1024
// SHA-256 of a File (lowercase hex), matching the server's content hash, or
// null when the file is too large to hash safely in the browser.
export async function sha256Hex(file) {
if (file.size > CLIENT_HASH_LIMIT) return null
const digest = await crypto.subtle.digest('SHA-256', await file.arrayBuffer())
return [...new Uint8Array(digest)].map((b) => b.toString(16).padStart(2, '0')).join('')
}
frontend/src/components/Gallery.jsx
+11 -7
View File
@@ -1,4 +1,4 @@
import { useEffect, useRef, useState } from 'react'
import { useCallback, useRef, useState } from 'react'
import { imgUrl } from '../api'
// True justified layout: pack photos greedily into rows at their real aspect
@@ -32,15 +32,19 @@ function layoutRows(photos, containerWidth, targetHeight, gap) {
}
export default function Gallery({ photos, onOpen, overlay, selected, onToggleSelect }) {
const containerRef = useRef(null)
const [width, setWidth] = useState(0)
const observerRef = useRef(null)
useEffect(() => {
const el = containerRef.current
if (!el) return
// Callback ref: (re)attaches the observer whenever the container node
// mounts. A plain mount-effect misses the case where Gallery first renders
// empty (no container) and photos arrive later.
const containerRef = useCallback((node) => {
observerRef.current?.disconnect()
if (!node) return
const observer = new ResizeObserver((entries) => setWidth(entries[0].contentRect.width))
observer.observe(el)
return () => observer.disconnect()
observer.observe(node)
observerRef.current = observer
setWidth(node.getBoundingClientRect().width)
}, [])
if (photos.length === 0) return null
frontend/src/pages/AlbumPage.jsx
+4 -1
View File
@@ -71,13 +71,16 @@ function UploadZone({ albumId, onUploaded }) {
setQueue((q) => q.map((x) => (x.key === item.key ? { ...x, ...patch } : x)))
const transfer = async () => {
// Hash locally first: content the album already has is skipped
// without transferring a single byte.
// without transferring a single byte. Files too large to hash in the
// browser (null) fall straight through to a normal upload.
update({ status: 'checking' })
try {
const hash = await sha256Hex(item.file)
if (hash) {
await api(`/api/albums/${albumId}/photos/by-hash/${hash}`)
update({ status: 'skipped', progress: 1 })
return
}
} catch {
// 404 (not there yet) or hashing unavailable — upload normally.
}
migrations/0003_multi_tenant.sql
+18 -1
View File
@@ -2,8 +2,25 @@
-- all hang off albums, so this single column scopes everything.
alter table albums add column owner_id uuid references users(id);
-- Existing albums belong to the instance's original photographer.
-- Backfill: pre-tenancy albums had no owner. Assigning them to "the" original
-- photographer is only unambiguous when exactly one user exists. With several
-- (v0.1.0 let every allowed email share all albums) the correct owner is
-- unknowable, so refuse rather than silently transfer everyone's work to one
-- account — the operator must assign ownership manually before migrating.
do $$
declare
n_users int;
n_albums int;
begin
select count(*) into n_users from users;
select count(*) into n_albums from albums;
if n_albums > 0 and n_users <> 1 then
raise exception
'multi-tenant migration: % albums exist but there are % users (need exactly 1 to auto-assign ownership); set albums.owner_id manually first',
n_albums, n_users;
end if;
update albums set owner_id = (select id from users order by created_at limit 1);
end $$;
alter table albums alter column owner_id set not null;
src/auth.rs
+7 -1
View File
@@ -114,11 +114,17 @@ pub(crate) fn base_cookie(name: &'static str, value: String, secure: bool) -> Co
.build()
}
/// The signed session-cookie payload, in one place so tests and the
/// mint_session dev tool can't drift from what user_from_jar parses.
pub fn session_payload(user_id: Uuid, email: &str, exp: i64) -> String {
format!("{user_id}|{exp}|{email}")
}
fn session_cookie(state: &AppState, user_id: Uuid, email: &str) -> Cookie<'static> {
let exp = Utc::now().timestamp() + SESSION_DAYS * 86400;
let mut cookie = base_cookie(
SESSION_COOKIE,
format!("{user_id}|{exp}|{email}"),
session_payload(user_id, email, exp),
state.config.cookie_secure(),
);
cookie.set_max_age(time::Duration::days(SESSION_DAYS));
src/routes/images.rs
+12 -1
View File
@@ -28,9 +28,20 @@ async fn authorize_photo(
.await?;
let photo = photo.ok_or_else(ApiError::not_found)?;
if user_from_jar(state, jar).is_some() {
// Photographers may only reach their OWN photos (scoped by album owner);
// otherwise fall through to share-cookie access.
if let Some(user) = user_from_jar(state, jar) {
let owns: Option<(Uuid,)> = sqlx::query_as(
"select a.id from albums a where a.id = $1 and a.owner_id = $2",
)
.bind(photo.album_id)
.bind(user.id)
.fetch_optional(&state.db)
.await?;
if owns.is_some() {
return Ok(photo);
}
}
authorize_album_via_cookie(state, jar, photo.album_id, need_download).await?;
// Clients may only reach photos the share listing exposes.
if photo.status != PhotoStatus::Ready {
src/routes/photos.rs
+21 -2
View File
@@ -41,6 +41,25 @@ fn sanitize_filename(raw: &str) -> Result<String, ApiError> {
Ok(cleaned.chars().take(150).collect())
}
/// A dedup hit on a photo that previously failed processing means the user is
/// re-uploading to fix it — reset it and re-enqueue instead of handing back a
/// broken row that the UI would report as "already uploaded".
async fn heal_if_errored(state: &AppState, photo: Photo) -> ApiResult<Photo> {
if photo.status != PhotoStatus::Error {
return Ok(photo);
}
let mut tx = state.db.begin().await?;
let healed: Photo =
sqlx::query_as("update photos set status = $2, error = null where id = $1 returning *")
.bind(photo.id)
.bind(PhotoStatus::Uploaded.as_str())
.fetch_one(&mut *tx)
.await?;
jobs::ensure_process_photo(&mut tx, photo.id).await?;
tx.commit().await?;
Ok(healed)
}
pub async fn upload(
State(state): State<AppState>,
user: AuthUser,
@@ -94,7 +113,7 @@ pub async fn upload(
.fetch_optional(&state.db)
.await?;
if let Some(existing) = existing {
return Ok(Json(existing));
return Ok(Json(heal_if_errored(&state, existing).await?));
}
// Upload to S3 first, then create the row and enqueue processing in one
@@ -153,7 +172,7 @@ pub async fn upload(
.fetch_optional(&state.db)
.await?;
if let Some(winner) = winner {
return Ok(Json(winner));
return Ok(Json(heal_if_errored(&state, winner).await?));
}
}
return Err(e.into());
src/routes/zip.rs
+40 -54
View File
@@ -137,9 +137,6 @@ struct Entry {
offset: u64,
dos_time: u16,
dos_date: u16,
/// Stored at upload/processing time; photos from before hashes existed
/// have None and take the slower spool path (which backfills it).
crc: Option<u32>,
}
struct ZipPlan {
@@ -180,7 +177,6 @@ fn plan_zip(photos: &[Photo]) -> Result<ZipPlan, ApiError> {
offset: entry_offset,
dos_time,
dos_date,
crc: photo.crc32.map(|v| v as u32),
});
}
let cd_offset = offset;
@@ -282,23 +278,26 @@ fn stream_zip(state: AppState, photos: Vec<Photo>, album_name: &str) -> ApiResul
.map_err(|e| anyhow::anyhow!("building response: {e}").into())
}
enum Fetched {
/// CRC already known — the body streams straight into the response.
Direct(Box<aws_sdk_s3::operation::get_object::GetObjectOutput>),
/// Pre-hash photo: spooled to a temp file to compute the CRC first.
Spooled(tokio::fs::File, u32),
struct Fetched {
file: tokio::fs::File,
crc: u32,
/// sha256 hex — always computed so legacy photos (crc-only) get their
/// content hash backfilled, restoring upload dedup for them.
sha256: String,
}
/// Start fetching an original. With a known CRC this only opens the S3
/// response (the body is consumed later, straight into the zip stream);
/// otherwise the object is spooled to an anonymous temp file to compute the
/// CRC, verifying the byte count the zip plan promised.
/// Fetch an original and spool it to an anonymous temp file, computing crc32
/// and sha256 and verifying the byte count the zip plan promised. Spooling
/// (rather than streaming the live S3 body straight through) is deliberate:
/// the prefetched entry is fully read immediately, so no S3 connection sits
/// idle across the previous entry's (possibly slow) client stream — which S3
/// idle-timeouts would otherwise reset mid-download.
fn fetch_entry(
state: &AppState,
key: String,
expected_size: u64,
crc_known: bool,
) -> JoinHandle<anyhow::Result<Fetched>> {
use sha2::Digest;
let state = state.clone();
tokio::spawn(async move {
let object = state
@@ -309,12 +308,10 @@ fn fetch_entry(
.send()
.await
.map_err(|e| anyhow::anyhow!("fetching {key}: {e}"))?;
if crc_known {
return Ok(Fetched::Direct(Box::new(object)));
}
let mut file = tokio::fs::File::from_std(tempfile::tempfile()?);
let mut reader = object.body.into_async_read();
let mut hasher = crc32fast::Hasher::new();
let mut crc = crc32fast::Hasher::new();
let mut sha = sha2::Sha256::new();
let mut written: u64 = 0;
let mut buf = vec![0u8; 128 * 1024];
loop {
@@ -322,7 +319,8 @@ fn fetch_entry(
if n == 0 {
break;
}
hasher.update(&buf[..n]);
crc.update(&buf[..n]);
sha.update(&buf[..n]);
file.write_all(&buf[..n]).await?;
written += n as u64;
}
@@ -332,7 +330,11 @@ fn fetch_entry(
);
file.flush().await?;
file.seek(std::io::SeekFrom::Start(0)).await?;
Ok(Fetched::Spooled(file, hasher.finalize()))
Ok(Fetched {
file,
crc: crc.finalize(),
sha256: hex::encode(sha.finalize()),
})
})
}
@@ -345,30 +347,23 @@ async fn write_zip(
const FLAGS: u16 = 0x0800;
let mut crcs = Vec::with_capacity(plan.entries.len());
// Prefetch: start fetching the next object while streaming the current one.
// Prefetch: spool the next object to a temp file while streaming the
// current one — the prefetched body is drained immediately, never held
// open across the current entry's client stream.
let mut pending: Option<JoinHandle<anyhow::Result<Fetched>>> = None;
for (i, entry) in plan.entries.iter().enumerate() {
let current = match pending.take() {
Some(handle) => handle,
None => fetch_entry(state, entry.s3_key.clone(), entry.size, entry.crc.is_some()),
None => fetch_entry(state, entry.s3_key.clone(), entry.size),
};
if let Some(next) = plan.entries.get(i + 1) {
pending = Some(fetch_entry(
state,
next.s3_key.clone(),
next.size,
next.crc.is_some(),
));
pending = Some(fetch_entry(state, next.s3_key.clone(), next.size));
}
let fetched = current
let mut fetched = current
.await
.map_err(|e| anyhow::anyhow!("fetch task failed: {e}"))?
.map_err(|e| anyhow::anyhow!("photo {}: {e:#}", entry.photo_id))?;
let crc = match &fetched {
Fetched::Direct(_) => entry.crc.expect("direct fetch implies known crc"),
Fetched::Spooled(_, crc) => *crc,
};
crcs.push(crc);
crcs.push(fetched.crc);
let mut lfh = Vec::with_capacity(30 + entry.name.len());
lfh.extend_from_slice(&0x04034b50u32.to_le_bytes());
@@ -377,36 +372,27 @@ async fn write_zip(
lfh.extend_from_slice(&0u16.to_le_bytes()); // method: stored
lfh.extend_from_slice(&entry.dos_time.to_le_bytes());
lfh.extend_from_slice(&entry.dos_date.to_le_bytes());
lfh.extend_from_slice(&crc.to_le_bytes());
lfh.extend_from_slice(&fetched.crc.to_le_bytes());
lfh.extend_from_slice(&(entry.size as u32).to_le_bytes()); // compressed
lfh.extend_from_slice(&(entry.size as u32).to_le_bytes()); // uncompressed
lfh.extend_from_slice(&(entry.name.len() as u16).to_le_bytes());
lfh.extend_from_slice(&0u16.to_le_bytes()); // extra len
lfh.extend_from_slice(&entry.name);
out.write_all(&lfh).await?;
match fetched {
Fetched::Direct(object) => {
let mut reader = object.body.into_async_read();
let copied = tokio::io::copy(&mut reader, &mut out).await?;
anyhow::ensure!(
copied == entry.size,
"{} is {copied} bytes in s3 but {} in the database",
entry.s3_key,
entry.size
);
}
Fetched::Spooled(mut file, crc) => {
tokio::io::copy(&mut file, &mut out).await?;
// Self-heal: store the freshly computed crc so the next
// download of this photo streams directly.
let _ = sqlx::query("update photos set crc32 = coalesce(crc32, $2) where id = $1")
tokio::io::copy(&mut fetched.file, &mut out).await?;
// Self-heal legacy photos: backfill both hashes so future zips and
// upload dedup both work for them.
let _ = sqlx::query(
"update photos set crc32 = coalesce(crc32, $2), sha256 = coalesce(sha256, $3)
where id = $1",
)
.bind(entry.photo_id)
.bind(i64::from(crc))
.bind(i64::from(fetched.crc))
.bind(&fetched.sha256)
.execute(&state.db)
.await;
}
}
}
// Central directory.
for (entry, crc) in plan.entries.iter().zip(&crcs) {
tests/tenancy.rs
+20 -1
View File
@@ -44,10 +44,12 @@ fn test_config(database_url: String) -> Config {
fn session_for(user_id: Uuid, email: &str) -> String {
let key = Key::from(&Sha512::digest(SECRET.as_bytes()));
let exp = chrono::Utc::now().timestamp() + 3600;
// Use the production payload builder so the test can't drift from what
// user_from_jar parses.
let mut jar = CookieJar::new();
jar.signed_mut(&key).add(Cookie::new(
"photos_session",
format!("{user_id}|{exp}|{email}"),
photos::auth::session_payload(user_id, email, exp),
));
format!("photos_session={}", jar.get("photos_session").unwrap().value())
}
@@ -184,6 +186,23 @@ async fn tenant_isolation_matrix() {
assert_eq!(status, StatusCode::OK);
assert_eq!(list.as_array().unwrap().len(), 0, "bob must see no albums");
// ---- Dual-auth image routes must ALSO reject a non-owning photographer.
// Bob owns no share and doesn't own the album, so authorize_photo falls
// through to the share-cookie path and 401s BEFORE any S3 access — the
// cross-tenant original leak these routes previously allowed. ----
for path in [
format!("/api/img/{photo_id}/thumb"),
format!("/api/img/{photo_id}/preview"),
format!("/api/photos/{photo_id}/original"),
] {
let (status, _) = request(&router, "GET", &path, Some(&cookie_b), None).await;
assert_eq!(
status,
StatusCode::UNAUTHORIZED,
"cross-tenant image GET {path} must not authorize"
);
}
// ---- Alice keeps full access to her own resources. ----
let (status, list) = request(&router, "GET", "/api/albums", Some(&cookie_a), None).await;
assert_eq!(status, StatusCode::OK);