diff --git a/app/api/shared_links.py b/app/api/shared_links.py index d452e6f6..3336edda 100644 --- a/app/api/shared_links.py +++ b/app/api/shared_links.py @@ -36,10 +36,10 @@ DbSession = Annotated[Session, Depends(get_db)] # Constants # --------------------------------------------------------------------------- -#: PBKDF2 salt for shared-link password hashing (not secret, but fixed). -_PWD_HASH_SALT = b"shared-link-v1" -#: PBKDF2 iteration count. -_PWD_HASH_ITERATIONS = 100_000 +#: PBKDF2 iteration count — matches OWASP 2023 recommendation for PBKDF2-HMAC-SHA256. +_PWD_HASH_ITERATIONS = 600_000 +#: Length of the random per-password salt in bytes (128-bit entropy). +_PWD_SALT_BYTES = 16 # Valid expiry durations (in hours) presented in the UI. EXPIRY_OPTIONS: dict[str, int] = { @@ -81,26 +81,52 @@ def _generate_token() -> str: def _hash_password(password: str) -> str: - """Return a PBKDF2-HMAC-SHA256 hex digest of *password*. + """Hash *password* with PBKDF2-HMAC-SHA256 and a random per-password salt. + + The returned string uses the format ``{salt_hex}:{dk_hex}`` so that + both the salt and the digest can be recovered from a single column. Args: password: Plaintext password string. Returns: - 128-character lowercase hex string. + String in the form ``<32-char salt hex>:<64-char digest hex>``, + totalling 97 characters (well within the 128-char column limit). """ + salt = secrets.token_bytes(_PWD_SALT_BYTES) dk = hashlib.pbkdf2_hmac( "sha256", password.encode("utf-8"), - _PWD_HASH_SALT, + salt, _PWD_HASH_ITERATIONS, ) - return dk.hex() + return f"{salt.hex()}:{dk.hex()}" def _verify_password(password: str, stored_hash: str) -> bool: - """Check *password* against *stored_hash* using constant-time comparison.""" - return secrets.compare_digest(_hash_password(password), stored_hash) + """Verify *password* against a hash produced by :func:`_hash_password`. + + Uses constant-time comparison to prevent timing attacks. + + Args: + password: Plaintext password to check. + stored_hash: The value previously returned by :func:`_hash_password`. + + Returns: + ``True`` if *password* matches, ``False`` otherwise. + """ + try: + salt_hex, dk_hex = stored_hash.split(":", 1) + salt = bytes.fromhex(salt_hex) + except (ValueError, TypeError): + return False + dk = hashlib.pbkdf2_hmac( + "sha256", + password.encode("utf-8"), + salt, + _PWD_HASH_ITERATIONS, + ) + return secrets.compare_digest(dk.hex(), dk_hex) def _is_link_valid(link: SharedLink) -> bool: @@ -369,13 +395,40 @@ def get_shared_link_info( def download_via_shared_link( token: str, db: DbSession, - password: str | None = Query(None, description="Password (if the link is password-protected)"), ) -> FileResponse: - """Download a file via a shared link (no authentication required). + """Download a file via a shared link that does NOT require a password. + + For password-protected links use ``POST /api/share/{token}/download`` + with ``{"password": ""}`` in the JSON body instead. Increments the view counter and validates expiry / view limit before serving the file. """ + return _serve_shared_file(token, db, password=None) + + +class PasswordBody(BaseModel): + """Request body for password-protected shared link downloads.""" + + password: str = Field(..., min_length=1, max_length=128, description="Password for the shared link") + + +@public_router.post("/share/{token}/download") +def download_via_shared_link_with_password( + token: str, + body: PasswordBody, + db: DbSession, +) -> FileResponse: + """Download a password-protected file via a shared link. + + Accepts the password in the JSON request body to avoid it appearing in + server access logs, browser history, or ``Referer`` headers. + """ + return _serve_shared_file(token, db, password=body.password) + + +def _serve_shared_file(token: str, db: Session, password: str | None) -> FileResponse: + """Core download logic shared by the GET and POST download endpoints.""" link = db.query(SharedLink).filter(SharedLink.token == token).first() if not link: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="Link not found or expired") @@ -401,13 +454,18 @@ def download_via_shared_link( if not file_path: raise HTTPException(status_code=status.HTTP_404_NOT_FOUND, detail="File not available on disk") - # Increment view count (best-effort — don't fail the request if this errors). + # Increment view count — fail the request if this cannot be persisted so + # that view-limited links are not bypassed during temporary DB outages. try: link.view_count = (link.view_count or 0) + 1 db.commit() except Exception: db.rollback() - logger.warning("Failed to increment view_count for shared link id=%s", link.id) + logger.error("Failed to increment view_count for shared link id=%s — aborting download", link.id) + raise HTTPException( + status_code=status.HTTP_503_SERVICE_UNAVAILABLE, + detail="Service temporarily unavailable. Please try again.", + ) return FileResponse( path=file_path, diff --git a/frontend/templates/shared_link_view.html b/frontend/templates/shared_link_view.html index fdae448a..e9e94cd7 100644 --- a/frontend/templates/shared_link_view.html +++ b/frontend/templates/shared_link_view.html @@ -185,11 +185,13 @@ return; } - // Attempt download — if password is wrong the server returns 403. - const url = `${DOWNLOAD_URL}?password=${encodeURIComponent(pwd)}`; - - // Use a hidden iframe trick to detect errors vs. successful binary downloads. - fetch(url) + // Send the password in the POST body (never in the URL) to prevent it + // appearing in server access logs, browser history, or Referer headers. + fetch(DOWNLOAD_URL, { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify({ password: pwd }), + }) .then(async (resp) => { if (resp.ok) { errEl.classList.add('hidden'); diff --git a/tests/test_shared_links.py b/tests/test_shared_links.py index d0237d6f..2a003bd5 100644 --- a/tests/test_shared_links.py +++ b/tests/test_shared_links.py @@ -154,7 +154,12 @@ class TestCreateSharedLink: db_link = sess.query(SharedLink).filter(SharedLink.token == data["token"]).first() assert db_link is not None assert db_link.password_hash != "secret" - assert len(db_link.password_hash) == 64 # hex digest length + # Hash format is "{salt_hex}:{dk_hex}" — check it can verify correctly. + assert ":" in db_link.password_hash + from app.api.shared_links import _verify_password + + assert _verify_password("secret", db_link.password_hash) is True + assert _verify_password("wrong", db_link.password_hash) is False finally: _cleanup(app) @@ -627,7 +632,7 @@ class TestPublicDownload: @pytest.mark.unit def test_download_password_required(self, sl_engine, sl_session): - """Download endpoint returns 401 when password is required but not supplied.""" + """GET download endpoint returns 401 when password is required but not supplied.""" from app.api.shared_links import _hash_password link = SharedLink( @@ -655,6 +660,7 @@ class TestPublicDownload: app.dependency_overrides[get_db] = _override_db client = TestClient(app, base_url="http://localhost", raise_server_exceptions=False) try: + # GET without password returns 401 resp = client.get("/api/share/pwdlink111/download") assert resp.status_code == 401 finally: @@ -662,7 +668,7 @@ class TestPublicDownload: @pytest.mark.unit def test_download_wrong_password(self, sl_engine, sl_session): - """Download endpoint returns 403 for wrong password.""" + """POST download endpoint returns 403 for wrong password.""" from app.api.shared_links import _hash_password link = SharedLink( @@ -690,7 +696,8 @@ class TestPublicDownload: app.dependency_overrides[get_db] = _override_db client = TestClient(app, base_url="http://localhost", raise_server_exceptions=False) try: - resp = client.get("/api/share/pwdlink222/download?password=wrong") + # Password is supplied via POST body, not URL query param. + resp = client.post("/api/share/pwdlink222/download", json={"password": "wrong"}) assert resp.status_code == 403 finally: app.dependency_overrides.clear() @@ -747,12 +754,17 @@ class TestHelpers: @pytest.mark.unit def test_hash_password_deterministic(self): - """Hashing the same password always produces the same hex digest.""" - from app.api.shared_links import _hash_password + """Two calls with the same password produce different hashes (random salt).""" + from app.api.shared_links import _hash_password, _verify_password - h = _hash_password("mysecret") - assert h == _hash_password("mysecret") - assert len(h) == 64 + h1 = _hash_password("mysecret") + h2 = _hash_password("mysecret") + # Salt is random, so hashes differ — but both verify correctly. + assert h1 != h2 + assert _verify_password("mysecret", h1) is True + assert _verify_password("mysecret", h2) is True + # Format is "{salt_hex}:{dk_hex}" + assert ":" in h1 @pytest.mark.unit def test_verify_password_correct(self):