From 7b66ceffc0bba0d5bf3e1ba770614c1485f20d8e Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Tue, 15 Sep 2026 13:07:14 +0000 Subject: [PATCH 1/9] Validate entry IDs at construction Make EntryID an opaque value constructed through NewEntryID so handlers, stores, and callers cannot pass malformed IDs. SQLite now converts IDs at SQL boundaries and rejects malformed persisted IDs while listing metadata. Tests cover EntryID validation and use valid constructor-created IDs. --- garbagecollect/collect_test.go | 48 +++++++++++++++++-------- handlers/delete.go | 4 ++- handlers/delete_test.go | 6 ++-- handlers/download.go | 6 ++-- handlers/download_test.go | 20 ++++++++--- handlers/guest_links_test.go | 2 +- handlers/upload.go | 48 +++++++++---------------- handlers/upload_test.go | 14 ++++---- handlers/views.go | 8 ++--- picoshare/entry_id_test.go | 60 ++++++++++++++++++++++++++++++++ picoshare/picoshare.go | 32 +++++++++++++++-- store/sqlite/downloads.go | 2 +- store/sqlite/entries.go | 19 ++++++---- store/sqlite/entries_test.go | 34 ++++++++++++------ store/sqlite/file/reader.go | 6 ++-- store/sqlite/file/writer.go | 2 +- store/sqlite/file/writer_test.go | 39 ++++++++++++++------- 17 files changed, 243 insertions(+), 107 deletions(-) create mode 100644 picoshare/entry_id_test.go diff --git a/garbagecollect/collect_test.go b/garbagecollect/collect_test.go index 681b5764..c9b30250 100644 --- a/garbagecollect/collect_test.go +++ b/garbagecollect/collect_test.go @@ -35,17 +35,22 @@ func TestCollectDoesNothingWhenStoreIsEmpty(t *testing.T) { func TestCollectExpiredFile(t *testing.T) { dataStore := test_sqlite.New(t) + aID := mustCreateEntryID(t, "AAAAAAAAAA") + bID := mustCreateEntryID(t, "BBBBBBBBBB") + cID := mustCreateEntryID(t, "CCCCCCCCCC") + dID := mustCreateEntryID(t, "DDDDDDDDDD") + eID := mustCreateEntryID(t, "EEEEEEEEEE") d := "dummy data" expireInFiveMins := mustParseExpirationTime("2025-01-01T00:05:00Z") dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: picoshare.EntryID("AAAAAAAAAAAA"), + ID: aID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("2024-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntryDownload( - picoshare.EntryID("AAAAAAAAAAAA"), + aID, picoshare.DownloadRecord{ Time: mustParseTime("2023-06-01T12:00:00Z"), ClientIP: "192.168.1.1", @@ -53,28 +58,28 @@ func TestCollectExpiredFile(t *testing.T) { }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: picoshare.EntryID("BBBBBBBBBBBB"), + ID: bID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("3000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: picoshare.EntryID("CCCCCCCCCCCC"), + ID: cID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: picoshare.NeverExpire, Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: picoshare.EntryID("DDDDDDDDDDDD"), + ID: dID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("2024-12-31T23:59:59Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: picoshare.EntryID("EEEEEEEEEEEE"), + ID: eID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: expireInFiveMins, Size: mustParseFileSize(len(d)), @@ -93,19 +98,19 @@ func TestCollectExpiredFile(t *testing.T) { expected := []picoshare.UploadMetadata{ { - ID: picoshare.EntryID("BBBBBBBBBBBB"), + ID: bID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("3000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }, { - ID: picoshare.EntryID("CCCCCCCCCCCC"), + ID: cID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: picoshare.NeverExpire, Size: mustParseFileSize(len(d)), }, { - ID: picoshare.EntryID("EEEEEEEEEEEE"), + ID: eID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: expireInFiveMins, Size: mustParseFileSize(len(d)), @@ -122,24 +127,27 @@ func TestCollectExpiredFile(t *testing.T) { func TestCollectDoesNothingWhenNoFilesAreExpired(t *testing.T) { dataStore := test_sqlite.New(t) + aID := mustCreateEntryID(t, "AAAAAAAAAA") + bID := mustCreateEntryID(t, "BBBBBBBBBB") + cID := mustCreateEntryID(t, "CCCCCCCCCC") d := "dummy data" dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: picoshare.EntryID("AAAAAAAAAAAA"), + ID: aID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("4000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: picoshare.EntryID("BBBBBBBBBBBB"), + ID: bID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("3000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: picoshare.EntryID("CCCCCCCCCCCC"), + ID: cID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: picoshare.NeverExpire, Size: mustParseFileSize(len(d)), @@ -163,19 +171,19 @@ func TestCollectDoesNothingWhenNoFilesAreExpired(t *testing.T) { expected := []picoshare.UploadMetadata{ { - ID: picoshare.EntryID("AAAAAAAAAAAA"), + ID: aID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("4000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }, { - ID: picoshare.EntryID("BBBBBBBBBBBB"), + ID: bID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("3000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }, { - ID: picoshare.EntryID("CCCCCCCCCCCC"), + ID: cID, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: picoshare.NeverExpire, Size: mustParseFileSize(len(d)), @@ -215,3 +223,13 @@ func mustParseFileSize(val int) picoshare.FileSize { return fileSize } + +func mustCreateEntryID(t *testing.T, raw string) picoshare.EntryID { + t.Helper() + + id, err := picoshare.NewEntryID(raw) + if err != nil { + t.Fatalf("failed to create entry ID %q: %v", raw, err) + } + return id +} diff --git a/handlers/delete.go b/handlers/delete.go index 6b84a76f..29ef11de 100644 --- a/handlers/delete.go +++ b/handlers/delete.go @@ -6,11 +6,13 @@ import ( "net/http" "github.com/gorilla/mux" + + "github.com/mtlynch/picoshare/picoshare" ) func (s Server) entryDelete() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) diff --git a/handlers/delete_test.go b/handlers/delete_test.go index d0a88754..7fb840c2 100644 --- a/handlers/delete_test.go +++ b/handlers/delete_test.go @@ -22,7 +22,7 @@ func TestDeleteExistingFile(t *testing.T) { fileContents := "dummy data" dataStore.InsertEntry(strings.NewReader(fileContents), picoshare.UploadMetadata{ - ID: picoshare.EntryID("hR87apiUCj"), + ID: mustCreateEntryID(t, "hR87apiUCj"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("2024-01-01T00:00:00Z"), Size: mustParseFileSize(len(fileContents)), @@ -40,9 +40,9 @@ func TestDeleteExistingFile(t *testing.T) { status, http.StatusOK) } - _, err := dataStore.GetEntryMetadata(picoshare.EntryID("hR87apiUCj")) + _, err := dataStore.GetEntryMetadata(mustCreateEntryID(t, "hR87apiUCj")) if _, ok := err.(store.EntryNotFoundError); !ok { - t.Fatalf("expected entry %v to be deleted", picoshare.EntryID("hR87apiUCj")) + t.Fatalf("expected entry %v to be deleted", mustCreateEntryID(t, "hR87apiUCj")) } } diff --git a/handlers/download.go b/handlers/download.go index 7c39c1e8..28347789 100644 --- a/handlers/download.go +++ b/handlers/download.go @@ -28,7 +28,7 @@ type entryUnlockRequest struct { func (s Server) entryGet() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -60,7 +60,7 @@ func (s Server) entryGet() http.HandlerFunc { func (s Server) entryUnlockGet() http.HandlerFunc { t := parseTemplates("templates/pages/entry-unlock.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -140,7 +140,7 @@ func (s Server) entryUnlockPost() http.HandlerFunc { } func parseEntryUnlockRequest(r *http.Request) (entryUnlockRequest, error) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { return entryUnlockRequest{}, err } diff --git a/handlers/download_test.go b/handlers/download_test.go index 560d277e..2186118a 100644 --- a/handlers/download_test.go +++ b/handlers/download_test.go @@ -14,7 +14,7 @@ import ( ) type mockEntry struct { - ID picoshare.EntryID + ID string Filename picoshare.Filename ContentType picoshare.ContentType } @@ -128,7 +128,7 @@ func TestEntryGet(t *testing.T) { data := "dummy data" entry := picoshare.UploadEntry{ UploadMetadata: picoshare.UploadMetadata{ - ID: mockEntry.ID, + ID: mustCreateEntryID(t, mockEntry.ID), Filename: mockEntry.Filename, ContentType: mockEntry.ContentType, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), @@ -176,7 +176,7 @@ func TestEntryGet(t *testing.T) { func TestProtectedEntryDownload(t *testing.T) { type fakeEntry struct { - ID picoshare.EntryID + ID string Contents string DownloadPassphrase picoshare.DownloadPassphrase } @@ -395,7 +395,7 @@ func TestProtectedEntryDownload(t *testing.T) { t.Run(tt.explanation, func(t *testing.T) { dataStore := test_sqlite.New(t) if err := dataStore.InsertEntry(strings.NewReader(tt.entryInStore.Contents), picoshare.UploadMetadata{ - ID: tt.entryInStore.ID, + ID: mustCreateEntryID(t, tt.entryInStore.ID), Filename: "test.txt", ContentType: "text/plain", Uploaded: mustParseTime("2023-01-01T00:00:00Z"), @@ -448,7 +448,7 @@ func TestProtectedEntryDownloadRequiresPassphraseEveryDownload(t *testing.T) { dataStore := test_sqlite.New(t) data := "protected file contents" if err := dataStore.InsertEntry(strings.NewReader(data), picoshare.UploadMetadata{ - ID: "PPPPPPPPPP", + ID: mustCreateEntryID(t, "PPPPPPPPPP"), Filename: "protected.txt", ContentType: "text/plain", Uploaded: mustParseTime("2023-01-01T00:00:00Z"), @@ -502,3 +502,13 @@ func mustCreateDownloadPassphrase(t *testing.T, value string) picoshare.Download return passphrase } + +func mustCreateEntryID(t *testing.T, raw string) picoshare.EntryID { + t.Helper() + + id, err := picoshare.NewEntryID(raw) + if err != nil { + t.Fatalf("failed to create entry ID %q: %v", raw, err) + } + return id +} diff --git a/handlers/guest_links_test.go b/handlers/guest_links_test.go index 7816c720..e34ac5fa 100644 --- a/handlers/guest_links_test.go +++ b/handlers/guest_links_test.go @@ -308,7 +308,7 @@ func TestDeleteExistingGuestLink(t *testing.T) { _, err := dataStore.GetGuestLink(picoshare.GuestLinkID("dummy-guest-link-id")) if _, ok := err.(store.GuestLinkNotFoundError); !ok { - t.Fatalf("expected entry %v to be deleted, got: %v", picoshare.EntryID("abcdefgh23456789"), err) + t.Fatalf("expected entry %v to be deleted, got: %v", "abcdefgh23456789", err) } } diff --git a/handlers/upload.go b/handlers/upload.go index cc27bfbb..530013af 100644 --- a/handlers/upload.go +++ b/handlers/upload.go @@ -16,8 +16,6 @@ import ( "github.com/mtlynch/picoshare/store" ) -const EntryIDLength = 10 - // Omit visually similar characters (I,l,1), (0,O) var entryIDCharacters = []rune("abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789") @@ -69,7 +67,7 @@ func (s Server) entryPost() http.HandlerFunc { func (s Server) entryPut() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -209,26 +207,12 @@ func (s Server) entryMetadataFromRequest(r *http.Request) (picoshare.UploadMetad } func generateEntryID() picoshare.EntryID { - return picoshare.EntryID(random.String(EntryIDLength, entryIDCharacters)) -} - -func parseEntryID(s string) (picoshare.EntryID, error) { - if len(s) != EntryIDLength { - return picoshare.EntryID(""), fmt.Errorf("entry ID (%v) has invalid length: got %d, want %d", s, len(s), EntryIDLength) - } - - // We could do this outside the function and store the result. - idCharsHash := map[rune]bool{} - for _, c := range entryIDCharacters { - idCharsHash[c] = true - } - - for _, c := range s { - if _, ok := idCharsHash[c]; !ok { - return picoshare.EntryID(""), fmt.Errorf("entry ID (%s) contains invalid character: %v", s, c) - } + raw := random.String(10, entryIDCharacters) + id, err := picoshare.NewEntryID(raw) + if err != nil { + panic(fmt.Sprintf("generated invalid entry ID: %v", err)) } - return picoshare.EntryID(s), nil + return id } func (s Server) insertFileFromRequest(r *http.Request, expiration picoshare.ExpirationTime, guestLinkID picoshare.GuestLinkID) (picoshare.EntryID, error) { @@ -236,7 +220,7 @@ func (s Server) insertFileFromRequest(r *http.Request, expiration picoshare.Expi // limit to avoid exhausting RAM on servers with limited resources. multipartMaxMemory := mibToBytes(1) if err := r.ParseMultipartForm(multipartMaxMemory); err != nil { - return picoshare.EntryID(""), err + return picoshare.EntryID{}, err } defer func() { if err := r.MultipartForm.RemoveAll(); err != nil { @@ -246,41 +230,41 @@ func (s Server) insertFileFromRequest(r *http.Request, expiration picoshare.Expi reader, metadata, err := r.FormFile("file") if err != nil { - return picoshare.EntryID(""), err + return picoshare.EntryID{}, err } fileSize, err := picoshare.FileSizeFromInt64(metadata.Size) if err != nil { - return picoshare.EntryID(""), err + return picoshare.EntryID{}, err } filename, err := parse.Filename(metadata.Filename) if err != nil { - return picoshare.EntryID(""), err + return picoshare.EntryID{}, err } contentType, err := parseContentType(metadata.Header.Get("Content-Type")) if err != nil { - return picoshare.EntryID(""), err + return picoshare.EntryID{}, err } note, err := parse.FileNote(r.FormValue("note")) if err != nil { - return picoshare.EntryID(""), err + return picoshare.EntryID{}, err } if guestLinkID != "" && note.Value != nil { - return picoshare.EntryID(""), errors.New("guest uploads cannot have file notes") + return picoshare.EntryID{}, errors.New("guest uploads cannot have file notes") } downloadPassphrase := picoshare.DownloadPassphrase{} if rawDownloadPassphrase := r.FormValue("downloadPassphrase"); rawDownloadPassphrase != "" { if guestLinkID != "" { - return picoshare.EntryID(""), errors.New("guest uploads cannot have download passphrases") + return picoshare.EntryID{}, errors.New("guest uploads cannot have download passphrases") } downloadPassphrase, err = picoshare.NewDownloadPassphrase(rawDownloadPassphrase) if err != nil { - return picoshare.EntryID(""), err + return picoshare.EntryID{}, err } } @@ -301,7 +285,7 @@ func (s Server) insertFileFromRequest(r *http.Request, expiration picoshare.Expi }) if err != nil { log.Printf("failed to save entry: %v", err) - return picoshare.EntryID(""), dbError{err} + return picoshare.EntryID{}, dbError{err} } return id, nil diff --git a/handlers/upload_test.go b/handlers/upload_test.go index 499ad7c8..4b92feff 100644 --- a/handlers/upload_test.go +++ b/handlers/upload_test.go @@ -146,7 +146,7 @@ func TestEntryPost(t *testing.T) { t.Fatalf("response is not valid JSON: %v", body) } - entry, err := dataStore.GetEntryMetadata(picoshare.EntryID(response.ID)) + entry, err := dataStore.GetEntryMetadata(mustCreateEntryID(t, response.ID)) if err != nil { t.Fatalf("failed to get expected entry %v from data store: %v", response.ID, err) } @@ -177,7 +177,7 @@ func TestEntryPost(t *testing.T) { func TestEntryPut(t *testing.T) { type fakeEntry struct { - ID picoshare.EntryID + ID string Filename picoshare.Filename Expires picoshare.ExpirationTime DownloadPassphrase string @@ -410,7 +410,7 @@ func TestEntryPut(t *testing.T) { dataStore := test_sqlite.New(t) originalData := "dummy original data" metadata := picoshare.UploadMetadata{ - ID: tt.entryInStore.ID, + ID: mustCreateEntryID(t, tt.entryInStore.ID), Filename: tt.entryInStore.Filename, ContentType: picoshare.ContentType("audio/mpeg"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), @@ -439,7 +439,7 @@ func TestEntryPut(t *testing.T) { t.Fatalf("status=%d, want=%d", got, want) } - entry, err := dataStore.GetEntryMetadata(tt.entryInStore.ID) + entry, err := dataStore.GetEntryMetadata(mustCreateEntryID(t, tt.entryInStore.ID)) if err != nil { t.Fatalf("failed to get expected entry %v from data store: %v", tt.entryInStore.ID, err) } @@ -596,7 +596,7 @@ func TestGuestUpload(t *testing.T) { entriesInStore: []picoshare.UploadEntry{ { UploadMetadata: picoshare.UploadMetadata{ - ID: picoshare.EntryID("dummy-entry1"), + ID: mustCreateEntryID(t, "AAAAAAAAAA"), Uploaded: mustParseTime("2024-02-01T00:00:00Z"), GuestLink: picoshare.GuestLink{ ID: picoshare.GuestLinkID("abcdefgh23456789"), @@ -606,7 +606,7 @@ func TestGuestUpload(t *testing.T) { }, { UploadMetadata: picoshare.UploadMetadata{ - ID: picoshare.EntryID("dummy-entry2"), + ID: mustCreateEntryID(t, "BBBBBBBBBB"), Uploaded: mustParseTime("2024-02-02T00:00:00Z"), GuestLink: picoshare.GuestLink{ ID: picoshare.GuestLinkID("abcdefgh23456789"), @@ -817,7 +817,7 @@ func TestGuestUpload(t *testing.T) { t.Fatalf("response is not valid JSON: %v", body) } - entry, err := dataStore.GetEntryMetadata(picoshare.EntryID(response.ID)) + entry, err := dataStore.GetEntryMetadata(mustCreateEntryID(t, response.ID)) if err != nil { t.Fatalf("failed to get expected entry %v from data store: %v", response.ID, err) } diff --git a/handlers/views.go b/handlers/views.go index 7c91fa06..5883d15a 100644 --- a/handlers/views.go +++ b/handlers/views.go @@ -215,7 +215,7 @@ func (s Server) fileEditGet() http.HandlerFunc { "templates/pages/file-edit.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -268,7 +268,7 @@ func (s Server) fileInfoGet() http.HandlerFunc { "templates/pages/file-info.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -316,7 +316,7 @@ func (s Server) fileDownloadsGet() http.HandlerFunc { t := parseTemplatesWithFuncs(fns, "templates/pages/file-downloads.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -394,7 +394,7 @@ func (s Server) fileConfirmDeleteGet() http.HandlerFunc { t := parseTemplates("templates/pages/file-delete.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := parseEntryID(mux.Vars(r)["id"]) + id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) diff --git a/picoshare/entry_id_test.go b/picoshare/entry_id_test.go new file mode 100644 index 00000000..3ff03594 --- /dev/null +++ b/picoshare/entry_id_test.go @@ -0,0 +1,60 @@ +package picoshare_test + +import ( + "testing" + + "github.com/mtlynch/picoshare/picoshare" +) + +func TestNewEntryID(t *testing.T) { + for _, tt := range []struct { + explanation string + input string + isValidExpected bool + }{ + { + explanation: "ten allowed characters are valid", + input: "aA23456789", + isValidExpected: true, + }, + { + explanation: "an ID shorter than ten characters is invalid", + input: "aA2345678", + isValidExpected: false, + }, + { + explanation: "an ID longer than ten characters is invalid", + input: "aA23456789a", + isValidExpected: false, + }, + { + explanation: "a visually ambiguous uppercase I is invalid", + input: "aA2345678I", + isValidExpected: false, + }, + { + explanation: "a visually ambiguous lowercase l is invalid", + input: "aA2345678l", + isValidExpected: false, + }, + { + explanation: "a Unicode character is invalid", + input: "aA2345678é", + isValidExpected: false, + }, + } { + t.Run(tt.explanation, func(t *testing.T) { + id, err := picoshare.NewEntryID(tt.input) + isValid := err == nil + if got, want := isValid, tt.isValidExpected; got != want { + t.Fatalf("NewEntryID validity=%t, want=%t", got, want) + } + if !isValid { + return + } + if got, want := id.String(), tt.input; got != want { + t.Errorf("ID=%q, want=%q", got, want) + } + }) + } +} diff --git a/picoshare/picoshare.go b/picoshare/picoshare.go index b7493503..3828f501 100644 --- a/picoshare/picoshare.go +++ b/picoshare/picoshare.go @@ -1,12 +1,13 @@ package picoshare import ( + "fmt" "io" + "strings" "time" ) type ( - EntryID string Filename string ContentType string ExpirationTime time.Time @@ -46,11 +47,38 @@ type ( } ) +// entryIDCharacters omits visually similar characters (I, l, 1), (0, O). +const ( + entryIDLength = 10 + entryIDCharacters = "abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789" +) + +// EntryID identifies an uploaded entry. +type EntryID struct { + value string +} + // Treat a distant expiration time as sort of a sentinel value signifying a "never expire" option. var NeverExpire = ExpirationTime(time.Date(2999, time.December, 31, 0, 0, 0, 0, time.UTC)) func (id EntryID) String() string { - return string(id) + return id.value +} + +// NewEntryID constructs an entry ID from user-provided text. +func NewEntryID(raw string) (EntryID, error) { + if len(raw) != entryIDLength { + return EntryID{}, fmt.Errorf( + "entry ID has invalid length: got %d, want %d", len(raw), entryIDLength) + } + + for _, character := range raw { + if !strings.ContainsRune(entryIDCharacters, character) { + return EntryID{}, fmt.Errorf("entry ID contains invalid character: %q", character) + } + } + + return EntryID{value: raw}, nil } func (f Filename) String() string { diff --git a/store/sqlite/downloads.go b/store/sqlite/downloads.go index 6dc49caa..0c51afa1 100644 --- a/store/sqlite/downloads.go +++ b/store/sqlite/downloads.go @@ -41,7 +41,7 @@ func (s Store) GetEntryDownloads(id picoshare.EntryID) ([]picoshare.DownloadReco WHERE entry_id=:entry_id ORDER BY - download_timestamp DESC`, sql.Named("entry_id", id)) + download_timestamp DESC`, sql.Named("entry_id", id.String())) if err == sql.ErrNoRows { return []picoshare.DownloadRecord{}, nil } else if err != nil { diff --git a/store/sqlite/entries.go b/store/sqlite/entries.go index 60fc2783..928c9c52 100644 --- a/store/sqlite/entries.go +++ b/store/sqlite/entries.go @@ -3,6 +3,7 @@ package sqlite import ( "context" "database/sql" + "fmt" "io" "log" @@ -49,6 +50,10 @@ func (s Store) GetEntriesMetadata() ([]picoshare.UploadMetadata, error) { if err = rows.Scan(&id, &filename, ¬e, &contentType, &uploadTimeRaw, &expirationTimeRaw, &fileSizeRaw); err != nil { return []picoshare.UploadMetadata{}, err } + entryID, err := picoshare.NewEntryID(id) + if err != nil { + return []picoshare.UploadMetadata{}, fmt.Errorf("failed to parse entry ID from database: %w", err) + } ut, err := parseDatetime(uploadTimeRaw) if err != nil { @@ -66,7 +71,7 @@ func (s Store) GetEntriesMetadata() ([]picoshare.UploadMetadata, error) { } ee = append(ee, picoshare.UploadMetadata{ - ID: picoshare.EntryID(id), + ID: entryID, Filename: picoshare.Filename(filename), Note: picoshare.FileNote{Value: note}, ContentType: picoshare.ContentType(contentType), @@ -120,7 +125,7 @@ func (s Store) GetEntryMetadata(id picoshare.EntryID) (picoshare.UploadMetadata, id ) sizes ON entries.id = sizes.id WHERE - entries.id = :entry_id`, sql.Named("entry_id", id)).Scan(&filename, ¬e, &contentType, &uploadTimeRaw, &expirationTimeRaw, &downloadPassphraseRaw, &fileSizeRaw, &guestLinkID) + entries.id = :entry_id`, sql.Named("entry_id", id.String())).Scan(&filename, ¬e, &contentType, &uploadTimeRaw, &expirationTimeRaw, &downloadPassphraseRaw, &fileSizeRaw, &guestLinkID) if err == sql.ErrNoRows { return picoshare.UploadMetadata{}, store.EntryNotFoundError{ID: id} } else if err != nil { @@ -198,7 +203,7 @@ func (s Store) InsertEntry(reader io.Reader, metadata picoshare.UploadMetadata) download_passphrase ) VALUES(:entry_id, NULLIF(:guest_link_id, ''), :filename, :note, :content_type, :upload_time, :expiration_time, :download_passphrase)`, - sql.Named("entry_id", metadata.ID), + sql.Named("entry_id", metadata.ID.String()), sql.Named("guest_link_id", metadata.GuestLink.ID), sql.Named("filename", metadata.Filename), sql.Named("note", metadata.Note.Value), @@ -246,7 +251,7 @@ func (s Store) UpdateEntryMetadata(id picoshare.EntryID, metadata picoshare.Uplo sql.Named("expiration_time", formatExpirationTime(metadata.Expires)), sql.Named("note", metadata.Note.Value), sql.Named("download_passphrase", downloadPassphraseString(metadata.DownloadPassphrase)), - sql.Named("entry_id", id)) + sql.Named("entry_id", id.String())) if err != nil { return err } @@ -280,7 +285,7 @@ func (s Store) DeleteEntry(id picoshare.EntryID) error { DELETE FROM downloads WHERE - entry_id = :entry_id`, sql.Named("entry_id", id)); err != nil { + entry_id = :entry_id`, sql.Named("entry_id", id.String())); err != nil { log.Printf("delete from downloads table failed, aborting transaction: %v", err) return err } @@ -289,7 +294,7 @@ func (s Store) DeleteEntry(id picoshare.EntryID) error { DELETE FROM entries_data WHERE - id = :entry_id`, sql.Named("entry_id", id)); err != nil { + id = :entry_id`, sql.Named("entry_id", id.String())); err != nil { log.Printf("delete from entries_data table failed, aborting transaction: %v", err) return err } @@ -298,7 +303,7 @@ func (s Store) DeleteEntry(id picoshare.EntryID) error { DELETE FROM entries WHERE - id = :entry_id`, sql.Named("entry_id", id)); err != nil { + id = :entry_id`, sql.Named("entry_id", id.String())); err != nil { log.Printf("delete from entries table failed, aborting transaction: %v", err) return err } diff --git a/store/sqlite/entries_test.go b/store/sqlite/entries_test.go index e87da9cd..29f29539 100644 --- a/store/sqlite/entries_test.go +++ b/store/sqlite/entries_test.go @@ -15,10 +15,11 @@ import ( func TestInsertDeleteSingleEntry(t *testing.T) { chunkSize := uint64(5) dataStore := test_sqlite.NewWithChunkSize(t, chunkSize) + entryID := mustCreateEntryID(t, "abcdefghij") input := "hello, world!" if err := dataStore.InsertEntry(bytes.NewBufferString(input), picoshare.UploadMetadata{ - ID: picoshare.EntryID("dummy-id"), + ID: entryID, Filename: "dummy-file.txt", Uploaded: mustParseTime("2025-05-25T00:00:00Z"), Expires: mustParseExpirationTime("2040-01-01T00:00:00Z"), @@ -27,7 +28,7 @@ func TestInsertDeleteSingleEntry(t *testing.T) { t.Fatalf("failed to insert file into sqlite: %v", err) } - entryFile, err := dataStore.ReadEntryFile("dummy-id") + entryFile, err := dataStore.ReadEntryFile(entryID) if err != nil { t.Fatalf("failed to get entry from DB: %v", err) } @@ -62,7 +63,7 @@ func TestInsertDeleteSingleEntry(t *testing.T) { t.Fatalf("filename=%s, want=%s", got, want) } - err = dataStore.DeleteEntry(picoshare.EntryID("dummy-id")) + err = dataStore.DeleteEntry(entryID) if err != nil { t.Fatalf("failed to delete entry: %v", err) } @@ -80,10 +81,11 @@ func TestInsertDeleteSingleEntry(t *testing.T) { func TestReadLastByteOfEntry(t *testing.T) { chunkSize := uint64(5) db := test_sqlite.NewWithChunkSize(t, chunkSize) + entryID := mustCreateEntryID(t, "abcdefghij") input := "hello, world!" if err := db.InsertEntry(bytes.NewBufferString(input), picoshare.UploadMetadata{ - ID: picoshare.EntryID("dummy-id"), + ID: entryID, Filename: "dummy-file.txt", Uploaded: mustParseTime("2025-05-25T00:00:00Z"), Expires: mustParseExpirationTime("2040-01-01T00:00:00Z"), @@ -92,7 +94,7 @@ func TestReadLastByteOfEntry(t *testing.T) { t.Fatalf("failed to insert file into sqlite: %v", err) } - entryFile, err := db.ReadEntryFile(picoshare.EntryID("dummy-id")) + entryFile, err := db.ReadEntryFile(entryID) if err != nil { t.Fatalf("failed to read entry: %v", err) } @@ -119,6 +121,7 @@ func TestReadLastByteOfEntry(t *testing.T) { func TestUpdateEntryMetadata(t *testing.T) { dataStore := test_sqlite.New(t) + entryID := mustCreateEntryID(t, "abcdefghij") passphrase, err := picoshare.NewDownloadPassphrase("correct horse battery staple") if err != nil { t.Fatalf("failed to create download passphrase: %v", err) @@ -126,7 +129,7 @@ func TestUpdateEntryMetadata(t *testing.T) { data := "dummy data" if err := dataStore.InsertEntry(strings.NewReader(data), picoshare.UploadMetadata{ - ID: picoshare.EntryID("dummy-id"), + ID: entryID, Filename: "dummy-file.txt", Uploaded: mustParseTime("2025-05-25T00:00:00Z"), Expires: mustParseExpirationTime("2040-01-01T00:00:00Z"), @@ -136,7 +139,7 @@ func TestUpdateEntryMetadata(t *testing.T) { t.Fatalf("failed to insert file into sqlite: %v", err) } - metadata, err := dataStore.GetEntryMetadata("dummy-id") + metadata, err := dataStore.GetEntryMetadata(entryID) if err != nil { t.Fatalf("failed to retrieve entry metadata: %v", err) } @@ -144,7 +147,7 @@ func TestUpdateEntryMetadata(t *testing.T) { t.Errorf("download passphrase=%q, want=%q", got, want) } - if err := dataStore.UpdateEntryMetadata("dummy-id", picoshare.UploadMetadata{ + if err := dataStore.UpdateEntryMetadata(entryID, picoshare.UploadMetadata{ Filename: "renamed-file.txt", Expires: mustParseExpirationTime("2041-01-01T00:00:00Z"), Note: picoshare.FileNote{Value: new("updated note")}, @@ -153,7 +156,7 @@ func TestUpdateEntryMetadata(t *testing.T) { t.Fatalf("failed to update entry metadata: %v", err) } - metadata, err = dataStore.GetEntryMetadata("dummy-id") + metadata, err = dataStore.GetEntryMetadata(entryID) if err != nil { t.Fatalf("failed to retrieve entry metadata: %v", err) } @@ -176,6 +179,7 @@ func TestUpdateEntryMetadata(t *testing.T) { // leaves them out. func TestGetEntriesMetadataOmitsDownloadPassphrase(t *testing.T) { dataStore := test_sqlite.New(t) + entryID := mustCreateEntryID(t, "abcdefghij") passphrase, err := picoshare.NewDownloadPassphrase("correct horse battery staple") if err != nil { t.Fatalf("failed to create download passphrase: %v", err) @@ -183,7 +187,7 @@ func TestGetEntriesMetadataOmitsDownloadPassphrase(t *testing.T) { data := "dummy data" if err := dataStore.InsertEntry(strings.NewReader(data), picoshare.UploadMetadata{ - ID: picoshare.EntryID("dummy-id"), + ID: entryID, Filename: "dummy-file.txt", Uploaded: mustParseTime("2025-05-25T00:00:00Z"), Expires: mustParseExpirationTime("2040-01-01T00:00:00Z"), @@ -229,3 +233,13 @@ func mustParseFileSize(val int) picoshare.FileSize { return fileSize } + +func mustCreateEntryID(t *testing.T, raw string) picoshare.EntryID { + t.Helper() + + id, err := picoshare.NewEntryID(raw) + if err != nil { + t.Fatalf("failed to create entry ID %q: %v", raw, err) + } + return id +} diff --git a/store/sqlite/file/reader.go b/store/sqlite/file/reader.go index ecbd4975..e5152133 100644 --- a/store/sqlite/file/reader.go +++ b/store/sqlite/file/reader.go @@ -99,7 +99,7 @@ func (fr *fileReader) populateBuffer() error { chunk_index=? ORDER BY chunk_index ASC - `, fr.entryID, chunkIndex).Scan(&chunk); err != nil { + `, fr.entryID.String(), chunkIndex).Scan(&chunk); err != nil { log.Printf("reading chunk failed: %v", err) return err } @@ -127,7 +127,7 @@ func getFileLength(db *sql.DB, id picoshare.EntryID, chunkSize int64) (int64, er ORDER BY chunk_index DESC LIMIT 1 - `, id).Scan(&chunkIndex, &chunkLen); err != nil { + `, id.String()).Scan(&chunkIndex, &chunkLen); err != nil { return 0, err } @@ -150,7 +150,7 @@ func getChunkSize(db *sql.DB, id picoshare.EntryID) (int64, error) { chunk_index ASC LIMIT 1 `, - sql.Named("id", id), + sql.Named("id", id.String()), ).Scan(&chunkSize); err != nil { return 0, err } diff --git a/store/sqlite/file/writer.go b/store/sqlite/file/writer.go index 752cf401..7e5a6ec5 100644 --- a/store/sqlite/file/writer.go +++ b/store/sqlite/file/writer.go @@ -73,7 +73,7 @@ func (w *writer) flush(n int) error { chunk_index, chunk ) - VALUES(?,?,?)`, w.entryID, idx, w.buf[0:n]) + VALUES(?,?,?)`, w.entryID.String(), idx, w.buf[0:n]) return err } diff --git a/store/sqlite/file/writer_test.go b/store/sqlite/file/writer_test.go index 1c019276..49543c87 100644 --- a/store/sqlite/file/writer_test.go +++ b/store/sqlite/file/writer_test.go @@ -26,11 +26,15 @@ type ( var errMockSqlFailure = errors.New("fake SQL error") func (db *mockSqlDB) Exec(query string, args ...any) (sql.Result, error) { + id, err := picoshare.NewEntryID(args[0].(string)) + if err != nil { + return nil, err + } chunk := args[2].([]byte) chunkCopy := make([]byte, len(chunk)) copy(chunkCopy, chunk) db.rows = append(db.rows, mockChunkRow{ - id: args[0].(picoshare.EntryID), + id: id, chunkIndex: args[1].(int), chunk: chunkCopy, }) @@ -38,6 +42,7 @@ func (db *mockSqlDB) Exec(query string, args ...any) (sql.Result, error) { } func TestWriteFile(t *testing.T) { + entryID := mustCreateEntryID(t, "abcdefghij") for _, tt := range []struct { explanation string id picoshare.EntryID @@ -49,12 +54,12 @@ func TestWriteFile(t *testing.T) { }{ { explanation: "data is smaller than chunk size", - id: picoshare.EntryID("dummy-id"), + id: entryID, data: []byte("hello, world!"), chunkSize: 25, rowsExpected: []mockChunkRow{ { - id: picoshare.EntryID("dummy-id"), + id: entryID, chunkIndex: 0, chunk: []byte("hello, world!"), }, @@ -62,12 +67,12 @@ func TestWriteFile(t *testing.T) { }, { explanation: "data fits exactly in single chunk", - id: picoshare.EntryID("dummy-id"), + id: entryID, data: []byte("01234"), chunkSize: 5, rowsExpected: []mockChunkRow{ { - id: picoshare.EntryID("dummy-id"), + id: entryID, chunkIndex: 0, chunk: []byte("01234"), }, @@ -75,17 +80,17 @@ func TestWriteFile(t *testing.T) { }, { explanation: "data occupies a partial chunk after the first", - id: picoshare.EntryID("dummy-id"), + id: entryID, data: []byte("0123456"), chunkSize: 5, rowsExpected: []mockChunkRow{ { - id: picoshare.EntryID("dummy-id"), + id: entryID, chunkIndex: 0, chunk: []byte("01234"), }, { - id: picoshare.EntryID("dummy-id"), + id: entryID, chunkIndex: 1, chunk: []byte("56"), }, @@ -93,17 +98,17 @@ func TestWriteFile(t *testing.T) { }, { explanation: "data spans exactly two chunks", - id: picoshare.EntryID("dummy-id"), + id: entryID, data: []byte("0123456789"), chunkSize: 5, rowsExpected: []mockChunkRow{ { - id: picoshare.EntryID("dummy-id"), + id: entryID, chunkIndex: 0, chunk: []byte("01234"), }, { - id: picoshare.EntryID("dummy-id"), + id: entryID, chunkIndex: 1, chunk: []byte("56789"), }, @@ -111,7 +116,7 @@ func TestWriteFile(t *testing.T) { }, { explanation: "write fails when SQL transaction returns error", - id: picoshare.EntryID("dummy-id"), + id: entryID, data: []byte("0123456789"), chunkSize: 5, sqlExecErr: errMockSqlFailure, @@ -146,3 +151,13 @@ func TestWriteFile(t *testing.T) { }) } } + +func mustCreateEntryID(t *testing.T, raw string) picoshare.EntryID { + t.Helper() + + id, err := picoshare.NewEntryID(raw) + if err != nil { + t.Fatalf("failed to create entry ID %q: %v", raw, err) + } + return id +} From 269b83bd3103e8f6baac334d9edf5571e3f238ea Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Tue, 15 Sep 2026 13:14:57 +0000 Subject: [PATCH 2/9] Share EntryID test constructor Add MustCreateEntryID beside NewEntryID and use it in all test packages. Inline garbage-collection test IDs to avoid unnecessary temporary variables. --- garbagecollect/collect_test.go | 48 ++++++++++---------------------- handlers/delete_test.go | 6 ++-- handlers/download_test.go | 16 ++--------- handlers/upload_test.go | 12 ++++---- picoshare/picoshare.go | 9 ++++++ store/sqlite/entries_test.go | 18 +++--------- store/sqlite/file/writer_test.go | 12 +------- 7 files changed, 41 insertions(+), 80 deletions(-) diff --git a/garbagecollect/collect_test.go b/garbagecollect/collect_test.go index c9b30250..a8605abc 100644 --- a/garbagecollect/collect_test.go +++ b/garbagecollect/collect_test.go @@ -35,22 +35,17 @@ func TestCollectDoesNothingWhenStoreIsEmpty(t *testing.T) { func TestCollectExpiredFile(t *testing.T) { dataStore := test_sqlite.New(t) - aID := mustCreateEntryID(t, "AAAAAAAAAA") - bID := mustCreateEntryID(t, "BBBBBBBBBB") - cID := mustCreateEntryID(t, "CCCCCCCCCC") - dID := mustCreateEntryID(t, "DDDDDDDDDD") - eID := mustCreateEntryID(t, "EEEEEEEEEE") d := "dummy data" expireInFiveMins := mustParseExpirationTime("2025-01-01T00:05:00Z") dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: aID, + ID: picoshare.MustCreateEntryID("AAAAAAAAAA"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("2024-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntryDownload( - aID, + picoshare.MustCreateEntryID("AAAAAAAAAA"), picoshare.DownloadRecord{ Time: mustParseTime("2023-06-01T12:00:00Z"), ClientIP: "192.168.1.1", @@ -58,28 +53,28 @@ func TestCollectExpiredFile(t *testing.T) { }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: bID, + ID: picoshare.MustCreateEntryID("BBBBBBBBBB"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("3000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: cID, + ID: picoshare.MustCreateEntryID("CCCCCCCCCC"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: picoshare.NeverExpire, Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: dID, + ID: picoshare.MustCreateEntryID("DDDDDDDDDD"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("2024-12-31T23:59:59Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: eID, + ID: picoshare.MustCreateEntryID("EEEEEEEEEE"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: expireInFiveMins, Size: mustParseFileSize(len(d)), @@ -98,19 +93,19 @@ func TestCollectExpiredFile(t *testing.T) { expected := []picoshare.UploadMetadata{ { - ID: bID, + ID: picoshare.MustCreateEntryID("BBBBBBBBBB"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("3000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }, { - ID: cID, + ID: picoshare.MustCreateEntryID("CCCCCCCCCC"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: picoshare.NeverExpire, Size: mustParseFileSize(len(d)), }, { - ID: eID, + ID: picoshare.MustCreateEntryID("EEEEEEEEEE"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: expireInFiveMins, Size: mustParseFileSize(len(d)), @@ -127,27 +122,24 @@ func TestCollectExpiredFile(t *testing.T) { func TestCollectDoesNothingWhenNoFilesAreExpired(t *testing.T) { dataStore := test_sqlite.New(t) - aID := mustCreateEntryID(t, "AAAAAAAAAA") - bID := mustCreateEntryID(t, "BBBBBBBBBB") - cID := mustCreateEntryID(t, "CCCCCCCCCC") d := "dummy data" dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: aID, + ID: picoshare.MustCreateEntryID("AAAAAAAAAA"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("4000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: bID, + ID: picoshare.MustCreateEntryID("BBBBBBBBBB"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("3000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }) dataStore.InsertEntry(strings.NewReader(d), picoshare.UploadMetadata{ - ID: cID, + ID: picoshare.MustCreateEntryID("CCCCCCCCCC"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: picoshare.NeverExpire, Size: mustParseFileSize(len(d)), @@ -171,19 +163,19 @@ func TestCollectDoesNothingWhenNoFilesAreExpired(t *testing.T) { expected := []picoshare.UploadMetadata{ { - ID: aID, + ID: picoshare.MustCreateEntryID("AAAAAAAAAA"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("4000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }, { - ID: bID, + ID: picoshare.MustCreateEntryID("BBBBBBBBBB"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("3000-01-01T00:00:00Z"), Size: mustParseFileSize(len(d)), }, { - ID: cID, + ID: picoshare.MustCreateEntryID("CCCCCCCCCC"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: picoshare.NeverExpire, Size: mustParseFileSize(len(d)), @@ -223,13 +215,3 @@ func mustParseFileSize(val int) picoshare.FileSize { return fileSize } - -func mustCreateEntryID(t *testing.T, raw string) picoshare.EntryID { - t.Helper() - - id, err := picoshare.NewEntryID(raw) - if err != nil { - t.Fatalf("failed to create entry ID %q: %v", raw, err) - } - return id -} diff --git a/handlers/delete_test.go b/handlers/delete_test.go index 7fb840c2..448aaca1 100644 --- a/handlers/delete_test.go +++ b/handlers/delete_test.go @@ -22,7 +22,7 @@ func TestDeleteExistingFile(t *testing.T) { fileContents := "dummy data" dataStore.InsertEntry(strings.NewReader(fileContents), picoshare.UploadMetadata{ - ID: mustCreateEntryID(t, "hR87apiUCj"), + ID: picoshare.MustCreateEntryID("hR87apiUCj"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), Expires: mustParseExpirationTime("2024-01-01T00:00:00Z"), Size: mustParseFileSize(len(fileContents)), @@ -40,9 +40,9 @@ func TestDeleteExistingFile(t *testing.T) { status, http.StatusOK) } - _, err := dataStore.GetEntryMetadata(mustCreateEntryID(t, "hR87apiUCj")) + _, err := dataStore.GetEntryMetadata(picoshare.MustCreateEntryID("hR87apiUCj")) if _, ok := err.(store.EntryNotFoundError); !ok { - t.Fatalf("expected entry %v to be deleted", mustCreateEntryID(t, "hR87apiUCj")) + t.Fatalf("expected entry %v to be deleted", picoshare.MustCreateEntryID("hR87apiUCj")) } } diff --git a/handlers/download_test.go b/handlers/download_test.go index 2186118a..23f9dc68 100644 --- a/handlers/download_test.go +++ b/handlers/download_test.go @@ -128,7 +128,7 @@ func TestEntryGet(t *testing.T) { data := "dummy data" entry := picoshare.UploadEntry{ UploadMetadata: picoshare.UploadMetadata{ - ID: mustCreateEntryID(t, mockEntry.ID), + ID: picoshare.MustCreateEntryID(mockEntry.ID), Filename: mockEntry.Filename, ContentType: mockEntry.ContentType, Uploaded: mustParseTime("2023-01-01T00:00:00Z"), @@ -395,7 +395,7 @@ func TestProtectedEntryDownload(t *testing.T) { t.Run(tt.explanation, func(t *testing.T) { dataStore := test_sqlite.New(t) if err := dataStore.InsertEntry(strings.NewReader(tt.entryInStore.Contents), picoshare.UploadMetadata{ - ID: mustCreateEntryID(t, tt.entryInStore.ID), + ID: picoshare.MustCreateEntryID(tt.entryInStore.ID), Filename: "test.txt", ContentType: "text/plain", Uploaded: mustParseTime("2023-01-01T00:00:00Z"), @@ -448,7 +448,7 @@ func TestProtectedEntryDownloadRequiresPassphraseEveryDownload(t *testing.T) { dataStore := test_sqlite.New(t) data := "protected file contents" if err := dataStore.InsertEntry(strings.NewReader(data), picoshare.UploadMetadata{ - ID: mustCreateEntryID(t, "PPPPPPPPPP"), + ID: picoshare.MustCreateEntryID("PPPPPPPPPP"), Filename: "protected.txt", ContentType: "text/plain", Uploaded: mustParseTime("2023-01-01T00:00:00Z"), @@ -502,13 +502,3 @@ func mustCreateDownloadPassphrase(t *testing.T, value string) picoshare.Download return passphrase } - -func mustCreateEntryID(t *testing.T, raw string) picoshare.EntryID { - t.Helper() - - id, err := picoshare.NewEntryID(raw) - if err != nil { - t.Fatalf("failed to create entry ID %q: %v", raw, err) - } - return id -} diff --git a/handlers/upload_test.go b/handlers/upload_test.go index 4b92feff..f3a6bd16 100644 --- a/handlers/upload_test.go +++ b/handlers/upload_test.go @@ -146,7 +146,7 @@ func TestEntryPost(t *testing.T) { t.Fatalf("response is not valid JSON: %v", body) } - entry, err := dataStore.GetEntryMetadata(mustCreateEntryID(t, response.ID)) + entry, err := dataStore.GetEntryMetadata(picoshare.MustCreateEntryID(response.ID)) if err != nil { t.Fatalf("failed to get expected entry %v from data store: %v", response.ID, err) } @@ -410,7 +410,7 @@ func TestEntryPut(t *testing.T) { dataStore := test_sqlite.New(t) originalData := "dummy original data" metadata := picoshare.UploadMetadata{ - ID: mustCreateEntryID(t, tt.entryInStore.ID), + ID: picoshare.MustCreateEntryID(tt.entryInStore.ID), Filename: tt.entryInStore.Filename, ContentType: picoshare.ContentType("audio/mpeg"), Uploaded: mustParseTime("2023-01-01T00:00:00Z"), @@ -439,7 +439,7 @@ func TestEntryPut(t *testing.T) { t.Fatalf("status=%d, want=%d", got, want) } - entry, err := dataStore.GetEntryMetadata(mustCreateEntryID(t, tt.entryInStore.ID)) + entry, err := dataStore.GetEntryMetadata(picoshare.MustCreateEntryID(tt.entryInStore.ID)) if err != nil { t.Fatalf("failed to get expected entry %v from data store: %v", tt.entryInStore.ID, err) } @@ -596,7 +596,7 @@ func TestGuestUpload(t *testing.T) { entriesInStore: []picoshare.UploadEntry{ { UploadMetadata: picoshare.UploadMetadata{ - ID: mustCreateEntryID(t, "AAAAAAAAAA"), + ID: picoshare.MustCreateEntryID("AAAAAAAAAA"), Uploaded: mustParseTime("2024-02-01T00:00:00Z"), GuestLink: picoshare.GuestLink{ ID: picoshare.GuestLinkID("abcdefgh23456789"), @@ -606,7 +606,7 @@ func TestGuestUpload(t *testing.T) { }, { UploadMetadata: picoshare.UploadMetadata{ - ID: mustCreateEntryID(t, "BBBBBBBBBB"), + ID: picoshare.MustCreateEntryID("BBBBBBBBBB"), Uploaded: mustParseTime("2024-02-02T00:00:00Z"), GuestLink: picoshare.GuestLink{ ID: picoshare.GuestLinkID("abcdefgh23456789"), @@ -817,7 +817,7 @@ func TestGuestUpload(t *testing.T) { t.Fatalf("response is not valid JSON: %v", body) } - entry, err := dataStore.GetEntryMetadata(mustCreateEntryID(t, response.ID)) + entry, err := dataStore.GetEntryMetadata(picoshare.MustCreateEntryID(response.ID)) if err != nil { t.Fatalf("failed to get expected entry %v from data store: %v", response.ID, err) } diff --git a/picoshare/picoshare.go b/picoshare/picoshare.go index 3828f501..ff92f5f5 100644 --- a/picoshare/picoshare.go +++ b/picoshare/picoshare.go @@ -81,6 +81,15 @@ func NewEntryID(raw string) (EntryID, error) { return EntryID{value: raw}, nil } +// MustCreateEntryID constructs an entry ID or panics when raw is invalid. +func MustCreateEntryID(raw string) EntryID { + id, err := NewEntryID(raw) + if err != nil { + panic(fmt.Sprintf("failed to create entry ID: %v", err)) + } + return id +} + func (f Filename) String() string { return string(f) } diff --git a/store/sqlite/entries_test.go b/store/sqlite/entries_test.go index 29f29539..c71b051b 100644 --- a/store/sqlite/entries_test.go +++ b/store/sqlite/entries_test.go @@ -15,7 +15,7 @@ import ( func TestInsertDeleteSingleEntry(t *testing.T) { chunkSize := uint64(5) dataStore := test_sqlite.NewWithChunkSize(t, chunkSize) - entryID := mustCreateEntryID(t, "abcdefghij") + entryID := picoshare.MustCreateEntryID("abcdefghij") input := "hello, world!" if err := dataStore.InsertEntry(bytes.NewBufferString(input), picoshare.UploadMetadata{ @@ -81,7 +81,7 @@ func TestInsertDeleteSingleEntry(t *testing.T) { func TestReadLastByteOfEntry(t *testing.T) { chunkSize := uint64(5) db := test_sqlite.NewWithChunkSize(t, chunkSize) - entryID := mustCreateEntryID(t, "abcdefghij") + entryID := picoshare.MustCreateEntryID("abcdefghij") input := "hello, world!" if err := db.InsertEntry(bytes.NewBufferString(input), picoshare.UploadMetadata{ @@ -121,7 +121,7 @@ func TestReadLastByteOfEntry(t *testing.T) { func TestUpdateEntryMetadata(t *testing.T) { dataStore := test_sqlite.New(t) - entryID := mustCreateEntryID(t, "abcdefghij") + entryID := picoshare.MustCreateEntryID("abcdefghij") passphrase, err := picoshare.NewDownloadPassphrase("correct horse battery staple") if err != nil { t.Fatalf("failed to create download passphrase: %v", err) @@ -179,7 +179,7 @@ func TestUpdateEntryMetadata(t *testing.T) { // leaves them out. func TestGetEntriesMetadataOmitsDownloadPassphrase(t *testing.T) { dataStore := test_sqlite.New(t) - entryID := mustCreateEntryID(t, "abcdefghij") + entryID := picoshare.MustCreateEntryID("abcdefghij") passphrase, err := picoshare.NewDownloadPassphrase("correct horse battery staple") if err != nil { t.Fatalf("failed to create download passphrase: %v", err) @@ -233,13 +233,3 @@ func mustParseFileSize(val int) picoshare.FileSize { return fileSize } - -func mustCreateEntryID(t *testing.T, raw string) picoshare.EntryID { - t.Helper() - - id, err := picoshare.NewEntryID(raw) - if err != nil { - t.Fatalf("failed to create entry ID %q: %v", raw, err) - } - return id -} diff --git a/store/sqlite/file/writer_test.go b/store/sqlite/file/writer_test.go index 49543c87..21d73ec8 100644 --- a/store/sqlite/file/writer_test.go +++ b/store/sqlite/file/writer_test.go @@ -42,7 +42,7 @@ func (db *mockSqlDB) Exec(query string, args ...any) (sql.Result, error) { } func TestWriteFile(t *testing.T) { - entryID := mustCreateEntryID(t, "abcdefghij") + entryID := picoshare.MustCreateEntryID("abcdefghij") for _, tt := range []struct { explanation string id picoshare.EntryID @@ -151,13 +151,3 @@ func TestWriteFile(t *testing.T) { }) } } - -func mustCreateEntryID(t *testing.T, raw string) picoshare.EntryID { - t.Helper() - - id, err := picoshare.NewEntryID(raw) - if err != nil { - t.Fatalf("failed to create entry ID %q: %v", raw, err) - } - return id -} From 4952713878bbfd9644dc95735e499bf5480af9a6 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Tue, 15 Sep 2026 13:34:53 +0000 Subject: [PATCH 3/9] Share entry ID length with generator Expose the domain entry ID length so generated IDs always match the constructor's validation rule. --- handlers/upload.go | 2 +- picoshare/picoshare.go | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/handlers/upload.go b/handlers/upload.go index 530013af..70755e75 100644 --- a/handlers/upload.go +++ b/handlers/upload.go @@ -207,7 +207,7 @@ func (s Server) entryMetadataFromRequest(r *http.Request) (picoshare.UploadMetad } func generateEntryID() picoshare.EntryID { - raw := random.String(10, entryIDCharacters) + raw := random.String(picoshare.EntryIDLength, entryIDCharacters) id, err := picoshare.NewEntryID(raw) if err != nil { panic(fmt.Sprintf("generated invalid entry ID: %v", err)) diff --git a/picoshare/picoshare.go b/picoshare/picoshare.go index ff92f5f5..708cd658 100644 --- a/picoshare/picoshare.go +++ b/picoshare/picoshare.go @@ -47,11 +47,11 @@ type ( } ) +// EntryIDLength is the number of characters in an entry ID. +const EntryIDLength = 10 + // entryIDCharacters omits visually similar characters (I, l, 1), (0, O). -const ( - entryIDLength = 10 - entryIDCharacters = "abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789" -) +const entryIDCharacters = "abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789" // EntryID identifies an uploaded entry. type EntryID struct { @@ -67,9 +67,9 @@ func (id EntryID) String() string { // NewEntryID constructs an entry ID from user-provided text. func NewEntryID(raw string) (EntryID, error) { - if len(raw) != entryIDLength { + if len(raw) != EntryIDLength { return EntryID{}, fmt.Errorf( - "entry ID has invalid length: got %d, want %d", len(raw), entryIDLength) + "entry ID has invalid length: got %d, want %d", len(raw), EntryIDLength) } for _, character := range raw { From faf0f04a5b0453b84ed13100ca8f75f5acb7dfec Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Tue, 15 Sep 2026 13:41:04 +0000 Subject: [PATCH 4/9] Rename entry ID parser Make EntryIDFromString explicit at parsing callsites so NewEntryID can name random entry ID generation in the next change. --- handlers/delete.go | 2 +- handlers/download.go | 6 +++--- handlers/upload.go | 4 ++-- handlers/views.go | 8 ++++---- picoshare/entry_id_test.go | 19 ++++++++++--------- picoshare/picoshare.go | 6 +++--- store/sqlite/entries.go | 2 +- store/sqlite/file/writer_test.go | 25 ++++++++++++------------- 8 files changed, 36 insertions(+), 36 deletions(-) diff --git a/handlers/delete.go b/handlers/delete.go index 29ef11de..f24e2650 100644 --- a/handlers/delete.go +++ b/handlers/delete.go @@ -12,7 +12,7 @@ import ( func (s Server) entryDelete() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) diff --git a/handlers/download.go b/handlers/download.go index 28347789..88a75e72 100644 --- a/handlers/download.go +++ b/handlers/download.go @@ -28,7 +28,7 @@ type entryUnlockRequest struct { func (s Server) entryGet() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -60,7 +60,7 @@ func (s Server) entryGet() http.HandlerFunc { func (s Server) entryUnlockGet() http.HandlerFunc { t := parseTemplates("templates/pages/entry-unlock.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -140,7 +140,7 @@ func (s Server) entryUnlockPost() http.HandlerFunc { } func parseEntryUnlockRequest(r *http.Request) (entryUnlockRequest, error) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { return entryUnlockRequest{}, err } diff --git a/handlers/upload.go b/handlers/upload.go index 70755e75..6ed02fc9 100644 --- a/handlers/upload.go +++ b/handlers/upload.go @@ -67,7 +67,7 @@ func (s Server) entryPost() http.HandlerFunc { func (s Server) entryPut() http.HandlerFunc { return func(w http.ResponseWriter, r *http.Request) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -208,7 +208,7 @@ func (s Server) entryMetadataFromRequest(r *http.Request) (picoshare.UploadMetad func generateEntryID() picoshare.EntryID { raw := random.String(picoshare.EntryIDLength, entryIDCharacters) - id, err := picoshare.NewEntryID(raw) + id, err := picoshare.EntryIDFromString(raw) if err != nil { panic(fmt.Sprintf("generated invalid entry ID: %v", err)) } diff --git a/handlers/views.go b/handlers/views.go index 5883d15a..3a02fa68 100644 --- a/handlers/views.go +++ b/handlers/views.go @@ -215,7 +215,7 @@ func (s Server) fileEditGet() http.HandlerFunc { "templates/pages/file-edit.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -268,7 +268,7 @@ func (s Server) fileInfoGet() http.HandlerFunc { "templates/pages/file-info.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -316,7 +316,7 @@ func (s Server) fileDownloadsGet() http.HandlerFunc { t := parseTemplatesWithFuncs(fns, "templates/pages/file-downloads.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) @@ -394,7 +394,7 @@ func (s Server) fileConfirmDeleteGet() http.HandlerFunc { t := parseTemplates("templates/pages/file-delete.html") return func(w http.ResponseWriter, r *http.Request) { - id, err := picoshare.NewEntryID(mux.Vars(r)["id"]) + id, err := picoshare.EntryIDFromString(mux.Vars(r)["id"]) if err != nil { log.Printf("error parsing ID: %v", err) http.Error(w, fmt.Sprintf("bad entry ID: %v", err), http.StatusBadRequest) diff --git a/picoshare/entry_id_test.go b/picoshare/entry_id_test.go index 3ff03594..b9202851 100644 --- a/picoshare/entry_id_test.go +++ b/picoshare/entry_id_test.go @@ -1,12 +1,13 @@ package picoshare_test import ( + "strings" "testing" "github.com/mtlynch/picoshare/picoshare" ) -func TestNewEntryID(t *testing.T) { +func TestEntryIDFromString(t *testing.T) { for _, tt := range []struct { explanation string input string @@ -14,40 +15,40 @@ func TestNewEntryID(t *testing.T) { }{ { explanation: "ten allowed characters are valid", - input: "aA23456789", + input: strings.Repeat("a", picoshare.EntryIDLength), isValidExpected: true, }, { explanation: "an ID shorter than ten characters is invalid", - input: "aA2345678", + input: strings.Repeat("a", picoshare.EntryIDLength-1), isValidExpected: false, }, { explanation: "an ID longer than ten characters is invalid", - input: "aA23456789a", + input: strings.Repeat("a", picoshare.EntryIDLength+1), isValidExpected: false, }, { explanation: "a visually ambiguous uppercase I is invalid", - input: "aA2345678I", + input: strings.Repeat("a", picoshare.EntryIDLength-1) + "I", isValidExpected: false, }, { explanation: "a visually ambiguous lowercase l is invalid", - input: "aA2345678l", + input: strings.Repeat("a", picoshare.EntryIDLength-1) + "l", isValidExpected: false, }, { explanation: "a Unicode character is invalid", - input: "aA2345678é", + input: strings.Repeat("a", picoshare.EntryIDLength-1) + "é", isValidExpected: false, }, } { t.Run(tt.explanation, func(t *testing.T) { - id, err := picoshare.NewEntryID(tt.input) + id, err := picoshare.EntryIDFromString(tt.input) isValid := err == nil if got, want := isValid, tt.isValidExpected; got != want { - t.Fatalf("NewEntryID validity=%t, want=%t", got, want) + t.Fatalf("EntryIDFromString validity=%t, want=%t", got, want) } if !isValid { return diff --git a/picoshare/picoshare.go b/picoshare/picoshare.go index 708cd658..6683a7bf 100644 --- a/picoshare/picoshare.go +++ b/picoshare/picoshare.go @@ -65,8 +65,8 @@ func (id EntryID) String() string { return id.value } -// NewEntryID constructs an entry ID from user-provided text. -func NewEntryID(raw string) (EntryID, error) { +// EntryIDFromString constructs an entry ID from user-provided text. +func EntryIDFromString(raw string) (EntryID, error) { if len(raw) != EntryIDLength { return EntryID{}, fmt.Errorf( "entry ID has invalid length: got %d, want %d", len(raw), EntryIDLength) @@ -83,7 +83,7 @@ func NewEntryID(raw string) (EntryID, error) { // MustCreateEntryID constructs an entry ID or panics when raw is invalid. func MustCreateEntryID(raw string) EntryID { - id, err := NewEntryID(raw) + id, err := EntryIDFromString(raw) if err != nil { panic(fmt.Sprintf("failed to create entry ID: %v", err)) } diff --git a/store/sqlite/entries.go b/store/sqlite/entries.go index 928c9c52..013f9ee2 100644 --- a/store/sqlite/entries.go +++ b/store/sqlite/entries.go @@ -50,7 +50,7 @@ func (s Store) GetEntriesMetadata() ([]picoshare.UploadMetadata, error) { if err = rows.Scan(&id, &filename, ¬e, &contentType, &uploadTimeRaw, &expirationTimeRaw, &fileSizeRaw); err != nil { return []picoshare.UploadMetadata{}, err } - entryID, err := picoshare.NewEntryID(id) + entryID, err := picoshare.EntryIDFromString(id) if err != nil { return []picoshare.UploadMetadata{}, fmt.Errorf("failed to parse entry ID from database: %w", err) } diff --git a/store/sqlite/file/writer_test.go b/store/sqlite/file/writer_test.go index 024c51dc..875c64f8 100644 --- a/store/sqlite/file/writer_test.go +++ b/store/sqlite/file/writer_test.go @@ -26,7 +26,7 @@ type ( var errMockSqlFailure = errors.New("fake SQL error") func (db *mockSqlDB) Exec(query string, args ...any) (sql.Result, error) { - id, err := picoshare.NewEntryID(args[0].(sql.NamedArg).Value.(string)) + id, err := picoshare.EntryIDFromString(args[0].(sql.NamedArg).Value.(string)) if err != nil { return nil, err } @@ -43,7 +43,6 @@ func (db *mockSqlDB) Exec(query string, args ...any) (sql.Result, error) { } func TestWriteFile(t *testing.T) { - entryID := picoshare.MustCreateEntryID("abcdefghij") for _, tt := range []struct { explanation string id picoshare.EntryID @@ -55,12 +54,12 @@ func TestWriteFile(t *testing.T) { }{ { explanation: "data is smaller than chunk size", - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), data: []byte("hello, world!"), chunkSize: 25, rowsExpected: []mockChunkRow{ { - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), chunkIndex: 0, chunk: []byte("hello, world!"), }, @@ -68,12 +67,12 @@ func TestWriteFile(t *testing.T) { }, { explanation: "data fits exactly in single chunk", - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), data: []byte("01234"), chunkSize: 5, rowsExpected: []mockChunkRow{ { - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), chunkIndex: 0, chunk: []byte("01234"), }, @@ -81,17 +80,17 @@ func TestWriteFile(t *testing.T) { }, { explanation: "data occupies a partial chunk after the first", - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), data: []byte("0123456"), chunkSize: 5, rowsExpected: []mockChunkRow{ { - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), chunkIndex: 0, chunk: []byte("01234"), }, { - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), chunkIndex: 1, chunk: []byte("56"), }, @@ -99,17 +98,17 @@ func TestWriteFile(t *testing.T) { }, { explanation: "data spans exactly two chunks", - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), data: []byte("0123456789"), chunkSize: 5, rowsExpected: []mockChunkRow{ { - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), chunkIndex: 0, chunk: []byte("01234"), }, { - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), chunkIndex: 1, chunk: []byte("56789"), }, @@ -117,7 +116,7 @@ func TestWriteFile(t *testing.T) { }, { explanation: "write fails when SQL transaction returns error", - id: entryID, + id: picoshare.MustCreateEntryID("abcdefghij"), data: []byte("0123456789"), chunkSize: 5, sqlExecErr: errMockSqlFailure, From cbab0a4c0372cd0da0379c82a989e13ef06f4eda Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Tue, 15 Sep 2026 13:42:14 +0000 Subject: [PATCH 5/9] Generate entry IDs in picoshare Expose NewEntryID for secure ID generation and remove the upload handler's duplicate generator and alphabet. EOF && git status --short --- handlers/upload.go | 15 +-------------- picoshare/entry_id_test.go | 12 ++++++++++++ picoshare/picoshare.go | 7 +++++++ 3 files changed, 20 insertions(+), 14 deletions(-) diff --git a/handlers/upload.go b/handlers/upload.go index 6ed02fc9..9eed6f5e 100644 --- a/handlers/upload.go +++ b/handlers/upload.go @@ -12,13 +12,9 @@ import ( "github.com/mtlynch/picoshare/handlers/parse" "github.com/mtlynch/picoshare/picoshare" - "github.com/mtlynch/picoshare/random" "github.com/mtlynch/picoshare/store" ) -// Omit visually similar characters (I,l,1), (0,O) -var entryIDCharacters = []rune("abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789") - type ( EntryPostResponse struct { ID string `json:"id"` @@ -206,15 +202,6 @@ func (s Server) entryMetadataFromRequest(r *http.Request) (picoshare.UploadMetad }, nil } -func generateEntryID() picoshare.EntryID { - raw := random.String(picoshare.EntryIDLength, entryIDCharacters) - id, err := picoshare.EntryIDFromString(raw) - if err != nil { - panic(fmt.Sprintf("generated invalid entry ID: %v", err)) - } - return id -} - func (s Server) insertFileFromRequest(r *http.Request, expiration picoshare.ExpirationTime, guestLinkID picoshare.GuestLinkID) (picoshare.EntryID, error) { // ParseMultipartForm can go above the limit we set, so set a conservative RAM // limit to avoid exhausting RAM on servers with limited resources. @@ -268,7 +255,7 @@ func (s Server) insertFileFromRequest(r *http.Request, expiration picoshare.Expi } } - id := generateEntryID() + id := picoshare.NewEntryID() err = s.store.InsertEntry(reader, picoshare.UploadMetadata{ ID: id, diff --git a/picoshare/entry_id_test.go b/picoshare/entry_id_test.go index b9202851..2ef37bc8 100644 --- a/picoshare/entry_id_test.go +++ b/picoshare/entry_id_test.go @@ -59,3 +59,15 @@ func TestEntryIDFromString(t *testing.T) { }) } } + +func TestNewEntryID(t *testing.T) { + id := picoshare.NewEntryID() + if got, want := len(id.String()), picoshare.EntryIDLength; got != want { + t.Errorf("length=%d, want=%d", got, want) + } + + _, err := picoshare.EntryIDFromString(id.String()) + if err != nil { + t.Errorf("failed to parse generated entry ID: %v", err) + } +} diff --git a/picoshare/picoshare.go b/picoshare/picoshare.go index 6683a7bf..432d5563 100644 --- a/picoshare/picoshare.go +++ b/picoshare/picoshare.go @@ -5,6 +5,8 @@ import ( "io" "strings" "time" + + "github.com/mtlynch/picoshare/random" ) type ( @@ -81,6 +83,11 @@ func EntryIDFromString(raw string) (EntryID, error) { return EntryID{value: raw}, nil } +// NewEntryID generates an entry ID. +func NewEntryID() EntryID { + return EntryID{value: random.String(EntryIDLength, []rune(entryIDCharacters))} +} + // MustCreateEntryID constructs an entry ID or panics when raw is invalid. func MustCreateEntryID(raw string) EntryID { id, err := EntryIDFromString(raw) From 6427b6daa5726849c7ee1bc277c90fcbed63c636 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Tue, 15 Sep 2026 22:54:43 +0000 Subject: [PATCH 6/9] Retain entry ID generation comment --- handlers/upload.go | 1 + 1 file changed, 1 insertion(+) diff --git a/handlers/upload.go b/handlers/upload.go index 9eed6f5e..218e81ef 100644 --- a/handlers/upload.go +++ b/handlers/upload.go @@ -255,6 +255,7 @@ func (s Server) insertFileFromRequest(r *http.Request, expiration picoshare.Expi } } + // NewEntryID omits visually similar characters (I, l, 1), (0, O). id := picoshare.NewEntryID() err = s.store.InsertEntry(reader, picoshare.UploadMetadata{ From dc92ebb7140a7eea96cd1e95624d1115df683a88 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Tue, 15 Sep 2026 19:39:32 -0400 Subject: [PATCH 7/9] Remove dumb comment --- handlers/upload.go | 1 - 1 file changed, 1 deletion(-) diff --git a/handlers/upload.go b/handlers/upload.go index 218e81ef..9eed6f5e 100644 --- a/handlers/upload.go +++ b/handlers/upload.go @@ -255,7 +255,6 @@ func (s Server) insertFileFromRequest(r *http.Request, expiration picoshare.Expi } } - // NewEntryID omits visually similar characters (I, l, 1), (0, O). id := picoshare.NewEntryID() err = s.store.InsertEntry(reader, picoshare.UploadMetadata{ From 84cce7dafff47097f50cdc4e4211a46d7c2cb284 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Wed, 16 Sep 2026 00:24:31 +0000 Subject: [PATCH 8/9] Precompute valid entry ID characters Use a package-level character set when validating entry IDs rather than scanning the allowed-character string for each input character. --- picoshare/picoshare.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/picoshare/picoshare.go b/picoshare/picoshare.go index 432d5563..3e9b25b2 100644 --- a/picoshare/picoshare.go +++ b/picoshare/picoshare.go @@ -3,7 +3,6 @@ package picoshare import ( "fmt" "io" - "strings" "time" "github.com/mtlynch/picoshare/random" @@ -55,6 +54,14 @@ const EntryIDLength = 10 // entryIDCharacters omits visually similar characters (I, l, 1), (0, O). const entryIDCharacters = "abcdefghijkmnopqrstuvwxyzABCDEFGHJKLMNPQRSTUVWXYZ23456789" +var entryIDCharacterSet = func() map[rune]struct{} { + characters := make(map[rune]struct{}, len(entryIDCharacters)) + for _, character := range entryIDCharacters { + characters[character] = struct{}{} + } + return characters +}() + // EntryID identifies an uploaded entry. type EntryID struct { value string @@ -75,7 +82,7 @@ func EntryIDFromString(raw string) (EntryID, error) { } for _, character := range raw { - if !strings.ContainsRune(entryIDCharacters, character) { + if _, ok := entryIDCharacterSet[character]; !ok { return EntryID{}, fmt.Errorf("entry ID contains invalid character: %q", character) } } From c549a1f744eb170be267a0c29591d6ebb3bbd6e3 Mon Sep 17 00:00:00 2001 From: Michael Lynch Date: Wed, 16 Sep 2026 08:04:26 -0400 Subject: [PATCH 9/9] Inline entryID vars --- store/sqlite/entries_test.go | 24 ++++++++++-------------- 1 file changed, 10 insertions(+), 14 deletions(-) diff --git a/store/sqlite/entries_test.go b/store/sqlite/entries_test.go index c71b051b..e20ceaf9 100644 --- a/store/sqlite/entries_test.go +++ b/store/sqlite/entries_test.go @@ -15,11 +15,10 @@ import ( func TestInsertDeleteSingleEntry(t *testing.T) { chunkSize := uint64(5) dataStore := test_sqlite.NewWithChunkSize(t, chunkSize) - entryID := picoshare.MustCreateEntryID("abcdefghij") input := "hello, world!" if err := dataStore.InsertEntry(bytes.NewBufferString(input), picoshare.UploadMetadata{ - ID: entryID, + ID: picoshare.MustCreateEntryID("abcdefghij"), Filename: "dummy-file.txt", Uploaded: mustParseTime("2025-05-25T00:00:00Z"), Expires: mustParseExpirationTime("2040-01-01T00:00:00Z"), @@ -28,7 +27,7 @@ func TestInsertDeleteSingleEntry(t *testing.T) { t.Fatalf("failed to insert file into sqlite: %v", err) } - entryFile, err := dataStore.ReadEntryFile(entryID) + entryFile, err := dataStore.ReadEntryFile(picoshare.MustCreateEntryID("abcdefghij")) if err != nil { t.Fatalf("failed to get entry from DB: %v", err) } @@ -63,7 +62,7 @@ func TestInsertDeleteSingleEntry(t *testing.T) { t.Fatalf("filename=%s, want=%s", got, want) } - err = dataStore.DeleteEntry(entryID) + err = dataStore.DeleteEntry(picoshare.MustCreateEntryID("abcdefghij")) if err != nil { t.Fatalf("failed to delete entry: %v", err) } @@ -81,11 +80,10 @@ func TestInsertDeleteSingleEntry(t *testing.T) { func TestReadLastByteOfEntry(t *testing.T) { chunkSize := uint64(5) db := test_sqlite.NewWithChunkSize(t, chunkSize) - entryID := picoshare.MustCreateEntryID("abcdefghij") input := "hello, world!" if err := db.InsertEntry(bytes.NewBufferString(input), picoshare.UploadMetadata{ - ID: entryID, + ID: picoshare.MustCreateEntryID("abcdefghij"), Filename: "dummy-file.txt", Uploaded: mustParseTime("2025-05-25T00:00:00Z"), Expires: mustParseExpirationTime("2040-01-01T00:00:00Z"), @@ -94,7 +92,7 @@ func TestReadLastByteOfEntry(t *testing.T) { t.Fatalf("failed to insert file into sqlite: %v", err) } - entryFile, err := db.ReadEntryFile(entryID) + entryFile, err := db.ReadEntryFile(picoshare.MustCreateEntryID("abcdefghij")) if err != nil { t.Fatalf("failed to read entry: %v", err) } @@ -121,7 +119,6 @@ func TestReadLastByteOfEntry(t *testing.T) { func TestUpdateEntryMetadata(t *testing.T) { dataStore := test_sqlite.New(t) - entryID := picoshare.MustCreateEntryID("abcdefghij") passphrase, err := picoshare.NewDownloadPassphrase("correct horse battery staple") if err != nil { t.Fatalf("failed to create download passphrase: %v", err) @@ -129,7 +126,7 @@ func TestUpdateEntryMetadata(t *testing.T) { data := "dummy data" if err := dataStore.InsertEntry(strings.NewReader(data), picoshare.UploadMetadata{ - ID: entryID, + ID: picoshare.MustCreateEntryID("abcdefghij"), Filename: "dummy-file.txt", Uploaded: mustParseTime("2025-05-25T00:00:00Z"), Expires: mustParseExpirationTime("2040-01-01T00:00:00Z"), @@ -139,7 +136,7 @@ func TestUpdateEntryMetadata(t *testing.T) { t.Fatalf("failed to insert file into sqlite: %v", err) } - metadata, err := dataStore.GetEntryMetadata(entryID) + metadata, err := dataStore.GetEntryMetadata(picoshare.MustCreateEntryID("abcdefghij")) if err != nil { t.Fatalf("failed to retrieve entry metadata: %v", err) } @@ -147,7 +144,7 @@ func TestUpdateEntryMetadata(t *testing.T) { t.Errorf("download passphrase=%q, want=%q", got, want) } - if err := dataStore.UpdateEntryMetadata(entryID, picoshare.UploadMetadata{ + if err := dataStore.UpdateEntryMetadata(picoshare.MustCreateEntryID("abcdefghij"), picoshare.UploadMetadata{ Filename: "renamed-file.txt", Expires: mustParseExpirationTime("2041-01-01T00:00:00Z"), Note: picoshare.FileNote{Value: new("updated note")}, @@ -156,7 +153,7 @@ func TestUpdateEntryMetadata(t *testing.T) { t.Fatalf("failed to update entry metadata: %v", err) } - metadata, err = dataStore.GetEntryMetadata(entryID) + metadata, err = dataStore.GetEntryMetadata(picoshare.MustCreateEntryID("abcdefghij")) if err != nil { t.Fatalf("failed to retrieve entry metadata: %v", err) } @@ -179,7 +176,6 @@ func TestUpdateEntryMetadata(t *testing.T) { // leaves them out. func TestGetEntriesMetadataOmitsDownloadPassphrase(t *testing.T) { dataStore := test_sqlite.New(t) - entryID := picoshare.MustCreateEntryID("abcdefghij") passphrase, err := picoshare.NewDownloadPassphrase("correct horse battery staple") if err != nil { t.Fatalf("failed to create download passphrase: %v", err) @@ -187,7 +183,7 @@ func TestGetEntriesMetadataOmitsDownloadPassphrase(t *testing.T) { data := "dummy data" if err := dataStore.InsertEntry(strings.NewReader(data), picoshare.UploadMetadata{ - ID: entryID, + ID: picoshare.MustCreateEntryID("abcdefghij"), Filename: "dummy-file.txt", Uploaded: mustParseTime("2025-05-25T00:00:00Z"), Expires: mustParseExpirationTime("2040-01-01T00:00:00Z"),