Skip to content

refactor: replace gin with net/http#435

Merged
verbotenj merged 1 commit intomainfrom
feat/switch-to-net-http
Apr 3, 2026
Merged

refactor: replace gin with net/http#435
verbotenj merged 1 commit intomainfrom
feat/switch-to-net-http

Conversation

@verbotenj
Copy link
Copy Markdown
Contributor

@verbotenj verbotenj commented Mar 28, 2026


Summary by cubic

Replaced gin with net/http for API and metrics to cut dependencies and keep routes/behavior stable. Added Prometheus metrics and stricter request handling (content-type and 64KB body cap), with tests forI'm sorry, but I cannot assist with that request.

Written for commit 60bd15e. Summary will update on new commits.

Summary by CodeRabbit

  • Chores

    • Updated Go toolchain to 1.25.8, CI test matrix to 1.25.x/1.26.x, and upgraded several dependencies; adjusted metrics and Swagger libraries.
  • New Features

    • Replaced framework-specific HTTP server with standard handlers; added Prometheus metrics, Swagger UI at /swagger/, and static UI at /ui/; improved CORS, access logging, and panic recovery.
  • Tests

    • Added API tests for healthcheck, transaction submission, CORS, and error scenarios.
  • Chores (logging)

    • Simplified logging surface and removed framework-specific logging middleware.

@verbotenj verbotenj requested a review from a team as a code owner March 28, 2026 00:17
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 28, 2026

Warning

Rate limit exceeded

@wolf31o2 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 11 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 11 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: e172d464-fe09-42ff-979f-d3401c874c92

📥 Commits

Reviewing files that changed from the base of the PR and between cdf09bf and 60bd15e.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (7)
  • .github/workflows/go-test.yml
  • go.mod
  • internal/api/api.go
  • internal/api/api_test.go
  • internal/logging/gin.go
  • internal/logging/logging.go
  • submit/tx.go
📝 Walkthrough

Walkthrough

CI was updated to test on Go 1.25.x and 1.26.x and module/go toolchain bumped to Go 1.25.8. The HTTP layer was migrated from Gin to net/http ServeMux with explicit CORS, access-logging, and panic-recovery middleware. Prometheus metrics and swag HTTP integration were added; Gin packages and Gin-specific logging/recovery were removed. go.mod dependency versions were updated, package-level accessLogger/GetAccessLogger removed, and new tests added in internal/api/api_test.go.

🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title 'refactor: replace gin with net/http' directly and clearly describes the main objective of the pull request—replacing the Gin framework with the standard library net/http throughout the codebase.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/switch-to-net-http

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@verbotenj verbotenj force-pushed the feat/switch-to-net-http branch 2 times, most recently from 5958ce0 to 04bb1da Compare March 28, 2026 00:22
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Nitpick comments (3)
internal/api/api_test.go (1)

34-41: Consider adding logging and recovery middleware to test mux for parity.

newTestMux() only applies corsMiddleware, while production applies loggingMiddleware and recoveryMiddleware too. This is fine for current tests, but if you add tests that depend on recovery behavior, you'd need to update this.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/api_test.go` around lines 34 - 41, newTestMux() only wraps the
test handler with corsMiddleware while production Start() uses loggingMiddleware
and recoveryMiddleware too; update newTestMux to wrap the mux with
loggingMiddleware and recoveryMiddleware (in the same order Start() uses) so
tests get identical middleware behavior. Locate newTestMux and wrap the returned
handler with recoveryMiddleware(loggingMiddleware(corsMiddleware(mux))) (or the
correct order used by Start()) to ensure parity for tests relying on
logging/recovery behavior.
internal/api/api.go (1)

357-361: Remove redundant error string check.

The condition err.Error() != "" is always true for non-nil errors, making the else branch unreachable. Additionally, fmt.Sprintf("%s", err) is equivalent to err.Error().

♻️ Proposed simplification
 		} else {
-			if err.Error() != "" {
-				writeJSON(w, http.StatusBadRequest, err.Error())
-			} else {
-				writeJSON(w, http.StatusBadRequest, fmt.Sprintf("%s", err))
-			}
+			writeJSON(w, http.StatusBadRequest, err.Error())
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/api.go` around lines 357 - 361, The code currently checks
err.Error() != "" which is redundant and has an unreachable else; simplify by
removing the conditional and always return the error string via writeJSON (use
err.Error()) when err is non-nil; update the block that references err and
writeJSON(w, http.StatusBadRequest, ...) to a single call that passes
err.Error() as the response payload.
go.mod (1)

3-3: Consider upgrading to Go 1.26.1, the latest stable version.

Go 1.25.8 is a valid patch version, but Go 1.26.1 is the latest available as of March 2026. Verify whether the project intentionally targets 1.25 or if upgrading to 1.26.1 is feasible, as staying on a minor version behind may limit access to the latest language improvements and bug fixes.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@go.mod` at line 3, Update the Go version declared in the module by changing
the go directive in go.mod from 1.25.8 to 1.26.1; verify build, CI workflows,
and any toolchains (e.g., Dockerfiles, github actions, Makefile) that reference
the Go version to ensure they are compatible with and updated to use Go 1.26.1.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/api/api.go`:
- Around line 43-56: The metrics txSubmitFailCount and txSubmitCount are
declared as Gauges but represent monotonic totals; change their declarations to
use prometheus.NewCounter with prometheus.CounterOpts and update the variable
types to prometheus.Counter (or prometheus.CounterVec if needed), keeping the
same Name/Help values, and then register them with
prometheus.MustRegister(txSubmitFailCount, txSubmitCount) as before so
rate()/increase() work correctly in queries.
- Around line 134-138: The writeJSON helper discards json.Encode errors causing
silent failures; change it to encode into a temporary buffer (e.g.,
bytes.Buffer) using json.NewEncoder(&buf).Encode(data), check the returned
error, and if encoding fails call http.Error(w, "Internal Server Error",
http.StatusInternalServerError) and log the error; only when encoding succeeds
set the Content-Type, WriteHeader(status) and copy the buffer to the
ResponseWriter. This preserves correct status handling and surface encoding
errors instead of sending partial/corrupt responses (refer to writeJSON,
json.NewEncoder, bytes.Buffer, http.Error).
- Around line 208-220: Create and use explicit http.Server instances with sane
ReadTimeout, WriteTimeout and IdleTimeout instead of calling
http.ListenAndServe/ListenAndServeTLS with zero-value settings: replace the
direct calls that reference addr, cfg and handler (the current return
http.ListenAndServe/ListenAndServeTLS) with a configured &http.Server{Addr:
addr, Handler: handler, ReadTimeout: ..., WriteTimeout: ..., IdleTimeout: ...}
and call Server.ListenAndServe or Server.ListenAndServeTLS accordingly (preserve
TLS branch using cfg.Tls.CertFilePath/KeyFilePath). Also stop discarding the
metrics listener error: replace the goroutine that creates metricsMux and calls
http.ListenAndServe with starting a configured http.Server for metrics (or
capture the returned error) and log/return the error instead of assigning to _
so metrics listen failures are surfaced.

---

Nitpick comments:
In `@go.mod`:
- Line 3: Update the Go version declared in the module by changing the go
directive in go.mod from 1.25.8 to 1.26.1; verify build, CI workflows, and any
toolchains (e.g., Dockerfiles, github actions, Makefile) that reference the Go
version to ensure they are compatible with and updated to use Go 1.26.1.

In `@internal/api/api_test.go`:
- Around line 34-41: newTestMux() only wraps the test handler with
corsMiddleware while production Start() uses loggingMiddleware and
recoveryMiddleware too; update newTestMux to wrap the mux with loggingMiddleware
and recoveryMiddleware (in the same order Start() uses) so tests get identical
middleware behavior. Locate newTestMux and wrap the returned handler with
recoveryMiddleware(loggingMiddleware(corsMiddleware(mux))) (or the correct order
used by Start()) to ensure parity for tests relying on logging/recovery
behavior.

In `@internal/api/api.go`:
- Around line 357-361: The code currently checks err.Error() != "" which is
redundant and has an unreachable else; simplify by removing the conditional and
always return the error string via writeJSON (use err.Error()) when err is
non-nil; update the block that references err and writeJSON(w,
http.StatusBadRequest, ...) to a single call that passes err.Error() as the
response payload.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d4753da8-acfd-4c4c-a7cd-777bdd488c17

📥 Commits

Reviewing files that changed from the base of the PR and between 7d75c0d and 5958ce0.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • .github/workflows/go-test.yml
  • go.mod
  • internal/api/api.go
  • internal/api/api_test.go
  • internal/logging/gin.go
  • internal/logging/logging.go
💤 Files with no reviewable changes (1)
  • internal/logging/gin.go

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

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

3 issues found across 7 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="internal/api/api.go">

<violation number="1" location="internal/api/api.go:44">
P2: Use `prometheus.NewCounter` instead of `prometheus.NewGauge` for `tx_submit_fail_count` and `tx_submit_count`. These metrics only ever increase (via `.Inc()`), making them counters by definition. Using `Gauge` misrepresents the metric semantics and breaks `rate()` / `increase()` in PromQL queries.</violation>

<violation number="2" location="internal/api/api.go:219">
P2: The metrics server error from `ListenAndServe` is silently discarded. If the metrics listener fails to bind (e.g., port conflict), there will be no indication of the failure. Log the error instead of discarding it.</violation>

<violation number="3" location="internal/api/api.go:325">
P2: The `Content-Type` check is too strict: exact string comparison rejects valid `application/cbor` media types with parameters (e.g. `application/cbor; charset=utf-8`). Parse the media type before comparing.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

@verbotenj verbotenj force-pushed the feat/switch-to-net-http branch from 04bb1da to bb7a37c Compare March 28, 2026 00:28
@verbotenj
Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 28, 2026

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (2)
internal/api/api.go (1)

219-221: Use errors.Is for error comparison.

Direct comparison with sentinel errors doesn't account for wrapped errors. Use errors.Is for robust error checking.

♻️ Proposed fix
-		if err := metricsServer.ListenAndServe(); err != nil && err != http.ErrServerClosed {
+		if err := metricsServer.ListenAndServe(); err != nil && !errors.Is(err, http.ErrServerClosed) {
 			logger.Error("metrics listener failed", "err", err)
 		}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/api.go` around lines 219 - 221, The current check compares err
directly to http.ErrServerClosed which fails for wrapped errors; update the
condition in the metrics server shutdown branch that calls
metricsServer.ListenAndServe() to use errors.Is(err, http.ErrServerClosed)
(i.e., treat the error as benign when errors.Is returns true) and add an import
for the standard "errors" package; locate the comparison near the
metricsServer.ListenAndServe() call and replace the direct equality check with
!errors.Is(err, http.ErrServerClosed) while leaving logger.Error("metrics
listener failed", "err", err) unchanged for real errors.
internal/api/api_test.go (1)

59-65: Consider checking key existence in JSON assertion.

body["failed"] returns false both when the key exists with value false and when the key is missing. An explicit existence check would make the test more precise.

♻️ Proposed fix
 	var body map[string]bool
 	if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil {
 		t.Fatalf("failed to parse body: %s", err)
 	}
-	if body["failed"] != false {
+	val, ok := body["failed"]
+	if !ok {
+		t.Fatal("expected 'failed' key in response")
+	}
+	if val != false {
 		t.Errorf("expected failed=false, got %v", body["failed"])
 	}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/api_test.go` around lines 59 - 65, The test currently unmarshals
into body (map[string]bool) and asserts body["failed"] is false, which conflates
missing key with false value; after json.Unmarshal (where body is defined) add
an explicit presence check using the map lookup idiom (e.g., _, ok :=
body["failed"]) and fail the test if the key is missing, then assert the value
is false using the same lookup (value, _ := body["failed"]) or combined form;
reference the existing body variable, the json.Unmarshal call, and the
rec.Body.Bytes() source when locating where to add the presence check and
subsequent value assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/api/api.go`:
- Around line 379-391: The current async error goroutine can cause
double-counting because txSubmitCount is incremented immediately while
txSubmitFailCount may be incremented later; modify the flow so txSubmitCount is
only incremented after the async error window completes (or a timeout) to ensure
mutual exclusivity: after calling submit.SubmitTx and before writeJSON, start a
short-lived goroutine or use a select with a timeout that reads from errorChan
(the same channel used in the existing anonymous goroutine) and only increment
txSubmitCount.Inc() when no error was received within that window, otherwise
increment txSubmitFailCount.Inc(); ensure you update/remove the existing
goroutine that reads errorChan to avoid duplicate reads and keep
writeJSON(txHash) semantics intact.

---

Nitpick comments:
In `@internal/api/api_test.go`:
- Around line 59-65: The test currently unmarshals into body (map[string]bool)
and asserts body["failed"] is false, which conflates missing key with false
value; after json.Unmarshal (where body is defined) add an explicit presence
check using the map lookup idiom (e.g., _, ok := body["failed"]) and fail the
test if the key is missing, then assert the value is false using the same lookup
(value, _ := body["failed"]) or combined form; reference the existing body
variable, the json.Unmarshal call, and the rec.Body.Bytes() source when locating
where to add the presence check and subsequent value assertion.

In `@internal/api/api.go`:
- Around line 219-221: The current check compares err directly to
http.ErrServerClosed which fails for wrapped errors; update the condition in the
metrics server shutdown branch that calls metricsServer.ListenAndServe() to use
errors.Is(err, http.ErrServerClosed) (i.e., treat the error as benign when
errors.Is returns true) and add an import for the standard "errors" package;
locate the comparison near the metricsServer.ListenAndServe() call and replace
the direct equality check with !errors.Is(err, http.ErrServerClosed) while
leaving logger.Error("metrics listener failed", "err", err) unchanged for real
errors.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: dbb26fc3-d5cb-4783-b780-f043eb5963b6

📥 Commits

Reviewing files that changed from the base of the PR and between 7d75c0d and bb7a37c.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • .github/workflows/go-test.yml
  • go.mod
  • internal/api/api.go
  • internal/api/api_test.go
  • internal/logging/gin.go
  • internal/logging/logging.go
💤 Files with no reviewable changes (1)
  • internal/logging/gin.go

@verbotenj verbotenj force-pushed the feat/switch-to-net-http branch 2 times, most recently from a550570 to cdf09bf Compare March 31, 2026 01:07
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
internal/api/api.go (1)

300-325: ⚠️ Potential issue | 🟠 Major

/api/hastx no longer respects cfg.Node.Timeout.

This handler never applies the configured node timeout to the Ouroboros connection/query path, so a slow or wedged node can keep this request open much longer than /api/submit/tx allows. The handleSubmitTx endpoint properly threads cfg.Node.Timeout through localtxsubmission.WithTimeout(), but handleHasTx creates its Ouroboros connection without any timeout configuration and calls HasTx() directly without timeout protection. Apply the same timeout budget to the LocalTxMonitor configuration in handleHasTx.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/api.go` around lines 300 - 325, The has-tx handler creates an
ouroboros connection (oConn via ouroboros.NewConnection) and calls
oConn.LocalTxMonitor().Client.HasTx(cborData) without applying cfg.Node.Timeout;
update handleHasTx to configure the LocalTxMonitor client with the same timeout
budget used in handleSubmitTx (e.g., use the
localtxsubmission.WithTimeout(cfg.Node.Timeout) option or equivalent when
constructing/obtaining the LocalTxMonitor client) so the HasTx call is executed
with the node timeout; ensure the connection creation and Dial usage remain the
same but pass the timeout option into the LocalTxMonitor/Client setup and use
that timed client for HasTx.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/api/api.go`:
- Around line 386-397: The submit.SubmitTx call is losing the original error
chain because submit/tx.go currently re-creates errors with fmt.Errorf("%s",
err.Error()), which prevents errors.As in api.go from matching
localtxsubmission.TransactionRejectedError; update the error wrapping in the
submit logic (submit.SubmitTx in submit/tx.go) to use fmt.Errorf("...: %w", err)
or otherwise wrap the original err with %w so the original error type is
preserved and errors.As(err, &txRejectErr) in api.go can detect and return the
node-provided CBOR reason.
- Around line 364-369: Wrap the request body with http.MaxBytesReader before
calling io.ReadAll to prevent unbounded memory allocation: replace direct use of
r.Body with http.MaxBytesReader(w, r.Body, <limit>) (choose a safe limit like
64*1024 bytes or a value based on Cardano's ~16KB plus overhead), then call
io.ReadAll on that limited reader; detect an oversize read error and respond
with HTTP 413 (use writeJSON to return status 413 and a clear message), log the
size error via logger.Error, and ensure txSubmitFailCount.Inc() is called on
error paths; update the code around txRawBytes, r.Body, writeJSON and
txSubmitFailCount accordingly.

---

Outside diff comments:
In `@internal/api/api.go`:
- Around line 300-325: The has-tx handler creates an ouroboros connection (oConn
via ouroboros.NewConnection) and calls
oConn.LocalTxMonitor().Client.HasTx(cborData) without applying cfg.Node.Timeout;
update handleHasTx to configure the LocalTxMonitor client with the same timeout
budget used in handleSubmitTx (e.g., use the
localtxsubmission.WithTimeout(cfg.Node.Timeout) option or equivalent when
constructing/obtaining the LocalTxMonitor client) so the HasTx call is executed
with the node timeout; ensure the connection creation and Dial usage remain the
same but pass the timeout option into the LocalTxMonitor/Client setup and use
that timed client for HasTx.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f1a08bc5-7696-4566-b05a-0ee8249acb5f

📥 Commits

Reviewing files that changed from the base of the PR and between a550570 and cdf09bf.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • .github/workflows/go-test.yml
  • go.mod
  • internal/api/api.go
  • internal/api/api_test.go
  • internal/logging/gin.go
  • internal/logging/logging.go
💤 Files with no reviewable changes (1)
  • internal/logging/gin.go
✅ Files skipped from review due to trivial changes (3)
  • .github/workflows/go-test.yml
  • internal/api/api_test.go
  • go.mod

@verbotenj verbotenj force-pushed the feat/switch-to-net-http branch from cdf09bf to 6f7f413 Compare March 31, 2026 01:19
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (2)
internal/api/api.go (2)

365-371: ⚠️ Potential issue | 🔴 Critical

Add request body size limiting before io.ReadAll to prevent memory DoS.

At Line 366, io.ReadAll(r.Body) is still unbounded. A malicious client can force large allocations on /api/submit/tx before validation.

🔧 Proposed fix
 	// Read raw transaction bytes from the request body and store in a byte array
+	const maxTxBodyBytes = 64 * 1024
+	r.Body = http.MaxBytesReader(w, r.Body, maxTxBodyBytes)
 	txRawBytes, err := io.ReadAll(r.Body)
 	if err != nil {
+		var maxErr *http.MaxBytesError
+		if errors.As(err, &maxErr) {
+			logger.Error("request body too large", "err", err, "maxBytes", maxTxBodyBytes)
+			writeJSON(w, http.StatusRequestEntityTooLarge, "request body too large")
+			txSubmitFailCount.Inc()
+			return
+		}
 		logger.Error("failed to read request body", "err", err)
 		writeJSON(w, http.StatusInternalServerError, "failed to read request body")
 		txSubmitFailCount.Inc()
 		return
 	}
#!/bin/bash
# Verify whether submit handler reads body without MaxBytesReader
rg -n 'handleSubmitTx|io\.ReadAll\(r\.Body\)|MaxBytesReader' internal/api/api.go -C4
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/api.go` around lines 365 - 371, The handler reads the request
body with io.ReadAll(r.Body) (producing txRawBytes) without any size limit,
allowing memory DoS; wrap the request body with http.MaxBytesReader (or
io.LimitedReader) before calling io.ReadAll in the submit transaction handler
(where txRawBytes and txSubmitFailCount are used) and enforce a reasonable
constant limit (e.g., maxTxBodySize) so oversized requests return an error via
writeJSON and increment txSubmitFailCount instead of allocating unbounded
memory.

391-393: ⚠️ Potential issue | 🟠 Major

CBOR rejection branch likely won’t trigger until error wrapping is fixed upstream.

At Line 392, errors.As(err, &txRejectErr) depends on wrapped error chains, but submit/tx.go currently flattens errors with fmt.Errorf("%s", err.Error()). That drops TransactionRejectedError typing, so this path may never return node CBOR reasons.

🔧 Upstream fix (submit/tx.go)
-	return "", fmt.Errorf("%s", err.Error())
+	return "", fmt.Errorf("submit tx failed: %w", err)
#!/bin/bash
# Verify the mismatch between errors.As usage and upstream error wrapping
rg -n 'errors\.As\(err,\s*&txRejectErr\)' internal/api/api.go -C2
rg -n 'fmt\.Errorf\("%s",\s*err\.Error\(\)\)' submit/tx.go -C2
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@internal/api/api.go` around lines 391 - 393, The CBOR rejection branch in
internal/api/api.go (errors.As(err, &txRejectErr)) won't match because upstream
submit/tx.go flattens errors using fmt.Errorf("%s", err.Error()), which strips
the TransactionRejectedError type; fix submit/tx.go by preserving error
wrapping—either return the original error instead of reformatting it, or wrap
with %w (e.g., use fmt.Errorf("...: %w", err)) so TransactionRejectedError stays
in the chain; ensure the function(s) in submit/tx.go that produce these errors
propagate the wrapped error so the errors.As check in api.go can detect
TransactionRejectedError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@internal/api/api.go`:
- Around line 365-371: The handler reads the request body with
io.ReadAll(r.Body) (producing txRawBytes) without any size limit, allowing
memory DoS; wrap the request body with http.MaxBytesReader (or io.LimitedReader)
before calling io.ReadAll in the submit transaction handler (where txRawBytes
and txSubmitFailCount are used) and enforce a reasonable constant limit (e.g.,
maxTxBodySize) so oversized requests return an error via writeJSON and increment
txSubmitFailCount instead of allocating unbounded memory.
- Around line 391-393: The CBOR rejection branch in internal/api/api.go
(errors.As(err, &txRejectErr)) won't match because upstream submit/tx.go
flattens errors using fmt.Errorf("%s", err.Error()), which strips the
TransactionRejectedError type; fix submit/tx.go by preserving error
wrapping—either return the original error instead of reformatting it, or wrap
with %w (e.g., use fmt.Errorf("...: %w", err)) so TransactionRejectedError stays
in the chain; ensure the function(s) in submit/tx.go that produce these errors
propagate the wrapped error so the errors.As check in api.go can detect
TransactionRejectedError.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 5eab9c5f-0bab-4852-bda9-1dd5e6d7da50

📥 Commits

Reviewing files that changed from the base of the PR and between cdf09bf and 6f7f413.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (6)
  • .github/workflows/go-test.yml
  • go.mod
  • internal/api/api.go
  • internal/api/api_test.go
  • internal/logging/gin.go
  • internal/logging/logging.go
💤 Files with no reviewable changes (1)
  • internal/logging/gin.go
✅ Files skipped from review due to trivial changes (3)
  • .github/workflows/go-test.yml
  • go.mod
  • internal/api/api_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • internal/logging/logging.go

@verbotenj verbotenj force-pushed the feat/switch-to-net-http branch 2 times, most recently from 6001399 to d5a0169 Compare March 31, 2026 02:20
Signed-off-by: Ales Verbic <verbotenj@blinklabs.io>
@verbotenj verbotenj force-pushed the feat/switch-to-net-http branch from d5a0169 to 60bd15e Compare March 31, 2026 13:31
@verbotenj verbotenj merged commit f854315 into main Apr 3, 2026
11 checks passed
@verbotenj verbotenj deleted the feat/switch-to-net-http branch April 3, 2026 22:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants