fix(security): harden auth, storage limits and deployment defaults

Dockerfile

@@ -24,5 +24,18 @@ RUN apk add ca-certificates
 
 COPY --from=builder /app/target/release/opentubex-sync /app/opentubex-sync-server
 
+# Run unprivileged.
+#
+# NOTE for SQLite deployments: a bind-mounted host directory keeps its host
+# ownership and shadows the ownership set here, so the host directory must be
+# owned by this uid or the database and its WAL sidecars are not writable:
+#     sudo chown -R 10001:10001 ./data
+# See the "Running as non-root" section in README.md.
+RUN addgroup -S -g 10001 opentubex \
+    && adduser -S -u 10001 -G opentubex opentubex \
+    && mkdir -p /app/data \
+    && chown -R opentubex:opentubex /app
+USER 10001:10001
+
 EXPOSE 8080
 CMD ["./opentubex-sync-server"]

README.md

@@ -44,13 +44,89 @@ There are two ways to configure `sync-server`
 | Config option                   | Description                                          | Default | Example              |
 | ----------------------          | ---------------------------------------------------- | ------- | -------------------- |
 | `database_url`                  | Connection string for the database                   | None    | sqlite://./db.sql    |
-| `secret_key`                    | Used to sign authentication tokens                   | None    | SomeVeryLongString64 |
+| `secret_key`                    | Used to sign authentication tokens. Required, min. 32 bytes | None | output of `openssl rand -hex 32` |
+| `username_secret`               | Used to derive account name hashes. Set it so that `secret_key` stays rotatable | falls back to `secret_key` | output of `openssl rand -hex 32` |
+| `trust_forwarded_for`           | Derive rate limiting client addresses from `X-Forwarded-For`. Required behind a reverse proxy | `false` | `true` |
+| `trusted_proxy_hops`            | Number of proxies in front of this server, used to pick the right `X-Forwarded-For` entry | `1` | `2` |
 | `allow_registration`            | Whether to allow registering on this server          | `true`  | `false`              |
 | `validate_submitted_metadata`   | Whether to check incoming video data against YouTube | `true`  | `false`              |
-| `migration_approval`            | Exact comma-separated pending migration versions approved for an existing database after separately verifying a backup | None | `2026-07-21-180000-0000` |
+| `migration_approval`            | Exact comma-separated pending migration versions approved for an existing database after separately verifying a backup | None | `202607211800000000` |
+
+### Running behind a reverse proxy
+
+`/account/register` and `/account/login` are rate limited per client address. That
+address is the immediate peer by default, which is only correct when the server is
+directly reachable.
+
+The example compose files publish to `127.0.0.1`, so a reverse proxy is the normal
+setup — and there every request arrives from the proxy's own address. Without
+extra configuration all clients would then share a single bucket and legitimate
+users would rate limit each other. Set `trust_forwarded_for = true` so the limit
+is applied per real client:
+
+```yaml
+environment:
+  - "TRUST_FORWARDED_FOR=true"
+```
+
+The address is taken from the *last* `X-Forwarded-For` entry, which is the one
+your proxy appended, so a client cannot pick its own bucket by sending the header
+itself. Only enable this when the server is not reachable directly, otherwise a
+client can do exactly that.
+
+If you have more than one proxy in the chain — for example Cloudflare in front of
+nginx — set `trusted_proxy_hops` to how many there are. Each proxy appends an
+entry, so with two the last one is the inner proxy's address rather than the
+client's, and leaving this at `1` would put every client in one bucket again.
+
+This limiter is per process and best-effort. Rate limiting at the proxy as well
+is still recommended, and is required if you run multiple replicas.
+
+### Running as non-root
+
+The image runs as uid `10001`. A bind-mounted host directory keeps its host
+ownership and shadows the ownership set in the image, so for SQLite deployments
+prepare the data directory once before the first start:
+
+```sh
+mkdir -p ./data
+sudo chown -R 10001:10001 ./data
+```
+
+Without this the server cannot create `db.sqlite` or its WAL sidecar files. Note
+that the failure surfaces as a database connection timeout rather than an obvious
+permission error. If you are upgrading from an older image that ran as root, run
+the same command against your existing `./data` directory — this changes
+ownership only and does not touch the database contents.
+
+If you cannot use `sudo`, or your data directory is already owned by your own
+user, override the container user instead of changing ownership:
+
+```yaml
+services:
+  sync:
+    user: "1000:1000" # your own uid:gid, from `id -u`:`id -g`
+```
+
+The server only needs to write inside `/app/data`, so any uid that owns the data
+directory works. This still avoids running as root.
+
+### Secrets
+
+The server refuses to start if `secret_key` is missing, shorter than 32 bytes, or
+left at a placeholder such as `changeme`. Generate one with `openssl rand -hex 32`.
+
+Accounts are looked up by `HMAC(username)`, so changing the secret that derives
+that hash makes every existing account unreachable. Set `username_secret`
+explicitly (initially to the same value as `secret_key`) so that `secret_key`
+itself can later be rotated — for example after a suspected leak — without
+locking anyone out.
 
 Existing databases never apply pending migrations implicitly. After you create and verify
-a separate backup, `migration_approval` must exactly match every pending version. SQLite
+a separate backup, `migration_approval` must exactly match every pending version.
+Versions are digits only, without the hyphens used in the migration directory
+names — the startup error prints the exact value to use, so the simplest approach
+is to start the server once and copy it from the log. SQLite
 also creates a consistent `*.pre-migration-<version>` backup immediately before changing
 the schema. Remove the approval after deployment so it cannot authorize a later migration.
 

config.toml

@@ -1,4 +1,13 @@
 database_url = "./db.sqlite"
-secret_key = "changeme"
+# Generate with `openssl rand -hex 32`. The server refuses to start on a
+# placeholder value or anything shorter than 32 bytes.
+secret_key = ""
+# Set this to the same value as secret_key above. Keeping it separate is what
+# allows secret_key to be rotated later without locking out every account.
+# Left unset it falls back to secret_key.
+# username_secret = ""
+# Enable only when the server is reachable exclusively through a reverse proxy,
+# so that rate limiting sees real client addresses instead of the proxy's.
+# trust_forwarded_for = true
 allow_registration = true
 validate_submitted_metadata = true

docker-compose.postgres.yml

@@ -7,8 +7,18 @@ services:
       args:
         - "DATABASE_BACKEND=postgres"
     environment:
-      - "DATABASE_URL=postgres://postgres:password@db/opentubex-sync"
-      - "SECRET_KEY=changeme"
+      - "DATABASE_URL=postgres://postgres:${POSTGRES_PASSWORD:?generate one with: openssl rand -hex 32}@db/opentubex-sync"
+      # REQUIRED. Generate with `openssl rand -hex 32` and put it in a .env file
+      # next to this compose file. The server refuses to start on a placeholder
+      # value or anything shorter than 32 bytes.
+      - "SECRET_KEY=${SECRET_KEY:?generate one with: openssl rand -hex 32}"
+      # Set to the same value as SECRET_KEY. Keeping it separate is what allows
+      # SECRET_KEY to be rotated later without locking out every account.
+      - "USERNAME_SECRET=${USERNAME_SECRET:?set this to the same value as SECRET_KEY}"
+      # These files publish to 127.0.0.1, i.e. behind a reverse proxy, where
+      # every request arrives from the proxy and all clients would otherwise
+      # share one rate limit bucket. Remove this if exposing the port directly.
+      - "TRUST_FORWARDED_FOR=true"
       - "ALLOW_REGISTRATION=true"
       - "VALIDATE_SUBMITTED_METADATA=true"
     # Or use a configuration file:
@@ -25,7 +35,7 @@ services:
     container_name: opentubex-sync-postgres
     environment:
       - "POSTGRES_USER=postgres"
-      - "POSTGRES_PASSWORD=password"
+      - "POSTGRES_PASSWORD=${POSTGRES_PASSWORD:?generate one with: openssl rand -hex 32}"
       - "POSTGRES_DB=opentubex-sync"
     volumes:
       - ./data:/var/lib/postgresql

docker-compose.yml

@@ -8,13 +8,26 @@ services:
         - "DATABASE_BACKEND=sqlite"
     environment:
       - "DATABASE_URL=./data/db.sqlite"
-      - "SECRET_KEY=changeme"
+      # REQUIRED. Generate with `openssl rand -hex 32` and put it in a .env file
+      # next to this compose file. The server refuses to start on a placeholder
+      # value or anything shorter than 32 bytes.
+      - "SECRET_KEY=${SECRET_KEY:?generate one with: openssl rand -hex 32}"
+      # Set to the same value as SECRET_KEY. Keeping it separate is what allows
+      # SECRET_KEY to be rotated later without locking out every account.
+      - "USERNAME_SECRET=${USERNAME_SECRET:?set this to the same value as SECRET_KEY}"
+      # These files publish to 127.0.0.1, i.e. behind a reverse proxy, where
+      # every request arrives from the proxy and all clients would otherwise
+      # share one rate limit bucket. Remove this if exposing the port directly.
+      - "TRUST_FORWARDED_FOR=true"
       - "ALLOW_REGISTRATION=true"
       - "VALIDATE_SUBMITTED_METADATA=true"
       # Existing databases refuse pending migrations unless this exactly lists
       # every pending migration version. Create and verify a backup before approval.
-      # - "MIGRATION_APPROVAL=2026-07-21-180000-0000"
+      # - "MIGRATION_APPROVAL=202607211800000000"
     volumes:
+      # The container runs as uid 10001 and this bind mount keeps host
+      # ownership, so create and chown the directory before the first start:
+      #     mkdir -p ./data && sudo chown -R 10001:10001 ./data
       - ./data:/app/data
     ports:
       - "127.0.0.1:8080:8080"

migrations/2026-07-28-150000-0000_watch_history_account_index/down.sql

@@ -0,0 +1 @@
+DROP INDEX IF EXISTS watch_history_account_id;

migrations/2026-07-28-150000-0000_watch_history_account_index/up.sql

@@ -0,0 +1,5 @@
+-- watch_history is keyed by (video_id, account_id), so account_id is not the
+-- leading column and per-account lookups cannot use the primary key index.
+-- The row quota counts rows per account on every write, and clearing or
+-- migrating an account's history also filters on it alone.
+CREATE INDEX IF NOT EXISTS watch_history_account_id ON watch_history (account_id);

src/auth.rs

@@ -19,15 +19,23 @@ pub fn bytes_to_hex_string(bytes: &[u8]) -> String {
             .join("")
 }
 
+/// How long a freshly minted token stays valid. Should be enough in most cases.
+const TOKEN_TTL: Duration = Duration::from_hours(365 * 24);
+
+/// Allowance for clock skew between minting and verifying.
+const EXPIRY_LEEWAY: Duration = Duration::from_secs(5 * 60);
+
+fn unix_now() -> u64 {
+    SystemTime::now()
+        .duration_since(UNIX_EPOCH)
+        .unwrap_or_default()
+        .as_secs()
+}
+
 pub fn generate_jwt(account: &Account, secret_key: &[u8]) -> jsonwebtoken::errors::Result<String> {
     let key = EncodingKey::from_secret(secret_key);
-    // tokens are valid for one year, should be enough in most cases
-    let expiration_date = SystemTime::now()
-        .checked_add(Duration::from_hours(365 * 24))
-        .unwrap()
-        .duration_since(UNIX_EPOCH)
-        .unwrap()
-        .as_millis();
+    // `exp` is defined in seconds since the epoch, not milliseconds.
+    let expiration_date = unix_now().saturating_add(TOKEN_TTL.as_secs());
 
     let claims = JwtClaims {
         sub: account.id.clone(),
@@ -36,10 +44,27 @@ pub fn generate_jwt(account: &Account, secret_key: &[u8]) -> jsonwebtoken::error
     encode(&Header::default(), &claims, &key)
 }
 
+/// Whether `exp` is close enough to now that this server could have minted it.
+///
+/// `Validation::default()` only rejects expiries in the past, with no upper
+/// bound. Tokens minted before `exp` was corrected from milliseconds to seconds
+/// carry values around the year 59000, so without this check they would stay
+/// valid effectively forever and the expiry fix would only apply to new logins.
+fn expiry_is_plausible(exp: u64, now: u64) -> bool {
+    exp <= now
+        .saturating_add(TOKEN_TTL.as_secs())
+        .saturating_add(EXPIRY_LEEWAY.as_secs())
+}
+
 /// Returns the User ID on success.
 pub fn verify_jwt(encoded_jwt: &str, secret_key: &[u8]) -> jsonwebtoken::errors::Result<String> {
     let key = DecodingKey::from_secret(secret_key);
     let claims: JwtClaims = decode(encoded_jwt.as_bytes(), &key, &Validation::default())?.claims;
+
+    if !expiry_is_plausible(claims.exp as u64, unix_now()) {
+        return Err(jsonwebtoken::errors::ErrorKind::InvalidToken.into());
+    }
+
     Ok(claims.sub)
 }
 
@@ -72,3 +97,38 @@ pub fn hash_accountname(accountname: &str, secret_key: &[u8]) -> String {
     let result = &mac.finalize().into_bytes();
     bytes_to_hex_string(result)
 }
+
+#[cfg(test)]
+mod tests {
+    use super::{EXPIRY_LEEWAY, TOKEN_TTL, expiry_is_plausible};
+
+    const NOW: u64 = 1_800_000_000;
+
+    #[test]
+    fn normal_expiries_are_accepted() {
+        assert!(expiry_is_plausible(NOW + 60, NOW));
+        assert!(expiry_is_plausible(NOW + TOKEN_TTL.as_secs(), NOW));
+        // a little clock skew is tolerated
+        assert!(expiry_is_plausible(
+            NOW + TOKEN_TTL.as_secs() + EXPIRY_LEEWAY.as_secs(),
+            NOW
+        ));
+    }
+
+    /// Tokens minted before `exp` was corrected carry milliseconds, which
+    /// `Validation::default()` would happily accept for the next 57000 years.
+    #[test]
+    fn legacy_millisecond_expiries_are_rejected() {
+        let legacy_exp = (NOW + TOKEN_TTL.as_secs()) * 1000;
+        assert!(!expiry_is_plausible(legacy_exp, NOW));
+    }
+
+    #[test]
+    fn expiries_beyond_the_ttl_are_rejected() {
+        assert!(!expiry_is_plausible(
+            NOW + TOKEN_TTL.as_secs() + EXPIRY_LEEWAY.as_secs() + 1,
+            NOW
+        ));
+        assert!(!expiry_is_plausible(u64::MAX, NOW));
+    }
+}