refactor(api): return bad request for malformed payloads

[sudhir-intc]

Classify malformed client payloads as 400 Bad Request rather than allowing them to fall through to the generic 500 response.

Covers malformed JSON syntax and type mismatches, invalid RFC3339 timestamps, empty/truncated request bodies, and invalid provisioning certificates.

Validation: go test -p 1 ./internal/controller/httpapi/v1 -count=1

[codecov]

Codecov Report

❌ Patch coverage is 83.67347% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.81%. Comparing base (8156048) to head (0dcd666).

Files with missing lines	Patch %	Lines
internal/controller/httpapi/v1/error.go	81.08%	7 Missing ⚠️
internal/controller/openapi/devices.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1248      +/-   ##
==========================================
+ Coverage   56.71%   56.81%   +0.10%     
==========================================
  Files         149      149              
  Lines       12154    12192      +38     
==========================================
+ Hits         6893     6927      +34     
- Misses       5260     5264       +4     
  Partials        1        1              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

🟡 Changes recommended

The runtime 400-response behavior needs corresponding Fuego/OpenAPI updates (and one client-facing error message improvement was identified) to keep the published API contract aligned.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

This PR updates v1 HTTP error handling so malformed client payloads are consistently classified as 400 Bad Request (instead of falling through to generic 500), including invalid JSON, JSON type mismatches, invalid RFC3339 timestamps, empty/truncated bodies, and invalid provisioning certificates.

Changes:

• Adds a domain-level CertFormatError and wraps invalid base64 provisioning cert failures to support clean 400 responses.
• Extends the v1 error handler to treat JSON unmarshal errors, timestamp parse errors, and EOF/UnexpectedEOF as validation errors (400).
• Updates unit tests and Postman collections to reflect the new 400 behavior for malformed payload scenarios.

File summaries

File	Description
internal/usecase/domains/usecase.go	Wraps invalid base64 provisioning cert decode failures with a typed domain error.
internal/usecase/domains/usecase_test.go	Adds coverage for invalid base64 provisioning certificate handling.
internal/usecase/domains/certpassword.go	Introduces CertFormatError for invalid provisioning certificate format.
internal/controller/httpapi/v1/error.go	Classifies malformed payload/binding errors (JSON/time/EOF) and invalid cert format as 400.
internal/controller/httpapi/v1/error_test.go	Adds tests ensuring malformed payloads and invalid cert format return 400.
internal/controller/httpapi/v1/devicemanagement_test.go	Updates expected status codes from 500 to 400 for malformed request bodies/type mismatches.
internal/controller/httpapi/v1/boot_test.go	Updates expected status code from 500 to 400 for invalid JSON body.
integration-test/collections/console_rps_apis.postman_collection.json	Adds a domain-create test ensuring invalid provisioning cert is rejected with 400.
integration-test/collections/console_mps_apis.postman_collection.json	Adds tests ensuring invalid JSON type and invalid timestamps return 400.

Review details

Suppressed comments (1)

internal/controller/httpapi/v1/error.go:108

• This new CertFormatError mapping changes /api/v1/admin/domains failure behavior to return 400 for invalid provisioning certificates; the corresponding Fuego/OpenAPI route declarations should explicitly include a 400 Bad Request response (internal/controller/openapi/domains.go currently only applies protectedRouteOptions and does not add a 400 response).

	case errors.As(err, &certFormatErr):
		msg := certFormatErr.Console.FriendlyMessage()
		c.AbortWithStatusJSON(http.StatusBadRequest, response{Error: msg, Message: msg})

• Files reviewed: 9/9 changed files
• Comments generated: 2
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

🟡 Changes recommended

The OpenAPI spec change documents 400 only for protected routes, leaving public routes that also return 400 (e.g. /api/v1/authorize) out of sync, and there are a couple of error-classification/message consistency issues to address.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

internal/usecase/domains/usecase.go:338

• pkcs12.Decode failures are always surfaced as CertPasswordError when cert==nil, which can mislead clients when the base64 payload decodes but the PKCS#12/PFX bytes are malformed (i.e., not a password problem). Now that CertFormatError exists, consider distinguishing “bad PFX/certificate data” vs “incorrect password”, or at least avoid the password-specific friendly message for non-password decode failures.

	// Convert the PFX data to x509 cert
	_, cert, err := pkcs12.Decode(pfxData, domain.ProvisioningCertPassword)
	if err != nil && cert == nil {
		return nil, ErrCertPassword.Wrap("DecryptAndCheckCertExpiration", "pkcs12.Decode", err)
	}

internal/usecase/domains/certpassword.go:13

• CertFormatError.Error currently returns a constant string, which drops the wrapped call/function/original error details captured in e.Console during Wrap(). That makes logs/debugging much harder than CertPasswordError / CertStoreError, which preserve trace info via e.Console.Error(). Consider returning the console error when available, while keeping the constant fallback for unwrapped instances.

• Files reviewed: 11/11 changed files
• Comments generated: 1
• Review effort level: Lite

[madhavilosetty-intel]

@sudhir-intc a concern on placement, handleJSONBindingErrors matches on error type only, and runs before handleDomainErrors/handleTypedErrors (error.go:66, :192-206). It can't tell a client's request body from an AMT device's response.

error source	main	here
AMT returns an empty response body	500	400 request body is empty
device connection drops mid-request	504	400 request body is empty
corrupt deviceinfo column from the DB	500	400 + internal trace

In the top two the request was fine — the device failed. 400 tells the caller to fix a body that was never wrong, and 400 is non-retryable, so a transient device error becomes a hard failure. Is that intended?

[madhavilosetty-intel]

Was swapping notValidErr ahead of validatorErr intentional? It changes the error body on every validation failure that goes through NotValidError.Wrap:

main: Key: 'X.ProfileName' Error:Field validation ...
here: Invalid input: Key: 'X.ProfileName' Error:Field validation ...

Status stays 400, so no test catches it. Is the body change intended for v1?

[sudhir-intc]

Valid issue in the code, while the doing the changes this got lost. Fixed the swap.
@madhavilosetty-intel : Please check

[madhavilosetty-intel]

CertFormatError.Error() returns a fixed string; CertPasswordError.Error() (line 27) returns e.Console.Error().

The logger uses err.Error(), not FriendlyMessage() (domains.go:99) — so the log line becomes the same string the client got, and illegal base64 data at input byte N is lost. Was the difference from the other error types intended?

[sudhir-intc]

while the doing the changes this got lost. Fixed to return the original error before falling back to the generic friendly message. Adding additional rps api tests too.
@madhavilosetty-intel : Please check

[sudhir-intc]

@sudhir-intc a concern on placement, handleJSONBindingErrors matches on error type only, and runs before handleDomainErrors/handleTypedErrors (error.go:66, :192-206). It can't tell a client's request body from an AMT device's response.

error source main here
AMT returns an empty response body 500 400 request body is empty
device connection drops mid-request 504 400 request body is empty
corrupt deviceinfo column from the DB 500 400 + internal trace
In the top two the request was fine — the device failed. 400 tells the caller to fix a body that was never wrong, and 400 is non-retryable, so a transient device error becomes a hard failure. Is that intended?

M yunderstanding is for the scenario's you have mentioned it should hit handleValidationErrors() these should still be treated as 5xx responses. Do you have test-case to reproduce this ?