Skip to content

Potential fix for code scanning alert no. 17: Database query built from user-controlled sources - #31

Closed
intel352 wants to merge 1 commit into
mainfrom
alert-autofix-17
Closed

intel352 wants to merge 1 commit into
mainfrom
alert-autofix-17

Conversation

@intel352

Copy link
Copy Markdown
Contributor

Potential fix for https://github.com/GoCodeAlone/workflow/security/code-scanning/17

In general, to fix “query built from user-controlled sources” issues, we should (a) use parameter placeholders for values, and (b) ensure that any part of the query that cannot be parameterized (like table and column names, or ASC/DESC) is derived from a strict allowlist, not directly from tainted input. The current code already validates tableName and sortCol against the database schema and normalizes the order direction, but CodeQL still observes tainted flow from r.URL into the string. We should explicitly map the user-provided order to a fixed, untainted value and keep the rest of the logic unchanged.

The best minimal change, without altering functionality, is:

  • Replace the direct normalization of order:

    order := strings.ToUpper(r.URL.Query().Get("order"))
    if order != "DESC" {
        order = "ASC"
    }

    with a pattern that maps the tainted string to one of two hard-coded constants using a local variable, e.g.:

    orderParam := strings.ToUpper(r.URL.Query().Get("order"))
    order := "ASC"
    if orderParam == "DESC" {
        order = "DESC"
    }

    This maintains exactly the same behavior but makes the safe values ("ASC" / "DESC") explicit and independent of arbitrary user content. Static analysis tools typically recognize that order is drawn from a fixed finite set and is not directly tainted.

  • Keep the sortCol validation and tableName validation as-is, because they already enforce allowlists based on the database schema.

All changes occur in plugin/storebrowser/handler.go around lines 147–151, within the provided snippet. No new methods or imports are necessary.

Suggested fixes powered by Copilot Autofix. Review carefully before merging.

…om user-controlled sources

Co-authored-by: Copilot Autofix powered by AI <62310815+github-advanced-security[bot]@users.noreply.github.com>
@intel352 intel352 closed this Feb 17, 2026
intel352 added a commit that referenced this pull request Apr 23, 2026
Move all cloud-provider-specific bucket-creation logic out of wfctl core
and behind the IaCProvider plugin interface:

- interfaces/iac_provider.go: add BootstrapStateBackend(ctx, cfg)
  (*BootstrapResult, error) to IaCProvider; add BootstrapResult struct
  (Bucket, Region, Endpoint, EnvVars)
- infra_bootstrap.go: remove all DO/AWS SDK imports and S3-specific code;
  bootstrapStateBackend now finds the first iac.provider module, calls
  resolveIaCProvider, then delegates to provider.BootstrapStateBackend;
  self-contained backends (filesystem/memory/postgres/"") remain no-ops
- deploy_providers.go: wire BootstrapStateBackend on remoteIaCProvider
  via InvokeService + anyToStruct, consistent with rest of bridge
- All IaCProvider test stubs: add BootstrapStateBackend(nil, nil) stub
- Delete infra_bootstrap_bucket_test.go: DO-specific S3 tests move to
  DO plugin (follow-up #31, workflow-plugin-digitalocean v0.7.4)
- Rewrite infra_bootstrap_env_test.go + infra_bootstrap_url_export_test.go:
  replace bootstrapDOSpacesBucketFn overrides with resolveIaCProvider
  overrides using envBootstrapProvider / urlBootstrapProvider fakes
- Add infra_bootstrap_provider_test.go: TestBootstrapStateBackend_DispatchesToProvider
  (full round-trip), SelfContainedBackendsSkipped, NoProviderModuleErrors

Addresses review comment #3132352899 (intel352): DO-specific bootstrap
should not live in wfctl core.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
intel352 added a commit that referenced this pull request Apr 23, 2026
…troy/status/drift direct paths + bootstrap URL export (#471)

* feat(wfctl): persist ResourceState after provider.Apply — fixes BMW state loss

Refactors applyWithProvider → applyWithProviderAndStore(ctx, provider,
providerType, specs, current, store). After provider.Apply returns success,
each ResourceOutput is written to the store; save failures log a loud warning
but do not abort the apply. Delete actions trigger store.DeleteResource after
the Apply call.

Also introduces resolveStateStore(cfgFile) which reads the iac.state module
config and instantiates the correct backend (filesystem or Spaces), rewriting
loadCurrentState to delegate to it so that remote backends are supported
beyond the previous filesystem-only path.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* feat(wfctl): direct-path infra destroy + status + drift for infra.* modules

Adds destroyInfraModules, statusInfraModules, and driftInfraModules that load
state from the configured backend, group records by IaCProvider, and dispatch
to provider.Destroy/Status/DetectDrift respectively.

runInfraDestroy, runInfraStatus, and runInfraDrift now dispatch to the direct
path when hasInfraModules() is true, falling back to the legacy pipeline path
for platform.* configs.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* feat(wfctl): bootstrap exports Spaces bucket URL back to config

After bootstrapStateBackend creates or confirms a DO Spaces bucket,
prints 'export DO_SPACES_BUCKET=<name>' to stdout for CI capture via
$GITHUB_ENV, and calls writeBucketBackToConfig to bake the resolved
bucket name into the on-disk YAML so downstream wfctl commands can read
state without the env var being set again.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* chore: add v0.18.6 CHANGELOG entry

* fix(lint): resolve 8 gocritic/nilerr issues in infra apply/destroy/status/drift

- Extract provider-loop bodies into closures (applyGroup, destroyGroup,
  statusGroup, driftGroup) to fix deferInLoop in all four files
- Switch range-value loops to index loops for rangeValCopy (plan.Actions
  136B, ResourceState 208B) in infra_apply, infra_destroy, infra_status_drift
- Split combined err+len guard in resolveStateStore to satisfy nilerr;
  add //nolint:nilerr on the intentional err-swallow (noop on missing config)

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* fix(wfctl): provider-agnostic bootstrap dispatch + postgres state store + follow-up TODOs

bootstrapStateBackend now dispatches by iac.state.backend:
- spaces → bootstrapStateBackendSpaces (DO endpoint URL constructed here)
- s3/gcs/azure → clear not-yet-implemented errors with contribution guidance
- filesystem/memory/postgres/"" → no-op

bootstrapStateBackendSpaces: exports WFCTL_STATE_BUCKET (generic) +
SPACES_BUCKET (backend-specific, no cloud-provider prefix). bootstrapDOSpacesBucket
now accepts caller-supplied endpoint; creds error message genericised; log
message updated to "created spaces state bucket".

resolveStateStore: wires module.PostgresIaCStateStore for backend=postgres;
s3/gcs/azure return clear errors instead of silent noop.

Misc:
- TODO(#28) added to deploy.go hardcoded credential checks
- TODO(#29) added to providerCredentialSubKeys map
- infra import --provider help text no longer lists specific providers
- All bootstrapDOSpacesBucketFn test fakes updated for new endpoint param
- CHANGELOG updated

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* refactor: BootstrapStateBackend → IaCProvider interface dispatch

Move all cloud-provider-specific bucket-creation logic out of wfctl core
and behind the IaCProvider plugin interface:

- interfaces/iac_provider.go: add BootstrapStateBackend(ctx, cfg)
  (*BootstrapResult, error) to IaCProvider; add BootstrapResult struct
  (Bucket, Region, Endpoint, EnvVars)
- infra_bootstrap.go: remove all DO/AWS SDK imports and S3-specific code;
  bootstrapStateBackend now finds the first iac.provider module, calls
  resolveIaCProvider, then delegates to provider.BootstrapStateBackend;
  self-contained backends (filesystem/memory/postgres/"") remain no-ops
- deploy_providers.go: wire BootstrapStateBackend on remoteIaCProvider
  via InvokeService + anyToStruct, consistent with rest of bridge
- All IaCProvider test stubs: add BootstrapStateBackend(nil, nil) stub
- Delete infra_bootstrap_bucket_test.go: DO-specific S3 tests move to
  DO plugin (follow-up #31, workflow-plugin-digitalocean v0.7.4)
- Rewrite infra_bootstrap_env_test.go + infra_bootstrap_url_export_test.go:
  replace bootstrapDOSpacesBucketFn overrides with resolveIaCProvider
  overrides using envBootstrapProvider / urlBootstrapProvider fakes
- Add infra_bootstrap_provider_test.go: TestBootstrapStateBackend_DispatchesToProvider
  (full round-trip), SelfContainedBackendsSkipped, NoProviderModuleErrors

Addresses review comment #3132352899 (intel352): DO-specific bootstrap
should not live in wfctl core.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

---------

Co-authored-by: Claude Sonnet 4.6 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant