mirror of
https://github.com/calibrain/shelfmark.git
synced 2026-11-14 10:08:24 +00:00
e575ff0fcb586bc0aa0600a2ea198d5b8c05dd5d
359
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 splitsec2
|
e575ff0fcb |
feat(activity): clearer cards, a Rejected view and Reopen for failed deliveries (#1618)
A batch of activity list changes from running it as an admin on my own
instance. They are separate commits that build on each other, so they
can be reviewed in order.
The cards read better. Every card gets a format pill (violet for audio,
orange for ebooks), the requester is named once, and finished cards say
how long a download took and how long ago it ended ("took 5m · 35m
ago"). A download that picked up work already under way shows a
Reattached pill.
Admins can tell what is automatic and what needs a person. A declined
request shows who declined it and when, anything with an `[auto]` note
reads "Declined automatically", and "Needs Review" becomes "No Match
Yet" with a line saying the search repeats on every sync.
There is a Rejected view next to History for admins, with Reopen on each
card and a way to hide the dead ones.
The part to read most carefully is the backend in `requests_service.py`
and `user_db.py`. An admin can now reopen a fulfilled request whose
download failed or was cancelled, and the reopen a failed download
triggers remembers which release failed, so the next search doesn't pick
it again.
I ran it on a scratch instance (when pulling them together to move from
my fork to upstream) as an admin with seeded requests and looked at the
pills, the durations, the wording and the Rejected view in a browser.
Reopening a fulfilled request I only exercised in the tests. The
frontend and Python suites, type check, ruff and basedpyright pass.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
<!-- coderabbit-ai-generated:v1 -->
## Summary by CodeRabbit
* **New Features**
* Admins can review rejected requests in a dedicated view, hide or
restore cards, and reopen requests.
* Fulfilled requests with failed or cancelled deliveries can be
reopened; the failed release is skipped in the next automatic search.
* Activity cards now show format badges, download timing, and when a
download resumes existing work.
* **Improvements**
* Activity views clarify automatic retries and actions, and show
reviewer names for admins.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
splitsec2
|
76d7df306f |
fix(requests): retrying a request after its client failed offers the same release again (#1677)
When a request's download fails in a way the queue can't resume, the request is reopened and its Retry button opens the release list. The release that just failed was drawn there as a red, disabled "Failed" button for as long as the failed task stayed in the queue status, so you couldn't pick it again even when the release was fine and the download client was the problem. A bad qBittorrent password is the easy way to hit it. Thanks @joshft91 for the report and the steps. What changes: - In the release list, a failed request download now shows Retry, the same as a failure the queue can retry already does. A direct download with nothing to retry from still shows Failed. The rule lives in one small helper used by the release modal and both button-state paths in `useDownloadTracking`. - A client that refuses the login is no longer held against the release. qbittorrent-api raises `LoginFailed` with no message for a wrong password, and only "Forbidden" or "Unauthorized" for a bad session or API key, so none of it matched the unreachable-client markers from #1655 and the release went into `failed_releases`. The qBittorrent client now says "qBittorrent authentication failed (...)" for these. - The request reopen and the auto-download retry used two copies of the same marker list. They now share one in `status_messages.py`, which also counts an explicit authentication failure as the client's fault. A bare 403 or 401 is not on it, since a dead .torrent link fails that way too and that one is the release's. Genuine release failures (no seeders, a rejected torrent, a dead .torrent link) are still remembered as #1655 intended. Tested with a scratch instance against a mock Prowlarr and a fake qBittorrent that rejects the login: before, the request's Retry opened the list with the failed release as a red X; after, it shows Retry, picking it fulfils the request again, and the request reopens without the release in `failed_releases`. New tests cover the qBittorrent messages, both reopen paths and the button state. Fixes #1674 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Failed releases are no longer skipped after download-client connection or login failures; download failures and stall cancellations remain eligible to be skipped on the next search. * Failed requests with an associated request can now show a retry option, while direct downloads without retries remain marked as failed. * qBittorrent login and authorization errors are reported more clearly. * **Documentation** * Clarified which failures cause a release to be skipped on a later search. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
e828bdbe9c |
feat(slskd): answer anti-leech chat challenges from uploaders, off by default (#1591)
Some Soulseek uploaders run a bot that blocks your downloads until you
type a word to them in a private message ("please type 'download' in
this chat"). The files sit queued until someone answers, and nobody
looking at Shelfmark would know why.
While a slskd download is polling, this checks slskd's unacknowledged
private messages about once a minute, replies with the word the message
asks for and marks the conversation read. It only matches three known
phrasings, the word has to fit `[A-Za-z0-9_-]{1,24}`, and each user gets
at most one reply per 12 hours (recorded after a successful send). Every
reply is logged at INFO.
The challenge patterns and the one-reply-per-12-hours idea come from
@Nezreka's [SoulSync](https://github.com/Nezreka/SoulSync)
(`core/chat_autoprove.py`, commit 445473b, MIT). I reworked it for
slskd's client and this download handler. The credit is in the module
docstring and the commit message too.
This sends messages to other people on your behalf without asking, so
the setting `SLSKD_CHAT_AUTOPROVE` is off by default here. Say if you'd
rather it not exist at all, or want it somewhere other than the slskd
settings.
It has run on my instance since October 22 without seeing a real
challenge, so the reply path is covered by tests but not by a live one.
The same code works in nezreka/soulsync with the same slskd client so I
am confident in it. Ruff, basedpyright and the slskd tests pass, and
`docs/environment-variables.md` and the readme line for slskd are
updated.
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
<!-- coderabbit-ai-generated:v1 -->
## Summary by CodeRabbit
* **New Features**
* Added an optional setting to automatically answer supported Soulseek
private-message challenges while a download is running. When enabled,
Shelfmark checks unread messages, replies to each person at most once
every 12 hours, logs replies, and marks answered conversations as read.
The setting is off by default.
* **Documentation**
* Updated the environment-variable and Soulseek setup documentation with
details about the setting and its behavior, including a reminder to
leave it off if you read slskd messages yourself.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
|
||
|
splitsec2
|
a01e700c81 |
fix(auto-download): only fulfil requests the owner could download directly (#1676)
Automatic downloads fulfil pending Hardcover requests as an admin, and they picked up every pending request with provider `hardcover`. A request a user makes from the search UI carries that provider too, so with automatic downloads on, a request from a user whose policy needs approval was approved by the pass instead of waiting for an admin. I added that behaviour in #1516 and #1535 and missed this case. The pass now runs the same check the download route makes. A request is searched only on the release sources where its owner's resolved policy (global settings plus their own overrides) is Download. Requests owned by an admin, installs without login, and users with requests turned off are not limited, since all of them can already download directly. A request left with no source stays pending and untouched: no search, no backoff state, no note. The 7-day and quick retries only reopen a failed download on the same terms, so a request the pass can't fulfil isn't moved back to pending for nothing. The pass summary gains an `awaiting_approval` count. The `AUTO_DOWNLOAD_ENABLED` description, the env var reference and the Hardcover sync docs say which requests it approves. Tests cover approval needed, direct download allowed, admin owned, blocked, a per-source rule, a per-user override, requests turned off, no login, and the retry reopen. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Automatic downloads now respect each request owner’s policy and permitted sources. Requests that require approval or are blocked by policy remain pending rather than being searched. * Failed downloads are reopened for retry only when the owner’s policy allows direct fulfillment. * Admin-owned requests and requests in unauthenticated mode retain their existing automatic fulfillment behavior. * **Documentation** * Clarified how request ownership, download policies, approval requirements, and retries affect automatic downloads. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
8c06862ec7 |
feat(requests): let a book be requested alongside downloads (#1579)
In Download mode there is no way to ask for a book that has nothing to download yet. The release modal has an "Add to requests" chip for admins, which sends a book-level request, and when the default mode is Download the server answers it with "Download policy requires a concrete release". That is #995, where @expostarv2 used to bookmark unreleased books this way. #1019 is the other side of it: @punkincognito wants a less technical user to be able to download from one source and, when that fails, ask for the book without choosing a release. A book-level request in Download mode now becomes a normal pending book request, stored with the mode `request_book`, when the actor is an admin or when a new setting allows it: **Allow book requests alongside downloads** (`REQUESTS_ALLOW_BOOK_REQUEST_WITH_DOWNLOAD`, off by default, per-user overridable). A request that names a release still goes straight to the download queue as before, and with the setting off a non-admin gets the same error as today, so nothing changes for an existing install unless it turns the setting on. The policy endpoint reports the setting, and the release modal shows "Add to requests" to a non-admin when requests are on and the setting allows it (admins keep it as before, and it stays off while fulfilling a request). The small `canRequestBookFromReleaseModal` helper holds that rule so it can be tested. I chose a setting over making this the default for everyone because it changes what Download mode means, which is yours to decide. The other way to read #1019 is a per-source rule, "Request Book" for Prowlarr and Download for the direct sources. That is not here: the rules matrix excludes Request Book today, the release modal shows such a source as unavailable, and it needs more UI work than I can check without trying it in a browser. This PR covers the half that works with the existing flow, and the rule version can follow if you want it. Tests: four route tests (a user with the setting on, an admin with it off, a release request that must still queue a download, and the policy endpoint reporting the setting), and a frontend test for the helper. The route tests that depend on the change fail on current main. Full backend and frontend suites pass, with ruff, oxlint and tsc clean. I have not tried it in a browser, so this is a draft until someone has looked at the modal. Closes #995. #1019 stays open for the per-source rule. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **New Features** * Added an off-by-default, user-overridable setting that lets users with download access request a book without selecting a release. These requests remain pending for admin action, and admins can always make book-level requests. * Eligible users can make book-level requests under the download policy when the setting is enabled. This option is unavailable in browse-fulfil mode. * Requests that include release details continue to queue downloads rather than create book-level requests. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
371ac75954 |
feat(activity): say in plain words what a failed download means and what happens next (#1617)
A failed or cancelled card showed the log line, things like `Peer transfer failed: Errored` or a 500 with a URL in it, so a red card meant going to the logs to find out whether anything was coming next. They now say what happened and what happens next. For a download that belongs to a request, the card says another copy will be tried in about N minutes when the quick retry from #1535 is on, and points at Retry or reopening the request when it isn't. A manual download is never promised an automatic retry. The Soulseek cases each get their own sentence: the peer dropped the transfer, the peer could not be reached, slskd refused to queue it, and the peer never answered. The "tried twice" wording leans on the retry from #1590. The saved message is left alone because auto-download reads it to tell a failed release from an interrupted one. Only what the activity list shows is reworded, on a copy, and anything the new code doesn't recognise comes back as it was. It matches on how the message starts, so it is only as current as the wording the rest of the code uses. A message that gets reworded elsewhere falls back to the original line instead of breaking, which seemed fine for something that only changes the display, but it is the part I would look at first. It has been running on my instance since mid-October. Ten new tests cover each message and the next step, and the activity route tests still pass. Ruff and basedpyright are clean on the changed files. |
||
  Matt Van HornandMatt Van Horn
|
0bb54ac8f8 |
fix(clients): use port 443 for HTTPS Transmission URLs that omit a port (#1673)
## Why Saving a Transmission address as HTTPS and leaving the port blank makes the connection test fail. The client opens port 9091 and reports an SSL wrong-version error, so a daemon published through a reverse proxy on the usual HTTPS port never answers. parse_transmission_url filled in 9091 whenever the URL omitted a port, and that default ignored the scheme. An https address therefore started TLS against 9091, where the peer is not speaking TLS. ## Summary An omitted port now follows the scheme: 443 for https, and 9091 for http, which still covers the empty-URL default. A port written in the URL is left as given. The parser tests cover https://transmission.myserver.org and expect https, that host, port 443, and the path /transmission/rpc. ## Testing The targeted tests for the shelfmark parse_transmission_url change pass locally. The tests passed with the shelfmark parse_transmission_url change applied; a run with it reverted gave no clear result, so no test is shown to catch this bug. Closes #1667 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * HTTPS Transmission URLs without an explicitly specified port now use port 443. HTTP URLs continue to default to port 9091, and the default RPC path is unchanged. * **Documentation** * Clarified the default ports for each URL scheme and noted that HTTPS instances on port 9091 require `:9091` in the URL. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> |
||
|
splitsec2
|
e8ec6cc715 |
fix(requests): reopening after a failed download remembers the release that failed (#1655)
When a download fails, the terminal hook reopens its request through reopen_failed_request straight away, with no admin involved. That path cleared the release without noting that it had failed, so the next search could pick the same release again. Only the admin reopen and the status sync remembered it. Both paths now add the release to failed_releases before the reopen clears it, the same list the unattended retry already skips, and a release that is already listed is not added twice. A download that failed because its client could not be reached (the same four markers auto_download uses) is not held against the release, since the same one is fine once the client is back. This is the upstream shape of something I have been running on my own instance since mid October. ruff, basedpyright and the requests_service tests (87) pass, with five new tests for the two paths, the dedupe and the unreachable client. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Failed requests are reopened after non-retryable delivery failures, with the failed release recorded when appropriate. * Connection failures caused by an unreachable client no longer mark the release as failed. * Reopening a request does not record the same release more than once or alter book details when no new release can be recorded. * Queue status updates apply the same release-tracking behavior, while requests that are not failed remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
eb9ee093de |
fix(auth): don't fail the lockout cleanup when a login clears the entry first (#1658)
cleanup_old_lockouts collects the expired usernames, logs each one and then deletes it. Under gevent the log write can yield, and a successful login for the same user in that gap clears the entry first, so the del raises KeyError in whatever request ran the cleanup. Both removals now use pop(username, None), and the new test reproduces the KeyError on main. While in there I dropped the if db_user check in the builtin login path. It is always true after the not db_user return just above it, and the failed response after it could never be reached. The diff looks bigger than it is because the block moves out one indent. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Login failures now follow a consistent response path when an account is unavailable or its password data is invalid. * Expired login lockouts can be cleaned up safely even if they are cleared at the same time. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
b246c6617b |
fix(app_setup): create the secret key file private instead of tightening it after the write (#1659)
load_or_create_secret_key writes the Flask secret with write_bytes and then chmods it to 0600. Until the chmod the key sits in a file with whatever the umask allows, and if the chmod fails it stays that way and only a warning is logged. The file is now opened with mode 0600, an existing one is fchmod'ed before anything is written, and then the key goes in. The new test makes chmod fail and checks the file still ends up private, which fails on main. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Secret-key files now receive restrictive permissions when created or replaced, helping prevent unauthorized access to the stored key. * Key creation maintains these permissions even when a later permission-setting operation fails. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
9d8e7eaad2 |
fix: keep exception text out of the queue, release, metadata and debug responses (#1643)
Now that the route moves have landed, this takes care of the code scanning alerts they brought along. Sixteen places in `queue_routes.py`, `release_routes.py`, `metadata_routes.py` and `system_routes.py` handed `str(e)` of an unexpected OSError, RuntimeError, TypeError or sqlite3 error back as the 500 body. It can carry file paths, URLs with tokens or driver text. Same idea as #1558 and #1562. Each now returns one fixed sentence ("Could not cancel the download", "Could not read the queue order", "Metadata search failed" and so on) and the full error and trace stay in the log. The routes covered are the cover proxy, cancel, retry, priority, reorder, queue order and active downloads; release download, search, source list and source record; the metadata providers, config, search and book routes; and the debug endpoint. I left alone the messages that are written for the person using the app: what a metadata provider raises as a ValueError or RuntimeError ("Hardcover API key is invalid"), `SourceUnavailableError`, and the reason `queue_release` gives. CodeQL still flags those, as I read it those are the ones that should be dismissed as intended, but that is your call. The same pattern is in the settings and onboarding routes, which aren't flagged, so I didn't touch them. There are 15 new tests, one per fixed route except the debug endpoint, which only exists in debug mode. All 15 fail without the change. Two queue tests that asserted the old text now assert the fixed message. The full suite, ruff and basedpyright are clean. @calibrain I pushed through the refactors keeping them the same functionality, this addresses "issues" flagged in them, I think some of these were dismissed before, but I gathered them all up here in a PR that you should have a say on as it is changing long standing code. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Error responses now show generic messages instead of operational exception details across metadata, queue, release, and debug endpoints. HTTP status codes and error logging remain unchanged. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
 Rupraj Datta
|
ed0777a2e0 |
Spool large debrid downloads to disk (#1632)
Fixes #1630. Debrid downloads currently buffer entire files in RAM. The four clients now opt into a spooled temporary file: downloads up to 8 MiB stay in memory, and larger downloads roll to disk. The clients copy from the file in chunks and close it after use. Other download_url callers retain the existing BytesIO behavior. Temporary files are closed on unsuccessful attempts and cancellation; successful resumed downloads remain open for their caller. Added offline coverage for disk rollover, timeout cleanup, and resumed content. Updated the Debrid-Link test doubles for the new opt-in argument. Validation: 113 tests passed across test_http_download_url, test_debridlink_client, and test_torbox_client; Ruff check and format check passed for changed files; git diff --check passed. No live debrid services were exercised. AI assistance was used in implementing this change. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Improvements** * Large downloads are handled more efficiently, reducing memory use while preserving download and resume behavior. * **Bug Fixes** * Temporary download data is cleaned up when a download is cancelled or fails, helping prevent unnecessary resource usage. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
622f6ac677 |
refactor: move the download recording into core/download_recording.py (#1642)
Last of the planned moves is the download recording (row 9 in Discussion #1536). The queue hooks that write a download to the history when it is queued, finalize it on a terminal status, notify, push the activity update and reopen a failed request move from `main.py` to `core/download_recording.py`, registered with `register_download_recording(...)`. None of it was tested directly, so the first commit adds 38 tests: the queued record for direct and request linked downloads, the terminal snapshot for complete, error and cancelled, ignoring non terminal statuses, who gets notified, reopening a failed request, and what happens when a service is missing or a call fails. They pass on current `main`. The second commit is the move. `_resolve_status_scope` and the request update emitter stay in `main.py` because other code uses them, so the emitter is passed in, and the user database and history service are looked up per call as before. The hooks are registered at the same spot. Some existing test lines changed with the move and no assertion did: the `notify_admin`, `notify_user` and `reopen_failed_request` patches point at the new module, the direct calls to `_record_download_queued` go through the queue's hook, and two tests import `QueueStatus` and `viewer_download_id` themselves now that `main.py` no longer imports them. The URL map is identical, all 77 rules, and the full suite, ruff and basedpyright are clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Improvements** * Download history and activity updates reflect queued downloads and their completion statuses; activity updates are emitted after successful finalization. * Admins and eligible download owners receive notifications when downloads reach a terminal status. * Requests linked to downloads that fail and cannot be retried are reopened with a failure reason. * Activity updates use an opaque download identifier rather than exposing the underlying task identifier. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
fa28bc2185 |
refactor: move the release routes into core/release_routes.py (#1640)
The release routes are the largest piece left in `main.py`. `/api/releases`, `/api/releases/download`, `/api/release-sources` and the source record route move to `core/release_routes.py`, registered with `register_release_routes(app, login_required, ...)`. They take the content type inference, the request policy checks that gate listing and download, and the release serializers with them. The inspect route is registered from the same function since it shares the policy gate. `main.py` is about 700 lines shorter. The first commit adds 34 tests for what the policy, budget, direct provider and title tests did not cover: content type inference on download, the policy refusal on download, the error statuses of the listing route, the source list and the record route. They pass on current `main`. The second commit is the move. The policy checks need the auth mode, the user database and the request policy settings, so they come in as callables that `main.py` resolves lazily, which keeps the existing tests that patch those on `shelfmark.main` working. One test line changed, the patch of `get_source_content_type_capabilities`, which now points at the new module. Log lines from these routes now name `shelfmark.core.release_routes`. The URL map is identical, all 77 rules. The full suite, ruff and basedpyright are clean, and I ran the app from the branch with the other two route PRs merged in and hit the source list, the listing route's validation, the record route and the download route, which answer as before. The exception text a few of them return in a 500 is unchanged, that is for the follow-up on exception text. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Release Discovery** * Release searches can use metadata providers, manual searches, or direct source browsing, with results and source details returned together. * Searches respect request policies and report unavailable sources or partial-search errors. * **Downloads** * Download requests validate submitted details, infer a content type when possible, and report whether the request was queued successfully. * **Source Browsing** * Available sources and individual source records can be retrieved, subject to applicable access policies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
2254e2bf93 |
refactor: move the proxy-auth and API-key middleware into core/auth_middleware.py (#1641)
Row 8 of the plan in Discussion #1536 is the auth middleware. The API key `before_request` hook, the proxy auth hook, the hook that strips the session cookie from API key requests and `_proxy_default_is_admin` move from `main.py` to `core/auth_middleware.py`, registered with `register_auth_middleware(app, resolve_auth_mode=..., get_user_db=...)`. This is the code that decides who a request is, so the first commit adds 41 tests before anything moves. They cover the hook order, the admin group mapping (comma and pipe delimiters, a custom group header, the WSGI fallback), proxy header handling in each auth mode, provisioning once per session, the API key scope (exempt paths, a read only key only on GET `/api/stats`, a wrong key) and the cookie stripping. They pass on current `main`. The second commit is the move. The call is made right after `register_security_headers`, so the `before_request` and `after_request` order is the same as before. The existing tests called `proxy_auth_middleware()` and `api_key_auth_middleware()` on `shelfmark.main` directly, and those names are gone, so 18 call lines in three test files now find the registered hook by name through a small helper. No patch targets changed since the auth mode and user database are still read from `main` at request time. The URL map and hook order are identical, and the full suite, ruff and basedpyright are clean. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Authentication** * API-key authentication supports admin and read-only access, with read-only keys limited to statistics requests. * Reverse-proxy authentication recognizes configured identity and group information, and applies admin access according to the configured role settings. * Requests with invalid credentials, missing proxy identities, or authentication errors receive appropriate error responses. * API-key requests no longer retain session cookies. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
384adc4c75 |
refactor: move the queue routes into core/queue_routes.py (#1637)
Next from the route plan in Discussion #1536 are the queue routes: status, cancel, retry, priority, reorder, queue order, active downloads, the local file download and the cover proxy. They move from `main.py` to `core/queue_routes.py`, registered with `register_queue_routes(app, login_required, ...)`, and `main.py` is about 600 lines shorter. The first commit adds 33 tests for what the guardrail tests did not cover, mostly the local download in each shape, the admin paths, the validation errors and the failure responses. They pass on current `main`. The second commit is the move. `resolve_status_scope` and the request update emitter stay in `main.py` because the release routes and the request code still use them, so they come in as parameters, with the user database and the history service as callables evaluated per request. Two test lines changed: the patches of `send_file` and of the delivery state sync now point at `queue_routes`, since that is where they get looked up. Log lines from these routes now name `shelfmark.core.queue_routes`. The URL map is identical, all 77 rules. The full suite, ruff and basedpyright are clean, and I ran the app from the branch and hit status, queue order, active downloads, the local download and the cover route, which answer as before. The exception text a few of them return in a 500 is unchanged, that is for the follow-up on exception text. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Improvements** * Queue status, downloads, and queue controls respect account ownership, with administrative access retained. * More specific errors clarify when queue items or files are missing or unavailable. * Queue actions support cancellation, retrying, prioritization, and reordering. * Cover requests can use cached images when caching is enabled. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
f06585aa01 |
refactor: move the secret key, session cookie, security headers and log filter into core/app_setup.py (#1639)
`main.py` also builds the app itself, and this moves the parts of that which need no route into `core/app_setup.py`: the persisted secret key and the session cookie settings, the security headers, and the filter that keeps status polling and the WebSocket upgrade noise out of the log. It is the app setup item in the plan from Discussion #1536. The first commit adds 14 tests for them, since none existed: the key reused, created with mode 600, replaced when too short, and still returned when the config directory cannot be written, the cookie settings, the headers on a real response and that a header a route set itself is kept, and each case the log filter drops or keeps. They pass on current `main`. The second commit is the move. The security headers hook is registered from the same spot in `main.py` as before, so the `after_request` order is unchanged. The secret key function takes the config directory as a parameter instead of reading `CONFIG_DIR`, which is the one change in the move, and the key tests call it with a temp directory. The URL map and the hook order are identical, and the full suite, ruff and basedpyright are clean. I also ran the app from the branch: the secret file is created with mode 600 and the four security headers come back on a normal response. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Sessions now remain consistent across app restarts, and session cookies use more protective browser settings, including HTTP-only access and a seven-day lifetime. * Responses include baseline security headers while preserving any headers already set by the app. * Routine status-polling and connection-upgrade messages are filtered from application logs, reducing unrelated log noise. * If saving a session key fails, the app can still start with a generated key. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
9ca1981d00 |
refactor: move the Socket.IO handlers into core/socket_handlers.py (#1638)
This one is small. The three Socket.IO handlers, connect, disconnect and request_status, move from `main.py` to `core/socket_handlers.py`, registered with `register_socket_handlers(socketio, ...)`. It is the Socket.IO item in the plan from Discussion #1536. They had no tests, so the first commit adds 9 that go through Flask-SocketIO's test client: what a client gets on connect with no auth, as an admin, as a user and as a visitor with no session, a failure reading the status, the manual status request, and the room bookkeeping on disconnect. They pass on current `main`. The second commit is the move. `resolve_status_scope` stays in `main.py` since the HTTP routes use it too, so it is passed in. One test line changed, the patch of `redact_status`, which now points at the new module. Log lines from the handlers now name `shelfmark.core.socket_handlers`. The URL map and the registered socket events are identical before and after, and the full suite, ruff and basedpyright are clean. I also started the app from the branch and the Socket.IO handshake answers. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> <!-- coderabbit-ai-generated:v1 --> ## Summary by CodeRabbit * **Bug Fixes** * Status requests now return a consistent empty update when access is denied and an error message when status retrieval fails. * Status visibility follows the requester’s access level, with non-admin results redacted. * **Refactor** * Connection, disconnection, and status-request handling now follows consistent room and connection-tracking behavior. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
splitsec2
|
c6685feba5 |
fix(docker): drop ffmpeg from the default image; install on demand in Debug mode (#1631)
ffmpeg is in the default image for one thing, the Debug mode screen recording (`_start_ffmpeg_recording`). Nothing else calls it, SeleniumBase included, and it brings 93 packages with it. On real builds that's 406 Trivy findings (149 High) and 78 MB of the gzipped amd64 image. This takes it out of the image and installs it when Debug mode needs it. It came out of the base image discussion on #1616 and helps on any base, though on Wolfi the install line would need `apk`. ## What changed - **Dockerfile:** ffmpeg is gone from the full image's install. - **Guard:** `_start_ffmpeg_recording` checks `shutil.which("ffmpeg")` first. If it's missing it logs a warning and returns, and the bypass carries on. Without this, Debug on with no ffmpeg fails every bypass with `FileNotFoundError`. There's a new test for it. - **Entrypoint:** when Debug is on, the internal bypasser is in use and ffmpeg isn't already there, it installs ffmpeg in the background so the app doesn't wait for it. The download and the install each run under a 120 s `timeout` with `DEBIAN_FRONTEND=noninteractive`. Downloading first means a hung mirror can only be killed before dpkg starts, and the next start runs `dpkg --configure -a` in case a restart cut dpkg off. It logs when it starts, succeeds, times out or fails. - **Docs:** a short paragraph in `docs/troubleshooting.md` covering the install and the non-root limit. ## Debug has two sources Debug can come from the `DEBUG` env var or from the Debug Mode setting in the UI, which is saved to `/config/plugins/advanced.json`. The existing block in `entrypoint.sh` only reads the env var, so an install keyed on `$DEBUG` would miss anyone who turned it on in the UI. The new check asks the app instead (`shelfmark.config.env.DEBUG`), which reads both. The UI setting is `requires_restart`, so the restart it already asks for is what brings it through the entrypoint. ## Non-root mode A container started as `1000:1000` can't run `apt-get`, so the entrypoint logs that it can't install ffmpeg, the guard warns on each bypass, and there's no recording. Everything else works. The workaround for a debugging session is to start as root with `PUID`/`PGID`. ## Startup cost None that I could measure. With Debug on, `/api/health` answers in the same time as main (3-4 s with Debug only in config, 9-10 s from env), and ffmpeg was ready 6-8 s later on the runners. The trade-off is that the first bypass after startup may be unrecorded while the install runs. It logs the warning and passes. I saw both outcomes on a bypass started right after health. From reading the code: the install lives in the container, so recreating the container installs it again. It also runs after `tor.sh` and `wireguard.sh`, so apt goes through whichever egress is active. ## Testing Real Docker builds on GitHub Actions on my fork, amd64 and arm64, with the same result on both for every case below. | Case | Result | |---|---| | Debug off | No install, `/api/health` in 3-4 s, same as main | | Debug only in config | Health in 3-4 s, ffmpeg ready 6-8 s later, real mp4 after a bypass | | Debug from env | Health in 9-10 s (same as main), real mp4 after a bypass | | Started as root, app dropped to uid 1000 | Smoke and bypass pass | | Started with `--user 1000`, Debug on | Logs both messages, bypass passes, no mp4 | | apt blocked (closed proxy) | Logs `Could not install ffmpeg`, bypass passes, no mp4 | | apt's proxy on a dropped address, so no TCP reply at all | Health in 9-10 s, a bypass during the install passes with the warning and no mp4. apt gives up by itself in under a minute. Nothing left running, `dpkg --audit` clean | | DNS server on a dropped address | Health in 9-10 s, `Timed out installing ffmpeg after 120s`. Nothing left running, `dpkg --audit` clean | | `--network none`, Debug on | No hang, health 200 from inside the container | | Container restarted while dpkg was running | Next start finishes the install, `dpkg --audit` clean, real mp4 | | Fresh named volume, Debug only in config | Recording made, uid 1000 can write | | Existing uid-1000 volume, Debug only in config | Same, nothing root-owned afterwards | | `find /config /var/log/shelfmark -user root` | 0 files, same as main | | `find /app -user root` | 245, same count as main | | `tests/e2e/platform/run-e2e.sh` with `dns-blocked` and `baseline` | Pass | The bypass checks ran against example.com. A real Cloudflare challenge was served by a demo page in the same runs, and no build solved it, including main, so that part doesn't say anything about this change. Locally: 5494 Python tests pass, and `ruff`, `basedpyright`, `vulture` and `shellcheck` are clean. The full results are in [the base image report](https://github.com/splitsec2/shelfmark/blob/7afcecfb76727724a71561cbc078f091569b58dc/docs/research/2026-11-01-base-image-report.md). |
||
|
splitsec2
|
34c9e6dbd7 |
refactor: move the auth routes into core/auth_routes.py (#1625)
The auth routes are the next group from the plan in Discussion #1536: `/api/auth/login`, `/logout` and `/check`, plus the login lockout helpers, move from `main.py` to `core/auth_routes.py`, registered with `register_auth_routes(app, ...)`. `main.py` is about 370 lines shorter. This is the login path, so I kept it strictly to a move. Two commits. The first adds 59 tests that pin the current behaviour before anything moves: login in each mode (builtin, none, CWA and the OIDC password fallback), wrong passwords, the lockout (the warning when five attempts are left, the 429 and its text on the tenth failure, per-user isolation, expiry), the local-auth switches, logout in each mode and `/check`. They pass on current `main`, and I ran the same file against `main` and against this branch. The second commit is the move, and no existing test changed. The lockout state is one dict, now living in `auth_routes`, and `main.py` imports the same object so its other users (the password-attempt limiter for the self-user routes, and the tests) see it. Everything else that comes from `main` (the user database, the auth mode, the client IP, the CWA database path, the local-auth settings and the password check) is passed in as a callable evaluated per request, so patches on `main` still apply. Two things to know. `main.py` re-exports the lockout names and keeps three lines assigning the view functions (`api_login = app.view_functions["api_login"]` and the other two), because the e2e tests call `main_module.api_login()` directly and I did not change tests in this PR. If you would rather drop both, I can edit those e2e tests instead. The other is that the lockout dict is no longer reset by a reload of `main`, which only matters in tests, and they clear it themselves. The URL map is identical, all 77 rules, and the full suite, ruff and basedpyright are clean. I also ran the app with builtin auth on a scratch instance and went through it over HTTP: a wrong password gets a 401, login works and `/check` shows the session, logout clears it, the tenth failed attempt gets the 429, and the right password is refused while the account is locked. |
||
|
splitsec2
|
d1e959758d |
refactor: move the system routes into core/system_routes.py (#1624)
The system routes are the next group from the plan in Discussion #1536: `/api/config`, `/api/health`, `/api/debug`, `/api/restart` and the routes that serve the frontend (`/`, the SPA catch-all, `/assets/...`, the logo, the favicons and `theme-init.js`) move from `main.py` to `core/system_routes.py`, registered with `register_system_routes(app, login_required, ...)`. `main.py` is about 250 lines shorter. Same two-commit layout as before. The first commit adds 14 tests for the frontend-serving routes and the `/api/config` error paths, which had none, and they pass on current `main`. The second commit is the move, and no existing test changed. The helpers only these routes use moved with them (`admin_required`, built on the `login_required` that is passed in, plus `_base_href` and `_serve_index_html`). What `main.py` still owns, the base path, the frontend folder, the auth mode and the session user id, is passed in as callables evaluated per request, so the tests that patch them on `main` still work. The debug routes are only registered when debug is on, as before, with the stop-GUI and run-script hooks handed in. The thing to look at is the SPA catch-all, which is now registered earlier in `main.py` than it used to be (the old comment said it must be last). Werkzeug picks the most specific rule whatever the registration order, and I checked it: all 76 rule and method pairs in the URL map still match their own endpoint, on `main` and on this branch, and an unknown `/api/...` path still gets the JSON 404. The Flask URL map is identical before and after, all 77 rules, and with debug on (79). The full suite, ruff and basedpyright are clean. I also ran the app from this branch: health, config, the logo, the favicon and the theme script answer as before, a client-side path like `/library/books` gets the index page, an unknown `/api/` path gets the JSON 404, and the page loads in a browser. I did not exercise `/api/restart` or a real debug script run, since nothing tests them. |
||
|
splitsec2
|
d41fa98749 |
refactor: move the metadata routes into core/metadata_routes.py (#1623)
The metadata routes are the next group from the plan in Discussion #1536: the eight `/api/metadata/...` handlers (providers, config, search, field-options, book and the three targets routes) move from `main.py` to `core/metadata_routes.py`, registered with `register_metadata_routes(app, ...)`. `main.py` is about 490 lines shorter. Same two-commit layout as the settings move (#1595). The first commit adds 39 tests that pass on current `main` before anything moves. They cover the 401 on all eight routes, the shape of the providers list and the config, the search errors, the book lookup (200, 404, 400 and 503), and how target errors map to 400, 502 and 500. The second commit is the move, and no test changed in it. Three helpers moved with the routes because nothing else uses them. `_attach_library_ownership` stays in `main.py` since the releases routes need it too, and is passed in along with the login decorator and the auth mode, the way `activity_routes` takes them. The auth mode is a callable evaluated per request, so the one test that patches it on `main` still works. The Flask URL map (every rule, its methods and its endpoint name) is identical before and after, all 77 rules. The full suite, ruff and basedpyright are clean. I also ran the app from this branch and hit the routes over HTTP: providers and config answer 200, a book that doesn't exist gets a 404, and search answers 503 with the "no metadata provider configured" message on an instance with none enabled. I did not look at it in a browser or run the e2e tests. Log lines for these routes now come from `shelfmark.core.metadata_routes`, and the error tuples are local copies so the module doesn't import `main`. One thing to know when reading the diff: the register call sits just after `_attach_library_ownership` and not where the first route used to be, because that function has to be defined before it can be passed in. |
||
|
splitsec2
|
9bd6c51388 |
fix(hardcover): read the featured series from a search result (#1589)
Closes #1585. @joeRob2468 traced this one to the parser, and the answer turned out to be sitting in the search response already. A Hardcover search hit carries the book's featured series as `featured_series`, with the same shape as the `featured_book_series` of a full record: the position and a nested series with its id, name and book count. `_parse_search_result` never read it, so a book requested from a search card was stored with `series_name: null` and the `{Series}` folder and the custom script payload came out empty. It now reads it, through a small helper that `_parse_book` uses too, so there is no extra Hardcover call per request (the issue's other option was fetching the full record, which costs one against the rate limit). I checked it against a live search for "Mistborn The Final Empire". The top hit parses to series "The Mistborn Saga: The Original Trilogy", position 1, id 10001, and its search title is still "Mistborn". That last part is on purpose. I left `_compute_search_title` alone, since passing it the series would change what gets searched for everyone and the series name here doesn't equal the "Mistborn" prefix anyway. Two things that behave differently. A search result now has `series_name` set, so anything in the UI that shows it for a full record (the series link, the bigger cover in the release modal) will show it on search results too. I ran the branch and looked at the release modal for the top Mistborn result: it shows "#1 of 3 in The Mistborn Saga: The Original Trilogy" with the View series link. The modal fetches the full record when it opens, so I can't say how much of that is new on this path. Requests already stored keep the empty series they were saved with. Tests: four in a new file (a hit with a series, hits with none or an incomplete one, a hit nested in a `document`, and the search title staying the same), the first and third fail on current main. The metadata and core suites, ruff and basedpyright are clean. |
||
|
splitsec2
|
cd5a478831 |
refactor: move the onboarding routes into core/onboarding_routes.py (#1594)
First of the route moves from Discussion #1536, following the plan table there: the three onboarding routes (`GET` and `POST /api/onboarding`, `POST /api/onboarding/skip`) leave `main.py` for `core/onboarding_routes.py`, registered with `register_onboarding_routes(app, login_required)` the way `release_inspect_routes` takes it. `main.py` goes down by 72 lines and the handler bodies are the same code (I diffed them after de-indenting: the only difference is the local name of the error tuple). There are two commits so the move can be reviewed on its own. The first adds 15 tests for these routes, which had none, and they pass on current `main` before anything moves. They pin the behaviour I most wanted to keep: a 401 without a session when auth is on, a 403 for a signed-in non-admin (that check lives in `login_required` and goes by path, which is why the same decorator has to be passed in), the 400 on an empty body, the result passed through on success and failure, the 500 with the error text on an operational error, and open access when no auth is configured. The second commit is the move, and no test changed in it. What I checked on the move: - The Flask URL map (every rule, its methods and its endpoint name) is identical before and after: 77 rules, the three onboarding ones included. - Full suite, ruff and basedpyright are clean. - I ran the app from this branch and stepped through the setup wizard in a browser. It loads its steps from the moved route and goes from Search Mode to Metadata Provider, and `POST /api/onboarding/skip` and the empty-body 400 answer as before. Two small things that do change. The log lines for these routes now come from `shelfmark.core.onboarding_routes` instead of `main`, and the error tuple is a local copy (`_ONBOARDING_ERRORS`, the same classes as `_IMPORT_OPERATIONAL_ERRORS`) so the module does not import `main`. Nothing else is different. |
||
|
splitsec2
|
69cbf72698 |
refactor: move the settings routes into core/settings_routes.py (#1595)
Second route move from the plan in Discussion #1536: the four settings routes (`GET /api/settings`, `GET` and `PUT /api/settings/<tab>`, and `POST /api/settings/<tab>/action/<key>`) go from `main.py` to `core/settings_routes.py`, registered with `register_settings_routes(app, login_required)`. `main.py` is 143 lines shorter. These handlers call `settings_registry` directly, so nothing else has to be passed in. The handler bodies are the same code, and the only difference I found after de-indenting them is that `ruff format` wrapped one signature that no longer fits on a line. Same two-commit layout as the onboarding one (#1594). The first commit adds 27 tests for these routes and they pass on current `main` before anything moves. They pin the 404 for an unknown tab, the 400 when a `PUT` body is not a JSON object (a list, a string, a number, or no body at all), the "No changes to save" answer for an empty object, the unsaved form values reaching an action, the result passed through with a 400 on failure, the 500 with the error text on an operational error, a 401 without a session, a 403 for a signed-in non-admin on all four routes, and open access with no auth. The second commit is the move and no test changed in it. The admin check for these paths lives in `login_required`, so the same decorator is passed in. What I checked: - The Flask URL map (every rule, its methods and its endpoint name) is identical before and after, all 77 rules. - Full suite, ruff and basedpyright are clean. - I ran the app from this branch and opened Settings in a browser: the modal lists every tab and the General tab shows its values, which come from the moved `GET`. Over HTTP, a `PUT` of a new search page title saved and read back, a list body got the 400 and an unknown tab the 404. As with #1594, log lines for these routes now come from `shelfmark.core.settings_routes`, and the error tuple is a local copy (`_SETTINGS_ERRORS`) so the module does not import `main`. The two PRs touch neighbouring lines of `main.py` and I checked that they merge together without a conflict, in either order. |
||
|
splitsec2
|
7583f997bb |
fix(oidc): catch joserfc's InvalidClaimError so the issuer guidance shows (#1603)
A wrong OIDC issuer, which is the usual IdP setup mistake, sends the
user back to the login page with a bare "Authentication failed". The
callback has a handler that explains it ("OIDC issuer validation failed.
Verify your discovery URL and IdP issuer/external URL configuration."),
and it never runs.
Authlib validates the ID token with joserfc now, so the error that comes
out of `authorize_access_token()` is `joserfc.errors.InvalidClaimError`.
The handler catches the one in `authlib.jose.errors`, which is a
different class with a different base, so the isinstance check is False.
I confirmed it by validating a real `CodeIDToken` with a wrong `iss` on
Authlib 1.9.1. I haven't run it against a live IdP.
The existing tests passed because they raise the old class from a mock.
The new one raises the error from a real `CodeIDToken`, and it fails on
the old import with `'Authentication failed'` in place of the guidance.
Two small things come with it:
- The new class names its attribute `claim` where the old one used
`claim_name`, so that read changes.
- `authlib.jose` is deprecated in favour of joserfc and Authlib's own
warning says it goes in 2.0. This was the only place Shelfmark used it,
so the import moving also takes us off the module that is going away.
joserfc is already installed as an Authlib requirement, so I added it to
`pyproject.toml` (`>=1.6.8`, Authlib's own floor) and the lock gets the
one entry. No new package.
Ruff, basedpyright and the backend suite pass (5424 passed).
|
||
 CaliBrain
|
947ce7bfe5 |
test(e2e): serve the mock Prowlarr .torrent from Prowlarr's own origin (#1602)
The nightly client-deluge profile has failed since
|
||
|
splitsec2
|
83e61975c4 |
fix(releases): apply the record policy to a provider named alongside a source (#1598)
Two small things I found while going back over what has landed since 1.5.0. `/api/releases` takes a `provider` and a `source`, and the policy check only looked at `source`. A user whose policy blocks one release source but not another could pass the blocked one as `provider` with the allowed one as `source`, and get the blocked source's record back. The record route already refuses that with a 403, so the listing route now applies the same refusal when `provider` names a source that is not being listed. The normal case, where provider and source are the same, is already gated and skips the extra check. The second commit is wording only. The Automatic Downloads setting and the Hardcover sync doc said "synced requests", but the pass picks up every pending Hardcover request, including ones a user made themselves with Request Book. The download is still queued as the user who asked (the admin is only the reviewer, same as a manual approval), and admins already see every user's requests. The text now says that, and a new test pins the behaviour for a user's own request. I opened a scratch instance and looked at the Hardcover Sync tab: the Automatic Downloads heading and the checkbox description read as intended and nothing else on the tab moved. Full suite, ruff and basedpyright pass, and the new policy test fails without the fix. |
||
|
splitsec2
|
cfba44b754 |
refactor: one module for taking credentials and links out of text (#1578)
Draft until #1571 merges: this is stacked on it, so the first two commits here are that PR's and only the last one is new. I will rebase once #1571 is in. Shelfmark strips credentials and links out of text in five places, written in different PRs with different patterns: the log formatter, `_safe_url` and `_redact_urls_in_text` in `torrent_utils.py`, `redact_status_message` in `viewer_redaction.py`, the `sed` rules in `genDebug.sh`, and the password handling in the settings registry. Between them there were three regexes that find a URL in text, two separate userinfo patterns and no shared list of what counts as a secret. #1571 made the gap visible, because every pattern added to the logger had to be remembered somewhere else too. This moves the text and URL part into `shelfmark/core/redaction.py`, which imports nothing from the app so the logger can depend on it: - `redact_secrets` (mask in place, for logs) moved from `core/logger.py`, patterns unchanged. The logger imports it. - `safe_url` and `redact_urls_in_text` (keep scheme, host and path, drop the query) moved from `torrent_utils.py` and are public now, so `alldebrid.py` no longer imports a private name from another module. - `strip_links` (remove links entirely) is what `redact_status_message` now calls. It and `redact_urls_in_text` share one link pattern, the one the viewer redaction already used. Two small behaviour changes come from sharing that pattern. A closing bracket or quote right after a URL is no longer swallowed into it when scrubbing an exception message. And the scheme-less path that requests prints after `url: ` now keeps only what is before its query in `redact_urls_in_text`, where before it passed through as it was (the log formatter masked the key, but the path and query stayed in the message). Also here: the two `sed` rules in `genDebug.sh` that mask userinfo now stop at `?` and `#` like the logger pattern does, with a test that fails on the old script. Not in this PR: the debug bundle zips the live log directory as it is, so log lines written before #1571 are not redacted in it. Fixing that means copying the logs to a staging folder and running them through this module, which I would do as a separate change. Tests: a new file covers `safe_url`, `redact_urls_in_text` and `strip_links` directly, and two checks that the viewer and the logger really use these. Full suite passes. |
||
|
splitsec2
|
7a4f1670dc |
fix(slskd): try a batch enqueue once more when slskd answers 500 (#1590)
slskd answers a bare 500 when the peer doesn't reply to a queue request inside its own 5 second wait (its log says "The wait timed out after 5000 milliseconds"). The peer often answers a moment later, so a Soulseek download that would have worked failed on the first try. The batch enqueue is now tried twice with 5 seconds between. A 4xx or a connection failure is not retried, only a 5xx. While it waits the card reads "The peer did not answer, trying again", and cancelling during the wait stops without the second try. If the second try fails too, the error message carries what slskd said (trimmed to 160 characters). Before, that text was dropped and the card only showed the status line. It's been running on my instance since mid-October, and the timeouts still show up in slskd's log, so I can't say how many it saved. The five new tests cover the retry that works, giving up after the second 500, a 400 and a connection error not being retried, and cancelling mid-wait. Ruff, basedpyright and the slskd tests pass. |
||
|
splitsec2
|
e8b28d05e0 |
fix(sources): take the retry download URL from the release cache only (#1597)
Newznab and Jackett fell back to the download URL sent with the queue request when the release was no longer in the cache. Prowlarr already returns no URL in that case, so this makes the other two match it: the retry URL comes from the cache and nothing else. A request for a release that has expired from the cache now fails with "Release not found in cache" instead of being queued with whatever URL came along. The new tests cover both handlers on a cache miss and with no source_id, and they fail on main. Full suite, ruff and basedpyright pass. |
||
|
splitsec2
|
8b80f32476 |
fix(sources): keep credentials out of a source-unavailable message (#1593)
Code scanning alert 106 (stack trace exposure on the release search 503) turned out to be worth a look. Jackett sends its API key as `?apikey=` in the query string, and when Jackett is unreachable the source builds its "unavailable" message from the requests exception, whose text includes the full request URL. `/api/releases` returns that message to whoever ran the search, so any signed-in user could read the Jackett key while Jackett was down. I reproduced it with Jackett pointed at a closed port: the 503 body had `apikey=<the key>` in it. Jackett was added in #1521 and isn't in a release yet, so nothing shipped with this. `SourceUnavailableError` now masks credentials in its message with the same `redact_secrets` the log formatter uses (query parameters like `apikey=`, `user:pass@` in URLs and so on). I did it in the exception rather than at the two Jackett sites that build the text, since its message is written for the user and any other source that ends up with a key in a URL would hit the same thing. The rest of the message stays readable, for example "Max retries exceeded with url: ...?apikey=(removed)&t=book". Prowlarr and slskd send their keys in headers so they didn't leak a key, but their messages do include the internal host and port. I left that alone here. Tests: two Jackett ones (a connection error on the search and on the indexer lookup, with the key in the exception text) and three on the exception itself. The four that check masking fail on current main. The full suite, ruff and basedpyright are clean. |
||
|
splitsec2
|
72b094db7a |
fix(qbittorrent): map the qBittorrent 5 stopped states and forcedMetaDL (#1570)
On qBittorrent 5 a stopped torrent shows up as an unknown state. 5.0 renamed `pausedDL` and `pausedUP` to `stoppedDL` and `stoppedUP`, and `get_status` only knows the old names, so the torrent comes back as state `unknown` with the raw state name as its message. The paused handling in the download handler never applies to it, for example the grace it gives a torrent that has not started. `forcedMetaDL` is missing from the same table, so a force-started magnet fetching its metadata also shows up as unknown, although it is already in the incomplete-states set and the metadata wait. The three missing states are now mapped like their neighbours: the two stopped states as paused, `forcedMetaDL` as downloading with "Fetching metadata". The state names are the ones `qbittorrent-api` lists in its `TorrentState` enum. Tests: one parametrized test covers the old and new paused names and `forcedMetaDL` (the two old paused names are there as controls). The three new cases fail without the change. Full suite passes. |
||
|
splitsec2
|
f02f0d7365 |
fix(notifications): say what applies on a user's own notification routes page (#1588)
This is the UI side of #1581. A non-admin looking at their own notification routes reads the same help text the admin does, including "or mailtos://...?to={Email} in the admin default". They can't set that default, a personal route can't use mailtos (personal routes are limited to ntfy, Gotify, Discord, Slack, Telegram, Pushover and PushBullet), and nothing on the page says whether an admin default exists. The default's URL is hidden from them on purpose since it can carry credentials, so the page just shows an empty All row, the same as when nothing is set up. What changes, on the user's own page only (the admin's view of a user keeps the full text): - The help text drops the admin default and mailtos part and lists the services a personal route can use. - The edit-context response now carries `adminDefaultRoutesSet`, a boolean that is true when an admin default route with a URL exists. The URL itself is still not sent, and the existing test that checks nothing from the admin's values leaks still passes. - When that is true, the help text adds a line saying a default applies until the user adds their own, and once they have, that theirs replace it. How routes are resolved is unchanged. A user's own routes still replace the default and don't merge with it. I didn't try to add mailtos for personal routes, since keeping SMTP credentials in a user's own URL is what the admin default avoids. Tests: two route tests (flag true with a URL in the admin default, false with an empty row), a vitest file for the description text, and the full frontend suite, oxlint, oxfmt, tsc, ruff and basedpyright are clean. I haven't looked at the page in a browser. |
||
|
splitsec2
|
46b9e7d8a3 |
fix(library): match a shelf title against the full title when the search title is only the series (#1587)
Fixes #1584. Thanks to @joeRob2468 for the write-up, which pinned this down to the line. Hardcover cuts a title like "Mistborn: The Final Empire" down to "Mistborn" for searching when it can't tell the series prefix from a title prefix, and search results don't carry the series. The library check then matched the shelf title "Mistborn: The Final Empire" against "Mistborn", saw "final empire" as extra words and took it for a longer, different work (the "Dune" vs "Dune Messiah" rule), so the book got no In library badge. `match_entries` now tries the search title first as before and, if that finds nothing and the full title is different, tries the full title too. The full title is the more specific of the two so it should rarely add false positives. Another book in the same series still doesn't match, there is a test for that, and the Dune and Dune Messiah test passes unchanged. The matching loop moved into a small helper to run it twice, nothing in it changed. This only touches the library check. The same short search title also feeds release searches and auto-download matching, which I left alone here because loosening or tightening those changes what gets searched and downloaded for everyone. Tests fail on current main without the change. The library tests, ruff and basedpyright are clean. |
||
|
splitsec2
|
ac14e4dd24 |
fix(downloads): a swarm note change is not activity for the stall timer (#1572)
The torrent stall timer can be deferred for as long as the swarm keeps changing. Since #1494 the torrent handler appends a note to its progress message (`0% · 2 seeds`, and `· 0 seeds, giving up in 7m` while the no-seeders clock runs). `update_download_status` treats any changed message as activity, so a seed count that goes 2, 3, 1 between polls, or the give-up countdown ticking down every minute, refreshes `_last_activity` and the torrent never stalls. A torrent whose seed count stays constant still stalls, which is why this is easy to miss. `shelfmark/download/activity.py` already says what counts: a changed status event or a changed progress value, and a message that changes on a timer proves nothing. So `update_download_status` now compares messages through a small `activity_key()` that drops that trailing seed note. The card still gets every message and shows the live seed count, only the stall clock ignores a change in the note. A new progress percentage, a different client message or a status change still counts as activity. The handler builds the note in `_seed_note` and `activity_key` has to match it, so one test builds the note for a range of seed counts and countdowns and checks the key strips all of them. That also covers the gap #1494 left, where no test connected a handler's message to the orchestrator: the other two drive `update_download_status` with real notes and a fake clock. Without the change the note test fails and the key tests cannot even import `activity_key`. The other orchestrator test is a control that a progress or status change still counts. Full suite passes, and each new test also passes when run alone. |
||
  misko903andClaude Opus 5.5
|
d50440f6c2 |
fix(webshare): clear the "Resolving" status once the download starts (#1575)
## What
A Webshare.cz download shows "Resolving Webshare link" for its entire
duration, so a large download (e.g. a 1.3 GB audiobook archive) looks
stuck.
## Why
After the link was resolved, the handler called
`status_callback("downloading", None)`. `update_download_status` treats
a `None` message as "keep the current message", so the "Resolving
Webshare link" detail stayed in the activity panel. The HTTP handler
sends `""` for exactly this reason.
## How
- Report `downloading` with an empty message, which clears the resolving
text.
- When the download response has no `Content-Length`, use the release's
size (`task.size`, parsed with `parse_size_string`) as the total for
progress. Without it, progress never moved, and stall detection would
flag a healthy long download after 5 minutes.
## Testing
- New tests in `tests/webshare/test_handler.py`: the downloading status
clears the message, and progress is computed from the release size when
there is no Content-Length.
- `uv run pytest tests/webshare` is green (34 passed), and `ruff check`
and `ruff format --check` are clean.
Follow-up to #1543.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
|
||
|
splitsec2
|
525b094b57 |
fix(clients): hand a finished wrong-type directory on instead of polling out the timeout (#1573)
A torrent that finishes with the wrong kind of files in it holds the download for the whole completed-path timeout (60 seconds by default, up to an hour) and then fails with "Completed download has no stable supported files yet." An ebook task whose torrent holds only an `.m4b`, an audiobook task that got an `.epub`, or a `.pdf`-only release with the default format list all land here. In v1.4.0 these failed with the specific format error from post-processing, but the wait for the sync to settle (#1464) only counts files of the task's own type, so it keeps polling for files that are never going to appear. If the directory has no supported files, `_wait_for_completed_path` now looks for finished book or audio files that this task does not accept (the audiobook formats, the default ebook formats and pdf). When it finds some and they are the same on two polls in a row, it returns the path so post-processing can say what is wrong with the formats. Cover art and notes alone do not count, because that is what a directory looks like while the real files are still syncing over a remote mount, so those keep waiting as before. A wrong-type file that is still growing keeps waiting too. With no wait configured there is only one sample, and a wrong-type file is handed on straight away. The existing test that pins "no book files, no wait configured" uses a jpg, so it stays as it is. I extended the test helper to take a content type, which the wait tests never passed before. Four new cases fail without the change (a wrong-type directory for ebook, audiobook and pdf tasks), and two more check that sidecar-only and still-growing directories keep waiting. Full suite passes. |
||
|
splitsec2
|
47f0acdd31 |
fix(logging): mask userinfo and path-segment credentials, route stray loggers through the formatter (#1571)
Logs get pasted into issues, and the formatter from #1488 only masks `?key=value` query parameters. I went through what else commonly ends up in a log line and found these unmasked: - credentials in a URL's userinfo, like a proxy's `socks5://user:pass@host:1080` or `http://admin:secret@qbittorrent:8080` - a passkey as a path segment, like `/rss/<key>` or `/passkey/<key>/get.torrent`, which private trackers use - the token in a Telegram bot, Discord webhook or Slack webhook URL, which is what a notification URL looks like when it gets logged All three are masked now, in `redact_secrets`, so they apply to every handler. The path rule only fires after a segment that names a secret (`passkey`, `apikey`, `rsskey`, `authkey`, `token`, `secret`, `rss`, `torznab`) followed by 16 or more token characters. I left out generic words like `download` on purpose, because `/api/download/<md5>/cancel` appears in our own request log and the ids there are useful when debugging. That is the tradeoff: a passkey in a `/download/<passkey>/` path still gets through. Separately, four modules created their logger with plain `logging.getLogger`, so nothing they logged went through the formatter at all: `api/websocket.py`, `download/clients/__init__.py`, and the Jackett, Newznab and Prowlarr package inits. They use `setup_logger` now like everything else. Tests: ten cases, seven for the new shapes and three that must stay readable (an md5 route id, a short `/rss/latest`, an email address), plus two that check the stray modules now carry the redacting formatter. Nine of them fail without the change. Full suite passes. Not covered here: `/api/debug` still zips the existing log files as they are on disk, so anything logged before this change rides along in a bundle. That is a separate change in `genDebug.sh`. |
||
|
splitsec2
|
ef40a38546 |
fix: report a stopped download coordinator in /api/health (#1568)
If the download coordinator thread dies, every task sits at "Queued" and nothing says why. The rest of the app keeps answering, `/api/health` stays green and the container looks fine. #823 and #1166 were both that, and the loop's `except Exception` cannot catch everything that can end a thread. `/api/health` now reports it. The response gets a `coordinator` field, `running` or `stopped`, and when it is stopped the status is `error` and the HTTP code is 503. The Docker healthcheck in the Dockerfile and the one in the readme both use `curl -f`, so the container shows as unhealthy and anything watching health (Uptime Kuma, autoheal) can act on it. It needs no login, same as before, and shows no queue contents. That 503 is the one decision here. Keeping the 200 and only adding a `degraded` entry would not trip a status-code health check, which is the whole point, so I went with 503. Easy to flip if you would rather not. The orchestrator gets a small `coordinator_running()` that asks whether the thread is alive. `start()` already knows how to restart a dead thread, so restarting it automatically would be the true fix, but I would rather the failure be visible first and decide on that separately. Tests: three for the route (ok, 503, no login needed) and three for `coordinator_running` (never started, alive, dead). They fail without the change. One existing test hits `/api/health` through an app that never starts the coordinator, so it now patches that. The readme's health section has a sentence about the 503. Full suite passes, ruff and basedpyright are clean. |
||
|
splitsec2
|
973cf472a2 |
test(image-cache): flush window test no longer depends on float rounding (#1580)
`test_cache_hit_flushes_the_index_once_the_window_has_passed` (from #1544) failed on the first attempt of two of my PRs' Python Tests runs today (#1576, #1579) and passed on rerun, with nothing in either PR touching the image cache. The test sets the clock to exactly `last_saved + ACCESS_FLUSH_SECONDS` and expects a flush. `(now + 30) - now` isn't always 30.0 in floating point. For example `105.65207077218265 + 30 - 105.65207077218265` is `29.999999999999986`, so the `>=` check is false and the flush never happens. That only shows up when the monotonic value is small, which fits a freshly booted runner (on this box the uptime is in the millions of seconds and it never fails). I haven't been able to see the runner's actual uptime in the CI logs, so that part is an inference, but pinning `_last_saved` to that value reproduces the exact failure (`assert [] == [1]`) locally. The test now puts the clock one second past the window. The production check is fine, real elapsed time doesn't have this edge, so only the test changes. |
||
|
splitsec2
|
b248bcd536 |
fix(clients): import a qBittorrent torrent only when every listed file is on disk (#1577)
Fixes #1561, with thanks to @tmussche for the write-up, which had the cause and the fix. When the files reach Shelfmark through a sync (Resilio in the report), the wait for the completed path accepts the folder once two samples a few seconds apart look the same. A sync that copies in batches can pause longer than that, so 10 of 23 files were imported and some of those were still partial copies. For qBittorrent the client can say what the finished download holds, so the wait now checks that. `DownloadClient` gets an optional `get_expected_files()` that returns `ExpectedFile(name, size)` entries, or None when the client cannot say, which is the default and keeps every other client as it was. qBittorrent answers it from `/api/v2/torrents/files`, leaving out files the torrent does not download (priority 0), zero-byte entries and BEP 47 padding files. `_wait_for_completed_path` asks once, and while any of those files is missing or smaller than its final size it keeps waiting, using the same timeout as before. When the wait runs out the error now says how far it got, for example "Completed download is not fully on disk yet (10 of 23 files)." Once everything is there the existing stability check still runs, so nothing is accepted that was accepted before. Names from the client start with the torrent's top-level folder, so each file is looked up with and without it, which also covers a torrent saved without a subfolder. A single-file torrent is checked by size too, since a file that is still syncing exists at a smaller size. Two things I would like a second look at: the comparison is on exact size, so a setup where something rewrites the files after the torrent finishes would never match and would run into the timeout; and the file list is read once per wait, not on every poll. If the client does not answer, nothing changes. I also moved the nested authenticated GET in the path derivation into a `_get_with_auth` method so both calls share it, and added a sentence about this to `docs/configuration.md`. Tests: eight for the wait (partial folder, truncated file, files arriving during the wait, complete folder, no top-level folder, single file both ways, and a client without a list), and six for the qBittorrent file list. The wait tests that depend on the new behaviour fail on current main. Full suite passes. |
||
|
splitsec2
|
9011319ade |
refactor: one lookup for the supported formats, public names for two shared helpers (#1576)
Six places read the Supported Book Formats and Supported Audiobook Formats settings on their own: post-processing, the post-process scan, IRC, slskd, Prowlarr and direct download. For ebooks they already agreed, since they all fall back to `DEFAULT_EBOOK_FORMATS`. For audiobooks they did not. The settings registry and slskd fall back to every audio format plus zip and rar, while post-processing and IRC fell back to m4b and mp3 only. They now share `shelfmark/core/formats.py`, with `get_supported_formats`, `get_supported_audiobook_formats` and a content-type aware `get_supported_formats_for`, and a shared `DEFAULT_AUDIOBOOK_FORMATS` in `core/utils.py` next to the ebook one. `policy.py` re-exports the same functions, so nothing that imports them from there changes. It is not in `policy.py` itself because importing the `postprocess` package pulls in the router, which imports the release sources, so a source could not import from it without a cycle. One behaviour difference, and only in the fallback: with the audiobook setting unset, or emptied, post-processing and IRC used to accept m4b and mp3 and now accept the full default list like slskd and the settings page already did. A configured list is read exactly as before. Direct download's own helper used the raw configured list and now reads the same normalized one, which is identical for any list the settings page can save. Also public now: `parse_size` in the Prowlarr source (slskd and Newznab imported it with the underscore) and `coerce_completed_path_timeout_seconds` in the client base handler (slskd imported that too). Not changed on purpose: slskd's `_coerce_int` looks like a copy of `coerce_int` but is not. It reads `"12.0"` as 12 and `True` as 0, where `coerce_int` gives 0 and 1. And the `_parse_size` in the IRC source is a different function, string to bytes. Tests: a new set checks that post-processing, IRC and slskd all fall back to the shared defaults and that the settings default is the shared one, and the scan-archive and IRC tests now patch the shared config instead of the per-module copies. Full suite passes. |
||
|
splitsec2
|
606e94fde2 |
fix(settings): offer the seed settings for qBittorrent only (#1574)
The two seed settings, Give Up With No Seeders and Minimum Seeders, show up for every torrent client, but only qBittorrent reports a seed count. With Transmission, Deluge or rTorrent you can set both and nothing happens. The first one says "Only qBittorrent reports seed counts" at the end of its description, the second says nothing about it. Both are now shown only when qBittorrent is the selected client, the same `show_when` the other qBittorrent fields use, and Minimum Seeders gets the same sentence. A value someone already saved for another client stays in their config, it just is not shown, and it had no effect before either. `docs/environment-variables.md` is regenerated for the changed description, which is two lines. One test checks that both fields are tied to qBittorrent and say so, and it fails without the change. |
||
|
splitsec2
|
70fe4b8c89 |
fix(hardcover): keep the scheduler alive on an unreadable interval, share one history connection per sync (#1569)
These are the follow-ups from the review notes on #1516 and #1534 that I said I would do once the series had merged. The scheduler waits for the next cycle outside the try that guards the cycle itself, so an interval setting that raised would end the thread and nothing would sync again until a restart. It now logs the error and tries again after six hours. `docs/hardcover-sync.md` says so. The history check in the sync opened a new SQLite connection for every book on the shelf. It opens one per sync now and shares it, which adds up on a slow disk or a small board. The two test nits from #1534 are in as well: a `parametrize` with a single case, and an unused argument on a helper. The thread surviving a failed wait and the shared connection each have a test that fails without the change. Ruff, basedpyright and the backend suite pass (5297 passed). |
||
|
splitsec2
|
a63d9e3e21 |
docs: regenerate the environment variable reference (#1565)
Sorry about this one. The environment variable references had also fallen well behind, and again I was a big part of that. A bunch of the gaps come from my own PRs (Hardcover sync, auto-download, the Audiobookshelf library check) where I charged ahead with the functionality and skipped the boring documentation part (did I mention that documentation is boring?). I again had Claude compare the docs to the code and generate the updates, so it would be good to have another human look this over before it merges. The doc diff is just the generator's output. The part worth a real read is the handful of new entries in `scripts/generate_env_docs.py`, since I wrote those descriptions from `entrypoint.sh` and `tor.sh` and not from a settings definition. What changed: - Regenerated `docs/environment-variables.md`. It had 287 variables and the generator now produces 311. The 21 that were missing are the Hardcover Sync tab (6), auto-download (11) and the Audiobookshelf library check (4). Nothing was removed and no existing default, type or option changed. - `SHELFMARK_API_KEY_READONLY` was only ever hand-edited into the doc, so the generator did not know it and a plain regeneration would have dropped the row. It is in the generator now. - Added `UMASK`, `TOR_CHECK_INTERVAL` and `TOR_MAX_FAILURES`. The code reads all three and no doc mentioned them. - The table of contents built its anchors by lowercasing and swapping spaces, so the Webshare.cz and Moly.hu links were already broken (GitHub drops the dots) and Soulseek (slskd) would have broken with the parentheses. They now follow GitHub's rule. Two tests cover the new entries and the anchors, and both fail without the generator change. To regenerate: `python scripts/generate_env_docs.py` on Python 3.14. A test that fails when the committed file differs from the generator output would have caught this drift, but then every PR that adds a setting has to regenerate the doc. I left that out, your call if you want it. |
||
|
splitsec2
|
d7a4b6b859 |
fix: keep exception text out of the status, config and release search responses (#1562)
Same idea as #1558, for three more places that hand `str(e)` to whoever is logged in. These are code scanning alerts 106, 142 and 143. `/api/status` and `/api/config` returned the raw exception text of an OSError, RuntimeError and so on with the 500. They now return "Could not read the download status" and "Could not load the configuration". The full error is still logged with its trace. For `/api/releases`, a source failing with an unexpected error put `source: <exception text>` into the error list that goes back to the caller. `search_source_releases` takes a new `include_error_detail` flag. The route passes True for admins (and when auth is off, where everyone is treated as an admin) and False for everyone else, who get `source: search failed`. The message of a `SourceUnavailableError` is written for the user, so it is kept either way, and the background auto-download caller keeps the default and sees the full text as before. Left alone on purpose: the `reason` in the release inspect response, since a test covers that it carries the error text and it looks intentional. Tests: two for the routes and two for the helper. All four fail without the change. Full suite passes (5295), ruff and basedpyright are clean. |
||
|
23b3b3a9d8 |
feat: add Jackett release source (#1521)
## Summary Adds Jackett as a separate release source (`jackett`), as requested in #1443. - New `shelfmark/release_sources/jackett/` package: API client, settings tab, source and handler. - Searches Jackett's Torznab endpoints: the `all` aggregate by default, or each selected indexer individually. Failed or timed-out searches raise `SourceUnavailableError` instead of reporting "no releases found". - Reuses Prowlarr code rather than duplicating it: the Torznab parser (now also reads `<jackettindexer>`), `_prowlarr_result_to_release` (new optional `source`/`source_id` args), the release cache, URL/protocol helpers, the timeout clamp and size-limit helper (new optional config-key args), and the handler's `list_files` / retry-restore logic. - Downloads use the existing shared torrent client settings. - `JACKETT_URL` is a trusted origin for `.torrent` fetching (it is usually on the LAN). Prowlarr's `X-Api-Key` is not sent to it. - Added a Jackett section to `docs/environment-variables.md`. ## Screenshots <img width="1117" height="936" alt="Screenshot 2026-10-21 at 13-45-34 Shelfmark" src="https://github.com/user-attachments/assets/00339999-72c7-4226-bd9a-df8b325a7e22" /> <img width="1117" height="938" alt="Screenshot 2026-10-21 at 13-50-38 Shelfmark" src="https://github.com/user-attachments/assets/3c9cd2b4-932a-4b5c-9e10-6ec22b5f987d" /> <img width="1117" height="937" alt="Screenshot 2026-10-21 at 13-50-57 Shelfmark" src="https://github.com/user-attachments/assets/9c4d0ed9-da2f-4323-b188-341b45066efd" /> ## Not included - First-run onboarding does not offer Jackett yet. - No Jackett-specific category settings; it uses the standard 7000 / 3030 categories. ## Testing - New unit tests in `tests/jackett/` (client, source, handler) using mocked responses. - Full suite passes except 11 `tests/e2e` tests that also fail on a clean tree here (cannot write `/var/log/shelfmark`). - Tested against a live Jackett instance. Closes #1443 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Sonnet 5.5 <noreply@anthropic.com> Co-authored-by: CaliBrain <calibrain@l4n.xyz> |
||
|
splitsec2
|
3633dd3d97 |
fix: keep file links and API keys out of AllDebrid logs, fix Newznab debug log (#1559)
Two log fixes in the same area, both found while going through the open
code scanning alerts.
AllDebrid: the unlock path logged the file link in full. The last-resort
fallback appends `apikey=` to the link and returns it, and the "Failed
to download from ..." error carries that same link into
`logger.exception`, so the key could land in the log and the traceback.
The links are now logged through `_safe_url` (scheme, host and path
only). The GET unlock fallback also logged the exception text at debug
level, and requests puts the request URL in that message, which includes
`apikey=`. That one now logs the exception type only. This covers code
scanning alert 124.
Newznab: `logger.debug("Newznab API: GET %s params=%s", url, *params)`
star-expands the dict keys into the log arguments, so with debug logging
on it raises `TypeError: not all arguments converted during string
formatting` and the line never prints. Logging `params` as is would
print the API key, so it now logs the sorted parameter names.
Tests: the Newznab one formats the logged line and checks the key is
absent. The AllDebrid ones check the fallback and failure paths log no
query string and that the fallback still returns the link with the key.
All three fail without the change. Ruff and basedpyright are clean,
`tests/newznab` and `tests/prowlarr` pass.
|
||
|
splitsec2
|
4f97fc9bec |
fix: keep server paths and URLs out of an error response and a debug log (#1558)
Two small leaks from going through the open CodeQL alerts (code scanning alerts 100 and 123). The local download route returned `str(e)` on an OSError, and for a missing or unreadable file that text is the file's path on the server (`[Errno 2] No such file or directory: '/audiobooks/Author/Title.epub'`). #1422 cuts a non-admin's download path down to the file name everywhere else, so this put it back. The route now answers "Could not read the file" and the detail stays in the log. `should_bypass_proxy` logged the whole request URL at debug when it could not be parsed. URLs can carry credentials, so it logs only the exception now. Each change has a test that fails without it. The full non-e2e suite, ruff and basedpyright pass. |