fix: address 10 security, stability, and correctness issues

[CybotTM]

Summary

Comprehensive codebase review uncovered 10 security, stability, and correctness issues. Each fix has dedicated tests written first (TDD), all passing with -race detection.

Security fixes (4)

• Credential leak via API — json:"-" on WebPasswordHash/WebSecretKey to hide them from /api/config
• CSRF bypass — Remove X-Requested-With header bypass that allowed skipping CSRF validation
• Rate limiter memory DoS — Implement CleanupOldLimiters with lastAccess tracking (was an empty stub)
• IP spoofing bypass — Only trust X-Forwarded-For/X-Real-IP from loopback addresses

Stability fixes (2)

• Double-close panic — sync.Once on daemon done channel to prevent crash when multiple goroutines detect errors
• Concurrent map crash — sync.RWMutex on Config job maps to prevent crash when Docker events race with config reload

Correctness fixes (4)

• Workflow dependencies broken — DependencyProvider interface instead of *BareJob type assertion so depends-on/on-success/on-failure work for ExecJob, RunJob, etc.
• Docker ops ignore cancellation — Propagate ctx.Ctx instead of context.Background() to Docker API calls
• Shutdown priority ignored — Execute hooks in priority groups (sequential between groups, concurrent within)
• Swarm exit codes swallowed — Return NonZeroExitError for non-zero service task exit codes

Test plan

• All new tests written before fixes (TDD)
• go test -race ./core/ ./web/ ./cli/ ./config/ ./middlewares/ ./metrics/ — all pass
• go vet ./... — clean
• Each commit is atomic and independently buildable
• CI passes

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[github-actions]

Automated approval for solo maintainer project

All CI checks passed. See SECURITY.md for compensating controls.

[github-actions]

✅ Mutation Testing Results

Mutation Score: 71.88% (threshold: 60%)

✨ Good job! Mutation score meets the threshold.

What is mutation testing?

Mutation testing measures test quality by introducing small changes (mutations) to the code and checking if tests detect them. A higher score means better test effectiveness.

• Killed mutants: Tests caught the mutation (good!)
• Survived mutants: Tests missed the mutation (needs improvement)

[Copilot]

Pull request overview

Addresses a set of security, stability, and correctness issues across the web auth layer, scheduler/workflow wiring, Docker job execution, and daemon/config concurrency.

Changes:

• Harden web auth and request handling (CSRF enforcement, rate-limiter cleanup, and forwarded-IP trust rules) and update/add tests accordingly.
• Fix core correctness around workflow dependency extraction, Docker context cancellation propagation, shutdown hook ordering, and swarm service exit-code handling.
• Improve daemon/config robustness (idempotent done-channel close, mutex-protected config job maps, and updated tests).

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
web/middleware.go	Rate limiter now extracts host from RemoteAddr and only trusts XFF from loopback.
web/middleware_mutation_test.go	Align tests with host-only rate-limit keying and trusted-loopback XFF behavior.
web/auth_secure.go	Implement limiter cleanup with last-access tracking; enforce CSRF token for all login requests; tighten client IP extraction.
web/auth_secure_ext_test.go	Add/adjust tests for CSRF bypass removal, limiter cleanup, and trusted proxy handling in getClientIP.
web/auth_secure_coverage_unit_test.go	Update cleanup tests for new CleanupOldLimiters signature and behavior.
web/auth_integration_test.go	Fetch CSRF token via /api/csrf-token and use it for /api/login integration flows.
web/missing_coverage_test.go	Update cleanup invocation to pass maxAge.
core/workflow.go	Replace *BareJob assertion with DependencyProvider interface to support embedded BareJob types.
core/workflow_test.go	Update/add tests ensuring dependencies are collected for embedded BareJob job types (incl. ExecJob).
core/execjob.go	Propagate ctx.Ctx to provider instead of context.Background().
core/runjob.go	Propagate ctx.Ctx through Docker operations for cancellation correctness.
core/runservice.go	Propagate ctx.Ctx; return NonZeroExitError for non-zero swarm task exit codes.
core/runservice_unit_test.go	Add tests for watchContainer/run non-zero exit behavior (needs loop-var capture fix for parallel subtests).
core/context_propagation_test.go	New tests verifying ctx propagation into provider calls for ExecJob/RunJob/RunServiceJob.
core/bare_job.go	Add dependency accessors to support DependencyProvider interface.
core/shutdown.go	Change shutdown hook execution to run concurrently within priority groups and sequentially between groups.
core/shutdown_test.go	Add tests for priority-group ordering and same-priority concurrency.
cli/daemon.go	Prevent double-close panics on done channel via sync.Once + helper.
cli/daemon_closedone_test.go	New tests verifying closeDone is safe concurrently and sequentially.
cli/config.go	Add RWMutex to protect job maps; lock around label/INI sync and use scheduler accessors for middleware updates.
cli/config_test.go	Fix loop variable capture and avoid deep-equality on Config containing a mutex by comparing relevant fields.
cli/config_security_test.go	Ensure JSON marshaling of Global config does not leak secret fields.
cli/config_concurrent_test.go	New race-focused tests validating locked concurrent access patterns to job maps.

Comments suppressed due to low confidence (1)

core/shutdown.go:153

• When shutdown hooks return errors, Shutdown() currently wraps them with ErrShutdownTimeout ("shutdown timed out") even though no timeout occurred. This makes it hard for callers to distinguish hook failures from an actual timeout and is semantically incorrect. Consider returning a dedicated hook-failure sentinel (or errors.Join of the hook errors) and reserve ErrShutdownTimeout only for ctx deadline expiry.

	sm.logger.Info("Graceful shutdown completed successfully")

	if len(shutdownErrors) > 0 {
		return fmt.Errorf("%w: %d errors occurred", ErrShutdownTimeout, len(shutdownErrors))
	}

[gemini-code-assist]

Summary of Changes

Hello, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request delivers a significant upgrade to the codebase's resilience and security posture. It systematically addresses a total of ten identified issues across security, stability, and correctness domains. The changes ensure that sensitive data is protected, concurrent operations are safe, and critical system behaviors, such as job dependencies and graceful shutdowns, function as expected under various conditions. The thorough test coverage provides confidence in the reliability of these improvements.

Highlights

• Security Fixes: Implemented four critical security enhancements: preventing credential leaks by hiding sensitive web authentication fields from API responses, closing a CSRF bypass vulnerability by enforcing CSRF token validation for all login requests, mitigating a rate limiter memory denial-of-service (DoS) attack by actively cleaning up old rate limiter entries, and preventing IP spoofing by only trusting X-Forwarded-For/X-Real-IP headers when the request originates from a loopback address.
• Stability Improvements: Addressed two stability issues: preventing a double-close panic in the daemon's shutdown mechanism by using sync.Once to ensure the done channel is closed exactly once, and resolving concurrent map access crashes in configuration job maps by introducing sync.RWMutex for safe read/write operations during Docker event processing and config reloads.
• Correctness Enhancements: Fixed four correctness issues: ensuring workflow dependencies (depends-on, on-success, on-failure) function correctly for all job types (ExecJob, RunJob, etc.) by using a DependencyProvider interface instead of a concrete type assertion, propagating context.Context to Docker API calls in ExecJob, RunJob, and RunServiceJob to enable proper cancellation and timeouts, enforcing shutdown hook execution based on priority groups (sequential between groups, concurrent within), and correctly reporting non-zero exit codes for Docker Swarm service tasks.
• Comprehensive Testing: All fixes are accompanied by new, dedicated tests written using a Test-Driven Development (TDD) approach. Extensive testing with go test -race and go vet confirms the absence of data races and other common issues, ensuring the robustness of the implemented solutions.

Changelog

• cli/config.go
  • Added sync package import.
  • Marked WebPasswordHash and WebSecretKey fields with json:"-" to prevent their serialization in JSON output.
  • Introduced sync.RWMutex (mu) to protect concurrent access to job maps.
  • Wrapped syncJobMap calls within dockerContainersUpdate with mu.Lock() and mu.Unlock().
  • Added mu.Lock() and defer mu.Unlock() to iniConfigUpdate to synchronize configuration updates.
  • Updated iniConfigUpdate to retrieve all jobs using c.sh.GetActiveJobs() and c.sh.GetDisabledJobs() for middleware updates.
• cli/config_concurrent_test.go
  • Added new test file to verify concurrent read/write access to Config job maps using sync.RWMutex.
• cli/config_security_test.go
  • Added new test file to ensure WebPasswordHash and WebSecretKey are not leaked in JSON API responses.
• cli/config_test.go
  • Updated for loops in TestConfigIni and TestLabelsConfig to use for i := range for compatibility with t.Parallel().
  • Modified assertions in TestLabelsConfig to compare individual job maps and global config for more precise testing.
• cli/daemon.go
  • Added sync package import.
  • Introduced sync.Once (doneOnce) to the DaemonCommand struct to protect the done channel.
  • Added closeDone() method to safely close the done channel at most once.
  • Replaced direct close(c.done) calls with c.closeDone() in start() method.
• cli/daemon_closedone_test.go
  • Added new test file to verify the closeDone() method handles multiple concurrent and sequential calls without panicking.
• core/bare_job.go
  • Added GetDependencies(), GetOnSuccess(), and GetOnFailure() methods to BareJob to expose workflow dependency information.
• core/context_propagation_test.go
  • Added new test file to verify that context.Context is correctly propagated to Docker API calls within ExecJob, RunJob, and RunServiceJob.
• core/execjob.go
  • Changed context.Background() to ctx.Ctx when calling j.Provider.RunExec() to ensure context propagation.
• core/runjob.go
  • Replaced context.Background() with ctx.Ctx for all Docker provider calls (ensureImageAvailable, createOrInspectContainer, deleteContainer, startAndWait) to enable context propagation.
• core/runservice.go
  • Replaced context.Background() with ctx.Ctx for all Docker provider calls (EnsureImage, buildService, watchContainer, deleteService) to enable context propagation.
  • Modified watchContainer to return NonZeroExitError for non-zero service task exit codes and ErrUnexpected for ExitCodeSwarmError.
• core/runservice_unit_test.go
  • Added unit tests for watchContainer and Run methods to verify correct error handling for non-zero exit codes from Docker Swarm services.
• core/shutdown.go
  • Added sort package import.
  • Refactored ShutdownManager.Shutdown() to execute hooks in priority groups, ensuring lower priority groups complete before higher priority groups start, while hooks within the same group run concurrently.
• core/shutdown_test.go
  • Added tests to verify the correct priority ordering and concurrent execution of shutdown hooks.
• core/workflow.go
  • Defined a new DependencyProvider interface to abstract access to job dependency information.
  • Updated collectDependencyEdges to use the DependencyProvider interface, allowing all job types embedding BareJob to correctly declare workflow dependencies.
• core/workflow_test.go
  • Modified TestCollectDependencyEdges_NonBareJobCollected to assert that jobs embedding BareJob correctly provide dependency edges.
  • Updated TestCollectDependencyEdges_MixedJobTypes to reflect that both BareJob and embedded job types contribute edges.
  • Added TestCollectDependencyEdges_ExecJobWithDependencies to specifically test dependency collection for ExecJob via the new interface.
• web/auth_integration_test.go
  • Added helper functions fetchCSRFToken and loginWithCSRF to streamline authentication tests.
  • Updated all login-related integration tests to use the new loginWithCSRF helper, ensuring CSRF token validation is part of the test flow and removing the X-Requested-With header.
• web/auth_secure.go
  • Added lastAccess map to RateLimiter to track the last access time for each limiter.
  • Updated GetLimiter to record the current time in lastAccess upon retrieval.
  • Implemented CleanupOldLimiters to remove rate limiters and their lastAccess entries that are older than a specified maxAge.
  • Removed the X-Requested-With: XMLHttpRequest header bypass from CSRF token validation in SecureLoginHandler.ServeHTTP, enforcing CSRF for all login requests.
  • Modified getClientIP to only trust X-Forwarded-For and X-Real-IP headers if the direct client connection originates from a loopback address.
• web/auth_secure_coverage_unit_test.go
  • Updated TestRateLimiter_CleanupOldLimiters to pass a maxAge parameter to CleanupOldLimiters.
• web/auth_secure_ext_test.go
  • Updated calls to CleanupOldLimiters to include the maxAge parameter.
  • Added tests for RateLimiterCleanupRemovesOldEntries and RateLimiterCleanupPreservesRecentEntries to verify the new cleanup logic.
  • Added TestCSRF_XRequestedWithBypassRemoved to confirm the removal of the CSRF bypass.
  • Added tests for TestGetClientIP_IgnoresXFFFromNonLoopback, TestGetClientIP_TrustsXFFFromLoopback, and TestGetClientIP_IPv6Loopback to validate the trusted proxy IP extraction logic.
• web/middleware.go
  • Added net and strings package imports.
  • Introduced extractRemoteIP and isLoopback helper functions for robust IP address parsing and loopback detection.
  • Modified the rate limiter middleware to use extractRemoteIP and isLoopback to conditionally trust X-Forwarded-For headers only when the request comes from a loopback address.
• web/middleware_mutation_test.go
  • Adjusted tests to use host-only IP addresses for rate limiter keys, reflecting the updated IP extraction logic.
  • Modified RemoteAddr in TestMiddleware_XForwardedFor to simulate a loopback address, aligning with the new trusted proxy logic.
  • Added TestRateLimiterMiddleware_IgnoresXFFFromNonLoopback to verify that X-Forwarded-For is ignored for non-loopback clients.
• web/missing_coverage_test.go
  • Updated TestRateLimiterCleanupOldLimiters to pass a maxAge parameter to CleanupOldLimiters.

Activity

• The author, CybotTM, performed a comprehensive codebase review to identify and address 10 issues.
• All fixes were developed using a Test-Driven Development (TDD) approach, with tests written before the corresponding code changes.
• The changes were validated by running go test -race ./core/ ./web/ ./cli/ ./config/ ./middlewares/ ./metrics/, with all tests passing.
• Static analysis with go vet ./... reported a clean codebase.
• Each commit in the pull request is atomic and independently buildable, facilitating easier review and understanding of changes.

Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point by creating a comment using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature	Command	Description
Code Review	/gemini review	Performs a code review for the current pull request in its current state.
Pull Request Summary	/gemini summary	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help	/gemini help	Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

[github-actions]

Automated approval for solo maintainer project

All CI checks passed. See SECURITY.md for compensating controls.

[gemini-code-assist]

Code Review

This pull request introduces a comprehensive set of fixes for 10 distinct security, stability, and correctness issues. The changes are well-structured, and each fix is accompanied by dedicated tests, many of which follow a TDD approach. Key improvements include patching a credential leak via the API, fixing a CSRF bypass, preventing a rate-limiter DoS, and hardening against IP spoofing. Stability is enhanced by preventing double-close panics and concurrent map access crashes. Correctness is improved by fixing workflow dependencies, ensuring context propagation for cancellation, respecting shutdown hook priorities, and correctly handling Swarm exit codes. My review found one area for improvement regarding the consistency of an IP spoofing fix. Overall, this is an excellent and impactful set of changes.

_{Note: Security Review did not run due to the size of the PR.}

[github-actions]

Automated approval for solo maintainer project

All CI checks passed. See SECURITY.md for compensating controls.

[github-actions]

Automated approval for solo maintainer project

All CI checks passed. See SECURITY.md for compensating controls.

[github-actions]

Automated approval for solo maintainer project

All CI checks passed. See SECURITY.md for compensating controls.

[github-actions]

🚀 Released in v0.21.2

Thank you for your contribution! 🙏

This is now available in the latest release. Please test and verify everything works as expected in your environment.

If you encounter any issues, please open a new issue.