Skip to content

🐛 fix(docker): read sockguard's reason field in docker error bodies - #80

Merged
scttbnsn merged 1 commit into
mainfrom
fix/deny-reason-field
Jul 29, 2026
Merged

🐛 fix(docker): read sockguard's reason field in docker error bodies#80
scttbnsn merged 1 commit into
mainfrom
fix/deny-reason-field

Conversation

@scttbnsn

@scttbnsn scttbnsn commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

#77 taught extractDockerErrorMessage (internal/docker/client.go) to parse the Docker error body's message field so denials reach the caller instead of a bare status code. But sockguard's DenialResponse struct (app/internal/filter/middleware.go in the sockguard repo) always sets message to the generic constant "request denied by sockguard policy" — the actual denial cause ("not allowed by portwing preset", "exec denied: privileged exec is not allowed", ...) lives in a separate reason field, populated only when sockguard's deny_verbosity is verbose (default is minimal, message-only). So #77's fix never actually surfaced sockguard's detailed reasons for the common case.

  • extractDockerErrorMessage now also reads reason. If both message and reason are non-empty and differ, they're combined as <message>: <reason>; if only one is present, it's used alone; the existing fallback to the raw trimmed body (when neither JSON field parses or both are empty) is unchanged, and plain Docker daemon errors ({"message": ...} only) behave exactly as before.
  • Extended internal/docker/client_dockererror_test.go with message+reason, reason-only, and identical-value cases, plus a call-site propagation test on CreateExec.
  • CHANGELOG and README updated to explain the reason field and sockguard's deny_verbosity setting.

Test plan

  • gofmt -l . clean
  • go vet ./...
  • go test ./internal/docker/...
  • go test across internal/... cmd/... (excluding internal/banner/gen, matching CI) — total 96.2%, floor is 96%

Changelog

  • ✨ Added support for Docker/sockguard reason field parsing (combined message: reason, reason-only, and identical-value handling).
  • 🔧 Updated Docker error extraction docs and CreateExec error propagation to include sockguard reason under verbose denial.
  • 🔧 Updated README sockguard guidance for deny_verbosity (detailed reason only with verbose; generic denial under minimal).
  • 🐛 Added/extended tests covering combined, reason-only, identical values, and CreateExec propagation.

Concerns / issues to address

  • Verify CHANGELOG content matches the README behavior for deny_verbosity: verbose vs minimal.
  • Ensure malformed/empty bodies still hit the existing fallback behavior for plain Docker errors.

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 54275274-d2b6-48bd-83cc-fd19b11a14fc

📥 Commits

Reviewing files that changed from the base of the PR and between 35f1f13 and 125f97d.

⛔ Files ignored due to path filters (1)
  • CHANGELOG.md is excluded by !CHANGELOG.md
📒 Files selected for processing (3)
  • README.md
  • internal/docker/client.go
  • internal/docker/client_dockererror_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
  • README.md
  • internal/docker/client_dockererror_test.go
  • internal/docker/client.go

📝 Walkthrough

Walkthrough

The Docker client now parses message and reason fields from structured error bodies, combines distinct values, avoids duplicate text, and falls back appropriately. Tests cover extraction and CreateExec propagation. The README documents verbose sockguard denial reasons and generic minimal-verbosity errors.

Possibly related PRs

  • CodesWhat/portwing#77: Modifies Docker exec error parsing and propagation in the same client code path.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
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.
✨ 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 fix/deny-reason-field

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.

@scttbnsn

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@scttbnsn

Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@scttbnsn
scttbnsn force-pushed the fix/deny-reason-field branch from 35f1f13 to 125f97d Compare July 29, 2026 04:55

@biggest-littlest biggest-littlest left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@ALARGECOMPANY ALARGECOMPANY left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@scttbnsn
scttbnsn merged commit 58e897f into main Jul 29, 2026
27 checks passed
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.

3 participants