Skip to content

fix: block SQL injection in role attributes#436

Merged
moizpgedge merged 3 commits into
mainfrom
Bug/PLAT-685/Security-Codacy-Triage-security-issues
Jul 21, 2026
Merged

fix: block SQL injection in role attributes#436
moizpgedge merged 3 commits into
mainfrom
Bug/PLAT-685/Security-Codacy-Triage-security-issues

Conversation

@moizpgedge

@moizpgedge moizpgedge commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes a real SQL injection bug in role creation, cleans up a false-positive finding on the same scan, risk-accepts a couple of CVEs that don't have fixes yet, and fixes the licenses-ci CI failure.

Changes

SQL injection fix. CreateUserRole in roles.go was building ALTER ROLE ... WITH <attr> by pasting each attribute string straight into the SQL. A bad attribute like "LOGIN; DROP TABLE users; --" would just run. Added an allowlist of the actual valid role attribute keywords (SUPERUSER, CREATEDB, LOGIN, etc. plus their NO* forms) and reject anything else before it gets near a SQL statement. Left out the attribute forms that take a literal value (CONNECTION LIMIT, VALID UNTIL, PASSWORD) since those can't be safely allowlisted this way and password already has its own field. Added tests in roles_test.go. Also tried this for real against a running dev cluster: sent the malicious attribute through the actual API, and it got rejected with "unsupported role attribute" before touching Postgres.

False positive cleanup. The scanner also flagged PostgresContainerExec in utils.go as SQL injection, but it's a Docker exec call, not SQL. Checked every place that calls it and confirmed the command is always built from fixed binaries, never raw input, and Docker doesn't run it through a shell anyway. Added a comment explaining why so it doesn't keep getting flagged.

Risk acceptances. Added two entries to the trivyignore file: the docker/docker CVEs that have no fix in the v27 line we're on (and we don't use the code paths they affect anyway), and the x/crypto/openpgp advisory, which doesn't apply since we don't import that package.

Fixed the licenses-ci failure. NOTICE.txt had gotten out of sync with go.mod, and separately go-licenses has a bug where it generates broken LICENSE links for some opentelemetry-go packages. Fixed the link bug in the Makefile so it's part of regeneration going forward instead of a one-off patch, then regenerated NOTICE.txt properly in a container that matches CI's Go version (couldn't do it locally on macOS, go-licenses crashes there for an unrelated reason). Ran it twice to make sure it's stable.

Testing

go build, go vet, go test all pass. Tested the injection fix live against a dev cluster. Regenerated NOTICE.txt in a matching CI container twice, same output both times.

Checklist

  • Tests added (roles_test.go)
  • Documentation updated (trivyignore, inline comments)
  • Issue linked (PLAT-685)
  • Changelog entry — still need to add one
  • No breaking changes, aside from attributes outside the allowlist now getting rejected instead of silently going through (nothing in this codebase relies on that)

Notes for reviewers

The NOTICE.txt/Makefile fix also applies to the PLAT-686 branch, which hit the same CI failure for the same reason.

Validate database_users[].attributes against a fixed keyword
allowlist in postgres.CreateUserRole instead of interpolating
them unescaped into ALTER ROLE. Also bump containerd, go-git,
go-billy, and the same pgx/otel/x-crypto/x-net/chi/Go-toolchain
set from PLAT-686 to close the remaining Codacy findings.
Risk-accept the unfixed docker/docker  CVEs in
.trivy/pgedge-control-plane.trivyignore.yaml.

PLAT-685
@coderabbitai

coderabbitai Bot commented Jul 17, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change adds Docker command and PostgreSQL role-attribute validation, refactors API, database, orchestration, monitoring, migration, and workflow paths into helpers, updates Go dependencies and toolchains, pins build inputs, refreshes license records, and expands Trivy suppressions.

Changes

Security and API validation

Layer / File(s) Summary
Command, role, and API validation
server/internal/docker/..., server/internal/postgres/..., server/internal/api/apiv1/...
Docker commands and role attributes are validated; credential decoding, failover preparation, and database specification validation are extracted into focused helpers.

Configuration and lifecycle refactoring

Layer / File(s) Summary
Configuration, resources, and database lifecycle
server/internal/config/..., server/internal/database/..., server/internal/etcd/..., server/internal/ipam/...
Configuration loading, resource construction, database initialization, replication, upgrades, deletion, and polling flows are reorganized into helper-based paths.

Orchestration, migration, monitoring, and workflows

Layer / File(s) Summary
Orchestration and service builders
server/internal/orchestrator/..., server/internal/pgbackrest/...
Patroni, certificates, restore operations, Swarm services, MCP/RAG configuration, and systemd options use dedicated builders and error paths.
Migrations, monitoring, and workflows
server/internal/resource/migrations/..., server/internal/monitor/..., server/internal/workflows/...
Migration transformations, topology processing, monitor state handling, candidate selection, workflow deletion, failover, locking, and statistics are extracted into helpers.

Dependency and build refresh

Layer / File(s) Summary
Toolchains, images, notices, and vulnerability records
go.mod, server/internal/resource/migrations/schematool/go.mod, NOTICE.txt, docker/..., docs/Dockerfile, .trivy/...
Go toolchains and dependencies, container and documentation build inputs, license links, and Trivy suppressions are updated.

Poem

I’m a bunny guarding commands tight,
Shells hop out of Docker’s sight.
Role words line up in a row,
Go modules bloom and grow.
Safe builds make my ears upright!

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 73.54% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 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.
Title check ✅ Passed The title is concise and accurately reflects the main change: blocking SQL injection via role attributes.
Description check ✅ Passed The description follows the template well, with Summary, Changes, Testing, Checklist, and Notes sections filled in.
✨ 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 Bug/PLAT-685/Security-Codacy-Triage-security-issues

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.

@codacy-production

codacy-production Bot commented Jul 17, 2026

Copy link
Copy Markdown

Up to standards ✅

🟢 Issues 0 issues

Results:
0 new issues

View in Codacy

🟢 Metrics 0 duplication

Metric Results
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
docs/Dockerfile (1)

3-6: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Use --no-cache-dir to reduce the image size.

Consider adding the --no-cache-dir flag to the pip install command. This prevents pip from caching the downloaded packages, which is generally unnecessary in Docker containers and helps keep the final image size smaller.

♻️ Proposed refactor
-RUN pip install \
+RUN pip install --no-cache-dir \
     pymdown-extensions==11.0.1 \
     mkdocs-redoc-tag==0.2.0 \
     mkdocs-github-admonitions-plugin==0.1.1
🤖 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 `@docs/Dockerfile` around lines 3 - 6, Update the pip install command in the
Dockerfile to include the --no-cache-dir option while preserving the existing
package versions and multiline installation structure.
🤖 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 `@docs/Dockerfile`:
- Around line 3-6: Update the pip install command in the Dockerfile to include
the --no-cache-dir option while preserving the existing package versions and
multiline installation structure.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 49658f9e-8be4-4fe5-bc88-f991d5845294

📥 Commits

Reviewing files that changed from the base of the PR and between 05f074d and 1a64c8f.

📒 Files selected for processing (5)
  • NOTICE.txt
  • docker/control-plane-ci/Dockerfile
  • docs/Dockerfile
  • server/internal/docker/docker.go
  • server/internal/docker/docker_test.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 9

Caution

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

⚠️ Outside diff range comments (1)
server/internal/orchestrator/swarm/orchestrator.go (1)

302-310: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Only reuse ResolvedImage when every image-selection input is unchanged.

A same-version service-type change reuses the old service’s image. Adding a user Image override also leaves both Image and ResolvedImage set, violating their mutual-exclusion contract.

Proposed fix
 func serviceVersionUnchanged(old, new *database.ServiceInstanceSpec) bool {
-	return old != nil && old.ServiceSpec != nil && new.ServiceSpec != nil &&
-		old.ServiceSpec.Version == new.ServiceSpec.Version
+	if old == nil || old.ServiceSpec == nil || new.ServiceSpec == nil {
+		return false
+	}
+	if old.ServiceSpec.ServiceType != new.ServiceSpec.ServiceType ||
+		old.ServiceSpec.Version != new.ServiceSpec.Version {
+		return false
+	}
+	opts := new.ServiceSpec.OrchestratorOpts
+	return opts == nil || opts.Swarm == nil || opts.Swarm.Image == ""
 }

Also applies to: 316-337

🤖 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 `@server/internal/orchestrator/swarm/orchestrator.go` around lines 302 - 310,
Update ReconcileServiceInstanceSpec and its serviceVersionUnchanged decision so
the old ResolvedImage is carried forward only when all image-selection inputs,
including service type and user Image override, are unchanged. When any such
input changes, clear the new Swarm.ResolvedImage so resolution uses the current
manifest or override, preserving the mutual exclusion between Image and
ResolvedImage.
🧹 Nitpick comments (1)
server/internal/api/apiv1/validate.go (1)

190-195: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Clarify the error message for invalid source node chaining.

The error message "source node must refer to an existing node" is slightly misleading here, as the node does exist in the spec (it passed the seenNodeNames check above). The actual failure is that the referenced node is also being provisioned (it declares its own source_node). Updating the message will help users diagnose the validation failure faster.

💡 Proposed fix
 		if newNodesWithSource.Has(src) {
 			errs = append(errs, validation.NewError(
-				errors.New("source node must refer to an existing node"),
+				errors.New("source node cannot refer to a node that is also being provisioned from a source"),
 				srcPath,
 			))
 		}
🤖 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 `@server/internal/api/apiv1/validate.go` around lines 190 - 195, Update the
validation error created in the newNodesWithSource check to state that source
nodes cannot themselves define a source_node or be provisioned from another
source, rather than claiming the referenced node does not exist. Keep the
existing validation condition and srcPath unchanged.
🤖 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 `@server/internal/api/apiv1/pre_init_handlers.go`:
- Around line 106-108: In the join-credentials handling around
decodeJoinCredentials, validate opts.Credentials for nil before passing it to
the decoder. Return an appropriate API error when credentials are absent, and
preserve the existing decode error path for non-nil credentials.

In `@server/internal/config/manager.go`:
- Around line 107-126: Correct the duplicated error messages in the
configuration merge and unmarshal paths. Update each fmt.Errorf call around
combinedK.Merge and generatedK/combinedK.Unmarshal to identify the specific
operation and configuration layer that failed, while preserving the existing
wrapped errors and return behavior.

In `@server/internal/monitor/instance_monitor.go`:
- Around line 90-93: Update checkStatus and the available-state transition
around updateInstanceStatus so failures from UpdateInstanceState are returned
instead of discarded. Ensure checkStatus does not publish a healthy status when
the instance state update fails, while preserving the existing status update
flow on success.

In `@server/internal/orchestrator/swarm/mcp_config.go`:
- Around line 246-273: In server/internal/orchestrator/swarm/mcp_config.go lines
246-273, update buildMCPKBConfig to assume validated input: remove the defensive
nil checks and related comment, and change it to return only *mcpKBConfig. In
server/internal/orchestrator/swarm/mcp_config.go lines 114-117, update the
caller to assign the direct result without handling an error or retaining the
removed error variable.

In `@server/internal/orchestrator/swarm/service_spec.go`:
- Around line 155-158: Update the extra-label merge in the swarm service
specification so entries from swarmOpts.ExtraLabels cannot overwrite the
control-plane identity labels pgedge.component or pgedge.service.instance.id.
Preserve all other user-provided labels, and ensure the control-plane values
remain authoritative for ServiceInspectByLabels reconciliation.

In `@server/internal/workflows/backend/etcd/etcd.go`:
- Around line 661-668: Update the transaction error handling around the
`b.store.Txn(...).Commit(ctx)` call so only the specific transaction-conflict
error returns `(nil, nil)` as a lock race; propagate all other etcd or
serialization errors to the caller instead of suppressing them.
- Around line 647-652: Update the lock reassignment branch in the activity-lock
handling around lock.CanBeReassignedTo so the Update operation uses
b.options.ActivityLockTimeout instead of b.options.WorkflowLockTimeout. Keep the
existing reassignment and update flow unchanged.

In `@server/internal/workflows/delete_database.go`:
- Around line 127-130: Update deleteDatabaseResources so the error returned by
operations.UpdateDatabase is passed through handleError before returning,
instead of being returned directly. Preserve the existing planning error context
while ensuring the caller receives handleError’s result and the database/task
failure state is recorded.

In `@server/internal/workflows/failover.go`:
- Around line 79-82: Update the skipped-candidate branch in the failover
workflow to propagate errors returned by updateFailoverTask instead of
discarding them. Preserve the existing success response when task completion
succeeds, but return the completion error so the workflow cannot report success
while the task remains running.

---

Outside diff comments:
In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 302-310: Update ReconcileServiceInstanceSpec and its
serviceVersionUnchanged decision so the old ResolvedImage is carried forward
only when all image-selection inputs, including service type and user Image
override, are unchanged. When any such input changes, clear the new
Swarm.ResolvedImage so resolution uses the current manifest or override,
preserving the mutual exclusion between Image and ResolvedImage.

---

Nitpick comments:
In `@server/internal/api/apiv1/validate.go`:
- Around line 190-195: Update the validation error created in the
newNodesWithSource check to state that source nodes cannot themselves define a
source_node or be provisioned from another source, rather than claiming the
referenced node does not exist. Keep the existing validation condition and
srcPath unchanged.
🪄 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: 885d23dc-d6a3-4da6-981d-98d183d4cb8f

📥 Commits

Reviewing files that changed from the base of the PR and between 1a64c8f and 0777720.

📒 Files selected for processing (49)
  • api/apiv1/design/database.go
  • api/apiv1/design/task.go
  • docker/control-plane/Dockerfile
  • docs/Dockerfile
  • server/internal/api/apiv1/post_init_handlers.go
  • server/internal/api/apiv1/pre_init_handlers.go
  • server/internal/api/apiv1/validate.go
  • server/internal/config/config.go
  • server/internal/config/manager.go
  • server/internal/database/instance_resource.go
  • server/internal/database/operations/end.go
  • server/internal/database/operations/populate_nodes.go
  • server/internal/database/operations/update_database.go
  • server/internal/database/postgres_database.go
  • server/internal/database/reconcile_versions.go
  • server/internal/database/replication_slot_advance_from_cts_resource.go
  • server/internal/database/script.go
  • server/internal/database/service.go
  • server/internal/database/spec.go
  • server/internal/database/wait_for_sync_event_resource.go
  • server/internal/etcd/provide.go
  • server/internal/etcd/rbac.go
  • server/internal/ipam/service.go
  • server/internal/migrate/migrations/add_task_scope.go
  • server/internal/monitor/instance_monitor.go
  • server/internal/monitor/provide.go
  • server/internal/monitor/service_instance_monitor.go
  • server/internal/orchestrator/common/patroni_config.go
  • server/internal/orchestrator/common/postgres_certs.go
  • server/internal/orchestrator/swarm/mcp_config.go
  • server/internal/orchestrator/swarm/orchestrator.go
  • server/internal/orchestrator/swarm/pgbackrest_restore.go
  • server/internal/orchestrator/swarm/rag_config.go
  • server/internal/orchestrator/swarm/service_instance.go
  • server/internal/orchestrator/swarm/service_spec.go
  • server/internal/orchestrator/systemd/pgbackrest_restore.go
  • server/internal/orchestrator/systemd/provide.go
  • server/internal/orchestrator/systemd/unit_options.go
  • server/internal/pgbackrest/config.go
  • server/internal/postgres/hba/parse.go
  • server/internal/resource/migrations/1_0_0.go
  • server/internal/resource/migrations/1_2_0.go
  • server/internal/resource/migrations/schematool/identifier.go
  • server/internal/resource/topo.go
  • server/internal/workflows/activities/check_cluster_health.go
  • server/internal/workflows/activities/select_candidate_instance.go
  • server/internal/workflows/backend/etcd/etcd.go
  • server/internal/workflows/delete_database.go
  • server/internal/workflows/failover.go

Comment thread server/internal/api/apiv1/pre_init_handlers.go Outdated
Comment thread server/internal/config/manager.go Outdated
Comment thread server/internal/monitor/instance_monitor.go Outdated
Comment thread server/internal/orchestrator/swarm/mcp_config.go Outdated
Comment thread server/internal/orchestrator/swarm/service_spec.go
Comment thread server/internal/workflows/backend/etcd/etcd.go Outdated
Comment thread server/internal/workflows/backend/etcd/etcd.go Outdated
Comment thread server/internal/workflows/delete_database.go Outdated
Comment thread server/internal/workflows/failover.go Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 7

Caution

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

⚠️ Outside diff range comments (4)
server/internal/docker/docker.go (1)

81-82: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Normalize the executable path before checking the shell denylist.

Equivalent paths such as /usr/bin/sh, /usr/local/bin/bash, and ./dash bypass the exact-string set and still enable -c command interpretation. Compare path.Base(command[0]) against interpreter names instead.

Proposed fix
-	if shellInterpreters.Has(command[0]) {
+	if shellInterpreters.Has(path.Base(command[0])) {

Add the standard-library path import.

🤖 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 `@server/internal/docker/docker.go` around lines 81 - 82, Update the shell
interpreter check in the command validation logic to compare
path.Base(command[0]) with shellInterpreters instead of the raw executable path.
Add the standard-library path import and preserve the existing
ErrShellInterpreterNotAllowed error behavior.
server/internal/database/service.go (1)

771-775: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Restore allocated fields during rollback.

The rollback releases newly allocated ports but leaves spec.Port and spec.PatroniPort populated. A retry can therefore skip allocation and persist a port that has already returned to the pool.

Proposed fix
 func (s *Service) allocateInstanceSpecPorts(ctx context.Context, spec *InstanceSpec) (func(error) error, error) {
+	originalPort := spec.Port
+	originalPatroniPort := spec.PatroniPort
 	var allocated []int
 	rollback := func(cause error) error {
 		rollbackCtx, cancel := context.WithTimeout(context.Background(), 15*time.Second)
 		defer cancel()

-		return errors.Join(cause, s.portsSvc.ReleasePort(rollbackCtx, spec.HostID, allocated...))
+		releaseErr := s.portsSvc.ReleasePort(rollbackCtx, spec.HostID, allocated...)
+		spec.Port = originalPort
+		spec.PatroniPort = originalPatroniPort
+		return errors.Join(cause, releaseErr)
 	}

Based on learnings, reconciliation retries must remain idempotent and non-destructive.

Also applies to: 778-794

🤖 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 `@server/internal/database/service.go` around lines 771 - 775, Update the
rollback closure to restore the allocation fields after releasing the ports:
clear spec.Port and spec.PatroniPort before returning, while preserving the
existing joined-error behavior and timeout cleanup. Ensure retries re-enter port
allocation rather than reusing released ports.

Source: Learnings

server/internal/workflows/backend/etcd/etcd.go (1)

591-639: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Stop routing events to an already-existing sub-workflow.

When the child exists, appendSubWorkflowStartOps enqueues SubWorkflowFailed, but Line 599 still sends the original start event to that existing execution. Return a routing flag and skip the event loop for this collision.

Proposed control-flow fix
-ops, err = b.appendSubWorkflowStartOps(...)
+var routeEvents bool
+ops, routeEvents, err = b.appendSubWorkflowStartOps(...)
 if err != nil {
   return nil, err
 }
+if !routeEvents {
+  continue
+}
🤖 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 `@server/internal/workflows/backend/etcd/etcd.go` around lines 591 - 639,
Update appendSubWorkflowStartOps and its caller to return a routing flag
indicating whether the child already exists; when that collision is detected,
enqueue SubWorkflowFailed but mark the event batch as not routable. In the
caller’s event-processing flow, skip the loop that appends PendingEvent records
for the original start event when the flag is false, while preserving normal
routing for newly created sub-workflows.
server/internal/workflows/service.go (1)

503-516: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Do not orphan a running remove-host workflow when task linkage fails.

CreateWorkflowInstance may succeed before UpdateTask fails. abortTasks only changes task metadata, so host removal can continue while the caller receives an error and potentially retries.

Persist a deterministic workflow instance ID before dispatch, or cancel the created workflow when linkage cannot be stored.

🤖 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 `@server/internal/workflows/service.go` around lines 503 - 516, The remove-host
flow around CreateWorkflowInstance and UpdateTask must not leave a successfully
created workflow running when task linkage fails. Persist a deterministic
workflow instance ID before dispatch where supported, or cancel the created
workflow instance before returning the UpdateTask error; retain abortTasks and
the existing error propagation.
🤖 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 `@server/internal/api/apiv1/post_init_handlers.go`:
- Line 510: Normalize the optional req.Options before calling
startBackupWorkflow in the relevant initialization handlers, including the paths
around the calls at lines 510 and 569-572. Ensure omitted options remain safely
represented as nil or are replaced with the established default without allowing
startBackupWorkflow to dereference a nil value.
- Around line 597-601: Make the switchover creation flow around
checkNoActiveSwitchover and startSwitchoverWorkflow atomic so concurrent
requests cannot create multiple switchovers. Use a transaction-scoped lock or
deterministic unique key/workflow instance ID enforced by the persistence layer,
and ensure conflicting creations are rejected while preserving the existing
successful workflow path.

In `@server/internal/database/mcp_service_config.go`:
- Around line 202-216: Update parseLLMFields so ollamaURL is parsed whenever
either the LLM configuration uses Ollama or embeddingProvider is Ollama,
including the llm_enabled path. Preserve the existing parseLLMFieldsEnabled
results while supplying the shared ollama_url for OpenAI-LLM/Ollama-embedding
configurations.

In `@server/internal/etcd/embedded.go`:
- Around line 667-675: Update finalizeClientModeConfig to persist the
client-mode generated configuration before calling os.RemoveAll(e.etcdDir()). If
cfg.UpdateGeneratedConfig fails, return the existing wrapped error without
deleting the embedded data; only remove the data directory after the
configuration update succeeds, preserving the existing removal error handling.

In `@server/internal/orchestrator/swarm/orchestrator.go`:
- Around line 1210-1241: Populate the InstanceID field in every
database.ValidationResult constructed by the validation paths around
parseImageTag and the additional reported ranges, using the current instance’s
identifier from cur. Preserve the existing validation status, node, host, and
error contents while ensuring all returned results carry the corresponding
InstanceID.

In `@server/internal/postgres/create_db.go`:
- Around line 184-210: Update subInterfaceSwapStatements to generate a
collision-resistant interfaceName suffix instead of using time.Now().Unix()
second resolution. Preserve the existing providerName-based naming format and
ensure concurrent updates or immediate retries produce distinct names before
node_add_interface executes.

In `@server/internal/workflows/switchover.go`:
- Around line 183-186: Update the candidateID == leaderInstanceID skip path to
capture and propagate errors returned by w.updateSwitchoverTask when marking the
task complete. Return the existing success result only when the completion
update succeeds; otherwise return the update error instead of ignoring it.

---

Outside diff comments:
In `@server/internal/database/service.go`:
- Around line 771-775: Update the rollback closure to restore the allocation
fields after releasing the ports: clear spec.Port and spec.PatroniPort before
returning, while preserving the existing joined-error behavior and timeout
cleanup. Ensure retries re-enter port allocation rather than reusing released
ports.

In `@server/internal/docker/docker.go`:
- Around line 81-82: Update the shell interpreter check in the command
validation logic to compare path.Base(command[0]) with shellInterpreters instead
of the raw executable path. Add the standard-library path import and preserve
the existing ErrShellInterpreterNotAllowed error behavior.

In `@server/internal/workflows/backend/etcd/etcd.go`:
- Around line 591-639: Update appendSubWorkflowStartOps and its caller to return
a routing flag indicating whether the child already exists; when that collision
is detected, enqueue SubWorkflowFailed but mark the event batch as not routable.
In the caller’s event-processing flow, skip the loop that appends PendingEvent
records for the original start event when the flag is false, while preserving
normal routing for newly created sub-workflows.

In `@server/internal/workflows/service.go`:
- Around line 503-516: The remove-host flow around CreateWorkflowInstance and
UpdateTask must not leave a successfully created workflow running when task
linkage fails. Persist a deterministic workflow instance ID before dispatch
where supported, or cancel the created workflow instance before returning the
UpdateTask error; retain abortTasks and the existing error propagation.
🪄 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: a0006f6b-2b21-430a-8446-c3d0a4b50b59

📥 Commits

Reviewing files that changed from the base of the PR and between 0777720 and c7043e7.

📒 Files selected for processing (42)
  • api/apiv1/design/database.go
  • api/apiv1/design/task.go
  • mqtt/endpoint.go
  • server/cmd/root.go
  • server/internal/api/apiv1/convert.go
  • server/internal/api/apiv1/post_init_handlers.go
  • server/internal/api/apiv1/validate.go
  • server/internal/app/app.go
  • server/internal/certificates/service.go
  • server/internal/config/config.go
  • server/internal/database/mcp_service_config.go
  • server/internal/database/operations/update_database.go
  • server/internal/database/postgrest_service_config.go
  • server/internal/database/service.go
  • server/internal/docker/docker.go
  • server/internal/etcd/embedded.go
  • server/internal/orchestrator/common/etcd_creds.go
  • server/internal/orchestrator/common/patroni_config_generator.go
  • server/internal/orchestrator/swarm/orchestrator.go
  • server/internal/orchestrator/swarm/rag_config.go
  • server/internal/orchestrator/swarm/service_instance.go
  • server/internal/orchestrator/swarm/spec.go
  • server/internal/orchestrator/systemd/orchestrator.go
  • server/internal/postgres/create_db.go
  • server/internal/resource/migrations/1_0_0.go
  • server/internal/resource/migrations/1_2_0.go
  • server/internal/resource/migrations/schemas/v1_2_0/swarm.go
  • server/internal/resource/migrations/schematool/inliner.go
  • server/internal/resource/migrations/schematool/output.go
  • server/internal/resource/migrations/schematool/parser.go
  • server/internal/resource/state.go
  • server/internal/storage/txn.go
  • server/internal/workflows/activities/apply_event.go
  • server/internal/workflows/backend/etcd/etcd.go
  • server/internal/workflows/common.go
  • server/internal/workflows/create_pgbackrest_backup.go
  • server/internal/workflows/pgbackrest_restore.go
  • server/internal/workflows/plan_update.go
  • server/internal/workflows/refresh_current_state.go
  • server/internal/workflows/service.go
  • server/internal/workflows/switchover.go
  • server/internal/workflows/update_database.go

Comment thread server/internal/api/apiv1/post_init_handlers.go Outdated
Comment thread server/internal/api/apiv1/post_init_handlers.go Outdated
Comment thread server/internal/database/mcp_service_config.go Outdated
Comment thread server/internal/etcd/embedded.go Outdated
Comment thread server/internal/orchestrator/swarm/orchestrator.go Outdated
Comment thread server/internal/postgres/create_db.go Outdated
Comment thread server/internal/workflows/switchover.go Outdated
@moizpgedge
moizpgedge force-pushed the Bug/PLAT-685/Security-Codacy-Triage-security-issues branch from c7043e7 to 05f074d Compare July 18, 2026 21:21
@moizpgedge moizpgedge changed the title fix: prevent SQL injection via role attributes (PLAT-685) fix: prevent SQL injection via role attributes Jul 20, 2026
@moizpgedge
moizpgedge requested a review from tsivaprasad July 20, 2026 16:16
@moizpgedge moizpgedge changed the title fix: prevent SQL injection via role attributes fix: block SQL injection in role attributes Jul 20, 2026

@tsivaprasad tsivaprasad left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good.

@moizpgedge
moizpgedge merged commit 39eba5f into main Jul 21, 2026
5 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.

2 participants