Skip to content

🛡️ Sentinel: [Medium] Fix silent failures from unhandled json.Marshal errors#113

Open
mattjoyce wants to merge 1 commit into
mainfrom
sentinel-fix-json-marshal-errors-13405454416954503388
Open

🛡️ Sentinel: [Medium] Fix silent failures from unhandled json.Marshal errors#113
mattjoyce wants to merge 1 commit into
mainfrom
sentinel-fix-json-marshal-errors-13405454416954503388

Conversation

@mattjoyce
Copy link
Copy Markdown
Owner

@mattjoyce mattjoyce commented Jun 3, 2026

🚨 Severity: MEDIUM
💡 Vulnerability: Errors returned by json.Marshal were ignored via the blank identifier (_), leading to silent failures.
🎯 Impact: This could result in unexpected behavior, and potentially unlogged or nil payloads being enqueued to the pipeline without any indication to the client.
🔧 Fix: Captured the error, securely logged it without leaking internal details, and safely returned an HTTP 500 status code back to the caller.
✅ Verification: Ran go test -v ./... and go test -v ./internal/api/... to ensure tests passed and no regressions were introduced.


PR created automatically by Jules for task 13405454416954503388 started by @mattjoyce

Summary by CodeRabbit

Bug Fixes

  • Fixed silent failures when serializing event payloads. Previously, errors during serialization were ignored, preventing proper error reporting. The system now properly detects serialization errors, logs them securely, and returns HTTP 500 responses to notify clients of failures.

Review Change Stack

Errors returned by `json.Marshal` were ignored via the blank identifier (`_`), which could lead to silent failures, unexpected behavior, and potentially unlogged or nil payloads being enqueued to the pipeline without any indication to the client. By capturing the error, securely logging it, and returning an HTTP 500 status code, we improve the reliability and defensive posture of the API endpoints.

Co-authored-by: mattjoyce <278869+mattjoyce@users.noreply.github.com>
@google-labs-jules
Copy link
Copy Markdown
Contributor

👋 Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Jun 3, 2026

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
ductile c1b5abb Commit Preview URL

Branch Preview URL
Jun 03 2026, 11:52 AM

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 3, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b0cbd868-ea81-4b94-a2b3-1f5b9d9e18a0

📥 Commits

Reviewing files that changed from the base of the PR and between cf887ea and c1b5abb.

📒 Files selected for processing (2)
  • .jules/sentinel.md
  • internal/api/handlers.go

📝 Walkthrough

Walkthrough

The PR adds error handling for JSON serialization in two event handler functions. Previously, json.Marshal errors on protocol.Event payloads were silently ignored during pipeline and plugin trigger command dispatch. Now, serialization errors are captured, logged, and returned as HTTP 500 responses with a failure message.

Changes

JSON Serialization Error Handling

Layer / File(s) Summary
Event payload serialization error handling
internal/api/handlers.go, .jules/sentinel.md
Pipeline and plugin trigger handlers now capture json.Marshal errors when wrapping event payloads for handle commands, log failures, and return HTTP 500 instead of silently ignoring errors. The fix is documented in the sentinel log.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

Poem

🐰 JSON errors hiding? Not on our watch today!
Marshal failures surfaced—five-hundred the way.
Silent no more, our handlers stand tall,
Logging each hiccup, returning with call.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: fixing silent failures from unhandled json.Marshal errors in the Sentinel codebase.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sentinel-fix-json-marshal-errors-13405454416954503388

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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

@qodo-code-review
Copy link
Copy Markdown

Review Summary by Qodo

Fix silent failures from unhandled json.Marshal errors

🐞 Bug fix

Grey Divider

Walkthroughs

Description
• Handle json.Marshal errors in two API handlers
• Log errors securely without leaking internal details
• Return HTTP 500 status on serialization failures
• Add sentinel documentation for vulnerability prevention
Diagram
flowchart LR
  A["json.Marshal call"] --> B{"Error occurred?"}
  B -->|Previously ignored| C["Silent failure"]
  B -->|Now handled| D["Log error securely"]
  D --> E["Return HTTP 500"]
  E --> F["Client notified"]

Loading

Grey Divider

File Changes

1. internal/api/handlers.go 🐞 Bug fix +14/-2

Add error handling for json.Marshal operations

• Added error handling for json.Marshal in handlePipelineTrigger function
• Added error handling for json.Marshal in handlePluginTrigger function
• Both locations now log errors and return HTTP 500 status on serialization failures
• Prevents silent failures and nil payloads from being enqueued

internal/api/handlers.go


2. .jules/sentinel.md 📝 Documentation +4/-0

Document json.Marshal error handling vulnerability

• Documents the vulnerability of ignored json.Marshal errors
• Explains the security and reliability impact of the issue
• Provides prevention guidance for future development
• References the "Fail securely" principle

.jules/sentinel.md


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

qodo-code-review Bot commented Jun 3, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0)

Context used
✅ Compliance rules (platform): 46 rules

Grey Divider


Remediation recommended

1. Contextless marshal error logs 🐞 Bug ◔ Observability
Description
The new json.Marshal error logs in handlePipelineTrigger and handlePluginTrigger only log the raw
error without pipeline/plugin/command identifiers, making it difficult to correlate serialization
failures to the triggering route/dispatch. This reduces observability compared to nearby
enqueue/context-creation error logs that include identifying fields.
Code

internal/api/handlers.go[246]

Evidence
The added marshal-failure log statements only include a static message and the error, while other
error logs in the same handlers include pipeline/plugin/command fields for correlation.

internal/api/handlers.go[240-268]
internal/api/handlers.go[410-442]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`json.Marshal` failures are now logged, but the log entries lack identifying context (e.g., pipeline name, plugin name, command, step_id/dispatch index). This makes it hard to trace which API call / route produced the error.

### Issue Context
Other error logs in the same handlers include contextual key/value pairs such as `pipeline`, `plugin`, and `command`, which makes incident triage much easier.

### Fix
- In `handlePipelineTrigger`, include fields like: `pipeline` (pipelineName), `plugin` (d.Plugin), `command` (d.Command), and optionally `step_id` (contextPlans[i].stepID when available) / `dispatch_index` (i).
- In `handlePluginTrigger`, include: `plugin` (pluginName) and `command` (commandName).

### Fix Focus Areas
- internal/api/handlers.go[240-268]
- internal/api/handlers.go[410-442]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@qodo-code-review
Copy link
Copy Markdown

CI Feedback 🧐

A test triggered by this PR failed. Here is an AI-generated analysis of the failure:

Action: fast-validation

Failed stage: Run fast tests [❌]

Failed test name: TestSpawnPluginFailsWhenStdoutExceedsLimit

Failure summary:

The action failed because the Go test suite (go test ./...) had a failing test:
TestSpawnPluginFailsWhenStdoutExceedsLimit.
- In output_limit_test.go:49, the test expected an
outputLimitError, but instead got start process: fork/exec
/tmp/TestSpawnPluginFailsWhenStdoutExceedsLimit.../plugin.sh: text file busy.
- This caused go test
to exit with FAIL and the workflow step ./scripts/test-fast to end with exit code 1.

Relevant error logs:
1:  ##[group]Runner Image Provisioner
2:  Hosted Compute Agent
...

187:  GOVERSION='go1.25.0'
188:  GOWORK=''
189:  PKG_CONFIG='pkg-config'
190:  ##[endgroup]
191:  ##[group]Run ./scripts/test-fast
192:  �[36;1m./scripts/test-fast�[0m
193:  shell: /usr/bin/bash -e {0}
194:  ##[endgroup]
195:  ==> test-fast: go test ./...
196:  ok  	github.com/mattjoyce/ductile/cmd/ductile	4.654s
197:  ok  	github.com/mattjoyce/ductile/internal/api	2.538s
198:  ok  	github.com/mattjoyce/ductile/internal/auth	0.005s
199:  ok  	github.com/mattjoyce/ductile/internal/baggage	(cached)
200:  ok  	github.com/mattjoyce/ductile/internal/config	0.102s
201:  ok  	github.com/mattjoyce/ductile/internal/configsnapshot	0.029s
202:  {"time":"2026-06-03T11:51:55.947877725Z","level":"ERROR","msg":"failed to decode plugin response","component":"dispatch","job_id":"b91946de-db3b-4b8b-8bb0-041560b4decc","plugin":"broken","command":"poll","error":"plugin output is not valid JSON: invalid character 'o' in literal null (expecting 'u')","stdout":"not valid json\n"}
203:  {"time":"2026-06-03T11:51:55.948124563Z","level":"ERROR","msg":"plugin spawn failed: decode response: plugin output is not valid JSON: invalid character 'o' in literal null (expecting 'u')","component":"dispatch","job_id":"b91946de-db3b-4b8b-8bb0-041560b4decc","plugin":"broken","command":"poll"}
204:  {"time":"2026-06-03T11:51:55.974042745Z","level":"ERROR","msg":"apply with remap for with-pipeline/notify: message: resolve \"payload.missing\": path not found","component":"dispatch","job_id":"ecdd171f-e6e0-477f-80da-ebcf3c6c7ca0","plugin":"echo","command":"handle"}
205:  --- FAIL: TestSpawnPluginFailsWhenStdoutExceedsLimit (0.00s)
206:  output_limit_test.go:49: spawnPlugin error = start process: fork/exec /tmp/TestSpawnPluginFailsWhenStdoutExceedsLimit1761370230/001/plugin.sh: text file busy, want outputLimitError
207:  {"time":"2026-06-03T11:52:08.27553811Z","level":"ERROR","msg":"failed to decode plugin response","error":"plugin output is not valid JSON: invalid character 'n' looking for beginning of object key string","stdout":"{not-json\n"}
208:  FAIL
...

216:  ok  	github.com/mattjoyce/ductile/internal/log	0.003s
217:  ok  	github.com/mattjoyce/ductile/internal/plugin	0.026s
218:  ok  	github.com/mattjoyce/ductile/internal/protocol	0.004s
219:  ok  	github.com/mattjoyce/ductile/internal/queue	0.312s
220:  ok  	github.com/mattjoyce/ductile/internal/relay	0.219s
221:  ok  	github.com/mattjoyce/ductile/internal/router	0.008s
222:  ok  	github.com/mattjoyce/ductile/internal/router/conditions	0.003s
223:  ok  	github.com/mattjoyce/ductile/internal/router/dsl	0.006s
224:  ok  	github.com/mattjoyce/ductile/internal/scheduleexpr	0.002s
225:  ok  	github.com/mattjoyce/ductile/internal/scheduler	0.004s
226:  ok  	github.com/mattjoyce/ductile/internal/state	0.327s
227:  ok  	github.com/mattjoyce/ductile/internal/stopwatch	0.007s
228:  ok  	github.com/mattjoyce/ductile/internal/storage	0.056s
229:  ok  	github.com/mattjoyce/ductile/internal/webhook	0.038s
230:  FAIL
231:  ##[error]Process completed with exit code 1.
232:  Post job cleanup.

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.

1 participant