Harden upload validation, token storage, auth revocation, and comment ownership - #146
Merged
Conversation
The extension-mismatch check only rejected a file when the detected content type was itself in the allowlist and differed from the claimed extension. A detected type outside the allowlist (exe, zip, pdf, ...) never triggered either branch, so the file was saved under the client's claimed extension despite the docstring's claim that this prevents extension spoofing. Reject unconditionally when the detected type isn't an allowed image format, then check for extension mismatch as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
The blacklist stored raw, reusable JWTs. Anyone reading the SQLite file (backup, restore leak, export) would gain directly reusable session artifacts valid for up to 24h. Hash with SHA-256 before storage and comparison; no schema change needed since the column already just stores an opaque TEXT value. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
️✅ There are no secrets present in this pull request anymore.If these secrets were true positive and are still valid, we highly recommend you to revoke them. 🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request. |
`if let Ok(true) = is_token_blacklisted(...)` silently treated a database error as "not blacklisted", letting a revoked token through. This is the only revocation check on admin routes: it caches Claims in the request extensions, so the downstream Claims extractor (which is already fail-closed) skips its own check when it finds them. Match the fail-closed pattern already used by the Claims/OptionalClaims extractors: map the error to a 500 instead of falling through. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
delete_comment authorized ownership via `comment.author == claims.sub`. `author` is a free-text display name guests can set almost arbitrarily, so an authenticated user could delete any guest comment where the guest happened to type that user's username. Add `author_username`/`is_guest` columns recording the real identity at comment-creation time. Two columns instead of one: guest comments are permanently NULL for author_username, so a naive NULL fallback to the old string match would keep the impersonation hole open for every future guest comment, not just historical rows. `is_guest` disambiguates "known guest" from "pre-migration row of unknown origin", the latter falling back to the legacy check as an accepted, time-bounded gap limited to rows that already existed when this shipped. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
/api/comments/{id}/vote had no GovernorLayer, unlike the other public
comment routes. An authenticated client could call it at unlimited
frequency; the unique-vote DB constraint prevents duplicate votes but
not the unbounded request volume. Group it with the existing
rate-limited comment routes under one shared layer.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
HSTS was auto-enabled by reading x-forwarded-proto from the request. When TRUST_PROXY_IP_HEADERS=true, that header isn't stripped before this middleware runs, so a client with direct access to the backend port could set it. Drop the auto-detection and rely solely on the existing explicit ENABLE_HSTS opt-in. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
No nightly-only features are used (MSRV 1.82 per Cargo.toml); nightly is a floating, unpinned toolchain and an unnecessary supply-chain and reproducibility risk for a production build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Not imported anywhere in src/ (MarkdownRenderer.jsx only uses rehypeKatex and rehypeHighlight). Left in place, it's a landmine: any future addition to the rehype pipeline without pairing it with sanitization would open a stored-XSS path via markdown content. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…on race, dedupe hashing - delete_comment: split the (None, _) wildcard arm into explicit (None, None) and (None, Some(false)) cases. The wildcard previously let a hypothetical row shape (author_username=None, is_guest=Some(false)) that no current insert path produces, but that nothing in the schema forbids, silently fall back to the spoofable author==claims.sub comparison. It now fails closed instead. - apply_comment_author_identity_migration: the check-then-ALTER TABLE sequence wasn't atomic, so two app instances starting concurrently against the same SQLite file (e.g. a rolling deploy) could both pass the column-missing check and race on ALTER TABLE, aborting the loser's startup with "duplicate column name". Now tolerates that specific error as a sign the migration already ran concurrently. - Extracted a shared security::sha256_hex helper and pointed both hash_login_identifier (handlers/auth.rs) and hash_token (repositories/token_blacklist.rs) at it instead of each hand-rolling the same Sha256::new/update/finalize recipe. - handlers/comments.rs: added Comment::from(models::Comment) and replaced four field-by-field struct reconstructions with it, keeping the DTO's serde(skip_serializing) privacy boundary intact. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
zerox80
force-pushed
the
security/harden-upload-auth-comments
branch
from
July 2, 2026 13:08
54efff3 to
aeac105
Compare
The blacklist-hashing fix changed the repository to store and look up only SHA-256 hashes, but a database written by a pre-hashing version still holds raw JWTs, which the hashed lookup can never match. Without a backfill, every token revoked before the upgrade (e.g. via logout) would silently become valid again for its remaining lifetime -- undoing exactly the revocations the hashing fix is meant to protect. Adds a one-time migration (gated by an app_metadata flag, following the existing comment_schema_fixed_v1 pattern) that rehashes any non-hex-shaped token in place. Raw JWTs always contain dot separators so the 64-hex-char check can never double-hash an already-migrated row, even if the flag is lost. Covered by a regression test that simulates a pre-hashing database and asserts the revoked raw token is still detected as blacklisted after (and after re-running) migrations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
zerox80
force-pushed
the
security/harden-upload-auth-comments
branch
from
July 2, 2026 13:12
aeac105 to
91064a4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Nine security hardening fixes, ordered by severity. Each is an isolated commit.
auth_middleware(MEDIUM-HIGH): a database error during the revocation check was silently treated as "not revoked", the only defense against revoked tokens on admin routes. Now fails closed (500) like theClaimsextractor already does.delete_commentcomparedcomment.author == claims.sub, butauthoris a free-text name guests can set almost arbitrarily. A registered user could delete a guest's comment if the guest happened to type that user's username. Addedauthor_username/is_guestcolumns to record real identity; pre-migration rows retain the old check as a documented, time-bounded residual risk./api/comments/{id}/votewas outside theGovernorLayerthat protects the other public comment routes. Grouped under the same layer.x-forwarded-proto(LOW): whenTRUST_PROXY_IP_HEADERS=true, that header isn't stripped, so a client with direct backend access could set it. Removed the auto-detection; HSTS now requires the existing explicitENABLE_HSTSopt-in.nightlytag (LOW): no nightly-only features are used (MSRV 1.82). Pinned torust:1.83-bookworm.ADMIN_PASSWORDexceeds 72 bytes rather than changing the validation cap, which risked locking out existing admins with longer passwords.rehype-rawdependency (LOW): not imported anywhere; left in place it's a stored-XSS landmine if ever wired into the markdown pipeline without sanitization.Deliberately out of scope (documented, not coded): CSP
style-src 'unsafe-inline'(20+ inline-style call sites, needs a larger frontend refactor with no active exploit path); JWT refresh/rotation (a feature, not a bug); comment content stored unescaped (safe today since only React renders it, a future concern only for non-React consumers).Test plan
cargo buildandcargo test— all 31 unit tests + all integration test files pass, including new tests for the blacklist hashing round-trip, the fail-closed middleware regression, and 5 new comment-ownership authorization cases (including the critical spoofed-guest-identity regression test)cargo clippy --all-targets— no warningscargo fmt --check— cleannpm run build— frontend builds successfully afterrehype-rawremovaldocker build -f backend/Dockerfile backend/— not verified in this environment (no Docker available); recommend confirming before merge🤖 Generated with Claude Code