Modernize crypto, rebuild database setup, and add SQL migrations - #222
Merged
Conversation
The LICENSE.md and API_DOCS.md links pointed at github.com/.../blob/<branch>/, which broke once on the master -> main rename. Relative links resolve correctly on GitHub and in local editors, and can't rot on a future rename. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ
Replaces the obsolete MD5CryptoServiceProvider/SHA1CryptoServiceProvider
(SYSLIB0021) with the static HashData APIs (CA1850), and collapses the
NETFX_CORE/SILVERLIGHT preprocessor block in the vendored sqlite-net down to
Volatile.Write (SYSLIB0054).
The four hash helpers had accreted three different output formats: string.MD5()
was dash-separated uppercase, string.SHA1() was uppercase undashed, and the
byte[]/Stream overloads were lowercase undashed. Nothing required the spread --
each helper just hand-rolled its own formatting -- so they all now return plain
lowercase hex.
That fixes a live bug. Lastfm.CompileApiCall builds the api_sig parameter
straight from string.MD5(), so WaveBox was sending a 47-character
dash-separated string where the Last.fm API requires "a 32-character
hexadecimal md5 hash". Every signed call -- auth.getSession, and scrobbling
from both /api/scrobble and the Subsonic scrobble endpoint -- was malformed.
Playlist.CalculateHash no longer needs its .Replace("-", string.Empty), and
Playlist.Md5Hash / ETags change case; both are self-consistent internal values
that regenerate, and nothing compares across the formats.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ
res/wavebox.db and res/wavebox_querylog.db were prebuilt SQLite files copied into the root dir on first run, with res/wavebox.sql sitting alongside as a second copy of the same schema that nothing at runtime ever read. Keeping the two in sync by hand is what made a migration necessary at all, and the binary had already drifted: its User table carried an ALTER-appended ", ApiKey TEXT)" tail rather than a clean declaration. Applying the schema script on first run takes moments, so the binaries are gone and the .sql files are now the only source of truth. Adds a res/wavebox_querylog.sql, which had no textual counterpart before. Verified the replacement is faithful before deleting: applying wavebox.sql to an empty database reproduces the old template exactly, schema and seed data alike (ItemType 12 rows, FileType 10 rows), modulo that ALTER artifact. With a fresh database always built from the current schema, UpgradeSchema() has nothing left to upgrade -- User.ApiKey and its unique index are already declared in wavebox.sql -- so it and its test go away. Note this leaves no migration mechanism; that's fine pre-release but will need one before the first release, and the (currently unused) Version table is where it should hang. Two behavioral notes: - Setup now keys off whether the database has any tables rather than whether the file exists. The connection pools are constructed before DatabaseSetup runs, so an empty file left by an early connection previously caused the copy to be skipped and the schema to go missing entirely. - A failed schema apply now propagates instead of being logged and swallowed. Continuing without a schema just produces confusing null references later. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ
The password scheme was PBKDF2-HMACSHA1 at 2500 iterations, roughly 240x below OWASP's floor for that construction, using the now-obsolete Rfc2898DeriveBytes constructor (SYSLIB0060) and RNGCryptoServiceProvider (SYSLIB0023). Rather than modernize the call sites in place, this moves to bcrypt. A bcrypt hash is self describing -- "$2a$<cost>$<salt><hash>" -- so the salt and work factor travel with it. That removes the separate PasswordSalt column, which is dropped from the schema outright; no migration, since nothing is deployed. This is a hard cut with no legacy verification path, so any user rows in an existing wavebox.db stop authenticating and need recreating. Adds BCrypt.Net-Next, which stays NativeAOT-clean: a first-class net10.0 target, zero package dependencies, and no reflection or P/Invoke in its sources. Verified by publishing NativeAOT and running the E2E suite against the published binary -- no trim or AOT warnings, all 19 pass. Deliberately not using EnhancedHashPassword: its unkeyed SHA-384 pre-hash is the password-shucking construction OWASP warns against, and it emits an ordinary "$2a$" string with no marker, so mispairing it with Verify later would fail silently. Instead, passwords over bcrypt's 72-byte input limit are rejected at the set-password boundary rather than being silently truncated. Work factor 11, chosen by measurement rather than default. On an M-series Mac: cost 10 ~115ms, 11 ~140ms, 12 ~279ms, 13 ~552ms. Pi-class linux-arm64 runs several times slower, which would put 12 past OWASP's one-second ceiling there. Tests drop to bcrypt's minimum via a module initializer, since fixtures hash constantly and the production factor would dominate the run. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ
The seed script runner split files on a bare ';' and had to carry the warning "do not point this at hand-written SQL", because that breaks on a semicolon inside a string literal, a comment, or a trigger body. Migrations will be hand-written, so the splitter had to go first. The fix isn't a smarter splitter, it's not splitting at all. sqlite3_prepare_v2 compiles one statement and hands back a pointer to the remainder, so SQLite's own tokenizer does the work. The vendored binding already declared that pzTail parameter and passed IntPtr.Zero, which is exactly why Execute() only ever ran the first statement of a script. Adds SQLite3.ExecuteScript plus a prepare_v2 overload that keeps the tail, and surfaces it as ISQLiteConnection.ExecuteScript so callers don't reach for the raw handle. No new dependency: SQLitePCLRaw.core would mean two SQLite bindings in one process, Microsoft.Data.Sqlite is a second data-access stack, and DbUp's splitting is SQL-Server oriented. The script is marshalled as an explicit UTF-8 buffer and walked by byte offset, which sidesteps two latent bugs in the existing string overload rather than inheriting them: it marshals as ANSI (the system codepage on Windows), and it passes a character count where sqlite3_prepare_v2 wants a byte count, so any non-ASCII SQL is silently truncated. The pre-existing call sites still have both; those are unrelated to this path. wavebox.sql loses the PRAGMA/BEGIN TRANSACTION/COMMIT wrapper that sqlite3 .dump emits, since the caller owns the transaction now. That's what lets the statement filtering disappear along with the splitter. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ
Collapsing the ApiKey migration into the seed was the right call while nothing
was deployed, but it left no upgrade path at all: an existing database silently
kept its old schema and only failed later with a cryptic "no such column". The
server is about to run on a real machine, so schema changes now need to reach
databases that already exist.
Migrations are ordered SQL files in res/migrations, named 00001_description.sql
-- readable and diffable individually rather than accumulating in one C# method.
The five zero-padded digits mean lexical and numeric order agree at a glance,
though the runner sorts numerically regardless. How far a database has got lives
in the single-row Version table, which existed and was never used.
res/wavebox.sql becomes a frozen baseline at version 0, so a fresh database is
the seed plus every migration replayed in order. That keeps one code path
instead of reintroducing the seed/migration duplication just deleted, and means
every fresh install and CI run exercises the whole chain -- a broken migration
can't lurk until it reaches somebody's server.
Failures are loud, since the alternative is a database in an unknown state:
- A malformed file name or two files sharing a version number stop startup,
rather than being skipped without comment.
- A database whose version exceeds the newest bundled migration is refused with
"created by a newer version of WaveBox", which is the downgrade case.
- Each migration shares a transaction with its version bump, so a mid-chain
failure leaves the database cleanly at the last migration that did succeed.
WaveBoxMain now reports any database setup failure as a plain message and exits
1. These are operator-actionable ("you misnamed a migration"), and burying them
in an unhandled-exception abort under twenty lines of host machinery makes them
look like a crash instead.
Verified the upgrade path by hand against the published AOT binary, since no
automated test spans two builds: fresh boot lands at 0; adding a migration takes
it to 1 and adds the column; restarting re-runs nothing; and removing it again
refuses to start with the downgrade message.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ
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
1 participant
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.
Started as three small cleanups. Two of them turned out to be already fixed by #221, and the
third grew into replacing the password scheme and building a migration system.
Two things to know before merging
Last.fm scrobbling was broken, and this fixes it.
Lastfm.CompileApiCallbuilds theapi_sigparameter straight fromstring.MD5(), which returned dash-separated uppercase —so WaveBox was sending a 47-character string where the API requires
"a 32-character hexadecimal md5 hash". Every signed call was malformed:
auth.getSession,and scrobbling from both
/api/scrobbleand the Subsonicscrobbleendpoint. It falls outof normalizing the hash helpers, so it isn't visible from the commit subjects.
Existing databases need their users recreated. Passwords move to bcrypt with no legacy
verification path, so any user rows in an existing
wavebox.dbstop authenticating. Thefailure mode is a silent "wrong password", not an error. Nothing is deployed, so this is a
hard cut on purpose.
The arc
Commits 3 and 6 look contradictory in isolation — one deletes the migration machinery, the
other adds it back — so the order matters:
99cec85README links → relative, so they can't rot on another branch rename.5bd9e53Obsolete crypto APIs (SYSLIB0021,SYSLIB0054) → the staticHashDataAPIs and
Volatile.Write. The four hash helpers had drifted into three different outputformats with nothing requiring the spread; they all return lowercase hex now, which is
what fixes Last.fm.
9eb90d3Deletes the prebuiltres/*.dbtemplates in favour of applying the SQLschema on first run. Keeping a binary and a
.sqlfile in sync by hand is what made theone existing migration necessary, and the binary had already drifted — its
Usertablecarried an
ALTER-appended, ApiKey TEXT)tail. With a fresh database always builtfrom the current schema,
UpgradeSchema()had nothing left to do.b3c6e0cPBKDF2-HMACSHA1 at 2500 iterations (~240× below OWASP's floor, on the wrongPRF, via obsolete APIs) → bcrypt.
BCrypt.Net-Nextkeeps NativeAOT clean:net10.0target, zero dependencies, no reflection or P/Invoke. Work factor 11 by measurement, not
default — cost 12 is ~279ms here but several times that on Pi-class
linux-arm64, pastOWASP's one-second ceiling.
dd14d2eThe seed runner split SQL on a bare;and carried the warning "do notpoint this at hand-written SQL" — which migrations are. The fix isn't a smarter splitter,
it's not splitting:
sqlite3_prepare_v2returns a pointer to the remaining SQL, and thevendored binding already declared that parameter and passed
IntPtr.Zero.cabe601Ordered migrations inres/migrations/00001_description.sql, tracked in theVersiontable that has existed unused all along.wavebox.sqlis now a frozen baselineat version 0, so a fresh database is the seed plus every migration replayed — one code
path, and the chain is exercised by every fresh install and CI run rather than first
meeting a real database on someone's server.
Notes for review
EnhancedHashPassword. Its unkeyed SHA-384 pre-hash is thepassword-shucking construction OWASP warns against,
and it emits an ordinary-looking
$2a$string with no marker, so pairing it withVerifylater would fail silently. Over-72-byte passwords are rejected instead of truncated.
ExecuteScriptmarshals UTF-8 explicitly rather than reusing the existing stringoverload, which marshals as ANSI and passes a character count where SQLite wants a byte
count. Both are invisible today only because every
.sqlfile here is ASCII; thepre-existing call sites still have them.
newer than the build.
WaveBoxMainreports these as a plain message and exits 1 ratherthan as an unhandled-exception abort, since they're operator-actionable.
res/migrations/ships with only its README for now; there's no pending schema change anda no-op migration would be noise.
Verification
443 unit/integration + 19 E2E pass. The E2E suite also runs against the NativeAOT-published
binary, which is what proves both bcrypt and the new P/Invoke survive AOT — no trim or AOT
warnings. A clean build is down to 4 warnings from 8 in the touched categories, leaving only
the two
SYSLIB0014WebRequest.Createcalls inLastfm.cs.The upgrade path itself was checked by hand against the published binary, since no automated
test spans two builds: fresh boot lands at version 0; adding a migration takes it to 1 and
adds the column; restarting re-runs nothing; removing it refuses to start with the downgrade
message.
Follow-ups, not in this PR
CA2013ReferenceEqualsonint?/long?(boxes, so always false — e.g. the null guard inUser.Deletenever fires),CA1806ignoredTryParseresults,CA2022inexactFileStream.Read.SYSLIB0014WebRequest.Create→HttpClientinLastfm.cs, plus that file's untestedCompileApiCall.Utility.RandomStringseeds session IDs fromSystem.Random, not a CSPRNG.🤖 Generated with Claude Code
https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ