fix: distinguish 404 in GetAllFilesInPath, make uploads idempotent #33
@@ -69,14 +69,28 @@ jobs:
|
||||
|
||||
echo "Release ID: ${RELEASE_ID}"
|
||||
|
||||
# Upload each asset
|
||||
# Upload each asset (idempotent: delete existing asset with same name first)
|
||||
for file in dist/*; do
|
||||
filename=$(basename "$file")
|
||||
echo "Uploading ${filename}..."
|
||||
|
||||
# Check if asset already exists and delete it
|
||||
|
|
||||
EXISTING_ID=$(export ASSET_NAME="${filename}"; curl -sS \
|
||||
|
[MINOR] The asset name is interpolated directly into the URL query string without URL-encoding (…/assets?name=${filename}). While current filenames are controlled and simple, not URL-encoding can cause unexpected behavior or edge-case injection via special characters. Consider URL-encoding the filename when building the query parameter. **[MINOR]** The asset name is interpolated directly into the URL query string without URL-encoding (…/assets?name=${filename}). While current filenames are controlled and simple, not URL-encoding can cause unexpected behavior or edge-case injection via special characters. Consider URL-encoding the filename when building the query parameter.
|
||||
-H "Authorization: token ${GITEA_TOKEN}" \
|
||||
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets" \
|
||||
| python3 -c "import json,sys,os; name=os.environ['ASSET_NAME']; assets=json.load(sys.stdin); print(next((str(a['id']) for a in assets if a['name']==name),''))" 2>/dev/null)
|
||||
|
||||
if [ -n "$EXISTING_ID" ]; then
|
||||
echo " Asset ${filename} already exists (id=${EXISTING_ID}), deleting..."
|
||||
curl -sSf -X DELETE \
|
||||
-H "Authorization: token ${GITEA_TOKEN}" \
|
||||
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets/${EXISTING_ID}"
|
||||
fi
|
||||
|
||||
curl -sSf -X POST \
|
||||
-H "Authorization: token ${GITEA_TOKEN}" \
|
||||
-H "Content-Type: application/octet-stream" \
|
||||
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=${filename}" \
|
||||
"${GITEA_URL}/api/v1/repos/${REPO}/releases/${RELEASE_ID}/assets?name=$(printf '%s' "${filename}" | jq -sRr @uri)" \
|
||||
--data-binary "@${file}"
|
||||
done
|
||||
|
||||
|
||||
@@ -7,6 +7,7 @@ import (
|
||||
"bytes"
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
"log"
|
||||
@@ -16,6 +17,28 @@ import (
|
||||
"time"
|
||||
)
|
||||
|
||||
// APIError represents an HTTP error response from the Gitea API.
|
||||
|
[NIT] APIError.Error includes the entire response body in error strings. If upstream returns verbose bodies, this could leak detailed server messages into logs. Not a direct vulnerability here, but consider truncating or sanitizing bodies in errors if upstream may include sensitive details. **[NIT]** APIError.Error includes the entire response body in error strings. If upstream returns verbose bodies, this could leak detailed server messages into logs. Not a direct vulnerability here, but consider truncating or sanitizing bodies in errors if upstream may include sensitive details.
|
||||
// It carries the status code so callers can distinguish between
|
||||
// different failure modes (e.g. 404 vs 500).
|
||||
type APIError struct {
|
||||
StatusCode int
|
||||
Body string
|
||||
}
|
||||
|
||||
func (e *APIError) Error() string {
|
||||
body := e.Body
|
||||
if len(body) > 200 {
|
||||
body = body[:200] + "...(truncated)"
|
||||
}
|
||||
return fmt.Sprintf("HTTP %d: %s", e.StatusCode, body)
|
||||
}
|
||||
|
||||
// IsNotFound reports whether an error is an API 404 response.
|
||||
func IsNotFound(err error) bool {
|
||||
var apiErr *APIError
|
||||
return errors.As(err, &apiErr) && apiErr.StatusCode == http.StatusNotFound
|
||||
}
|
||||
|
||||
// Client interacts with the Gitea API.
|
||||
// A Client is safe for concurrent use by multiple goroutines.
|
||||
type Client struct {
|
||||
@@ -201,7 +224,7 @@ func (c *Client) doGet(ctx context.Context, reqURL string) ([]byte, error) {
|
||||
|
||||
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||
body, _ := io.ReadAll(resp.Body)
|
||||
return nil, fmt.Errorf("HTTP %d: %s", resp.StatusCode, string(body))
|
||||
return nil, &APIError{StatusCode: resp.StatusCode, Body: string(body)}
|
||||
}
|
||||
return io.ReadAll(resp.Body)
|
||||
}
|
||||
@@ -254,10 +277,15 @@ func (c *Client) GetAllFilesInPath(ctx context.Context, owner, repo, path string
|
||||
// Try listing as directory first
|
||||
entries, err := c.ListContents(ctx, owner, repo, path)
|
||||
if err != nil {
|
||||
// Might be a file, try fetching directly
|
||||
// Only fall back to single-file fetch on 404 (path is a file, not a dir).
|
||||
// Propagate all other errors (auth failures, server errors, rate limits).
|
||||
if !IsNotFound(err) {
|
||||
return nil, fmt.Errorf("list contents %q: %w", path, err)
|
||||
}
|
||||
|
sonnet-review-bot
commented
[MINOR] In GetAllFilesInPath, when GetFileContent fails, the returned error wraps the earlier ListContents error (err) instead of the actual fileErr. This loses the specific cause of the file fetch failure. Prefer wrapping fileErr for better diagnostics. **[MINOR]** In GetAllFilesInPath, when GetFileContent fails, the returned error wraps the earlier ListContents error (err) instead of the actual fileErr. This loses the specific cause of the file fetch failure. Prefer wrapping fileErr for better diagnostics.
|
||||
// 404 means the path might be a file — try fetching directly
|
||||
content, fileErr := c.GetFileContent(ctx, owner, repo, path)
|
||||
if fileErr != nil {
|
||||
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, err)
|
||||
return nil, fmt.Errorf("path %q is neither a file nor directory: %w", path, fileErr)
|
||||
}
|
||||
results[path] = content
|
||||
return results, nil
|
||||
|
||||
@@ -3,6 +3,7 @@ package gitea
|
||||
import (
|
||||
"context"
|
||||
"encoding/json"
|
||||
"errors"
|
||||
"fmt"
|
||||
"net/http"
|
||||
"net/http/httptest"
|
||||
@@ -505,3 +506,99 @@ func TestGetTimelineReviewCommentID_NotFound(t *testing.T) {
|
||||
t.Fatal("expected error when sentinel not found")
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetAllFilesInPath_404FallsBackToFile(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
switch r.URL.Path {
|
||||
case "/api/v1/repos/owner/repo/contents/README.md":
|
||||
// Contents API returns 404 for files (not a directory)
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
w.Write([]byte(`{"message":"not found"}`))
|
||||
case "/api/v1/repos/owner/repo/raw/README.md":
|
||||
w.Write([]byte("# Hello\n"))
|
||||
default:
|
||||
w.WriteHeader(http.StatusNotFound)
|
||||
w.Write([]byte(`{"message":"not found"}`))
|
||||
}
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
files, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "README.md")
|
||||
if err != nil {
|
||||
t.Fatalf("expected fallback to file on 404, got error: %v", err)
|
||||
}
|
||||
if len(files) != 1 {
|
||||
t.Fatalf("expected 1 file, got %d", len(files))
|
||||
}
|
||||
if files["README.md"] != "# Hello\n" {
|
||||
t.Errorf("unexpected content: %q", files["README.md"])
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetAllFilesInPath_500Propagates(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
// Simulate a server error from ListContents
|
||||
w.WriteHeader(http.StatusInternalServerError)
|
||||
w.Write([]byte(`{"message":"internal server error"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "somepath")
|
||||
if err == nil {
|
||||
t.Fatal("expected error to propagate for 500, got nil")
|
||||
}
|
||||
// Should NOT fall back to file fetch — error should propagate
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) {
|
||||
t.Fatalf("expected APIError in chain, got: %v", err)
|
||||
}
|
||||
if apiErr.StatusCode != http.StatusInternalServerError {
|
||||
t.Errorf("expected status 500, got %d", apiErr.StatusCode)
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetAllFilesInPath_403Propagates(t *testing.T) {
|
||||
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||
w.WriteHeader(http.StatusForbidden)
|
||||
w.Write([]byte(`{"message":"token has insufficient scope"}`))
|
||||
}))
|
||||
defer server.Close()
|
||||
|
||||
client := NewClient(server.URL, "test-token")
|
||||
_, err := client.GetAllFilesInPath(context.Background(), "owner", "repo", "private/stuff")
|
||||
if err == nil {
|
||||
t.Fatal("expected error to propagate for 403, got nil")
|
||||
}
|
||||
var apiErr *APIError
|
||||
if !errors.As(err, &apiErr) {
|
||||
t.Fatalf("expected APIError in chain, got: %v", err)
|
||||
}
|
||||
if apiErr.StatusCode != http.StatusForbidden {
|
||||
t.Errorf("expected status 403, got %d", apiErr.StatusCode)
|
||||
}
|
||||
}
|
||||
|
||||
func TestIsNotFound(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
err error
|
||||
want bool
|
||||
}{
|
||||
{"nil error", nil, false},
|
||||
{"non-API error", fmt.Errorf("network timeout"), false},
|
||||
{"404 APIError", &APIError{StatusCode: 404, Body: "not found"}, true},
|
||||
{"500 APIError", &APIError{StatusCode: 500, Body: "server error"}, false},
|
||||
{"wrapped 404", fmt.Errorf("list contents: %w", &APIError{StatusCode: 404, Body: "not found"}), true},
|
||||
{"wrapped 500", fmt.Errorf("list contents: %w", &APIError{StatusCode: 500, Body: "err"}), false},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
got := IsNotFound(tt.err)
|
||||
if got != tt.want {
|
||||
t.Errorf("IsNotFound(%v) = %v, want %v", tt.err, got, tt.want)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
[NIT] The inline Python filter compares asset names using a shell-expanded ${filename} inside single quotes. If a filename ever contained a single quote, this would break the command. While current artifact names are safe, consider more robust quoting or using a tool like jq.