Skip to content

feat:[AH-2685]: fix npm error handling while upload #90

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abhinavcode wants to merge 1 commit into
base: main
Choose a base branch
from
Open

feat:[AH-2685]: fix npm error handling while upload #90

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 25 additions & 25 deletions registry/app/pkg/npm/local.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -406,74 +406,74 @@ func (c *localRegistry) parseAndUploadNPMPackage(
if errors.Is(err, io.EOF) || strings.Contains(err.Error(), "EOF") {
break
}
return types.FileInfo{}, fmt.Errorf("failed to parse JSON: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse JSON: %s", err.Error())
}

//nolint:nestif
if token, ok := token.(string); ok {
switch token {
case "_id":
if err := decoder.Decode(&packageMetadata.ID); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse _id: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse _id: %s", err.Error())
}
case "name":
if err := decoder.Decode(&packageMetadata.Name); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse name: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse name: %s", err.Error())
}
case "description":
if err := decoder.Decode(&packageMetadata.Description); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse description: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse description: %s", err.Error())
}
case "dist-tags":
packageMetadata.DistTags = make(map[string]string)
if err := decoder.Decode(&packageMetadata.DistTags); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse dist-tags: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse dist-tags: %s", err.Error())
}
case "versions":
packageMetadata.Versions = make(map[string]*npm2.PackageMetadataVersion)
if err := decoder.Decode(&packageMetadata.Versions); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse versions: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse versions: %s", err.Error())
}
case "readme":
if err := decoder.Decode(&packageMetadata.Readme); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse readme: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse readme: %s", err.Error())
}
case "maintainers":
if err := decoder.Decode(&packageMetadata.Maintainers); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse maintainers: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse maintainers: %s", err.Error())
}
case "time":
packageMetadata.Time = make(map[string]time.Time)
if err := decoder.Decode(&packageMetadata.Time); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse time: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse time: %s", err.Error())
}
case "homepage":
if err := decoder.Decode(&packageMetadata.Homepage); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse homepage: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse homepage: %s", err.Error())
}
case "keywords":
if err := decoder.Decode(&packageMetadata.Keywords); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse keywords: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse keywords: %s", err.Error())
}
case "repository":
if err := decoder.Decode(&packageMetadata.Repository); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse repository: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse repository: %s", err.Error())
}
case "author":
if err := decoder.Decode(&packageMetadata.Author); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse author: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse author: %s", err.Error())
}
case "readmeFilename":
if err := decoder.Decode(&packageMetadata.ReadmeFilename); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse readmeFilename: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse readmeFilename: %s", err.Error())
}
case "users":
if err := decoder.Decode(&packageMetadata.Users); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse users: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse users: %s", err.Error())
}
case "license":
if err := decoder.Decode(&packageMetadata.License); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse license: %w", err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse license: %s", err.Error())
}
case "_attachments":

Expand All @@ -490,7 +490,7 @@ func (c *localRegistry) parseAndUploadNPMPackage(
default:
var dummy any
if err := decoder.Decode(&dummy); err != nil {
return types.FileInfo{}, fmt.Errorf("failed to parse field %s: %w", token, err)
return types.FileInfo{}, usererror.BadRequestf("failed to parse field %s: %s", token, err.Error())
}
}
}
Expand Down Expand Up @@ -595,22 +595,22 @@ func (c *localRegistry) processBase64DataOptimized(
// Expecting `:` character first
startByte, err := streamReader.ReadByte()
if err != nil {
return types.FileInfo{}, fmt.Errorf("failed to upload attachment %s: Error while reading : character: %w",
attachmentKey, err)
return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: Error while reading : character: %s",
attachmentKey, err.Error())
}
if startByte != ':' {
return types.FileInfo{}, fmt.Errorf("failed to upload attachment %s: Expected : character, got %c",
return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: Expected : character, got %c",
attachmentKey, startByte)
}

// Now expecting `"`, marking start of JSON string
startByte, err = streamReader.ReadByte()
if err != nil {
return types.FileInfo{}, fmt.Errorf("failed to upload"+
" attachment %s: Error while reading \" character: %w", attachmentKey, err)
return types.FileInfo{}, usererror.BadRequestf("failed to upload"+
" attachment %s: Error while reading \" character: %s", attachmentKey, err.Error())
}
if startByte != '"' {
return types.FileInfo{}, fmt.Errorf("failed to upload"+
return types.FileInfo{}, usererror.BadRequestf("failed to upload"+
" attachment %s: Expected \" character, got %c", attachmentKey, startByte)
Comment on lines 596 to 614

@coderabbitai coderabbitai Bot Feb 23, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Raw byte parsing after decoder.Buffered() doesn't tolerate whitespace in the JSON stream

After decoder.Token() returns the key "data", decoder.Buffered() returns a reader starting at the first byte after the key's closing ". Per RFC 8259, insignificant whitespace is legal around : and before the value. If the client sends pretty-printed JSON (e.g., "data" : "base64..."), streamReader.ReadByte() at line 596 returns a space character, not :, and the upload fails with a misleading usererror.BadRequestf("...Expected : character, got ").

Standard npm clients emit compact JSON, so this is low-risk in practice, but a defensive fix is straightforward:

🛡️ Proposed fix — skip optional whitespace before `:` and `"` 
-	// Expecting `:` character first
-	startByte, err := streamReader.ReadByte()
-	if err != nil {
-		return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: Error while reading : character: %s",
-			attachmentKey, err.Error())
-	}
-	if startByte != ':' {
-		return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: Expected : character, got %c",
-			attachmentKey, startByte)
-	}
-
-	// Now expecting `"`, marking start of JSON string
-	startByte, err = streamReader.ReadByte()
-	if err != nil {
-		return types.FileInfo{}, usererror.BadRequestf("failed to upload"+
-			" attachment %s: Error while reading \" character: %s", attachmentKey, err.Error())
-	}
-	if startByte != '"' {
-		return types.FileInfo{}, usererror.BadRequestf("failed to upload"+
-			" attachment %s: Expected \" character, got %c", attachmentKey, startByte)
-	}
+	// Skip optional whitespace, then expect `:`
+	for {
+		startByte, err := streamReader.ReadByte()
+		if err != nil {
+			return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: error reading colon separator: %s",
+				attachmentKey, err.Error())
+		}
+		if startByte == ':' {
+			break
+		}
+		if startByte != ' ' && startByte != '\t' && startByte != '\n' && startByte != '\r' {
+			return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: expected ':' separator, got %c",
+				attachmentKey, startByte)
+		}
+	}
+	// Skip optional whitespace, then expect `"`
+	for {
+		startByte, err := streamReader.ReadByte()
+		if err != nil {
+			return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: error reading opening quote: %s",
+				attachmentKey, err.Error())
+		}
+		if startByte == '"' {
+			break
+		}
+		if startByte != ' ' && startByte != '\t' && startByte != '\n' && startByte != '\r' {
+			return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: expected '\"' opening quote, got %c",
+				attachmentKey, startByte)
+		}
+	}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@registry/app/pkg/npm/local.go` around lines 596 - 614, The code reading raw
bytes from decoder.Buffered() (via streamReader.ReadByte()) after
decoder.Token() doesn't tolerate optional JSON whitespace; change the logic
around the streamReader.ReadByte() calls (the block that checks for ':' and then
'"' using streamReader) to first loop-reading and discarding ASCII whitespace
(space, tab, CR, LF) until a non-whitespace byte is found, then assert that the
first non-whitespace is ':'; after consuming ':', again skip any whitespace
before asserting the next non-whitespace is '"' for the string start; keep the
same usererror.BadRequestf messages but report the actual non-whitespace byte
when expectations fail and reference attachmentKey in the error as before.

}

Expand All @@ -625,8 +625,8 @@ func (c *localRegistry) processBase64DataOptimized(
info.RegistryID)
if err != nil {
if strings.Contains(err.Error(), "unexpected EOF") {
return types.FileInfo{}, fmt.Errorf("failed to upload attachment %s: "+
"base64 data may be corrupted or missing closing quote: %w", attachmentKey, err)
return types.FileInfo{}, usererror.BadRequestf("failed to upload attachment %s: "+
"base64 data may be corrupted or missing closing quote: %s", attachmentKey, err.Error())
}
return types.FileInfo{}, fmt.Errorf("failed to upload attachment %s: %w", attachmentKey, err)
}
Expand Down