diff --git a/cmd/smr/upload.go b/cmd/smr/upload.go index a8bfaeb..6b6001c 100644 --- a/cmd/smr/upload.go +++ b/cmd/smr/upload.go @@ -1,7 +1,9 @@ package smr import ( + "archive/zip" "bytes" + "context" "encoding/json" "errors" "fmt" @@ -15,6 +17,7 @@ import ( "strings" "time" + "github.com/avast/retry-go" "github.com/spf13/cobra" "github.com/spf13/viper" @@ -26,6 +29,11 @@ const uploadVersionPartGQL = `mutation UploadVersionPart($modId: ModID!, $versio uploadVersionPart(modId: $modId, versionId: $versionId, part: $part, file: $file) }` +// maxErrorBodySnippet caps how much of an HTTP/GraphQL response body is +// embedded in an error message, so an unexpected large response doesn't +// bloat logs. +const maxErrorBodySnippet = 500 + func init() { Cmd.AddCommand(uploadCmd) } @@ -70,7 +78,9 @@ var uploadCmd = &cobra.Command{ return errors.New("file cannot be a directory") } - // TODO Validate .smod file before upload + if err := validateSmodFile(filePath); err != nil { + return err + } logBase := slog.With(slog.String("mod-id", modID), slog.String("path", filePath)) logBase.Info("creating a new mod version") @@ -83,21 +93,27 @@ var uploadCmd = &cobra.Command{ logBase = logBase.With(slog.String("version-id", createdVersion.GetVersionID())) logBase.Info("received version id") + f, err := os.Open(filePath) + if err != nil { + return fmt.Errorf("failed to open file: %w", err) + } + defer f.Close() + + httpClient := &http.Client{} + uploadURL := viper.GetString("api-base") + viper.GetString("graphql-api") + apiKey := viper.GetString("api-key") + // TODO Parallelize chunk uploading chunkCount := int(math.Ceil(float64(stat.Size()) / float64(chunkSize))) for i := 0; i < chunkCount; i++ { - chunkLog := logBase.With(slog.Int("chunk", i)) + // part is 1-indexed (matches the GraphQL "part" variable sent to the + // server) and is reused for both logging and the upload call so the + // two can never drift apart. + part := i + 1 + chunkLog := logBase.With(slog.Int("chunk", part)) chunkLog.Info("uploading chunk") - f, err := os.Open(filePath) - if err != nil { - return fmt.Errorf("failed to open file: %w", err) - } - offset := int64(i) * chunkSize - if _, err := f.Seek(offset, 0); err != nil { - return fmt.Errorf("failed to seek to chunk offset: %w", err) - } bufferSize := chunkSize if offset+chunkSize > stat.Size() { @@ -105,73 +121,14 @@ var uploadCmd = &cobra.Command{ } chunk := make([]byte, bufferSize) - if _, err := f.Read(chunk); err != nil { - return fmt.Errorf("failed to read from chunk offset: %w", err) + if err := readChunk(f, offset, chunk); err != nil { + return err } - operationBody, err := json.Marshal(map[string]interface{}{ - "query": uploadVersionPartGQL, - "variables": map[string]interface{}{ - "modId": modID, - "versionId": createdVersion.GetVersionID(), - "part": i + 1, - "file": nil, - }, - }) - if err != nil { - return fmt.Errorf("failed to serialize operation body: %w", err) + if err := uploadChunk(cmd.Context(), httpClient, uploadURL, apiKey, modID, createdVersion.GetVersionID(), part, chunk, filepath.Base(filePath)); err != nil { + chunkLog.Error("failed to upload chunk", slog.Any("err", err)) + return err } - - mapBody, err := json.Marshal(map[string]interface{}{ - "0": []string{"variables.file"}, - }) - if err != nil { - return fmt.Errorf("failed to serialize map body: %w", err) - } - - body := &bytes.Buffer{} - writer := multipart.NewWriter(body) - - operations, err := writer.CreateFormField("operations") - if err != nil { - return fmt.Errorf("failed to create operations field: %w", err) - } - - if _, err := operations.Write(operationBody); err != nil { - return fmt.Errorf("failed to write to operation field: %w", err) - } - - mapField, err := writer.CreateFormField("map") - if err != nil { - return fmt.Errorf("failed to create map field: %w", err) - } - - if _, err := mapField.Write(mapBody); err != nil { - return fmt.Errorf("failed to write to map field: %w", err) - } - - part, err := writer.CreateFormFile("0", filepath.Base(filePath)) - if err != nil { - return fmt.Errorf("failed to create file field: %w", err) - } - - if _, err := io.Copy(part, bytes.NewReader(chunk)); err != nil { - return fmt.Errorf("failed to write to file field: %w", err) - } - - if err := writer.Close(); err != nil { - return fmt.Errorf("failed to close body writer: %w", err) - } - - r, _ := http.NewRequest("POST", viper.GetString("api-base")+viper.GetString("graphql-api"), body) - r.Header.Add("Content-Type", writer.FormDataContentType()) - r.Header.Add("Authorization", viper.GetString("api-key")) - - client := &http.Client{} - if _, err := client.Do(r); err != nil { - return fmt.Errorf("failed to execute request: %w", err) - } - } logBase.Info("finalizing uploaded version") @@ -194,8 +151,8 @@ var uploadCmd = &cobra.Command{ logBase.Info("checking version upload state") state, err := ficsit.CheckVersionUploadState(cmd.Context(), global.APIClient, modID, createdVersion.GetVersionID()) if err != nil { - logBase.Error("failed to upload mod", slog.Any("err", err)) - return nil + logBase.Error("failed to check version upload state", slog.Any("err", err)) + return fmt.Errorf("failed to check version upload state after finalizing upload: %w", err) } if state == nil || state.GetState().Version.Id == "" { @@ -216,6 +173,218 @@ var uploadCmd = &cobra.Command{ }, } +// readChunk seeks to offset and reads exactly len(buf) bytes into buf, +// failing loudly instead of silently returning a short, zero-padded buffer +// (the bug in the original bare f.Read(chunk) call, which was not +// guaranteed to fill the buffer in one call). +func readChunk(r io.ReadSeeker, offset int64, buf []byte) error { + if _, err := r.Seek(offset, io.SeekStart); err != nil { + return fmt.Errorf("failed to seek to chunk offset: %w", err) + } + + if _, err := io.ReadFull(r, buf); err != nil { + return fmt.Errorf("failed to read from chunk offset: %w", err) + } + + return nil +} + +// uploadVersionPartResponse is the (loosely known) shape of the GraphQL +// response body for the UploadVersionPart mutation. +type uploadVersionPartResponse struct { + Data *struct { + // UploadVersionPart is a pointer so an explicit `false` (failure) can be + // told apart from a missing/null value. The GraphQL schema types this as + // `Boolean!` (non-null), so on a 2xx response with no GraphQL errors it + // must be present and concrete; uploadChunk therefore fails closed if it + // is missing/null rather than silently assuming success. + UploadVersionPart *bool `json:"uploadVersionPart"` + } `json:"data"` + Errors []struct { + Message string `json:"message"` + } `json:"errors"` +} + +// uploadChunkMaxAttempts and uploadChunkBaseDelay control the per-chunk retry +// policy. They are package variables (not consts) so tests can shrink the delay. +var ( + uploadChunkMaxAttempts uint = 4 + uploadChunkBaseDelay = 2 * time.Second +) + +// uploadChunk sends a single chunk of a mod file to the SMR upload endpoint as a +// GraphQL multipart request (the UploadVersionPart mutation) and validates the +// response. It never silently swallows a failed chunk. Transient failures (a +// transport error such as a dropped/corrupted TLS record, or a 5xx response) are +// retried up to uploadChunkMaxAttempts times with exponential backoff; permanent +// failures (a 4xx status, a GraphQL `errors` array, or an explicit +// `uploadVersionPart: false`) are NOT retried, since they will not succeed on a +// second attempt. +func uploadChunk(ctx context.Context, client *http.Client, url, apiKey, modID, versionID string, part int, chunk []byte, fileName string) error { + return retry.Do( + func() error { + return sendChunkOnce(ctx, client, url, apiKey, modID, versionID, part, chunk, fileName) + }, + retry.Attempts(uploadChunkMaxAttempts), + retry.Delay(uploadChunkBaseDelay), + retry.DelayType(retry.BackOffDelay), + retry.LastErrorOnly(true), + retry.Context(ctx), + retry.OnRetry(func(n uint, err error) { + slog.Warn("retrying chunk upload after a transient error", + slog.Int("chunk", part), slog.Uint64("failed_attempt", uint64(n+1)), slog.Any("err", err)) + }), + ) +} + +// sendChunkOnce performs a single upload attempt for one chunk. Retryable errors +// (transport failures, 5xx) are returned as plain errors; permanent errors are +// wrapped with retry.Unrecoverable so retry.Do stops immediately. +func sendChunkOnce(ctx context.Context, client *http.Client, url, apiKey, modID, versionID string, part int, chunk []byte, fileName string) error { + operationBody, err := json.Marshal(map[string]interface{}{ + "query": uploadVersionPartGQL, + "variables": map[string]interface{}{ + "modId": modID, + "versionId": versionID, + "part": part, + "file": nil, + }, + }) + if err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to serialize operation body: %w", part, err)) + } + + mapBody, err := json.Marshal(map[string]interface{}{ + "0": []string{"variables.file"}, + }) + if err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to serialize map body: %w", part, err)) + } + + body := &bytes.Buffer{} + writer := multipart.NewWriter(body) + + operations, err := writer.CreateFormField("operations") + if err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to create operations field: %w", part, err)) + } + + if _, err := operations.Write(operationBody); err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to write to operation field: %w", part, err)) + } + + mapField, err := writer.CreateFormField("map") + if err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to create map field: %w", part, err)) + } + + if _, err := mapField.Write(mapBody); err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to write to map field: %w", part, err)) + } + + filePart, err := writer.CreateFormFile("0", fileName) + if err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to create file field: %w", part, err)) + } + + if _, err := io.Copy(filePart, bytes.NewReader(chunk)); err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to write to file field: %w", part, err)) + } + + if err := writer.Close(); err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to close body writer: %w", part, err)) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, body) + if err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to create request: %w", part, err)) + } + + req.Header.Add("Content-Type", writer.FormDataContentType()) + req.Header.Add("Authorization", apiKey) + + resp, err := client.Do(req) + if err != nil { + // Transport-level failure (e.g. "tls: bad record MAC", connection reset) -- + // return plain (retryable) so retry.Do can try again. + return fmt.Errorf("chunk %d: request failed: %w", part, err) + } + defer resp.Body.Close() + + respBody, err := io.ReadAll(resp.Body) + if err != nil { + // Reading the response failed mid-stream -- treat as a retryable transport issue. + return fmt.Errorf("chunk %d: failed to read response body: %w", part, err) + } + + if resp.StatusCode >= 500 { + // Server-side transient error -- retryable. + return fmt.Errorf("chunk %d: upload failed with server status %d: %s", part, resp.StatusCode, errorBodySnippet(respBody)) + } + + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + // 4xx (bad request / auth) -- permanent; a retry will not help. + return retry.Unrecoverable(fmt.Errorf("chunk %d: upload failed with status %d: %s", part, resp.StatusCode, errorBodySnippet(respBody))) + } + + var parsed uploadVersionPartResponse + if err := json.Unmarshal(respBody, &parsed); err != nil { + return retry.Unrecoverable(fmt.Errorf("chunk %d: failed to parse response body: %w (body: %s)", part, err, errorBodySnippet(respBody))) + } + + if len(parsed.Errors) > 0 { + return retry.Unrecoverable(fmt.Errorf("chunk %d: server reported an error: %s", part, parsed.Errors[0].Message)) + } + + if parsed.Data == nil || parsed.Data.UploadVersionPart == nil { + // The schema types uploadVersionPart as Boolean! (non-null), so a 2xx + // response with no GraphQL errors must carry a concrete value. Treat a + // missing/null result as a failure rather than silently succeeding. + return retry.Unrecoverable(fmt.Errorf("chunk %d: upload response contained no uploadVersionPart result (status %d, body: %s)", part, resp.StatusCode, errorBodySnippet(respBody))) + } + + if !*parsed.Data.UploadVersionPart { + return retry.Unrecoverable(fmt.Errorf("chunk %d: server reported uploadVersionPart failure", part)) + } + + return nil +} + +// errorBodySnippet truncates a response body for inclusion in an error +// message so an unexpected large response doesn't bloat logs. +func errorBodySnippet(b []byte) string { + s := strings.TrimSpace(string(b)) + if len(s) > maxErrorBodySnippet { + return s[:maxErrorBodySnippet] + "...(truncated)" + } + return s +} + +// validateSmodFile performs a lightweight validation of a mod upload file before +// it is uploaded: it must have a .smod or .zip extension and must open as a +// valid zip archive. SMR's distributable format is .smod (itself a zip), while +// Alpakit's packaged upload artifact is a .zip -- both are accepted. This +// intentionally does NOT enforce any deeper internal structure (e.g. a specific +// .uplugin entry), since that could false-reject otherwise valid files. +func validateSmodFile(filePath string) error { + switch ext := strings.ToLower(filepath.Ext(filePath)); ext { + case ".smod", ".zip": + // accepted + default: + return fmt.Errorf("%q is not a valid mod upload file: unexpected extension %q, expected \".smod\" or \".zip\"", filePath, ext) + } + + zr, err := zip.OpenReader(filePath) + if err != nil { + return fmt.Errorf("%q is not a valid mod upload file: %w", filePath, err) + } + defer zr.Close() + + slog.Debug("validated mod upload archive", slog.String("path", filePath), slog.Int("entries", len(zr.File))) + + return nil +} + func init() { uploadCmd.PersistentFlags().Int64("chunk-size", 10000000, "Size of chunks to split uploaded mod in bytes") uploadCmd.PersistentFlags().String("stability", "release", "Stability of the uploaded mod (alpha, beta, release)") diff --git a/cmd/smr/upload_test.go b/cmd/smr/upload_test.go new file mode 100644 index 0000000..dc7fef4 --- /dev/null +++ b/cmd/smr/upload_test.go @@ -0,0 +1,353 @@ +package smr + +import ( + "archive/zip" + "bytes" + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + "os" + "path/filepath" + "strings" + "sync/atomic" + "testing" + "time" +) + +// --- Fix #1: uploadChunk must surface previously-silent failures --- + +func TestUploadChunk_HTTPErrorStatusSurfacesAsError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusUnauthorized) + _, _ = w.Write([]byte("unauthorized")) + })) + defer server.Close() + + err := uploadChunk(context.Background(), server.Client(), server.URL, "bad-key", "mod-1", "version-1", 1, []byte("chunk-data"), "test.smod") + if err == nil { + t.Fatal("expected an error for an HTTP 401 response, got nil") + } + if !strings.Contains(err.Error(), "chunk") { + t.Errorf("expected error to mention the chunk, got: %v", err) + } + if !strings.Contains(err.Error(), "401") { + t.Errorf("expected error to mention the status code, got: %v", err) + } +} + +func TestUploadChunk_GraphQLErrorsArraySurfacesAsError(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"errors":[{"message":"nope"}]}`)) + })) + defer server.Close() + + err := uploadChunk(context.Background(), server.Client(), server.URL, "key", "mod-1", "version-1", 2, []byte("chunk-data"), "test.smod") + if err == nil { + t.Fatal("expected an error for a GraphQL errors array, got nil") + } + if !strings.Contains(err.Error(), "chunk") { + t.Errorf("expected error to mention the chunk, got: %v", err) + } + if !strings.Contains(err.Error(), "nope") { + t.Errorf("expected error to include the GraphQL error message, got: %v", err) + } +} + +func TestUploadChunk_ExplicitSuccessIsAccepted(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":{"uploadVersionPart":true}}`)) + })) + defer server.Close() + + err := uploadChunk(context.Background(), server.Client(), server.URL, "key", "mod-1", "version-1", 3, []byte("chunk-data"), "test.smod") + if err != nil { + t.Fatalf("expected success, got error: %v", err) + } +} + +func TestUploadChunk_MissingSuccessFieldIsRejected(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":{}}`)) + })) + defer server.Close() + + err := uploadChunk(context.Background(), server.Client(), server.URL, "key", "mod-1", "version-1", 5, []byte("chunk-data"), "test.smod") + if err == nil { + t.Fatal("expected an error when uploadVersionPart is missing (schema types it Boolean!, so fail closed), got nil") + } + if !strings.Contains(err.Error(), "chunk") { + t.Errorf("expected error to mention the chunk, got: %v", err) + } +} + +// TestUploadChunk_ExplicitFailureIsRejected confirms that an explicit +// uploadVersionPart:false from the server hard-fails the chunk. +func TestUploadChunk_ExplicitFailureIsRejected(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":{"uploadVersionPart":false}}`)) + })) + defer server.Close() + + err := uploadChunk(context.Background(), server.Client(), server.URL, "key", "mod-1", "version-1", 4, []byte("chunk-data"), "test.smod") + if err == nil { + t.Fatal("expected an error for an explicit uploadVersionPart:false, got nil") + } + if !strings.Contains(err.Error(), "chunk") { + t.Errorf("expected error to mention the chunk, got: %v", err) + } +} + +// TestUploadChunk_SendsExpectedMultipartRequest is a parity check: the +// refactor into uploadChunk must not change the wire-level request (same +// operations/map/file multipart fields, same headers) that the server +// actually sees. +func TestUploadChunk_SendsExpectedMultipartRequest(t *testing.T) { + var gotAuth, gotContentType, gotOperations, gotMap, gotFileName string + var gotFileContent []byte + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + gotAuth = r.Header.Get("Authorization") + gotContentType = r.Header.Get("Content-Type") + + if err := r.ParseMultipartForm(32 << 20); err != nil { + t.Errorf("failed to parse multipart form: %v", err) + } + gotOperations = r.FormValue("operations") + gotMap = r.FormValue("map") + + file, header, err := r.FormFile("0") + if err != nil { + t.Errorf("failed to read file field: %v", err) + } else { + defer file.Close() + gotFileName = header.Filename + gotFileContent, _ = io.ReadAll(file) + } + + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":{"uploadVersionPart":true}}`)) + })) + defer server.Close() + + chunk := []byte("hello chunk contents") + err := uploadChunk(context.Background(), server.Client(), server.URL, "secret-key", "mod-42", "version-7", 3, chunk, "MyMod.smod") + if err != nil { + t.Fatalf("expected success, got error: %v", err) + } + + if gotAuth != "secret-key" { + t.Errorf("expected Authorization header %q, got %q", "secret-key", gotAuth) + } + if !strings.HasPrefix(gotContentType, "multipart/form-data") { + t.Errorf("expected multipart/form-data content type, got %q", gotContentType) + } + if !strings.Contains(gotOperations, `"part":3`) { + t.Errorf("expected operations field to include the part number, got %q", gotOperations) + } + if !strings.Contains(gotOperations, `"modId":"mod-42"`) { + t.Errorf("expected operations field to include modId, got %q", gotOperations) + } + if !strings.Contains(gotMap, "variables.file") { + t.Errorf("expected map field to reference variables.file, got %q", gotMap) + } + if gotFileName != "MyMod.smod" { + t.Errorf("expected uploaded file name %q, got %q", "MyMod.smod", gotFileName) + } + if !bytes.Equal(gotFileContent, chunk) { + t.Errorf("expected uploaded file contents %q, got %q", chunk, gotFileContent) + } +} + +func TestUploadChunk_RetriesTransientThenSucceeds(t *testing.T) { + // Shrink the retry delay so the test is fast. + orig := uploadChunkBaseDelay + uploadChunkBaseDelay = time.Millisecond + defer func() { uploadChunkBaseDelay = orig }() + + var calls int32 + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + if atomic.AddInt32(&calls, 1) < 3 { + w.WriteHeader(http.StatusInternalServerError) // transient 5xx + _, _ = w.Write([]byte("temporary server error")) + return + } + w.WriteHeader(http.StatusOK) + _, _ = w.Write([]byte(`{"data":{"uploadVersionPart":true}}`)) + })) + defer server.Close() + + err := uploadChunk(context.Background(), server.Client(), server.URL, "key", "mod-1", "version-1", 7, []byte("chunk-data"), "test.smod") + if err != nil { + t.Fatalf("expected success after transient 5xx errors were retried, got error: %v", err) + } + if got := atomic.LoadInt32(&calls); got < 3 { + t.Errorf("expected at least 3 attempts (2 failures + 1 success), got %d", got) + } +} + +// --- Fix #2: chunk reads must not silently short-read --- + +// shortReadSeeker is a test fixture that behaves like a file for Seek/Read +// purposes but deliberately hands back at most one byte per Read call -- +// the same kind of short read that a bare f.Read(chunk) is not guaranteed +// to survive. +type shortReadSeeker struct { + data []byte + pos int64 +} + +func (s *shortReadSeeker) Seek(offset int64, whence int) (int64, error) { + switch whence { + case io.SeekStart: + s.pos = offset + case io.SeekCurrent: + s.pos += offset + case io.SeekEnd: + s.pos = int64(len(s.data)) + offset + default: + return 0, fmt.Errorf("unsupported whence: %d", whence) + } + return s.pos, nil +} + +func (s *shortReadSeeker) Read(p []byte) (int, error) { + if s.pos >= int64(len(s.data)) { + return 0, io.EOF + } + n := copy(p, s.data[s.pos:s.pos+1]) + s.pos += int64(n) + return n, nil +} + +// TestReadChunk_FillsBufferDespiteShortReads exercises the actual production +// read path (readChunk, the helper the upload loop calls) against a source +// that only ever hands back one byte per Read call. The old bare +// `f.Read(chunk)` call was not guaranteed to fill the buffer in one call; +// readChunk must loop (via io.ReadFull) until the buffer is completely and +// correctly filled. +func TestReadChunk_FillsBufferDespiteShortReads(t *testing.T) { + original := []byte("this is definitely more than one byte of chunk data, repeated for length") + src := &shortReadSeeker{data: original} + + buf := make([]byte, len(original)) + if err := readChunk(src, 0, buf); err != nil { + t.Fatalf("expected readChunk to fill the buffer despite short reads, got error: %v", err) + } + if !bytes.Equal(buf, original) { + t.Fatalf("expected buffer to round-trip byte-identically, got %q, want %q", buf, original) + } +} + +// TestReadChunk_HonorsOffset confirms readChunk seeks to the requested chunk +// offset before reading, matching the per-iteration seek the upload loop +// relies on to walk through the file one chunk at a time. +func TestReadChunk_HonorsOffset(t *testing.T) { + original := []byte("0123456789ABCDEF") + src := &shortReadSeeker{data: original} + + buf := make([]byte, 4) + if err := readChunk(src, 10, buf); err != nil { + t.Fatalf("unexpected error: %v", err) + } + want := original[10:14] + if !bytes.Equal(buf, want) { + t.Fatalf("got %q, want %q", buf, want) + } +} + +// TestReadChunk_SurfacesShortReadError confirms the failure mode the old code +// could not detect: when fewer bytes are available than the chunk size +// requires, readChunk must return an error instead of silently returning a +// short, zero-padded buffer. +func TestReadChunk_SurfacesShortReadError(t *testing.T) { + src := &shortReadSeeker{data: []byte("short")} + + buf := make([]byte, 15) // longer than the source has available + if err := readChunk(src, 0, buf); err == nil { + t.Fatal("expected readChunk to return an error when the source is shorter than the buffer, got nil") + } +} + +// --- Fix #3: .smod validation before upload --- + +func TestValidateSmodFile_AcceptsRealZipNamedSmod(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "MyMod.smod") + writeMinimalZip(t, path) + + if err := validateSmodFile(path); err != nil { + t.Fatalf("expected a valid .smod zip to pass validation, got error: %v", err) + } +} + +func TestValidateSmodFile_AcceptsUppercaseExtension(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "MyMod.SMOD") + writeMinimalZip(t, path) + + if err := validateSmodFile(path); err != nil { + t.Fatalf("expected case-insensitive extension match, got error: %v", err) + } +} + +func TestValidateSmodFile_AcceptsZipExtension(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "MyMod.zip") + writeMinimalZip(t, path) + + if err := validateSmodFile(path); err != nil { + t.Fatalf("expected a .zip mod archive to pass validation, got error: %v", err) + } +} + +func TestValidateSmodFile_RejectsWrongExtension(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "MyMod.txt") + writeMinimalZip(t, path) // valid zip bytes, but the extension is wrong + + if err := validateSmodFile(path); err == nil { + t.Fatal("expected a .txt file to be rejected regardless of content, got nil error") + } +} + +func TestValidateSmodFile_RejectsNonZipContent(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "MyMod.smod") + if err := os.WriteFile(path, []byte("this is not a zip archive"), 0o644); err != nil { + t.Fatalf("failed to write test file: %v", err) + } + + if err := validateSmodFile(path); err == nil { + t.Fatal("expected a non-zip .smod file to be rejected, got nil error") + } +} + +func writeMinimalZip(t *testing.T, path string) { + t.Helper() + + f, err := os.Create(path) + if err != nil { + t.Fatalf("failed to create test zip file: %v", err) + } + defer f.Close() + + zw := zip.NewWriter(f) + + w, err := zw.Create("mod.uplugin") + if err != nil { + t.Fatalf("failed to create zip entry: %v", err) + } + if _, err := w.Write([]byte(`{"FriendlyName":"Test"}`)); err != nil { + t.Fatalf("failed to write zip entry: %v", err) + } + + if err := zw.Close(); err != nil { + t.Fatalf("failed to close zip writer: %v", err) + } +}