From 99cec85b71fc65eac7b4507419e1600cdc166ea0 Mon Sep 17 00:00:00 2001 From: Ben Baron Date: Sun, 26 Jul 2026 18:07:49 -0500 Subject: [PATCH 1/6] Use relative links in the README The LICENSE.md and API_DOCS.md links pointed at github.com/.../blob//, 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) Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 7be33b5..747aeb5 100644 --- a/README.md +++ b/README.md @@ -3,12 +3,12 @@ WaveBox A free, open source personal media server written in C# and available on all platforms thanks to the Mono framework. -Licensed under [The GNU General Public License v3.0](https://www.gnu.org/licenses/gpl.html "Permalink to The GNU General Public License v3.0 - GNU Project"), for more information please read the [LICENSE.md](https://github.com/einsteinx2/WaveBox/blob/main/LICENSE.md) file in this repository or visit the preceding link to the GNU website. +Licensed under [The GNU General Public License v3.0](https://www.gnu.org/licenses/gpl.html "Permalink to The GNU General Public License v3.0 - GNU Project"), for more information please read the [LICENSE.md](LICENSE.md) file in this repository or visit the preceding link to the GNU website. API --- -WaveBox features a very extensible JSON API, and is built with API developers in mind! Full API documentation can be found in [API_DOCS.md](https://github.com/einsteinx2/WaveBox/blob/main/API_DOCS.md). +WaveBox features a very extensible JSON API, and is built with API developers in mind! Full API documentation can be found in [API_DOCS.md](API_DOCS.md). Testing ------- From 5bd9e53b95d7df437d0f00536381c660b9f47a06 Mon Sep 17 00:00:00 2001 From: Ben Baron Date: Sun, 26 Jul 2026 18:10:02 -0500 Subject: [PATCH 2/6] Modernize hashing helpers and emit lowercase hex everywhere 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) Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ --- WaveBox.Core/src/Extensions/ByteExtensions.cs | 22 +++---------------- .../src/Extensions/StringExtensions.cs | 12 +++++----- WaveBox.Core/src/Model/Playlist.cs | 2 +- WaveBox.Core/src/SQLiteNet.cs | 8 +------ .../src/Extensions/StreamExtensions.cs | 22 +++---------------- .../WaveBox.Core.Tests/ByteExtensionsTests.cs | 4 +--- .../DateTimeExtensionsTests.cs | 4 ++-- .../StringExtensionsTests.cs | 20 ++++++++++------- 8 files changed, 29 insertions(+), 65 deletions(-) diff --git a/WaveBox.Core/src/Extensions/ByteExtensions.cs b/WaveBox.Core/src/Extensions/ByteExtensions.cs index 8a45b1d..2533bf0 100644 --- a/WaveBox.Core/src/Extensions/ByteExtensions.cs +++ b/WaveBox.Core/src/Extensions/ByteExtensions.cs @@ -9,27 +9,11 @@ namespace WaveBox.Core.Extensions { public static class ByteExtensions { /// - /// Generates a MD5 sum of a given byte array - /// Thanks: http://msdn.microsoft.com/en-us/library/s02tk69a.aspx + /// Generates a MD5 sum of a given byte array, as lowercase hex /// public static string MD5(this byte[] input) { - using (System.Security.Cryptography.MD5 md5 = System.Security.Cryptography.MD5.Create()) { - // Convert the input string to a byte array and compute the hash. - byte[] data = md5.ComputeHash(input); - - // Create a new Stringbuilder to collect the bytes - // and create a string. - StringBuilder sBuilder = new StringBuilder(); - - // Loop through each byte of the hashed data - // and format each one as a hexadecimal string. - for (int i = 0; i < data.Length; i++) { - sBuilder.Append(data[i].ToString("x2")); - } - - // Return the hexadecimal string. - return sBuilder.ToString(); - } + byte[] hash = System.Security.Cryptography.MD5.HashData(input); + return Convert.ToHexString(hash).ToLowerInvariant(); } } } diff --git a/WaveBox.Core/src/Extensions/StringExtensions.cs b/WaveBox.Core/src/Extensions/StringExtensions.cs index c7bab40..0eef794 100644 --- a/WaveBox.Core/src/Extensions/StringExtensions.cs +++ b/WaveBox.Core/src/Extensions/StringExtensions.cs @@ -38,15 +38,15 @@ public static bool IsTrue(this string boolString) { } /// - /// Generates a MD5 sum of a given string + /// Generates a MD5 sum of a given string, as lowercase hex /// public static string MD5(this string sumthis) { if (sumthis == "" || sumthis == null) { return ""; } - MD5CryptoServiceProvider md5 = new MD5CryptoServiceProvider(); - return BitConverter.ToString(md5.ComputeHash(System.Text.Encoding.ASCII.GetBytes(sumthis)), 0); + byte[] hash = System.Security.Cryptography.MD5.HashData(Encoding.ASCII.GetBytes(sumthis)); + return Convert.ToHexString(hash).ToLowerInvariant(); } /// @@ -84,15 +84,15 @@ public static int MonthForAbbreviation(this string abb) { } /// - /// Generates a SHA1 sum of a given string + /// Generates a SHA1 sum of a given string, as lowercase hex /// public static string SHA1(this string sumthis) { if (sumthis == "" || sumthis == null) { return ""; } - SHA1CryptoServiceProvider provider = new SHA1CryptoServiceProvider(); - return BitConverter.ToString(provider.ComputeHash(Encoding.ASCII.GetBytes(sumthis))).Replace("-", ""); + byte[] hash = System.Security.Cryptography.SHA1.HashData(Encoding.ASCII.GetBytes(sumthis)); + return Convert.ToHexString(hash).ToLowerInvariant(); } /// diff --git a/WaveBox.Core/src/Model/Playlist.cs b/WaveBox.Core/src/Model/Playlist.cs index a6db9d4..08ebc65 100644 --- a/WaveBox.Core/src/Model/Playlist.cs +++ b/WaveBox.Core/src/Model/Playlist.cs @@ -69,7 +69,7 @@ public string CalculateHash() { Injection.Get().CloseSqliteConnection(conn); } - return itemIds.ToString().MD5().Replace("-", string.Empty); + return itemIds.ToString().MD5(); } public void UpdateDatabase() { diff --git a/WaveBox.Core/src/SQLiteNet.cs b/WaveBox.Core/src/SQLiteNet.cs index e2ace2a..78d1b41 100644 --- a/WaveBox.Core/src/SQLiteNet.cs +++ b/WaveBox.Core/src/SQLiteNet.cs @@ -990,13 +990,7 @@ private void DoSavePointExecute(string savepoint, string cmd) { if (Int32.TryParse(savepoint.Substring(firstLen + 1), out depth)) { // TODO: Mild race here, but inescapable without locking almost everywhere. if (0 <= depth && depth < _trasactionDepth) { -#if NETFX_CORE - Volatile.Write (ref _trasactionDepth, depth); -#elif SILVERLIGHT - _trasactionDepth = depth; -#else - Thread.VolatileWrite(ref _trasactionDepth, depth); -#endif + Volatile.Write(ref _trasactionDepth, depth); Execute(cmd + savepoint); return; } diff --git a/WaveBox.Server/src/Extensions/StreamExtensions.cs b/WaveBox.Server/src/Extensions/StreamExtensions.cs index 6d14770..5a1e021 100644 --- a/WaveBox.Server/src/Extensions/StreamExtensions.cs +++ b/WaveBox.Server/src/Extensions/StreamExtensions.cs @@ -11,27 +11,11 @@ namespace WaveBox.Core.Extensions { public static class StreamExtensions { /// - /// Generates a MD5 sum of a given input Stream - /// Thanks: http://msdn.microsoft.com/en-us/library/s02tk69a.aspx + /// Generates a MD5 sum of a given input Stream, as lowercase hex /// public static string MD5(this Stream input) { - using (System.Security.Cryptography.MD5 md5 = System.Security.Cryptography.MD5.Create()) { - // Convert the input string to a byte array and compute the hash. - byte[] data = md5.ComputeHash(input); - - // Create a new Stringbuilder to collect the bytes - // and create a string. - StringBuilder sBuilder = new StringBuilder(); - - // Loop through each byte of the hashed data - // and format each one as a hexadecimal string. - for (int i = 0; i < data.Length; i++) { - sBuilder.Append(data[i].ToString("x2")); - } - - // Return the hexadecimal string. - return sBuilder.ToString(); - } + byte[] hash = System.Security.Cryptography.MD5.HashData(input); + return Convert.ToHexString(hash).ToLowerInvariant(); } /// diff --git a/tests/WaveBox.Core.Tests/ByteExtensionsTests.cs b/tests/WaveBox.Core.Tests/ByteExtensionsTests.cs index 6bbd595..2515bb3 100644 --- a/tests/WaveBox.Core.Tests/ByteExtensionsTests.cs +++ b/tests/WaveBox.Core.Tests/ByteExtensionsTests.cs @@ -6,9 +6,7 @@ namespace WaveBox.Core.Tests { public class ByteExtensionsTests { [Fact] - public void MD5_KnownVector_IsLowercaseHexWithoutDashes() { - // Note: byte[].MD5() is lowercase without dashes, unlike string.MD5() which is - // dash-separated uppercase — both formats are pinned deliberately + public void MD5_KnownVector_IsLowercaseHex() { byte[] input = Encoding.ASCII.GetBytes("test"); Assert.Equal("098f6bcd4621d373cade4e832627b4f6", input.MD5()); } diff --git a/tests/WaveBox.Core.Tests/DateTimeExtensionsTests.cs b/tests/WaveBox.Core.Tests/DateTimeExtensionsTests.cs index 78c82aa..4de27c0 100644 --- a/tests/WaveBox.Core.Tests/DateTimeExtensionsTests.cs +++ b/tests/WaveBox.Core.Tests/DateTimeExtensionsTests.cs @@ -63,8 +63,8 @@ public void ToETag_IsSha1OfRFC1123String() { DateTime dt = new DateTime(2001, 2, 3, 4, 5, 6, DateTimeKind.Utc); string etag = dt.ToETag(); Assert.Equal(dt.ToRFC1123().SHA1(), etag); - // 40 uppercase hex chars (SHA1 in this codebase's uppercase, dash-stripped format) - Assert.Matches(new Regex("^[0-9A-F]{40}$"), etag); + // 40 lowercase hex chars + Assert.Matches(new Regex("^[0-9a-f]{40}$"), etag); } } } diff --git a/tests/WaveBox.Core.Tests/StringExtensionsTests.cs b/tests/WaveBox.Core.Tests/StringExtensionsTests.cs index 377b73d..8011e97 100644 --- a/tests/WaveBox.Core.Tests/StringExtensionsTests.cs +++ b/tests/WaveBox.Core.Tests/StringExtensionsTests.cs @@ -1,4 +1,5 @@ using System; +using System.Text.RegularExpressions; using WaveBox.Core.Extensions; using Xunit; @@ -32,12 +33,15 @@ public void IsTrue_MatchesTruthyMatrix(string input, bool expected) { } [Fact] - public void MD5_KnownVector_IsDashSeparatedUppercaseHex() { - // Pinned: string.MD5() returns BitConverter.ToString output (dash-separated uppercase - // hex), unlike SHA1() which strips the dashes. The asymmetry is intentional pinning - // of current behavior — hashed values are compared against stored values in the same - // format, so changing it would break existing data. - Assert.Equal("09-8F-6B-CD-46-21-D3-73-CA-DE-4E-83-26-27-B4-F6", "test".MD5()); + public void MD5_KnownVector_IsLowercaseHex() { + Assert.Equal("098f6bcd4621d373cade4e832627b4f6", "test".MD5()); + } + + [Fact] + public void MD5_IsPlain32CharLowercaseHex() { + // Last.fm's api_sig must be "a 32-character hexadecimal md5 hash", and Lastfm.cs + // builds it straight from this method, so the exact shape is externally load-bearing + Assert.Matches(new Regex("^[0-9a-f]{32}$"), "any input at all".MD5()); } [Fact] @@ -47,8 +51,8 @@ public void MD5_EmptyOrNull_ReturnsEmptyString() { } [Fact] - public void SHA1_KnownVector_IsUppercaseHexWithoutDashes() { - Assert.Equal("A94A8FE5CCB19BA61C4C0873D391E987982FBBD3", "test".SHA1()); + public void SHA1_KnownVector_IsLowercaseHex() { + Assert.Equal("a94a8fe5ccb19ba61c4c0873d391e987982fbbd3", "test".SHA1()); } [Fact] From 9eb90d3e6b1c9502972ec3e29152079277f32684 Mon Sep 17 00:00:00 2001 From: Ben Baron Date: Sun, 26 Jul 2026 18:14:14 -0500 Subject: [PATCH 3/6] Build databases from their SQL schemas instead of shipping prebuilt ones 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) Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ --- .../src/Injection/Interfaces/IDatabase.cs | 4 +- WaveBox.Server/WaveBox.Server.csproj | 4 +- WaveBox.Server/res/wavebox.db | Bin 76800 -> 0 bytes WaveBox.Server/res/wavebox_querylog.db | Bin 3072 -> 0 bytes WaveBox.Server/res/wavebox_querylog.sql | 5 + WaveBox.Server/src/Static/Database.cs | 114 +++++++++--------- .../DatabaseSetupTests.cs | 47 ++++---- 7 files changed, 89 insertions(+), 85 deletions(-) delete mode 100644 WaveBox.Server/res/wavebox.db delete mode 100644 WaveBox.Server/res/wavebox_querylog.db create mode 100644 WaveBox.Server/res/wavebox_querylog.sql diff --git a/WaveBox.Core/src/Injection/Interfaces/IDatabase.cs b/WaveBox.Core/src/Injection/Interfaces/IDatabase.cs index 7683a3a..5fcc0fb 100644 --- a/WaveBox.Core/src/Injection/Interfaces/IDatabase.cs +++ b/WaveBox.Core/src/Injection/Interfaces/IDatabase.cs @@ -9,10 +9,10 @@ public interface IDatabase { int Version { get; } - string DatabaseTemplatePath { get; } + string DatabaseSchemaPath { get; } string DatabasePath { get; } - string QuerylogTemplatePath { get; } + string QuerylogSchemaPath { get; } string QuerylogPath { get; } void DatabaseSetup(); diff --git a/WaveBox.Server/WaveBox.Server.csproj b/WaveBox.Server/WaveBox.Server.csproj index c3253d5..b35130e 100644 --- a/WaveBox.Server/WaveBox.Server.csproj +++ b/WaveBox.Server/WaveBox.Server.csproj @@ -41,8 +41,8 @@ - - + + diff --git a/WaveBox.Server/res/wavebox.db b/WaveBox.Server/res/wavebox.db deleted file mode 100644 index b0ebca0ba60d0c6abd1dde35cc7f5ec96de76c0b..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 76800 zcmeI5S!^4}8Gv^<5--V#9YxtNieiR}B5&j)vZL6J<2W>RM6nJ=hZ8qQiIupr7vdq4 z%fxn$f^w1q4T==#OVfuIC{Up2`PlX`?NgfoMT;5!|OmHLEirTD8)ttei23gL!=~;=j$RE;+^ela!zC(XVzeC@mf1qEWuWvHr7##^90ZH0L zhlo|%d2Q|Prv0|0D?nw!>wnNsdkNJ4Ko1=zW%b|RMf)t74>Ve}oon=hMoZEeq#8AKHIHKakiTevkkXXm08oo$bRxo@@vo6JU^#DwlNetiBInDC&lLsbvdO?v9W_i zHgsTpdt&qB(Z%ubV)W$nc-WGL_XNZ26S0n`l9BM#?Be+3_NASugumjKqkw=PizTeR5wP z*+}rz#!Y7u@#S-;RY>imisQy9J*8>uF?HQ8T#bsxlBK zZ4~0TF^WoN*8*dquxwO7o6(ZGYO@gPCiIjRSB-NPpko2AJf}1OBywSM630Qj|F?hV za0dx&I|6wBzwL?~GmHdm0?_{ZrB@{B6>^%q>FM?y_qxLTc;H}$O*zb1Vs5wvBw`opc<9jtbbm_4~++A5u59N2XAb03>gpOzPx_J*P9&m!n z;V!RyYOvNVnQfe@EZwhzfgLY)n-3UMoCnqu#_i8ti7F+w|M1Hh^+C%E@-*uwVb)#XRN^$X4 zA85Kv&3tpm^$EVc&8^B5+N#XM#`!hFRmrt=Pt9$W?y7An;(T1qX&Ix!nmEgE(<@E} zmEoOU`9!GJWne~NIJxZ}W0y~sWl}VsEhn{1PUD-~-2k}E&ObkDoP8dwO74RS)c+v; zszg7c|D=CnGxC1PW&piSzfZr%g!n-MNT8VnZVA$kzR!I8@yAP|NM*oh5nKLj?Dr31%21i1K3OgXd4M2fqDe|w1dD;M37Z` z5(vQ7?}x1qeh7%y3){|4*miWlmQvVyJd_6b{9p9{t%n!SA%XTKfc^jNTeg@pB+!Te z_Ww5`gy)e!`x3zRzkSOVbA|*O5y1Ap5g|N}1lpGXw*T#0wwN;{(1-xG|BVRYc_h%j z1fc!*OOq1&%aJ`~j{JmtM32(sHRf2k%_|?5Yk4|x_oKuJ|LQFs>hZ}vJ!E5s`+7xl z2K&3i?c~HHwu=+ncRDy(Gi8Kho8^I(9DPE~izyN1&Rf%L4b_=!QeV}z3U@#7Wn0iM zvAHV3dCzhPE1P67c)M3V+EXioERBf~wvjLN@+mjMi3^g>y!ymfs3MPWvQa)oddl1n z?JU}}qxRiVZ<>rq0#78Ux@+SxnY)|4`YbPn)3Ep~-}Jdt$z^sL%TrN(_ELo>oXEpi zA*Z?=C58N?Zm<(oi=bN3G%C)LpPSJCw4Q|7GVTI?LetNlV^^75YPmL+%gg=jf{Joy zy(^w$ON{WZyNjWaPwwp{8@-%0hsesH-%T3CH9Ob*U6G_KUXyBl-{T&5wZ!(|t0i!{9aJCbwnA16Ia?=U*+Qmbkl8v} zYdn0$>C7MoabeJVX+6n?$zeFx9_wX`_Vm?Tw8t{V)_oKZb{8++&DUJIJnDi2!_QV2 z*uS?rB-bdlGyK)Va)!Ltv1~BkAb!PiOpej+!~ONvWpZYx1dQjnc={gh*dZ1uTF~-& zwrZ*P8gQc$Qu}fe$B=BtdbW0|L;)|cmwwjCSuCZq1&AuHvsg|mza%OYS6(%C5FG#C z>OXauek8CN0(k$w8G0BO32bcw*#Eb+%MVkJ1U5qe+yBkb!?;LbYZJise`}W?rXC4w zhQJQb%dG!@Ng`h)Ka!T{cj2XZ8Me z2vzC>t+BTKd=_3kCFXV6U7ezvHLm9KFJyDc+Oq|%5DRLG_1yCw+p=D2=!;)X+ughn zL}+O1TMGjqhcuHm5=q-nuQ9rBQXNoJwpQx z10IBceEol%|F?avAj~Nea1y}he`o^!!SVGfi41Z=Q`zsCn&-rdA}jeEB<(y_3xOJRjF`GkT^_xqjdD#hkv9H-}I} zgjhD6W^U}JOc@%i*GUaP*(e9w4u9%?b5+GIKEiQY%YpN~_}b^bYJ}9jaKv#LX?s+N zy?XeiA#<+CDXx4%eLkDh4bA%A?^bV#ZT*(OaiQ4C-AhzjA)(v7T4?L~5(y~3;>*UM zENu`BhF9untTTjz{{J`XHxBh8fvrFQpZ~W)DPbCsz>Oh*&;K`uD{4goTY&(!|68Gy zFpWsy#t^{v|Hg1dtw>-i5PdjG>*?ay2FvnBmM$enY!plm`pX z&4y!hvlG)(vBmJz00v|LZ9V$-j^~nPRWP>^1FQ+ueFo$FJp)w|=v5*p&k_?jFfKDm!Cx&QrpI4q5 zsEMd#k#Q;=jE?TV+GqzI(H<^N9N_On+E{a71GR^%pmIZUcE492o8pE_14Q;9fw)2( z>2T;0C+m#NRK2p42kX6?h?^3?j$r%W++R7gjs%)X0H6PxNgr(^f#wpx_P@FO(K-@n zCIPJf&7_aEkw9|^;QfDd`J;6t&`bi@{x_38+C~D+C4lXJbNQonB+yI(*#0+@KH5eC z%_RWszh8PolHMRAPnNn9CMOSiv#&5n&f6OL2~9|8ZPh=|3q99aaNk-gzal&g&> zmmc!TLxZF^%1th3vukNJx0YbjUDGGU0T4P~)uJo7Y0DW24BE#@unMZ|mvJfCQJ*{< zBE=zYdo`O%vc)RFA11CE=gf)Qk5#o>7SyzCMgW`U9(SzSD-J+oMA08+nPmodI}OIw z9Gib^#yKI(38KP|8KRyQ#@L~SdFv?ihrRMZh`ivoWM%8|8g^JBd5M+h#Z10-gk2sz zq_~SKRdYrHmYto+CSd|P1Yy2jQJ3Z_VL(3=G`l_z6vWZwaA_}(uP57lllSrUN z2w?qhk%GWvAb}wgoVqe&#tA_TDhw@5)?GLS$M2w?qh0(3Nq1X_du`2Pi@%aZgn(o0r70nevB z@6tQz%OJW*pC_Ztd#abLaYv^Dyu;Xh8#cTs;Z!x3Fh>}0aDs4q%qYs(QKqYhu!Xjm zPaQZ!%_K+RRo~26>STqplsIQQNr~f#GWUpAR(eU6YXR@LgzXXtPHi!lCii(z1`_Um zm)7!znqH46#qcqoJi|Qip5-dnQ|iSOTh)tu-X-GVXew!W%h?j9XsjB6NFxoVXPHwE z_{!PGRs8c79_4v9eb`vCjZ&WB72XrBJV)FCOluI#bUU>3ojUH55BHK{h-W(wZ}c#| z-KMx38+h%(3qB^xlvfL9+5i`${R54+m6^R#$ zg-lsF!Q2)5NwJHkd{w-ZBLSBlOS|=s4!6>>P^MxlEKjwF2Ujfk4Hl)xeeyAx6pwK$ zDy%h@uontr*DQZA4HrBqOu4;4Ys4@u^LSv^*2sN9OY5XT60rThNxyPv1_`tv0j&Qm zSSXkzByf`mVEw;IpwSExXh8y4|68z7FiA+@CK15;f0IC?86?nx1hD?MV4+}=kibnM F@PBu>0B-;Q diff --git a/WaveBox.Server/res/wavebox_querylog.db b/WaveBox.Server/res/wavebox_querylog.db deleted file mode 100644 index 0d594cf4ef5af67b6337cecf5f5ecdd79ab14358..0000000000000000000000000000000000000000 GIT binary patch literal 0 HcmV?d00001 literal 3072 zcmWFz^vNtqRY=P(%1ta$FlJz3U}R))P*7lCVBi5_W+-Na@>zg141mN40Uie3M{2x4 zRgBE(49w}w?1VIo%8!P?z=Xh0b9u%__RPGL)QZB=)S}Axoc#3o(t?!4lGON;%-mGC zAhUCjt7C|(f~TL0YlH$+80h95;_~v0O_n8zNjV6`g+Ph<;_{7RjO^mFvW$)OVCg`R z1wQ%dFq#Q!WQe1akE;SqR6#=t%=AoA0$LX0>h2n(;O8Hr;1}xSqYx0}>FXF2so?Dz zsiOc<7hF=5nU}7l5aJpER0LHBlnhJEDNQZLDys>~o4`yz3PwX + /// Creates a database from its bundled schema script if it has no tables yet. Checking for + /// tables rather than for the file means an empty database left behind by an earlier + /// connection still gets its schema, and makes repeat calls a no-op. + /// + private void ApplySchemaIfEmpty(string name, string schemaPath, Func open, Action close) { + ISQLiteConnection conn = null; + try { + conn = open(); - // close the template file - dbTemplate.Close(); - } catch (Exception e) { - logger.Error(e); + if (conn.ExecuteScalar("SELECT COUNT(*) FROM sqlite_master WHERE type = 'table'") > 0) { + return; } - } - // Upgrade databases created before newer columns existed (the bundled template's - // Version table is empty, so schema state is detected per-column instead) - this.UpgradeSchema(); + logger.IfInfo("Database " + name + " is empty; applying schema from " + schemaPath); - if (!File.Exists(QuerylogPath)) { + conn.BeginTransaction(); try { - logger.IfInfo("Query log database file doesn't exist; Creating it : " + QUERY_LOG_FILE_NAME); - - // new filestream on the template - FileStream dbTemplate = new FileStream(QuerylogTemplatePath, FileMode.Open); - - // a new byte array - byte[] dbData = new byte[dbTemplate.Length]; - - // read the template file into memory - dbTemplate.Read(dbData, 0, Convert.ToInt32(dbTemplate.Length)); - - // write it all out - System.IO.File.WriteAllBytes(QuerylogPath, dbData); - - // close the template file - dbTemplate.Close(); - } catch (Exception e) { - logger.Error(e); + foreach (string statement in ReadSchemaStatements(schemaPath)) { + conn.Execute(statement); + } + conn.Commit(); + } catch (Exception) { + conn.Rollback(); + throw; } + } catch (Exception e) { + // Log before rethrowing so the failure lands in the server log, then fail loudly: + // continuing with no schema only turns into confusing null references later on + logger.Error(e); + throw; + } finally { + close(conn); } } - private void UpgradeSchema() { - ISQLiteConnection conn = null; - try { - conn = GetSqliteConnection(); - - // Subsonic API keys: User.ApiKey column + unique index - int hasApiKey = conn.ExecuteScalar("SELECT COUNT(*) FROM pragma_table_info('User') WHERE name = 'ApiKey'"); - if (hasApiKey == 0) { - logger.IfInfo("Upgrading database schema: adding User.ApiKey"); - conn.Execute("ALTER TABLE User ADD COLUMN ApiKey TEXT"); + /// + /// Splits a schema script into individual statements. The vendored sqlite-net only executes + /// one statement per call and exposes no sqlite3_exec, so the script has to be split here. + /// Naive splitting on ';' is safe only because these files are sqlite3 .dump output whose + /// string literals contain no semicolons -- do not point this at hand-written SQL. + /// + /// A .dump also wraps itself in BEGIN TRANSACTION/COMMIT; those are dropped so the caller + /// owns the transaction and a failure part way through rolls the whole thing back. + /// + private static IEnumerable ReadSchemaStatements(string schemaPath) { + foreach (string statement in File.ReadAllText(schemaPath).Split(';')) { + string trimmed = statement.Trim(); + if (trimmed.Length == 0 || IsTransactionControl(trimmed)) { + continue; } - conn.Execute("CREATE UNIQUE INDEX IF NOT EXISTS user_ApiKey ON User(ApiKey)"); - } catch (Exception e) { - logger.Error(e); - } finally { - CloseSqliteConnection(conn); + + yield return trimmed; } } + private static bool IsTransactionControl(string statement) { + return statement.StartsWith("BEGIN", StringComparison.OrdinalIgnoreCase) + || statement.StartsWith("COMMIT", StringComparison.OrdinalIgnoreCase) + || statement.StartsWith("ROLLBACK", StringComparison.OrdinalIgnoreCase); + } + public ISQLiteConnection GetSqliteConnection() { if (isPoolingEnabled) { return mainPool.GetSqliteConnection(); diff --git a/tests/WaveBox.Server.Tests/DatabaseSetupTests.cs b/tests/WaveBox.Server.Tests/DatabaseSetupTests.cs index 9907d52..c503ec7 100644 --- a/tests/WaveBox.Server.Tests/DatabaseSetupTests.cs +++ b/tests/WaveBox.Server.Tests/DatabaseSetupTests.cs @@ -30,23 +30,41 @@ private static int Scalar(string sql) { } } - private static void Execute(string sql) { + private static int QuerylogScalar(string sql) { IDatabase db = Injection.Get(); ISQLiteConnection conn = null; try { - conn = db.GetSqliteConnection(); - conn.Execute(sql); + conn = db.GetQueryLogSqliteConnection(); + return conn.ExecuteScalar(sql); } finally { - db.CloseSqliteConnection(conn); + db.CloseQueryLogSqliteConnection(conn); } } [Fact] - public void SetupCopiesTemplateDatabasesIntoRoot() { + public void SetupCreatesDatabasesInRoot() { Assert.True(File.Exists(Path.Combine(harness.Root.Path, "wavebox.db"))); Assert.True(File.Exists(Path.Combine(harness.Root.Path, "wavebox_querylog.db"))); } + [Fact] + public void SetupAppliesFullSchemaFromScript() { + // The whole script has to run, not just its first statement: a partial apply would + // still produce a file and a User table, so assert on tables declared near the end + Assert.Equal(1, Scalar("SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'User'")); + Assert.Equal(1, Scalar("SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'Favorite'")); + Assert.Equal(1, Scalar("SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'MusicBrainzCheckDate'")); + Assert.Equal(1, QuerylogScalar("SELECT COUNT(*) FROM sqlite_master WHERE type = 'table' AND name = 'QueryLog'")); + } + + [Fact] + public void SetupSeedsLookupTables() { + // These rows came from the old prebuilt wavebox.db template; they now come from the + // INSERTs in wavebox.sql, and nothing else would notice if they went missing + Assert.Equal(12, Scalar("SELECT COUNT(*) FROM ItemType")); + Assert.Equal(10, Scalar("SELECT COUNT(*) FROM FileType")); + } + [Fact] public void SetupAddsApiKeyColumnAndUniqueIndex() { Assert.Equal(1, Scalar("SELECT COUNT(*) FROM pragma_table_info('User') WHERE name = 'ApiKey'")); @@ -61,27 +79,10 @@ public void SetupIsIdempotent() { db.DatabaseSetup(); Assert.Equal(1, Scalar("SELECT COUNT(*) FROM pragma_table_info('User') WHERE name = 'ApiKey'")); + Assert.Equal(12, Scalar("SELECT COUNT(*) FROM ItemType")); Assert.True(File.Exists(Path.Combine(harness.Root.Path, "wavebox.db"))); } - [Fact] - public void UpgradeSchemaRestoresApiKeyOnPreMigrationDatabase() { - // Simulate a database created before the ApiKey migration - Execute("DROP INDEX user_ApiKey"); - Execute("ALTER TABLE User DROP COLUMN ApiKey"); - Assert.Equal(0, Scalar("SELECT COUNT(*) FROM pragma_table_info('User') WHERE name = 'ApiKey'")); - - IDatabase db = Injection.Get(); - db.DatabaseSetup(); - - Assert.Equal(1, Scalar("SELECT COUNT(*) FROM pragma_table_info('User') WHERE name = 'ApiKey'")); - Assert.Equal(1, Scalar("SELECT COUNT(*) FROM sqlite_master WHERE type = 'index' AND name = 'user_ApiKey'")); - - // Running the migration again on an already-upgraded schema is a no-op - db.DatabaseSetup(); - Assert.Equal(1, Scalar("SELECT COUNT(*) FROM pragma_table_info('User') WHERE name = 'ApiKey'")); - } - [Fact] public void ApiKeyUniqueIndexRejectsDuplicateKeys() { IUserRepository users = Injection.Get(); From b3c6e0cc7fc6a4fd90908875b8b30a5833932e60 Mon Sep 17 00:00:00 2001 From: Ben Baron Date: Sun, 26 Jul 2026 18:20:55 -0500 Subject: [PATCH 4/6] Hash passwords with bcrypt instead of PBKDF2-HMACSHA1 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$$" -- 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) Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ --- WaveBox.Core/WaveBox.Core.csproj | 1 + WaveBox.Core/src/Model/User.cs | 61 ++------------ WaveBox.Core/src/Repository/UserRepository.cs | 7 +- WaveBox.Core/src/Static/PasswordHasher.cs | 69 ++++++++++++++++ WaveBox.Server/res/wavebox.sql | 1 - .../TestPasswordWorkFactor.cs | 16 ++++ tests/WaveBox.Core.Tests/UserCryptoTests.cs | 82 +++++++++++++------ .../TestPasswordWorkFactor.cs | 16 ++++ .../UserRepositoryTests.cs | 1 - 9 files changed, 169 insertions(+), 85 deletions(-) create mode 100644 WaveBox.Core/src/Static/PasswordHasher.cs create mode 100644 tests/WaveBox.Core.Tests/TestPasswordWorkFactor.cs create mode 100644 tests/WaveBox.Server.Tests/TestPasswordWorkFactor.cs diff --git a/WaveBox.Core/WaveBox.Core.csproj b/WaveBox.Core/WaveBox.Core.csproj index 8a54759..573aa66 100644 --- a/WaveBox.Core/WaveBox.Core.csproj +++ b/WaveBox.Core/WaveBox.Core.csproj @@ -6,6 +6,7 @@ + diff --git a/WaveBox.Core/src/Model/User.cs b/WaveBox.Core/src/Model/User.cs index 2554477..f8fac5f 100644 --- a/WaveBox.Core/src/Model/User.cs +++ b/WaveBox.Core/src/Model/User.cs @@ -1,8 +1,6 @@ using System; using System.Collections.Generic; using System.Linq; -using System.Text; -using System.Security.Cryptography; using Cirrious.MvvmCross.Plugins.Sqlite; using System.Text.Json.Serialization; using WaveBox.Core.Extensions; @@ -12,9 +10,6 @@ namespace WaveBox.Core.Model { public class User : IGroupingItem { - // PBKDF2 iterations - public const int HashIterations = 2500; - private static readonly WaveBox.Core.Logging.ILog logger = WaveBox.Core.Logging.LogManager.GetLogger(); [JsonPropertyName("userId")] @@ -39,9 +34,6 @@ public class User : IGroupingItem { [JsonIgnore] public string PasswordHash { get; set; } - [JsonIgnore] - public string PasswordSalt { get; set; } - [JsonIgnore, IgnoreRead, IgnoreWrite] public string SessionId { get; set; } @@ -106,17 +98,18 @@ public IList ListOfSessions() { } public bool UpdatePassword(string password) { - string salt = GeneratePasswordSalt(); - string hash = ComputePasswordHash(password, salt); + string hash = PasswordHasher.Hash(password); + if (hash == null) { + return false; + } ISQLiteConnection conn = null; try { conn = Injection.Get().GetSqliteConnection(); - int affected = conn.Execute("UPDATE User SET PasswordHash = ?, PasswordSalt = ? WHERE UserId = ?", hash, salt, this.UserId); + int affected = conn.Execute("UPDATE User SET PasswordHash = ? WHERE UserId = ?", hash, this.UserId); if (affected > 0) { this.PasswordHash = hash; - this.PasswordSalt = salt; return Injection.Get().UpdateUserCache(this); } @@ -212,24 +205,9 @@ public bool UpdateRole(Role role) { return false; } - // Verify password, using timing attack resistant approach - // Credit to PHP5.5 Password API for this method + // Verify password; the comparison is timing attack resistant public bool Authenticate(string password) { - // Compute hash - string hash = ComputePasswordHash(password, this.PasswordSalt); - - // Ensure hashes are same length - if (hash.Length != this.PasswordHash.Length) { - return false; - } - - // Compare ASCII value of each character, bitwise OR any diff - int status = 0; - for (int i = 0; i < hash.Length; i++) { - status |= ((int)hash[i] ^ (int)this.PasswordHash[i]); - } - - return status == 0; + return PasswordHasher.Verify(password, this.PasswordHash); } public bool CreateSession(string password, string clientName) { @@ -281,30 +259,5 @@ public static int CompareUsersByName(User x, User y) { return StringComparer.OrdinalIgnoreCase.Compare(x.UserName, y.UserName); } - // Compute password hash using PBKDF2 - public static string ComputePasswordHash(string password, string salt, int iterations = HashIterations) { - // Convert salt to byte array - byte[] saltBytes = Encoding.UTF8.GetBytes(salt); - - // Hash using PBKDF2 with salt and predefined iterations - using (Rfc2898DeriveBytes pbkdf2 = new Rfc2898DeriveBytes(password, saltBytes, iterations)) { - var key = pbkdf2.GetBytes(64); - return Convert.ToBase64String(key); - } - } - - // Use RNG crypto service to generate random bytes for salt - public static string GeneratePasswordSalt() { - // Create byte array to store salt - byte[] salt = new byte[32]; - - // Fill array using RNG - using (RNGCryptoServiceProvider rng = new RNGCryptoServiceProvider()) { - rng.GetBytes(salt); - } - - // Return string representation - return Convert.ToBase64String(salt); - } } } diff --git a/WaveBox.Core/src/Repository/UserRepository.cs b/WaveBox.Core/src/Repository/UserRepository.cs index 6be07d9..a5777d6 100644 --- a/WaveBox.Core/src/Repository/UserRepository.cs +++ b/WaveBox.Core/src/Repository/UserRepository.cs @@ -78,8 +78,10 @@ public User CreateUser(string userName, string password, Role role, long? delete return new User(); } - string salt = User.GeneratePasswordSalt(); - string hash = User.ComputePasswordHash(password, salt); + string hash = PasswordHasher.Hash(password); + if (hash == null) { + return null; + } var u = new User(); u.UserId = itemId; @@ -87,7 +89,6 @@ public User CreateUser(string userName, string password, Role role, long? delete u.Role = role; u.Password = password; u.PasswordHash = hash; - u.PasswordSalt = salt; u.CreateTime = DateTime.UtcNow.ToUnixTime(); u.DeleteTime = deleteTime; diff --git a/WaveBox.Core/src/Static/PasswordHasher.cs b/WaveBox.Core/src/Static/PasswordHasher.cs new file mode 100644 index 0000000..f7c5aad --- /dev/null +++ b/WaveBox.Core/src/Static/PasswordHasher.cs @@ -0,0 +1,69 @@ +using System; +using System.Text; + +namespace WaveBox.Core.Static { + /// + /// Password storage, wrapping bcrypt so the library stays out of the call sites. + /// + /// A bcrypt hash is self describing -- "$2a$<cost>$<22 char salt><31 char hash>" -- so the salt + /// and work factor travel with it and no separate salt column is needed. + /// + public static class PasswordHasher { + /// + /// bcrypt only consumes the first 72 bytes of a password and silently ignores the rest, so + /// longer passwords are rejected outright rather than quietly truncated. + /// + public const int MaxPasswordBytes = 72; + + /// + /// Work factor, as a power of two. OWASP's guidance is a minimum of 10, set as high as + /// verification performance allows while staying under a second on the slowest target. + /// + /// Measured on an M-series Mac: cost 10 ~115ms, 11 ~140ms, 12 ~279ms, 13 ~552ms. Pi-class + /// linux-arm64 hardware runs several times slower, which would put 12 over the one second + /// mark there, so 11 it is. Subsonic clients re-send credentials on every request, and + /// SubsonicAuth only caches a verification for ten minutes, so this sets a floor on + /// request latency for clients that don't keep connections alive. + /// + /// Tests dial this down to bcrypt's minimum, since they hash on nearly every fixture. + /// + internal static int WorkFactor = 11; + + /// + /// Hashes a password for storage, or returns null if it is unusable. bcrypt generates its + /// own salt, so calling this twice with the same password yields two different hashes. + /// + public static string Hash(string password) { + if (!IsAcceptable(password)) { + return null; + } + + return BCrypt.Net.BCrypt.HashPassword(password, WorkFactor); + } + + /// + /// Verifies a password against a stored hash, in time independent of how much of the hash + /// matched. Returns false for anything malformed rather than throwing. + /// + public static bool Verify(string password, string storedHash) { + if (String.IsNullOrEmpty(password) || String.IsNullOrEmpty(storedHash)) { + return false; + } + + try { + return BCrypt.Net.BCrypt.Verify(password, storedHash); + } catch (BCrypt.Net.SaltParseException) { + // Not a bcrypt hash at all + return false; + } + } + + /// + /// Whether a password can be stored, i.e. non-empty and within bcrypt's input limit. + /// + public static bool IsAcceptable(string password) { + return !String.IsNullOrEmpty(password) + && Encoding.UTF8.GetByteCount(password) <= MaxPasswordBytes; + } + } +} diff --git a/WaveBox.Server/res/wavebox.sql b/WaveBox.Server/res/wavebox.sql index 47e83bd..678e9ee 100644 --- a/WaveBox.Server/res/wavebox.sql +++ b/WaveBox.Server/res/wavebox.sql @@ -136,7 +136,6 @@ CREATE TABLE "User" ( "UserName" TEXT UNIQUE NOT NULL, "Role" INTEGER NOT NULL, "PasswordHash" TEXT NOT NULL, - "PasswordSalt" TEXT NOT NULL, "LastfmSession" TEXT, "CreateTime" INTEGER NOT NULL, "DeleteTime" INTEGER, diff --git a/tests/WaveBox.Core.Tests/TestPasswordWorkFactor.cs b/tests/WaveBox.Core.Tests/TestPasswordWorkFactor.cs new file mode 100644 index 0000000..981ae9e --- /dev/null +++ b/tests/WaveBox.Core.Tests/TestPasswordWorkFactor.cs @@ -0,0 +1,16 @@ +using System.Runtime.CompilerServices; +using WaveBox.Core.Static; + +namespace WaveBox.Core.Tests { + internal static class TestPasswordWorkFactor { + /// + /// Drops bcrypt to its minimum work factor for the duration of the test run. The production + /// value is tuned to take a noticeable fraction of a second per hash, and fixtures across + /// this assembly create users constantly, so leaving it alone would dominate the run time. + /// + [ModuleInitializer] + internal static void Reduce() { + PasswordHasher.WorkFactor = 4; + } + } +} diff --git a/tests/WaveBox.Core.Tests/UserCryptoTests.cs b/tests/WaveBox.Core.Tests/UserCryptoTests.cs index 4a3de4d..df82321 100644 --- a/tests/WaveBox.Core.Tests/UserCryptoTests.cs +++ b/tests/WaveBox.Core.Tests/UserCryptoTests.cs @@ -1,50 +1,80 @@ using System; using WaveBox.Core.Model; +using WaveBox.Core.Static; using Xunit; namespace WaveBox.Core.Tests { // Only User's static/pure members are tested here; everything else on User goes through // Injection and repositories, which are process-global and off limits for unit tests. - // PBKDF2 at 2500 iterations is deliberately slow, so hash computations are kept to a minimum. + // bcrypt is deliberately slow even at the reduced test work factor, so hash computations + // are kept to a minimum. public class UserCryptoTests { [Fact] - public void ComputePasswordHash_IsDeterministicForSameSaltAndDiffersAcrossSalts() { - string hash1 = User.ComputePasswordHash("password", "saltA"); - string hash2 = User.ComputePasswordHash("password", "saltA"); - string hash3 = User.ComputePasswordHash("password", "saltB"); - - Assert.Equal(hash1, hash2); - Assert.NotEqual(hash1, hash3); - - // 64-byte PBKDF2 output as base64: 88 chars - Assert.Equal(88, hash1.Length); - byte[] decoded = Convert.FromBase64String(hash1); - Assert.Equal(64, decoded.Length); + public void Hash_IsSelfSaltingSoRepeatsDiffer() { + // bcrypt generates its own salt per call, so unlike the old PBKDF2 scheme the same + // password never produces the same hash twice + string hash1 = PasswordHasher.Hash("password"); + string hash2 = PasswordHasher.Hash("password"); + + Assert.NotEqual(hash1, hash2); + Assert.True(PasswordHasher.Verify("password", hash1)); + Assert.True(PasswordHasher.Verify("password", hash2)); + } + + [Fact] + public void Hash_IsAModularCryptFormatBcryptString() { + string hash = PasswordHasher.Hash("password"); + + // "$2$$<22 char salt><31 char hash>" + Assert.StartsWith("$2", hash); + Assert.Equal(60, hash.Length); + } + + [Fact] + public void Verify_RejectsWrongPassword() { + string hash = PasswordHasher.Hash("password1"); + + Assert.False(PasswordHasher.Verify("password2", hash)); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData("not a bcrypt hash")] + [InlineData("$2a$truncated")] + public void Verify_ReturnsFalseForUnusableHashesInsteadOfThrowing(string storedHash) { + Assert.False(PasswordHasher.Verify("password", storedHash)); } [Fact] - public void ComputePasswordHash_DiffersAcrossPasswords() { - Assert.NotEqual( - User.ComputePasswordHash("password1", "saltA"), - User.ComputePasswordHash("password2", "saltA")); + public void Verify_ReturnsFalseForEmptyPassword() { + string hash = PasswordHasher.Hash("password"); + + Assert.False(PasswordHasher.Verify(null, hash)); + Assert.False(PasswordHasher.Verify("", hash)); } [Fact] - public void GeneratePasswordSalt_Is32RandomBytesBase64AndUnique() { - string salt1 = User.GeneratePasswordSalt(); - string salt2 = User.GeneratePasswordSalt(); + public void Hash_RejectsPasswordsBeyondBcryptsInputLimit() { + // bcrypt reads only the first 72 bytes and ignores the rest, so an over-long password + // must be refused rather than silently truncated to something weaker than it looks + Assert.Null(PasswordHasher.Hash(new string('a', PasswordHasher.MaxPasswordBytes + 1))); + Assert.NotNull(PasswordHasher.Hash(new string('a', PasswordHasher.MaxPasswordBytes))); + + // The limit is bytes, not characters: this is 73 bytes of UTF-8 in 25 characters + Assert.Null(PasswordHasher.Hash(new string('é', 36) + "a")); + } - Assert.Equal(32, Convert.FromBase64String(salt1).Length); - Assert.Equal(32, Convert.FromBase64String(salt2).Length); - Assert.NotEqual(salt1, salt2); + [Fact] + public void Hash_RejectsNullOrEmptyPassword() { + Assert.Null(PasswordHasher.Hash(null)); + Assert.Null(PasswordHasher.Hash("")); } [Fact] public void Authenticate_AcceptsCorrectPasswordAndRejectsWrongOne() { - string salt = "fixed-salt"; User user = new User { - PasswordSalt = salt, - PasswordHash = User.ComputePasswordHash("correct horse", salt) + PasswordHash = PasswordHasher.Hash("correct horse") }; Assert.True(user.Authenticate("correct horse")); diff --git a/tests/WaveBox.Server.Tests/TestPasswordWorkFactor.cs b/tests/WaveBox.Server.Tests/TestPasswordWorkFactor.cs new file mode 100644 index 0000000..24905f5 --- /dev/null +++ b/tests/WaveBox.Server.Tests/TestPasswordWorkFactor.cs @@ -0,0 +1,16 @@ +using System.Runtime.CompilerServices; +using WaveBox.Core.Static; + +namespace WaveBox.Server.Tests { + internal static class TestPasswordWorkFactor { + /// + /// Drops bcrypt to its minimum work factor for the duration of the test run. The production + /// value is tuned to take a noticeable fraction of a second per hash, and the integration + /// fixtures create a user per test method, so leaving it alone would dominate the run time. + /// + [ModuleInitializer] + internal static void Reduce() { + PasswordHasher.WorkFactor = 4; + } + } +} diff --git a/tests/WaveBox.Server.Tests/UserRepositoryTests.cs b/tests/WaveBox.Server.Tests/UserRepositoryTests.cs index 5434251..8fc1535 100644 --- a/tests/WaveBox.Server.Tests/UserRepositoryTests.cs +++ b/tests/WaveBox.Server.Tests/UserRepositoryTests.cs @@ -28,7 +28,6 @@ public void CreateUserRoundTripsThroughRepositoryAndAuthenticates() { Assert.NotNull(created.UserId); Assert.Equal("alice", created.UserName); Assert.NotNull(created.PasswordHash); - Assert.NotNull(created.PasswordSalt); Assert.NotNull(created.CreateTime); User fetched = users.UserForName("alice"); From dd14d2e97f0dd849e2c01b277f2f6ec61c6172b5 Mon Sep 17 00:00:00 2001 From: Ben Baron Date: Sun, 26 Jul 2026 19:36:33 -0500 Subject: [PATCH 5/6] Run SQL scripts through SQLite's parser instead of splitting on ';' 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) Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ --- WaveBox.Core/src/BaseClasses.cs | 6 + WaveBox.Core/src/SQLiteNet.cs | 90 +++++++++++ WaveBox.Server/res/wavebox.sql | 5 +- WaveBox.Server/src/Static/Database.cs | 30 +--- .../WaveBox.Server.Tests/SqliteScriptTests.cs | 140 ++++++++++++++++++ 5 files changed, 239 insertions(+), 32 deletions(-) create mode 100644 tests/WaveBox.Server.Tests/SqliteScriptTests.cs diff --git a/WaveBox.Core/src/BaseClasses.cs b/WaveBox.Core/src/BaseClasses.cs index 33c4529..2e67a64 100644 --- a/WaveBox.Core/src/BaseClasses.cs +++ b/WaveBox.Core/src/BaseClasses.cs @@ -107,6 +107,12 @@ public interface ISQLiteConnection : IDisposable { int Execute(string query, params object[] args); + /// + /// Executes every statement in a SQL script, for schema and migration files. Execute() + /// only ever runs the first statement it is given. + /// + void ExecuteScript(string script); + T ExecuteScalar(string query, params object[] args); List Query(string query, params object[] args) where T : new(); diff --git a/WaveBox.Core/src/SQLiteNet.cs b/WaveBox.Core/src/SQLiteNet.cs index 78d1b41..fa642a1 100644 --- a/WaveBox.Core/src/SQLiteNet.cs +++ b/WaveBox.Core/src/SQLiteNet.cs @@ -597,6 +597,15 @@ public ISQLiteCommand CreateCommand(string cmdText, params object[] ps) { /// /// The number of rows modified in the database as a result of this execution. /// + /// + /// WaveBox addition: executes every statement in a SQL script. Execute() above only ever + /// runs the first, since sqlite3_prepare_v2 compiles one statement at a time. + /// Takes no parameters -- this is for schema and migration scripts, not queries. + /// + public void ExecuteScript(string script) { + SQLite3.ExecuteScript(Handle, script); + } + public int Execute(string query, params object[] args) { var cmd = CreateCommand(query, args); @@ -2397,6 +2406,87 @@ public static IntPtr Prepare2(IntPtr db, string query) { return stmt; } + // WaveBox addition: the overload above discards pzTail, so it only ever compiles the + // first statement of a script. This one hands back the pointer to the remaining SQL, + // which is what lets ExecuteScript walk a multi-statement file. + [DllImport("e_sqlite3", EntryPoint = "sqlite3_prepare_v2", CallingConvention = CallingConvention.Cdecl)] + public static extern Result Prepare2(IntPtr db, IntPtr sql, int numBytes, + out IntPtr stmt, out IntPtr pzTail); + + /// + /// WaveBox addition: runs every statement in a SQL script. + /// + /// SQLite's parser does the tokenizing, so semicolons inside string literals, comments + /// and trigger bodies are all handled correctly -- unlike splitting the text on ';'. + /// The script is marshalled as an explicit UTF-8 buffer and walked by byte offset, + /// avoiding the ANSI marshalling and char-vs-byte length bugs in the string overload + /// of Prepare2 above. + /// + /// Transactions are the caller's business; scripts should not contain BEGIN or COMMIT. + /// + public static void ExecuteScript(IntPtr db, string script) { + if (String.IsNullOrEmpty(script)) { + return; + } + + byte[] utf8 = System.Text.Encoding.UTF8.GetBytes(script); + // sqlite3_prepare_v2 wants a NUL-terminated buffer when it reads past numBytes + Array.Resize(ref utf8, utf8.Length + 1); + + GCHandle pin = GCHandle.Alloc(utf8, GCHandleType.Pinned); + try { + IntPtr start = pin.AddrOfPinnedObject(); + int offset = 0; + + // utf8.Length - 1 excludes the NUL we appended + while (offset < utf8.Length - 1) { + IntPtr stmt; + IntPtr tail; + Result r = Prepare2(db, start + offset, utf8.Length - 1 - offset, out stmt, out tail); + + if (r != Result.OK) { + throw SQLiteException.New(r, GetErrmsg(db) + "\n" + RemainingText(utf8, offset)); + } + + int nextOffset = (int)(tail.ToInt64() - start.ToInt64()); + + // A trailing run of whitespace or comments compiles to no statement at all + if (stmt == IntPtr.Zero) { + if (nextOffset <= offset) { + break; + } + offset = nextOffset; + continue; + } + + try { + Result step = Step(stmt); + if (step != Result.Done && step != Result.Row) { + throw SQLiteException.New(step, GetErrmsg(db) + "\n" + StatementText(utf8, offset, nextOffset)); + } + } finally { + Finalize(stmt); + } + + if (nextOffset <= offset) { + break; + } + offset = nextOffset; + } + } finally { + pin.Free(); + } + } + + private static string StatementText(byte[] utf8, int offset, int end) { + return System.Text.Encoding.UTF8.GetString(utf8, offset, Math.Max(0, end - offset)).Trim(); + } + + private static string RemainingText(byte[] utf8, int offset) { + // On a compile failure there is no tail to bound the statement, so show what is left + return System.Text.Encoding.UTF8.GetString(utf8, offset, utf8.Length - 1 - offset).Trim(); + } + [DllImport("e_sqlite3", EntryPoint = "sqlite3_step", CallingConvention = CallingConvention.Cdecl)] public static extern Result Step(IntPtr stmt); diff --git a/WaveBox.Server/res/wavebox.sql b/WaveBox.Server/res/wavebox.sql index 678e9ee..df2b13f 100644 --- a/WaveBox.Server/res/wavebox.sql +++ b/WaveBox.Server/res/wavebox.sql @@ -1,6 +1,4 @@ -PRAGMA foreign_keys=OFF; -BEGIN TRANSACTION; CREATE TABLE ItemType ( "ItemTypeId" INTEGER PRIMARY KEY ASC AUTOINCREMENT, "Name" TEXT UNIQUE @@ -164,6 +162,8 @@ CREATE TABLE "Song" ( CREATE TABLE "Version" ( "VersionNumber" INTEGER NOT NULL ); +-- Schema baseline. This file is frozen; further changes go in res/migrations/. +INSERT INTO "Version" VALUES(0); CREATE TABLE "Favorite" ( "FavoriteId" INTEGER NOT NULL UNIQUE, "FavoriteUserId" INTEGER NOT NULL, @@ -209,4 +209,3 @@ CREATE INDEX "song_ItemId" ON "Song" ("ItemId"); CREATE INDEX "favorite_userId" ON "Favorite" ("FavoriteUserId"); CREATE UNIQUE INDEX "user_ApiKey" ON "User" ("ApiKey"); CREATE UNIQUE INDEX "album_AlbumNameArtistId" ON "Album" ("AlbumName", "AlbumArtistId"); -COMMIT; diff --git a/WaveBox.Server/src/Static/Database.cs b/WaveBox.Server/src/Static/Database.cs index 2be2bfc..9fd449b 100644 --- a/WaveBox.Server/src/Static/Database.cs +++ b/WaveBox.Server/src/Static/Database.cs @@ -78,9 +78,7 @@ private void ApplySchemaIfEmpty(string name, string schemaPath, Func - /// Splits a schema script into individual statements. The vendored sqlite-net only executes - /// one statement per call and exposes no sqlite3_exec, so the script has to be split here. - /// Naive splitting on ';' is safe only because these files are sqlite3 .dump output whose - /// string literals contain no semicolons -- do not point this at hand-written SQL. - /// - /// A .dump also wraps itself in BEGIN TRANSACTION/COMMIT; those are dropped so the caller - /// owns the transaction and a failure part way through rolls the whole thing back. - /// - private static IEnumerable ReadSchemaStatements(string schemaPath) { - foreach (string statement in File.ReadAllText(schemaPath).Split(';')) { - string trimmed = statement.Trim(); - if (trimmed.Length == 0 || IsTransactionControl(trimmed)) { - continue; - } - - yield return trimmed; - } - } - - private static bool IsTransactionControl(string statement) { - return statement.StartsWith("BEGIN", StringComparison.OrdinalIgnoreCase) - || statement.StartsWith("COMMIT", StringComparison.OrdinalIgnoreCase) - || statement.StartsWith("ROLLBACK", StringComparison.OrdinalIgnoreCase); - } - public ISQLiteConnection GetSqliteConnection() { if (isPoolingEnabled) { return mainPool.GetSqliteConnection(); diff --git a/tests/WaveBox.Server.Tests/SqliteScriptTests.cs b/tests/WaveBox.Server.Tests/SqliteScriptTests.cs new file mode 100644 index 0000000..6dcd234 --- /dev/null +++ b/tests/WaveBox.Server.Tests/SqliteScriptTests.cs @@ -0,0 +1,140 @@ +using System; +using System.IO; +using Cirrious.MvvmCross.Plugins.Sqlite; +using Xunit; + +namespace WaveBox.Server.Tests { + /// + /// ExecuteScript runs a whole SQL script through SQLite's own parser. Every case below except + /// the first breaks a splitter that cuts the text on ';', which is what this replaced. + /// + public class SqliteScriptTests : IDisposable { + private readonly string dbPath; + private readonly SQLite.SQLiteConnection conn; + + public SqliteScriptTests() { + dbPath = Path.Combine(Path.GetTempPath(), "wavebox-script-" + Guid.NewGuid().ToString("N") + ".db"); + conn = new SQLite.SQLiteConnection(dbPath); + } + + public void Dispose() { + conn.Dispose(); + File.Delete(dbPath); + } + + [Fact] + public void RunsEveryStatementNotJustTheFirst() { + conn.ExecuteScript(@" + CREATE TABLE A (x INTEGER); + CREATE TABLE B (y INTEGER); + INSERT INTO A VALUES (1); + INSERT INTO A VALUES (2); + "); + + Assert.Equal(1, conn.ExecuteScalar("SELECT COUNT(*) FROM sqlite_master WHERE name = 'B'")); + Assert.Equal(2, conn.ExecuteScalar("SELECT COUNT(*) FROM A")); + } + + [Fact] + public void PreservesSemicolonsInsideStringLiterals() { + conn.ExecuteScript(@" + CREATE TABLE T (v TEXT); + INSERT INTO T VALUES ('a;b;c'); + INSERT INTO T VALUES ('it''s; quoted'); + "); + + Assert.Equal("a;b;c", conn.ExecuteScalar("SELECT v FROM T ORDER BY rowid LIMIT 1")); + Assert.Equal("it's; quoted", conn.ExecuteScalar("SELECT v FROM T ORDER BY rowid DESC LIMIT 1")); + } + + [Fact] + public void IgnoresSemicolonsInLineAndBlockComments() { + conn.ExecuteScript(@" + -- a comment; with a semicolon + CREATE TABLE T (x INTEGER); + /* another; one + spanning lines; too */ + INSERT INTO T VALUES (42); + "); + + Assert.Equal(42, conn.ExecuteScalar("SELECT x FROM T")); + } + + [Fact] + public void HandlesTriggerBodiesContainingSemicolons() { + conn.ExecuteScript(@" + CREATE TABLE Src (x INTEGER); + CREATE TABLE Log (x INTEGER); + CREATE TRIGGER SrcInsert AFTER INSERT ON Src + BEGIN + INSERT INTO Log VALUES (NEW.x); + UPDATE Log SET x = x * 2; + END; + INSERT INTO Src VALUES (5); + "); + + Assert.Equal(10, conn.ExecuteScalar("SELECT x FROM Log")); + } + + [Fact] + public void RoundTripsNonAsciiText() { + // The script is marshalled as UTF-8 by byte length; the older string overload passed a + // character count as a byte count, which truncates as soon as the text is not ASCII + conn.ExecuteScript(@" + CREATE TABLE T (v TEXT); + INSERT INTO T VALUES ('Björk – Jóga ⟨æ⟩'); + "); + + Assert.Equal("Björk – Jóga ⟨æ⟩", conn.ExecuteScalar("SELECT v FROM T")); + } + + [Fact] + public void ToleratesTrailingWhitespaceAndComments() { + conn.ExecuteScript("CREATE TABLE T (x INTEGER);\n-- trailing comment\n \n"); + + Assert.Equal(1, conn.ExecuteScalar("SELECT COUNT(*) FROM sqlite_master WHERE name = 'T'")); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" \n ")] + [InlineData("-- nothing but a comment")] + public void EmptyScriptsAreNoOps(string script) { + conn.ExecuteScript(script); + + Assert.Equal(0, conn.ExecuteScalar("SELECT COUNT(*) FROM sqlite_master WHERE type = 'table'")); + } + + [Fact] + public void ReportsTheFailingStatement() { + SQLite.SQLiteException error = Assert.Throws(() => conn.ExecuteScript(@" + CREATE TABLE T (x INTEGER); + INSERT INTO NotATable VALUES (1); + ")); + + Assert.Contains("NotATable", error.Message); + + // The statements before the failure still ran; the caller owns the transaction + Assert.Equal(1, conn.ExecuteScalar("SELECT COUNT(*) FROM sqlite_master WHERE name = 'T'")); + } + + [Fact] + public void RollsBackWhenTheCallerOwnsATransaction() { + conn.ExecuteScript("CREATE TABLE Keep (x INTEGER);"); + + conn.BeginTransaction(); + try { + conn.ExecuteScript(@" + CREATE TABLE Dropped (x INTEGER); + INSERT INTO NotATable VALUES (1); + "); + } catch (SQLite.SQLiteException) { + conn.Rollback(); + } + + Assert.Equal(1, conn.ExecuteScalar("SELECT COUNT(*) FROM sqlite_master WHERE name = 'Keep'")); + Assert.Equal(0, conn.ExecuteScalar("SELECT COUNT(*) FROM sqlite_master WHERE name = 'Dropped'")); + } + } +} From cabe6017628c02516e53f610f2d49a5f5e3f12a2 Mon Sep 17 00:00:00 2001 From: Ben Baron Date: Sun, 26 Jul 2026 19:43:17 -0500 Subject: [PATCH 6/6] Add a versioned migration system for schema changes 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) Claude-Session: https://claude.ai/code/session_01TXVofbLdTr6YWA892F6pMQ --- .../src/Injection/Interfaces/IDatabase.cs | 2 + WaveBox.Server/WaveBox.Server.csproj | 3 + WaveBox.Server/res/migrations/README.md | 39 ++++ WaveBox.Server/src/Static/Database.cs | 21 ++ WaveBox.Server/src/Static/DatabaseMigrator.cs | 132 +++++++++++ WaveBox.Server/src/WaveBoxMain.cs | 12 +- .../DatabaseMigratorTests.cs | 216 ++++++++++++++++++ .../DatabaseSetupTests.cs | 21 ++ 8 files changed, 445 insertions(+), 1 deletion(-) create mode 100644 WaveBox.Server/res/migrations/README.md create mode 100644 WaveBox.Server/src/Static/DatabaseMigrator.cs create mode 100644 tests/WaveBox.Server.Tests/DatabaseMigratorTests.cs diff --git a/WaveBox.Core/src/Injection/Interfaces/IDatabase.cs b/WaveBox.Core/src/Injection/Interfaces/IDatabase.cs index 5fcc0fb..c0c2ce0 100644 --- a/WaveBox.Core/src/Injection/Interfaces/IDatabase.cs +++ b/WaveBox.Core/src/Injection/Interfaces/IDatabase.cs @@ -12,6 +12,8 @@ public interface IDatabase { string DatabaseSchemaPath { get; } string DatabasePath { get; } + string MigrationsPath { get; } + string QuerylogSchemaPath { get; } string QuerylogPath { get; } diff --git a/WaveBox.Server/WaveBox.Server.csproj b/WaveBox.Server/WaveBox.Server.csproj index b35130e..ba39a6b 100644 --- a/WaveBox.Server/WaveBox.Server.csproj +++ b/WaveBox.Server/WaveBox.Server.csproj @@ -42,6 +42,9 @@ + + diff --git a/WaveBox.Server/res/migrations/README.md b/WaveBox.Server/res/migrations/README.md new file mode 100644 index 0000000..3c94b9b --- /dev/null +++ b/WaveBox.Server/res/migrations/README.md @@ -0,0 +1,39 @@ +Database migrations +=================== + +Ordered SQL scripts that bring an existing `wavebox.db` up to the current schema. The server +applies any whose version is above the database's own on startup, then records the new version in +the single-row `Version` table. + +`../wavebox.sql` is a **frozen baseline at version 0**. Do not edit it to add or change a column — +write a migration instead. A fresh database is the baseline plus every migration replayed in order, +so migrations are exercised by every fresh install and every CI run rather than only being tried +for the first time against somebody's real database. + +Naming +------ + + 00001_add_user_nickname.sql + 00002_index_song_release_year.sql + +Five zero-padded digits, an underscore, then a short description of what the migration does. The +padding is so lexical and numeric order agree when you list the directory; the server sorts +numerically either way. Gaps are fine — two branches can merge out of order — but two files sharing +a version number is an error, and so is a name that doesn't match the pattern. Both fail at startup +rather than being skipped silently. + +Writing one +----------- + +* **No `BEGIN`, `COMMIT` or `ROLLBACK`.** The server wraps each migration in a transaction together + with its version bump, so either the whole file applies or none of it does. +* Multiple statements per file are fine. Scripts run through SQLite's own parser, so semicolons + inside string literals, comments and trigger bodies are handled correctly. +* Write them to be safe against a partially-set-up database where you can (`IF NOT EXISTS`), but + don't contort the SQL for it — a failed migration stops startup with the file name in the log. +* SQLite's `ALTER TABLE` is limited. Anything beyond adding a column or renaming means the usual + dance: create the new table, `INSERT INTO ... SELECT`, drop the old, rename. That is several + statements in one file, which is supported. + +Once a migration has shipped in a release, treat it as immutable — editing it will not re-run on +databases that already recorded its version. diff --git a/WaveBox.Server/src/Static/Database.cs b/WaveBox.Server/src/Static/Database.cs index 9fd449b..df8d471 100644 --- a/WaveBox.Server/src/Static/Database.cs +++ b/WaveBox.Server/src/Static/Database.cs @@ -19,6 +19,9 @@ public class Database : IDatabase { public string DatabaseSchemaPath { get { return ServerUtility.ExecutablePath() + "res" + Path.DirectorySeparatorChar + DATABASE_SCHEMA_FILE_NAME; } } public string DatabasePath { get { return ServerUtility.RootPath() + DATABASE_FILE_NAME; } } + private static readonly string MIGRATIONS_DIR_NAME = "migrations"; + public string MigrationsPath { get { return ServerUtility.ExecutablePath() + "res" + Path.DirectorySeparatorChar + MIGRATIONS_DIR_NAME; } } + private static readonly string QUERY_LOG_FILE_NAME = "wavebox_querylog.db"; private static readonly string QUERY_LOG_SCHEMA_FILE_NAME = "wavebox_querylog.sql"; public string QuerylogSchemaPath { get { return ServerUtility.ExecutablePath() + "res" + Path.DirectorySeparatorChar + QUERY_LOG_SCHEMA_FILE_NAME; } } @@ -57,9 +60,27 @@ public Database() { public void DatabaseSetup() { ApplySchemaIfEmpty(DATABASE_FILE_NAME, DatabaseSchemaPath, GetSqliteConnection, CloseSqliteConnection); + ApplyMigrations(); + + // The query log is a single table that has never changed, so it has no migrations ApplySchemaIfEmpty(QUERY_LOG_FILE_NAME, QuerylogSchemaPath, GetQueryLogSqliteConnection, CloseQueryLogSqliteConnection); } + private void ApplyMigrations() { + ISQLiteConnection conn = null; + try { + conn = GetSqliteConnection(); + DatabaseMigrator.Apply(conn, MigrationsPath); + } catch (Exception e) { + // Same reasoning as the schema apply: a half-migrated database only produces + // confusing errors later, so surface it now + logger.Error(e); + throw; + } finally { + CloseSqliteConnection(conn); + } + } + /// /// Creates a database from its bundled schema script if it has no tables yet. Checking for /// tables rather than for the file means an empty database left behind by an earlier diff --git a/WaveBox.Server/src/Static/DatabaseMigrator.cs b/WaveBox.Server/src/Static/DatabaseMigrator.cs new file mode 100644 index 0000000..cb8cc95 --- /dev/null +++ b/WaveBox.Server/src/Static/DatabaseMigrator.cs @@ -0,0 +1,132 @@ +using System; +using System.Collections.Generic; +using System.Globalization; +using System.IO; +using System.Text.RegularExpressions; +using Cirrious.MvvmCross.Plugins.Sqlite; +using WaveBox.Core.Extensions; + +namespace WaveBox.Static { + /// + /// Applies the ordered SQL migrations in res/migrations to a database, tracking how far it has + /// got in the single-row Version table. + /// + /// res/wavebox.sql is a frozen baseline at version 0; every schema change since is a migration, + /// so a fresh database is the seed plus every migration replayed in order. Keeping one path + /// means the migrations are exercised on every fresh install and every CI run, rather than + /// only ever being tried against a real database on someone's server. + /// + internal static class DatabaseMigrator { + private static readonly WaveBox.Core.Logging.ILog logger = WaveBox.Core.Logging.LogManager.GetLogger(); + + // 00001_add_something.sql -- zero padded so lexical and numeric order agree at a glance, + // though ordering below is numeric regardless + private static readonly Regex FileNamePattern = new Regex(@"^(\d{5})_.+\.sql$", RegexOptions.Compiled); + + internal sealed class Migration { + internal int Version { get; set; } + internal string Path { get; set; } + internal string Name { get; set; } + } + + /// + /// Reads the migrations directory, ordered by version. Throws on a malformed file name or a + /// duplicate version, so a typo fails loudly at startup instead of silently not running. + /// Gaps in numbering are fine -- branches merge out of order. + /// + internal static IList Discover(string directory) { + List migrations = new List(); + + if (!Directory.Exists(directory)) { + return migrations; + } + + Dictionary seen = new Dictionary(); + + foreach (string path in Directory.GetFiles(directory, "*.sql")) { + string name = System.IO.Path.GetFileName(path); + + Match match = FileNamePattern.Match(name); + if (!match.Success) { + throw new InvalidOperationException( + "Migration file name '" + name + "' is malformed; expected a 5 digit version, " + + "an underscore and a description, e.g. 00001_add_user_nickname.sql"); + } + + int version = Int32.Parse(match.Groups[1].Value, CultureInfo.InvariantCulture); + + string existing; + if (seen.TryGetValue(version, out existing)) { + throw new InvalidOperationException( + "Migrations '" + existing + "' and '" + name + "' share version " + version + + "; renumber one of them"); + } + seen[version] = name; + + migrations.Add(new Migration { Version = version, Path = path, Name = name }); + } + + migrations.Sort((a, b) => a.Version.CompareTo(b.Version)); + + return migrations; + } + + /// + /// Brings a database up to the newest bundled migration. Each migration and its version bump + /// share a transaction, so a failure part way through a chain leaves the database sitting + /// cleanly at the last migration that did succeed. + /// + internal static void Apply(ISQLiteConnection conn, string directory) { + IList migrations = Discover(directory); + + int current = CurrentVersion(conn); + int newest = migrations.Count > 0 ? migrations[migrations.Count - 1].Version : 0; + + if (current > newest) { + throw new InvalidOperationException( + "Database is at schema version " + current + " but this build only knows up to " + + newest + "; it was created by a newer version of WaveBox. Upgrade WaveBox, or " + + "delete the database to start fresh."); + } + + foreach (Migration migration in migrations) { + if (migration.Version <= current) { + continue; + } + + logger.IfInfo("Applying database migration " + migration.Name); + + conn.BeginTransaction(); + try { + conn.ExecuteScript(File.ReadAllText(migration.Path)); + SetVersion(conn, migration.Version); + conn.Commit(); + } catch (Exception e) { + conn.Rollback(); + logger.Error("Migration " + migration.Name + " failed; database left at version " + current, e); + throw; + } + + current = migration.Version; + } + } + + /// + /// The schema version, treating a missing row as 0. Databases created before versioning + /// existed have the table but no row in it. + /// + internal static int CurrentVersion(ISQLiteConnection conn) { + if (conn.ExecuteScalar("SELECT COUNT(*) FROM Version") == 0) { + return 0; + } + + return conn.ExecuteScalar("SELECT VersionNumber FROM Version LIMIT 1"); + } + + private static void SetVersion(ISQLiteConnection conn, int version) { + // Replace rather than update, so a database missing its row heals itself + conn.Execute("DELETE FROM Version"); + conn.Execute("INSERT INTO Version (VersionNumber) VALUES (?)", version); + } + } +} diff --git a/WaveBox.Server/src/WaveBoxMain.cs b/WaveBox.Server/src/WaveBoxMain.cs index f3f04e1..b22c05d 100644 --- a/WaveBox.Server/src/WaveBoxMain.cs +++ b/WaveBox.Server/src/WaveBoxMain.cs @@ -43,7 +43,17 @@ public void Start() { } // Perform initial setup of Settings, Database - Injection.Get().DatabaseSetup(); + try { + Injection.Get().DatabaseSetup(); + } catch (Exception e) { + // The database is unusable, so there is nothing to serve. Report it as a plain + // message rather than letting a stack trace be the whole story -- these failures + // are things an operator can act on, like a misnamed or broken migration. + logger.Error("Database setup failed: " + e.Message); + Console.Error.WriteLine("WaveBox cannot start: " + e.Message); + Environment.Exit(1); + } + Injection.Get().SettingsSetup(); // Start services diff --git a/tests/WaveBox.Server.Tests/DatabaseMigratorTests.cs b/tests/WaveBox.Server.Tests/DatabaseMigratorTests.cs new file mode 100644 index 0000000..858ce32 --- /dev/null +++ b/tests/WaveBox.Server.Tests/DatabaseMigratorTests.cs @@ -0,0 +1,216 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using Cirrious.MvvmCross.Plugins.Sqlite; +using WaveBox.Static; +using Xunit; + +namespace WaveBox.Server.Tests { + /// + /// Exercises the migrator directly against a throwaway database and migrations directory, so + /// none of this depends on the bundled res/migrations content (which is empty today). + /// + public class DatabaseMigratorTests : IDisposable { + private readonly string workDir; + private readonly string migrationsDir; + private readonly string dbPath; + private readonly SQLite.SQLiteConnection conn; + + public DatabaseMigratorTests() { + workDir = Directory.CreateTempSubdirectory("wavebox-migrations-").FullName; + migrationsDir = Path.Combine(workDir, "migrations"); + Directory.CreateDirectory(migrationsDir); + + dbPath = Path.Combine(workDir, "test.db"); + conn = new SQLite.SQLiteConnection(dbPath); + conn.ExecuteScript("CREATE TABLE Version (VersionNumber INTEGER NOT NULL); INSERT INTO Version VALUES (0);"); + } + + public void Dispose() { + conn.Dispose(); + try { + Directory.Delete(workDir, true); + } catch (IOException) { + } + } + + private void WriteMigration(string name, string sql) { + File.WriteAllText(Path.Combine(migrationsDir, name), sql); + } + + private bool ColumnExists(string table, string column) { + return conn.ExecuteScalar( + "SELECT COUNT(*) FROM pragma_table_info('" + table + "') WHERE name = '" + column + "'") > 0; + } + + // --- Discover ------------------------------------------------------- + + [Fact] + public void DiscoverOrdersNumericallyNotLexically() { + WriteMigration("00010_ten.sql", "SELECT 1;"); + WriteMigration("00002_two.sql", "SELECT 1;"); + WriteMigration("00001_one.sql", "SELECT 1;"); + + IList found = DatabaseMigrator.Discover(migrationsDir); + + Assert.Equal(new int[] { 1, 2, 10 }, found.Select(m => m.Version).ToArray()); + } + + [Fact] + public void DiscoverIgnoresNonSqlFiles() { + WriteMigration("00001_one.sql", "SELECT 1;"); + File.WriteAllText(Path.Combine(migrationsDir, "README.md"), "not a migration"); + File.WriteAllText(Path.Combine(migrationsDir, "notes.txt"), "also not"); + + Assert.Single(DatabaseMigrator.Discover(migrationsDir)); + } + + [Fact] + public void DiscoverReturnsEmptyForMissingDirectory() { + Assert.Empty(DatabaseMigrator.Discover(Path.Combine(workDir, "does-not-exist"))); + } + + [Theory] + [InlineData("1_short_version.sql")] + [InlineData("000001_six_digits.sql")] + [InlineData("00001-wrong-separator.sql")] + [InlineData("00001_.sql")] + [InlineData("no_version_at_all.sql")] + public void DiscoverThrowsOnMalformedFileName(string name) { + WriteMigration(name, "SELECT 1;"); + + InvalidOperationException error = Assert.Throws( + () => DatabaseMigrator.Discover(migrationsDir)); + + Assert.Contains(name, error.Message); + } + + [Fact] + public void DiscoverThrowsOnDuplicateVersion() { + WriteMigration("00001_one.sql", "SELECT 1;"); + WriteMigration("00001_one_again.sql", "SELECT 1;"); + + InvalidOperationException error = Assert.Throws( + () => DatabaseMigrator.Discover(migrationsDir)); + + Assert.Contains("share version 1", error.Message); + } + + [Fact] + public void DiscoverAllowsGapsInNumbering() { + WriteMigration("00001_one.sql", "SELECT 1;"); + WriteMigration("00007_seven.sql", "SELECT 1;"); + + Assert.Equal(new int[] { 1, 7 }, DatabaseMigrator.Discover(migrationsDir).Select(m => m.Version).ToArray()); + } + + // --- Apply ---------------------------------------------------------- + + [Fact] + public void ApplyRunsPendingMigrationsInOrderAndRecordsVersion() { + conn.ExecuteScript("CREATE TABLE T (a INTEGER);"); + WriteMigration("00001_add_b.sql", "ALTER TABLE T ADD COLUMN b INTEGER;"); + WriteMigration("00002_add_c.sql", "ALTER TABLE T ADD COLUMN c INTEGER;"); + + DatabaseMigrator.Apply(conn, migrationsDir); + + Assert.True(ColumnExists("T", "b")); + Assert.True(ColumnExists("T", "c")); + Assert.Equal(2, DatabaseMigrator.CurrentVersion(conn)); + } + + [Fact] + public void ApplySkipsMigrationsAtOrBelowCurrentVersion() { + conn.ExecuteScript("CREATE TABLE T (a INTEGER); DELETE FROM Version; INSERT INTO Version VALUES (1);"); + // Would fail if it ran, since column a already exists + WriteMigration("00001_already_applied.sql", "ALTER TABLE T ADD COLUMN a INTEGER;"); + WriteMigration("00002_add_b.sql", "ALTER TABLE T ADD COLUMN b INTEGER;"); + + DatabaseMigrator.Apply(conn, migrationsDir); + + Assert.True(ColumnExists("T", "b")); + Assert.Equal(2, DatabaseMigrator.CurrentVersion(conn)); + } + + [Fact] + public void ApplyIsANoOpWhenAlreadyCurrent() { + conn.ExecuteScript("CREATE TABLE T (a INTEGER);"); + WriteMigration("00001_add_b.sql", "ALTER TABLE T ADD COLUMN b INTEGER;"); + + DatabaseMigrator.Apply(conn, migrationsDir); + DatabaseMigrator.Apply(conn, migrationsDir); + + Assert.Equal(1, DatabaseMigrator.CurrentVersion(conn)); + } + + [Fact] + public void ApplyRollsBackAFailingMigrationAndStopsAtTheLastGoodVersion() { + conn.ExecuteScript("CREATE TABLE T (a INTEGER);"); + WriteMigration("00001_add_b.sql", "ALTER TABLE T ADD COLUMN b INTEGER;"); + WriteMigration("00002_broken.sql", "ALTER TABLE T ADD COLUMN c INTEGER;\nINSERT INTO NotATable VALUES (1);"); + WriteMigration("00003_add_d.sql", "ALTER TABLE T ADD COLUMN d INTEGER;"); + + Assert.ThrowsAny(() => DatabaseMigrator.Apply(conn, migrationsDir)); + + Assert.True(ColumnExists("T", "b")); + // 00002 is atomic, so its first statement is rolled back too, and 00003 never runs + Assert.False(ColumnExists("T", "c")); + Assert.False(ColumnExists("T", "d")); + Assert.Equal(1, DatabaseMigrator.CurrentVersion(conn)); + } + + [Fact] + public void ApplyThrowsWhenTheDatabaseIsNewerThanTheBuild() { + conn.ExecuteScript("DELETE FROM Version; INSERT INTO Version VALUES (9);"); + WriteMigration("00001_one.sql", "SELECT 1;"); + + InvalidOperationException error = Assert.Throws( + () => DatabaseMigrator.Apply(conn, migrationsDir)); + + Assert.Contains("newer version of WaveBox", error.Message); + } + + [Fact] + public void ApplyTreatsAMissingVersionRowAsZero() { + conn.ExecuteScript("CREATE TABLE T (a INTEGER); DELETE FROM Version;"); + WriteMigration("00001_add_b.sql", "ALTER TABLE T ADD COLUMN b INTEGER;"); + + Assert.Equal(0, DatabaseMigrator.CurrentVersion(conn)); + + DatabaseMigrator.Apply(conn, migrationsDir); + + Assert.True(ColumnExists("T", "b")); + Assert.Equal(1, DatabaseMigrator.CurrentVersion(conn)); + } + + [Fact] + public void ApplyKeepsASingleVersionRow() { + conn.ExecuteScript("CREATE TABLE T (a INTEGER);"); + WriteMigration("00001_add_b.sql", "ALTER TABLE T ADD COLUMN b INTEGER;"); + WriteMigration("00002_add_c.sql", "ALTER TABLE T ADD COLUMN c INTEGER;"); + + DatabaseMigrator.Apply(conn, migrationsDir); + + Assert.Equal(1, conn.ExecuteScalar("SELECT COUNT(*) FROM Version")); + } + + [Fact] + public void ApplyHandlesMultiStatementMigrations() { + // The SQLite table-rebuild dance, which is the realistic shape of a non-trivial migration + conn.ExecuteScript("CREATE TABLE T (a INTEGER, b TEXT); INSERT INTO T VALUES (1, 'keep;me');"); + WriteMigration("00001_rebuild.sql", @" + CREATE TABLE T_new (a INTEGER, b TEXT, c INTEGER DEFAULT 0); + INSERT INTO T_new (a, b) SELECT a, b FROM T; + DROP TABLE T; + ALTER TABLE T_new RENAME TO T; + "); + + DatabaseMigrator.Apply(conn, migrationsDir); + + Assert.True(ColumnExists("T", "c")); + Assert.Equal("keep;me", conn.ExecuteScalar("SELECT b FROM T")); + Assert.Equal(1, DatabaseMigrator.CurrentVersion(conn)); + } + } +} diff --git a/tests/WaveBox.Server.Tests/DatabaseSetupTests.cs b/tests/WaveBox.Server.Tests/DatabaseSetupTests.cs index c503ec7..b360ea9 100644 --- a/tests/WaveBox.Server.Tests/DatabaseSetupTests.cs +++ b/tests/WaveBox.Server.Tests/DatabaseSetupTests.cs @@ -83,6 +83,27 @@ public void SetupIsIdempotent() { Assert.True(File.Exists(Path.Combine(harness.Root.Path, "wavebox.db"))); } + [Fact] + public void SetupLeavesTheDatabaseAtTheNewestBundledMigration() { + // The seed is a frozen baseline at 0, so a fresh database ends up at whatever the + // highest migration in res/migrations is -- 0 while that directory is still empty + int expected = 0; + foreach (string path in Directory.GetFiles(Injection.Get().MigrationsPath, "*.sql")) { + expected = Math.Max(expected, Int32.Parse(Path.GetFileName(path).Substring(0, 5))); + } + + Assert.Equal(1, Scalar("SELECT COUNT(*) FROM Version")); + Assert.Equal(expected, Scalar("SELECT VersionNumber FROM Version")); + } + + [Fact] + public void SetupFindsTheBundledMigrationsDirectory() { + // Guards the csproj copy: if res/migrations stops being deployed next to the binary, + // migrations would silently never run rather than failing + Assert.True(Directory.Exists(Injection.Get().MigrationsPath), + "res/migrations was not copied to the output directory"); + } + [Fact] public void ApiKeyUniqueIndexRejectsDuplicateKeys() { IUserRepository users = Injection.Get();