Skip to content

Security tightening, image uploader attribution, and case-correct game detection - #729

Merged
ShaneIsrael merged 5 commits into
mainfrom
develop
Sep 14, 2026
Merged

ShaneIsrael merged 5 commits into
mainfrom
develop

Conversation

@ShaneIsrael

@ShaneIsrael ShaneIsrael commented Sep 14, 2026 •

Copy link
Copy Markdown
Collaborator

Rolls up four commits on develop: two reported bugs and a pass of security hardening.

Fixes #726 — images could not be re-attributed

The Videos tab has had a bulk Uploader action since ownership landed; the Images tab never grew one, so images indexed from disk before ownership existed were stuck unattributed with no way to adopt them.

The server side was already there — bulk-set-uploader has always accepted image_ids — so this is only the missing control: an Uploader button in the organize group (labelled on desktop, icon-only on mobile), opening the same account picker as videos, including the option to clear attribution.

Also fixes the success toast, which reported (0) on both tabs: runBulkAction counts the updated/moved/deleted array, but this endpoint answers with updated_images / updated_videos counts instead.

Fixes #728 — games differing only in case

"RUMBLE" and "rumble" are both real games, and clips of the first kept being suggested as the second. Two faults, and the first is the more serious of the two:

Local matching never worked at all. The match built its rapidfuzz choices as a list of (name, game) tuples, so the query was scored against the tuple rather than the name — every candidate returned 0.0, nothing could clear the cutoff, and no filename ever matched a game already in the library. Every lookup fell through to a SteamGridDB search. Confirmed against the pre-change code: even "rocket league clip" vs "Rocket League" returned no match. (Had anything ever scored, the game was read out of result[2], which for a list of choices is the index, not the element, so it would have thrown.)

The fallback threw the case away. clean_name = filename.lower() lowercased before searching, and the code took results[0] on faith — between two names differing only in case, a coin flip.

Matching now scores case-insensitively (what the lowercasing was reaching for, but it was applied to only one side), the filename keeps its original case so an exact hit is still recognisable, and an exact case-sensitive match wins outright both locally and among search results.

Where that still leaves a tie, it is settled on how far each candidate's own casing is echoed by the query. This matters more than it sounds: RUMBLE_2026-09-14_clip cleans to "RUMBLE clip", not "RUMBLE" — the word-stripping pattern uses \b and an underscore is a word character, so _clip never matches \bclip\b. That is not an exact hit, so it lands in the fuzzy path where both names score 100, and extractOne answers a tie with whichever it reached first. Which game got suggested came down to which had been added to the library first. Verified identical with the rows inserted in both orders.

Side effect worth knowing: since local matching was dead, every detection was hitting SteamGridDB. Libraries with games already linked should now match locally and make far fewer API calls.

Security hardening

Specifics are deliberately kept out of this description until a release is out. Each item was reproduced and verified against the code before being changed, and the reasoning is in the commit message and in comments at each site.

  • Session signing key. The shipped compose file no longer carries a default value for it. A key is now generated on first start and persisted under /data, so restarts no longer sign everyone out, and the old example value is refused if still present.
  • Media folder handling. The endpoints that move media between folders now resolve the requested folder against the media root and reject anything landing outside it, instead of trusting the value as given. The stored path is the normalised form.
  • Video details update. The update now writes an explicit set of fields rather than whatever the request body happened to contain, matching what the image endpoint already did and what the client actually sends.
  • Login. Failed sign-in attempts are now rate limited per source address and per address/account; failed MFA codes count against the same budget.
  • Authorization gaps. Three routes that spend the operator's third-party API quota required no authentication; the webhook test routes, which cause an outbound request, used an admin check that is intentionally permissive under DEMO_MODE. Both tightened.
  • Cookies and CORS. Cross-origin access is now opt-in via an explicit origin allowlist instead of permitting any origin, and SameSite is stated explicitly on both cookies rather than left to the browser default.

Reviewer notes

  • Two new env vars. CORS_ORIGINS (comma-separated allowlist; unset means same-origin only, and a literal * is rejected since it cannot be combined with credentials) and SECURE_COOKIES (defaults off — plenty of instances are reached over plain HTTP on a LAN, where defaulting it on would lock users out). Documentation for both is included.
  • CORS is now default-deny. Fireshare serves its frontend from the same origin, so nothing in a normal deployment needs it — but anyone serving the frontend separately will need CORS_ORIGINS set. This is the change most likely to surprise someone.
  • Operators still passing the old example signing key get a new one, which signs existing sessions out once. Called out in the warning text.
  • The login throttle is in-process (login_throttle.py), which assumes one server process per instance — true today, and it keeps a rate-limit dependency and the shared store it would want out of the deployment. Counters reset on restart.
  • Throttle keys include the source address on purpose. A global per-account lockout would let anyone on the internet lock a known username — most usefully admin — out of their own instance by failing logins deliberately.
  • DEMO_MODE is otherwise untouched. admin_required still admits any authenticated user in demo mode for the file-management routes. Closing that properly is a behaviour change for the demo instance and wants its own decision; only the routes that make outbound requests were shut here.
  • Not addressed, spotted in passing: that \b quirk means clip, gameplay, highlights and friends survive whenever they follow an underscore, so Rocket_League_gameplay cleans to "Rocket League gameplay". token_set_ratio ignores the extra word so matching still works, but the cleaning is not doing what it looks like it does.

Verification

Client builds. Server boots (171 routes). Beyond that, exercised against a live dev stack rather than reasoned about:

  • Uploader action assigns and clears attribution, verified in the database both ways, on desktop and at 375px.
  • Folder handling: out-of-root targets rejected with nothing written outside the media root, while flat and nested moves still succeed and store a normalised path.
  • Video details update applies only the intended fields, creates no duplicate rows, and the password flow still hashes.
  • Login throttle trips at the configured threshold with Retry-After, a success clears the counter, and legitimate logins are unaffected.
  • Non-admin under DEMO_MODE=true is refused on both webhook routes; admins still get through. Anonymous callers are refused on the SteamGridDB routes.
  • CORS: unset origin list produces no cross-origin headers at all; an allowlisted origin is echoed with credentials; * is refused.
  • Game detection: exact-case wins for both RUMBLE variants, normal fuzzy matching is unaffected (ROCKET LEAGUE 20260101 → Rocket League), unrelated footage still matches nothing, and results are identical with the library rows inserted in either order.

The Videos tab has had a bulk Uploader action since ownership landed, but
the Images tab never grew one, so images indexed from disk before
ownership existed were stuck unattributed with no way to adopt them. The
server side was already in place — bulk-set-uploader has always taken
image_ids — so this is only the missing control: an Uploader button in
the organize group, labelled on desktop and icon-only on mobile, opening
the same account picker as videos, including the option to clear
attribution.

The success toast reported "(0)" on both tabs, because runBulkAction
counts the updated/moved/deleted array while bulk-set-uploader answers
with updated_images / updated_videos counts instead. Both now prefer that
count when the response carries one.

Fixes #726
Findings from an external review, each confirmed against the code before
changing anything.

The example SECRET_KEY shipped uncommented in docker-compose.yml, so every
instance that never edited that line signed its session and remember-me
cookies with a key published in this repo — enough to compute an admin
cookie offline and present it without ever logging in. The line is now
commented out, the entrypoint generates a key on first start and keeps it
in /data so restarts do not sign everyone out, and both the entrypoint and
the app refuse the old placeholder if it is still set.

The three "move to folder" endpoints joined a client-supplied folder onto
the media root with pathlib, which drops the root entirely when the folder
is absolute, and "../" walked out of it just as easily. The new path was
then stored on the row and reused verbatim by delete, so a move could put
a file anywhere the process could write and the ordinary delete endpoint
would then unlink it. edit_own was enough to reach this. Folder names are
now resolved against the root and rejected if they land outside it, and
the normalised form is what gets stored.

PUT /api/video/details bulk-updated whatever remained in the request body
onto VideoInfo, which reached video_id — no uniqueness constraint, so a
row could be repointed at another user's video — and password_hash,raw,
side-stepping the hashing a few lines below that the surrounding code
comments describe as guaranteed. Only title, description and private are
the client's to set, matching what the image endpoint already did.

The password step of login had no limit of any kind, so a known username
could be guessed at as fast as the server would answer. Failures are now
counted per source address and per address/account in memory. The MFA cap
resets on every fresh password round, so failed codes count against the
same budget. Keying includes the address on purpose: a global per-account
lockout would let anyone lock a known username out of their own instance.

Also: the SteamGridDB search and asset routes had no authentication at all
while the routes either side of them require a permission, so anonymous
callers could spend the operator's API quota, and the search term went
into the request path unescaped; CORS was enabled with credentials and no
origin list, which flask-cors reads as "any origin" and answers by
reflecting the caller's own Origin, so it is now opt-in via CORS_ORIGINS
with SameSite stated explicitly on both cookies rather than left to the
browser default; and the webhook test routes, which make the server issue
an arbitrary outbound request, used the admin check that deliberately
admits any authenticated user under DEMO_MODE.
"RUMBLE" and "rumble" are both real games, and clips of the first kept being
suggested as the second.

Two faults behind it. The local match built its rapidfuzz choices as a list of
(name, game) tuples, so the query was scored against the tuple rather than the
name and every candidate came back 0 — nothing could clear the cutoff, so no
filename ever matched a game already in the library and every lookup fell
through to a SteamGridDB search. (Had one ever scored, the game was read out of
result[2], which for a list of choices is the index, not the element.) The
search it fell through to was handed a filename that had been lowercased for
cleaning, and took whichever hit SteamGridDB returned first — between two names
differing only in case, that is a coin flip.

Matching now scores case-insensitively, which is what the lowercasing was
reaching for, and the filename keeps its original case so an exact hit can still
be recognised. An exact, case-sensitive match wins outright, both against the
local library and among the search results. Where the library holds several
games whose names differ only in case and none matches exactly, no suggestion is
offered rather than guessing.

Fixes #728
Follow-up to 1dd88b9. That commit only separated names that were equal but for
case, and declined to answer when it could not tell them apart. Two gaps.

A name reaches the fuzzy matcher far more often than expected: "RUMBLE_<date>_clip"
cleans to "RUMBLE clip", not "RUMBLE", because the word-stripping pattern needs a
word boundary and an underscore is a word character. So it was not an exact hit,
and token_set_ratio — scoring case-folded — put "RUMBLE" and "rumble" at 100
apiece. extractOne answers a tie with whichever it reached first, so the game
suggested depended on which of the two had been added to the library first. The
earlier test only looked right because of the order the rows happened to go in.

Ties are now settled on how far each candidate's own casing is echoed by the
query, which is the only signal left once case has been folded away, and the
result no longer varies with row order. That also gives the ambiguous case a
nearest answer rather than no answer, so the no-suggestion path is gone.
Both were added without documentation. Security.md gains a section covering
them, leading with the fact that a normal deployment needs neither: Fireshare
serves its own frontend, so nothing is cross-origin, and Secure cookies are only
correct once HTTPS is actually in front of the instance. The failure mode for
each is the part worth writing down — an origin that does not match is silently
not allowed, and a Secure cookie on a plain-HTTP instance is discarded by the
browser after a login that otherwise looks like it worked.

The same section takes over describing the session signing key, which several
places still described as regenerated on every restart. It is now generated once
and persisted, so the standing advice to set one yourself, the troubleshooting
entry for being logged out on restart, and the environment variable table were
all telling operators to fix something that no longer happens.

Also drops the placeholder SECRET_KEY from the README's compose example, which
was still handing out a value to paste in. That example used a different
placeholder from the one docker-compose.yml carried, and only the latter was
refused on startup; both are now.
@ShaneIsrael
ShaneIsrael merged commit d633e05 into main Sep 14, 2026
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Game detection is not case sensitive Can't change Uploader of Images

1 participant