[client] Restrict debug bundle log path and upload destinations - #6975
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (23)
🚧 Files skipped from review as they are similar to previous changes (18)
📝 WalkthroughWalkthroughDebug bundle handling now validates upload destinations and transport security, restricts UI log registration and access by caller identity, adds ownership-checked file opening, and propagates insecure-upload selection through CLI, daemon, platform, and job-execution paths. ChangesDebug bundle security
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant DebugClient
participant DebugBundleRPC
participant BundleGenerator
participant UploadDebugBundle
participant UploadService
DebugClient->>DebugBundleRPC: submit DebugBundleRequest
DebugBundleRPC->>BundleGenerator: generate bundle with caller UI log opener
BundleGenerator-->>DebugBundleRPC: bundle path and management URL
DebugBundleRPC->>UploadDebugBundle: upload with secure or insecure mode
UploadDebugBundle->>UploadService: request upload URL and upload bundle
UploadService-->>DebugBundleRPC: upload key
DebugBundleRPC-->>DebugClient: debug bundle response
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/server/debug_gate.go`:
- Around line 57-60: Update the insecure upload guidance in denyPrivileged and
avoid concatenating rawURL directly into the ElevatedCommand string. Build the
command using separate arguments or apply the project’s established
shell-quoting utility to rawURL, while preserving the existing command and flag
behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 710f34c0-e690-466c-a96b-9a419b5deb3e
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/client.goclient/cmd/debug.goclient/internal/debug/debug.goclient/internal/debug/debug_ios.goclient/internal/debug/debug_logfiles_test.goclient/internal/debug/uilog_test.goclient/internal/debug/upload.goclient/internal/debug/upload_test.goclient/internal/ipcauth/ownedfile.goclient/internal/ipcauth/ownedfile_test.goclient/internal/ipcauth/ownedfile_unix.goclient/internal/ipcauth/ownedfile_unix_test.goclient/internal/ipcauth/ownedfile_windows.goclient/internal/ipcauth/ownedfile_windows_test.goclient/ios/NetBirdSDK/client.goclient/jobexec/executor.goclient/proto/daemon.protoclient/server/debug.goclient/server/debug_gate.goclient/server/debug_gate_test.goclient/server/server.go
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
lixmal
force-pushed
the
debug-bundle-hardening
branch
from
July 30, 2026 10:37
d774f50 to
8fe6af7
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/internal/debug/upload.go`:
- Around line 36-48: Update uploadClient so secure mode configures CheckRedirect
to reject any redirect target whose scheme is not https, while preserving normal
HTTPS redirects and the existing insecure-client behavior. Ensure both
presigned-URL GET and bundle PUT, which use uploadClient(false), apply this
protection.
In `@client/server/debug.go`:
- Line 79: Update the bundle construction flow around UILogOpener and
uiLogOpener so UI-log collection is authorized against the current bundle
requester rather than always uiLogOwner; permit it only when the requester
matches uiLogOwner or is privileged, otherwise omit the opener. Add a regression
test covering a second local user requesting a default/no-upload bundle and
verifying the first user’s UI log is not collected or uploaded.
- Around line 163-165: Update the UI log path validation around filepath.Clean,
filepath.IsAbs, and filepath.Base to reject Windows UNC paths and \\.\ or \\?\
device prefixes before RegisterUILog can invoke OpenOwnedFile. Preserve
acceptance of valid absolute paths ending in uiLogFileName, and add
Windows-specific tests covering each rejected path form.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 7a06a226-d6e1-4c73-b1f9-a50806b41ddb
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/client.goclient/cmd/debug.goclient/internal/debug/debug.goclient/internal/debug/debug_ios.goclient/internal/debug/debug_logfiles_test.goclient/internal/debug/uilog_test.goclient/internal/debug/upload.goclient/internal/debug/upload_test.goclient/internal/ipcauth/ownedfile.goclient/internal/ipcauth/ownedfile_test.goclient/internal/ipcauth/ownedfile_unix.goclient/internal/ipcauth/ownedfile_unix_test.goclient/internal/ipcauth/ownedfile_windows.goclient/internal/ipcauth/ownedfile_windows_test.goclient/ios/NetBirdSDK/client.goclient/jobexec/executor.goclient/proto/daemon.protoclient/server/debug.goclient/server/debug_gate.goclient/server/debug_gate_test.goclient/server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/internal/debug/upload_test.go
lixmal
force-pushed
the
debug-bundle-hardening
branch
from
July 30, 2026 10:54
8fe6af7 to
94db225
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
client/server/debug.go (2)
163-166: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUNC paths still accepted by
RegisterUILog.
filepath.IsAbstreats Windows UNC paths (\\attacker\share\gui-client.log) and device prefixes (\\.\...,\\?\...) as absolute, so they pass this check and reachOpenOwnedFilebefore the regular-file/ownership guard runs, letting a local caller point the root daemon at a remote or device namespace.🐛 Proposed fix
path := filepath.Clean(req.GetPath()) - if !filepath.IsAbs(path) || filepath.Base(path) != uiLogFileName { + if !filepath.IsAbs(path) || filepath.Base(path) != uiLogFileName || strings.HasPrefix(path, `\\`) { return nil, gstatus.Errorf(codes.InvalidArgument, "UI log path must be an absolute path ending in %s", uiLogFileName) }Add Windows-specific test coverage for UNC and device-prefix paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/server/debug.go` around lines 163 - 166, Update RegisterUILog’s path validation to reject Windows UNC and device-prefix paths before OpenOwnedFile, while retaining acceptance of valid local absolute paths ending in uiLogFileName. Add Windows-specific tests covering both UNC forms and device-prefix paths, including assertions that they return InvalidArgument.
72-92: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUI-log bundle still not bound to the requesting caller.
UILogOpener: uiLogOpener(s.uiLogOwner)always authorizes with the registering identity, regardless of who is callingDebugBundlenow. A second local user requesting a bundle (no upload URL needed, so no privilege check applies) still causes the daemon to collect the first user's registered UI log, sinceuiLogOpeneronly checks that the file belongs tos.uiLogOwner, never that the currentcallerCtxmatches it.🐛 Proposed fix
func (s *Server) DebugBundle(callerCtx context.Context, req *proto.DebugBundleRequest) (resp *proto.DebugBundleResponse, err error) { if err := requirePrivilegeForUploadURL(callerCtx, req.GetUploadURL(), req.GetUploadInsecure()); err != nil { return nil, err } + callerID, hasCaller := ipcauth.CallerIdentity(callerCtx) + s.mutex.Lock() defer s.mutex.Unlock() + + var uiLogOwner *ipcauth.Identity + if hasCaller && s.uiLogOwner != nil && + (ipcauth.IsPrivilegedCaller(callerID) || callerID.String() == s.uiLogOwner.String()) { + uiLogOwner = s.uiLogOwner + }- UILogOpener: uiLogOpener(s.uiLogOwner), + UILogOpener: uiLogOpener(uiLogOwner),Also add a regression test covering a second caller requesting a bundle after a different caller registered a UI log path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/server/debug.go` around lines 72 - 92, Bind UI-log authorization to the requesting caller by updating the DebugBundle flow and uiLogOpener usage so the opener validates callerCtx against the current caller rather than only s.uiLogOwner. Preserve collection for the registering caller while preventing a different local caller from accessing that UI log, and add a regression test where one caller registers the path and another requests a bundle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/server/debug_gate.go`:
- Around line 57-70: Update the upload URL validation flow around parsed and
rawURL so url.Parse, non-empty host validation, and http/https scheme validation
occur before the insecure denyPrivileged branch. Allow the insecure override to
relax only HTTPS enforcement for HTTP or untrusted TLS URLs, while rejecting
file, mailto, hostless, and other unsupported schemes with InvalidArgument.
---
Duplicate comments:
In `@client/server/debug.go`:
- Around line 163-166: Update RegisterUILog’s path validation to reject Windows
UNC and device-prefix paths before OpenOwnedFile, while retaining acceptance of
valid local absolute paths ending in uiLogFileName. Add Windows-specific tests
covering both UNC forms and device-prefix paths, including assertions that they
return InvalidArgument.
- Around line 72-92: Bind UI-log authorization to the requesting caller by
updating the DebugBundle flow and uiLogOpener usage so the opener validates
callerCtx against the current caller rather than only s.uiLogOwner. Preserve
collection for the registering caller while preventing a different local caller
from accessing that UI log, and add a regression test where one caller registers
the path and another requests a bundle.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6284c0fd-9e9d-49dc-971d-dc99acc5d60e
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/client.goclient/cmd/debug.goclient/internal/debug/debug.goclient/internal/debug/debug_ios.goclient/internal/debug/debug_logfiles_test.goclient/internal/debug/uilog_test.goclient/internal/debug/upload.goclient/internal/debug/upload_test.goclient/internal/ipcauth/ownedfile.goclient/internal/ipcauth/ownedfile_test.goclient/internal/ipcauth/ownedfile_unix.goclient/internal/ipcauth/ownedfile_unix_test.goclient/internal/ipcauth/ownedfile_windows.goclient/internal/ipcauth/ownedfile_windows_test.goclient/ios/NetBirdSDK/client.goclient/jobexec/executor.goclient/proto/daemon.protoclient/server/debug.goclient/server/debug_gate.goclient/server/debug_gate_test.goclient/server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/internal/debug/upload_test.go
- client/proto/daemon.proto
lixmal
force-pushed
the
debug-bundle-hardening
branch
from
July 30, 2026 11:10
94db225 to
b01c113
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/internal/ipcauth/ownedfile_windows.go`:
- Around line 24-32: Release the security-descriptor buffer returned by
GetSecurityInfo with deferred windows.LocalFree immediately after each
successful call; add this cleanup in
client/internal/ipcauth/ownedfile_windows.go:24-32 and the test helper in
client/internal/ipcauth/ownedfile_windows_test.go:22-26, using the sd pointer
converted to windows.Handle while preserving sd.Owner() usage before function
return.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 88b52a7c-8e03-4c92-bdaa-b55051daac2c
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/client.goclient/cmd/debug.goclient/internal/debug/debug.goclient/internal/debug/debug_ios.goclient/internal/debug/debug_logfiles_test.goclient/internal/debug/uilog_test.goclient/internal/debug/upload.goclient/internal/debug/upload_test.goclient/internal/ipcauth/ownedfile.goclient/internal/ipcauth/ownedfile_test.goclient/internal/ipcauth/ownedfile_unix.goclient/internal/ipcauth/ownedfile_unix_test.goclient/internal/ipcauth/ownedfile_windows.goclient/internal/ipcauth/ownedfile_windows_test.goclient/ios/NetBirdSDK/client.goclient/jobexec/executor.goclient/proto/daemon.protoclient/server/debug.goclient/server/debug_gate.goclient/server/debug_gate_test.goclient/server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/jobexec/executor.go
- client/proto/daemon.proto
lixmal
force-pushed
the
debug-bundle-hardening
branch
from
July 30, 2026 11:52
b01c113 to
051d656
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
client/server/debug.go (2)
78-79: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUI-log collection still authorized by registering identity, not the current requester.
UILogOpener: uiLogOpener(s.uiLogOwner)opens the UI log as whoever last calledRegisterUILog, regardless of who is callingDebugBundlenow (callerCtxisn't consulted here). A second local user requesting a bundle can still cause the daemon to collect (and, with an upload URL, exfiltrate) the first user's UI log.This was already raised on a previous commit of this PR and appears unresolved in the current code.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/server/debug.go` around lines 78 - 79, Update the DebugBundle setup around UILogOpener and uiLogOpener so UI-log collection is authorized using the current requester’s callerCtx rather than the registered s.uiLogOwner identity. Ensure a requester cannot collect or upload another user’s UI log, while preserving the existing behavior for authorized callers.
163-166: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winUNC paths still bypass the absolute-path check in
RegisterUILog.
filepath.IsAbstreats Windows UNC paths (e.g.\\attacker\share\gui-client.log) as absolute, so they pass this check before being opened viaOpenOwnedFile, letting a local caller point the privileged daemon at a remote/device namespace. Block\\...\and\\.\/\\?\prefixes here.Raised previously on this PR; the code is unchanged since then.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/server/debug.go` around lines 163 - 166, Update RegisterUILog’s path validation after filepath.Clean to reject Windows UNC and device namespace prefixes (\\...\, \\.\, and \\?\) before allowing the existing absolute-path and basename checks. Preserve acceptance of valid local absolute paths ending in uiLogFileName and the current InvalidArgument response.
🧹 Nitpick comments (1)
client/internal/debug/upload_test.go (1)
19-54: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider adding negative-path coverage for HTTPS enforcement.
This file only exercises the
insecure=truepath. Nothing here verifies thatinsecure=falserejects anhttp://service URL, anhttp://presigned response URL, or a redirect to a non-https target (requireHTTPS/rejectInsecureRedirectinclient/internal/debug/upload.go). Since these are the core security guarantees this PR introduces, a couple of focused unit tests would meaningfully increase confidence without much effort.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/debug/upload_test.go` around lines 19 - 54, Extend the upload tests around TestUpload and the HTTPS validation helpers in upload.go to cover insecure=false rejecting an http service URL, an http presigned response URL, and redirects to non-HTTPS targets. Add focused negative-path tests that assert each request fails with the expected HTTPS enforcement error, while preserving the existing insecure=true success coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/internal/ipcauth/ownedfile_windows.go`:
- Around line 12-17: Update openForRead in
client/internal/ipcauth/ownedfile_windows.go:12-17 to validate the opened
descriptor using its returned handle’s Stat metadata, rejecting non-regular
files instead of resolving and checking the caller-supplied path. In
client/internal/debug/debug.go:1144-1186, sort rotated log candidates using Stat
results from handles returned by the supplied opener, not path-based os.Stat;
apply the regular-file validation to those descriptor-derived results.
---
Duplicate comments:
In `@client/server/debug.go`:
- Around line 78-79: Update the DebugBundle setup around UILogOpener and
uiLogOpener so UI-log collection is authorized using the current requester’s
callerCtx rather than the registered s.uiLogOwner identity. Ensure a requester
cannot collect or upload another user’s UI log, while preserving the existing
behavior for authorized callers.
- Around line 163-166: Update RegisterUILog’s path validation after
filepath.Clean to reject Windows UNC and device namespace prefixes (\\...\,
\\.\, and \\?\) before allowing the existing absolute-path and basename checks.
Preserve acceptance of valid local absolute paths ending in uiLogFileName and
the current InvalidArgument response.
---
Nitpick comments:
In `@client/internal/debug/upload_test.go`:
- Around line 19-54: Extend the upload tests around TestUpload and the HTTPS
validation helpers in upload.go to cover insecure=false rejecting an http
service URL, an http presigned response URL, and redirects to non-HTTPS targets.
Add focused negative-path tests that assert each request fails with the expected
HTTPS enforcement error, while preserving the existing insecure=true success
coverage.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6add1e9b-f55c-4b8a-8160-7deacb5de9b2
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/client.goclient/cmd/debug.goclient/internal/debug/debug.goclient/internal/debug/debug_ios.goclient/internal/debug/debug_logfiles_test.goclient/internal/debug/uilog_test.goclient/internal/debug/upload.goclient/internal/debug/upload_test.goclient/internal/ipcauth/ownedfile.goclient/internal/ipcauth/ownedfile_test.goclient/internal/ipcauth/ownedfile_unix.goclient/internal/ipcauth/ownedfile_unix_test.goclient/internal/ipcauth/ownedfile_windows.goclient/internal/ipcauth/ownedfile_windows_test.goclient/ios/NetBirdSDK/client.goclient/jobexec/executor.goclient/proto/daemon.protoclient/server/debug.goclient/server/debug_gate.goclient/server/debug_gate_test.goclient/server/server.go
lixmal
force-pushed
the
debug-bundle-hardening
branch
2 times, most recently
from
July 30, 2026 13:37
8dac459 to
734ff2e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
client/server/debug_gate.go (1)
20-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the UI log filename constant instead of duplicating it.
uiLogFileNameduplicatesdebug.uiLogFile("gui-client.log") in a separate package. The comment itself notes it "must stay in sync"; sincedebug.uiLogFileis unexported, nothing enforces that today; a future edit to one literal without the other would silently break UI log registration/validation or rotated-log discovery.♻️ Proposed fix
-// uiLogFileName is the only file name the daemon accepts as a UI log path. It -// must stay in sync with the name the desktop UI writes (client/ui/uilogpath.go) -// and with the prefix the bundle globs for rotated siblings. -const uiLogFileName = "gui-client.log" +// uiLogFileName is the only file name the daemon accepts as a UI log path. +// It must stay in sync with the name the desktop UI writes +// (client/ui/uilogpath.go). +const uiLogFileName = debug.UILogFileAnd in
client/internal/debug/debug.go, export the constant:- uiLogFile = "gui-client.log" + UILogFile = "gui-client.log"(update the package-local uses of
uiLogFiletoUILogFileaccordingly)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/server/debug_gate.go` around lines 20 - 23, Export the debug package’s uiLogFile constant as UILogFile, update all package-local references accordingly, and replace the duplicate uiLogFileName declaration in the daemon code with the exported constant. Preserve the existing filename and validation/globbing behavior while ensuring both packages use the single shared symbol.client/server/debug_gate_test.go (1)
81-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive case: the legitimate owner (or a privileged caller) can open the file.
Both assertions here are refusals. Nothing confirms
uiLogOpeneractually succeeds for a requester who owns the file, so a future regression that over-tightens the check would slip through.✅ Suggested addition
// No verified identity: refused. if _, err := uiLogOpener(ipcauth.Identity{}, false)(path); err == nil { t.Fatal("expected an unidentified requester to be refused") } + + // The file's actual owner: allowed. + if _, err := uiLogOpener(ownerIdentity(), true)(path); err != nil { + t.Fatalf("expected the owning requester to be allowed, got %v", err) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/server/debug_gate_test.go` around lines 81 - 96, Add a successful-opening assertion to TestUILogOpenerBindsToRequester using the file owner identity or a privileged caller, and verify uiLogOpener returns the file without an error. Keep the existing refusal assertions for an unprivileged different user and an unverified identity.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/server/debug.go`:
- Around line 36-37: Bound the network work in the debug-bundle upload path
before acquiring s.mutex: ensure UploadDebugBundle receives a context with a
finite deadline, matching the mobile callers’ 2-minute timeout, so presigned-url
discovery and PUT cannot hold the lock indefinitely. Update the relevant debug
handler around UploadDebugBundle while preserving the existing mutex-protected
state handling.
---
Nitpick comments:
In `@client/server/debug_gate_test.go`:
- Around line 81-96: Add a successful-opening assertion to
TestUILogOpenerBindsToRequester using the file owner identity or a privileged
caller, and verify uiLogOpener returns the file without an error. Keep the
existing refusal assertions for an unprivileged different user and an unverified
identity.
In `@client/server/debug_gate.go`:
- Around line 20-23: Export the debug package’s uiLogFile constant as UILogFile,
update all package-local references accordingly, and replace the duplicate
uiLogFileName declaration in the daemon code with the exported constant.
Preserve the existing filename and validation/globbing behavior while ensuring
both packages use the single shared symbol.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3f26a4c0-917e-49a2-aecc-6c04f1fe0d21
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/client.goclient/cmd/debug.goclient/internal/debug/debug.goclient/internal/debug/debug_ios.goclient/internal/debug/debug_logfiles_test.goclient/internal/debug/uilog_test.goclient/internal/debug/upload.goclient/internal/debug/upload_test.goclient/internal/ipcauth/ownedfile.goclient/internal/ipcauth/ownedfile_test.goclient/internal/ipcauth/ownedfile_unix.goclient/internal/ipcauth/ownedfile_unix_test.goclient/internal/ipcauth/ownedfile_windows.goclient/internal/ipcauth/ownedfile_windows_test.goclient/ios/NetBirdSDK/client.goclient/jobexec/executor.goclient/proto/daemon.protoclient/server/debug.goclient/server/debug_gate.goclient/server/debug_gate_test.goclient/server/server.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/internal/debug/debug_ios.go
- client/server/server.go
lixmal
force-pushed
the
debug-bundle-hardening
branch
from
July 30, 2026 14:17
734ff2e to
e324a96
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/debug/upload_test.go (1)
19-53: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover the secure-mode rejection path.
This test only verifies
insecure=true. Add a companion assertion thatUploadDebugBundle(..., false)rejects the HTTP service URL, preventing an accidental weakening of the secure default.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/debug/upload_test.go` around lines 19 - 53, The TestUpload test only exercises the insecure upload path; add a companion call to UploadDebugBundle with insecure set to false against the HTTP testURL and assert that it returns an error, while preserving the existing successful upload assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@client/internal/debug/upload_test.go`:
- Around line 19-53: The TestUpload test only exercises the insecure upload
path; add a companion call to UploadDebugBundle with insecure set to false
against the HTTP testURL and assert that it returns an error, while preserving
the existing successful upload assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 852e5f51-f184-4df2-8989-c190d104fd1d
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/client.goclient/cmd/debug.goclient/internal/debug/debug.goclient/internal/debug/debug_ios.goclient/internal/debug/debug_logfiles_test.goclient/internal/debug/uilog_test.goclient/internal/debug/upload.goclient/internal/debug/upload_test.goclient/internal/ipcauth/ownedfile.goclient/internal/ipcauth/ownedfile_test.goclient/internal/ipcauth/ownedfile_unix.goclient/internal/ipcauth/ownedfile_unix_test.goclient/internal/ipcauth/ownedfile_windows.goclient/internal/ipcauth/ownedfile_windows_test.goclient/ios/NetBirdSDK/client.goclient/jobexec/executor.goclient/proto/daemon.protoclient/server/debug.goclient/server/debug_gate.goclient/server/debug_gate_test.goclient/server/server.go
🚧 Files skipped from review as they are similar to previous changes (1)
- client/server/server.go
riccardomanfrin
left a comment
riccardomanfrin
left a comment
Contributor
There was a problem hiding this comment.
gui-client.logis defined in three places.**debug.UILogFilein
client/internal/debug/debug.go:233, theuiLogFileNamealias in
client/server/debug_gate.go:23LogOpeneris threaded through four methods but only one call site needs
a non-default opener. maybe can be resolved in place where needed and have reduced signature args.getUploadURLbuilds the query by string concatenation.
|
|
||
| err = upload(ctx, filePath, response) | ||
| if !insecure { | ||
| if err := requireHTTPS("upload URL from service", response.URL); err != nil { |
Contributor
There was a problem hiding this comment.
should apply the other remediations of GHSA-7rcm (prefix/host match, reject
RFC1918, signed URL)
lixmal
force-pushed
the
debug-bundle-hardening
branch
2 times, most recently
from
July 31, 2026 14:50
3175d59 to
e223595
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/internal/ipcauth/ownedfile_test.go (1)
46-56: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSafely reject symlinks in
OpenOwnedFile.
openForReadalready usesO_NOFOLLOWon Unix, andTestOpenOwnedFileRefusesSymlinkcovers a symlink owned by the caller that points to another user’s file. Add or keep a comparable Windows coverage so this platform path has the same protection instead of using plainos.Open.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/ipcauth/ownedfile_test.go` around lines 46 - 56, Update OpenOwnedFile and its openForRead platform implementations to reject symlinks consistently on Windows, matching the Unix O_NOFOLLOW behavior instead of using plain os.Open. Add or retain a Windows-specific TestOpenOwnedFileRefusesSymlink covering a caller-owned symlink, while preserving existing ownership and regular-file validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/server/debug_gate_test.go`:
- Around line 31-64: Update the "traversal onto another file" case in
TestRegisterUILogRefusesForeignPath so it constructs a path that reliably
escapes the temporary directory and reaches the intended foreign target while
retaining the uiLogFileName suffix. Ensure the test exercises traversal
protection rather than being rejected only for an incorrect filename.
---
Nitpick comments:
In `@client/internal/ipcauth/ownedfile_test.go`:
- Around line 46-56: Update OpenOwnedFile and its openForRead platform
implementations to reject symlinks consistently on Windows, matching the Unix
O_NOFOLLOW behavior instead of using plain os.Open. Add or retain a
Windows-specific TestOpenOwnedFileRefusesSymlink covering a caller-owned
symlink, while preserving existing ownership and regular-file validation.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 389d42fd-a1fc-4621-9d7f-fb79b18a5b48
⛔ Files ignored due to path filters (1)
client/proto/daemon.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
client/android/client.goclient/cmd/debug.goclient/internal/debug/debug.goclient/internal/debug/debug_ios.goclient/internal/debug/debug_logfiles_test.goclient/internal/debug/uilog_test.goclient/internal/debug/upload.goclient/internal/debug/upload_test.goclient/internal/ipcauth/ownedfile.goclient/internal/ipcauth/ownedfile_test.goclient/internal/ipcauth/ownedfile_unix.goclient/internal/ipcauth/ownedfile_unix_test.goclient/internal/ipcauth/ownedfile_windows.goclient/internal/ipcauth/ownedfile_windows_test.goclient/ios/NetBirdSDK/client.goclient/jobexec/executor.goclient/proto/daemon.protoclient/server/debug.goclient/server/debug_gate.goclient/server/debug_gate_test.goclient/server/server.go
🚧 Files skipped from review as they are similar to previous changes (17)
- client/server/server.go
- client/jobexec/executor.go
- client/internal/debug/debug_ios.go
- client/android/client.go
- client/proto/daemon.proto
- client/internal/ipcauth/ownedfile_windows.go
- client/ios/NetBirdSDK/client.go
- client/internal/ipcauth/ownedfile_unix.go
- client/internal/ipcauth/ownedfile.go
- client/internal/ipcauth/ownedfile_unix_test.go
- client/cmd/debug.go
- client/internal/debug/uilog_test.go
- client/internal/debug/debug_logfiles_test.go
- client/server/debug_gate.go
- client/server/debug.go
- client/internal/debug/debug.go
- client/internal/ipcauth/ownedfile_windows_test.go
Collaborator
Author
Fixed
I'm not sure I have a good solution for that without making the code worse
Fixed |
lixmal
force-pushed
the
debug-bundle-hardening
branch
from
July 31, 2026 15:40
e223595 to
9f36909
Compare
|
riccardomanfrin
approved these changes
Jul 31, 2026
This was referenced Aug 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Describe your changes
Tightens how the daemon handles debug-bundle inputs that come over the local control socket. The UI log path is now validated and opened on behalf of the caller that registered it, and upload destinations are constrained so the root daemon does not fetch or send bundles to arbitrary endpoints.
gui-client.log) and open it as the registering caller, skipping files it does not own--upload-bundle-insecurefor self-hosted upload servers on http or with untrusted TLS (privileged only)Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#894
Summary by CodeRabbit
New Features
--upload-bundle-insecurefor privileged, opt-in uploads to non-HTTPS destinations.Bug Fixes
Tests