Skip to content

remove boltdb#3876

Open
fiftin wants to merge 8 commits into
developfrom
remove_boltdb
Open

remove boltdb#3876
fiftin wants to merge 8 commits into
developfrom
remove_boltdb

Conversation

@fiftin
Copy link
Copy Markdown
Collaborator

@fiftin fiftin commented May 21, 2026

  • feat: remove boltdb
  • feat: remove bolt usage
  • feat: remove store session
  • feat: replace boltdb -> sqlite

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Stale comment

Security review (PR 3876 — remove BoltDB)

Outcome: No medium, high, or critical vulnerabilities identified in the added or modified code paths reviewed.

Scope reviewed: api/auth.go, api/router.go, api/system_info.go, cli/setup/setup.go, db/Store.go, db/Template.go, db/factory/store.go, services/tasks/RemoteJob.go, services/tasks/TaskPool.go, util/config.go, deployment/devcontainer/dredd/example compose changes, deployment/docker/server/server-wrapper, and dependency impact (go.mod).

Notable checks:

  • db.StoreSession removal: For SQL backends, SqlDb.PermanentConnection() always returns true, so StoreSession only invoked the callback (no per-request connect/close). Supported deployments no longer include BoltDB’s non-permanent session model.
  • Authentication: helpers.Store(r) still resolves the app-wide store from request context (set in cli/cmd/root.go). Removing the StoreSession wrapper around authenticationHandler does not remove store injection or session validation logic.
  • Bolt dialect: Explicit bolt / DbDriverBolt paths now fail fast (panic in factory, errors in GetDBConfig / connection string helpers, interactive setup), which is a safe failure mode rather than an authz bypass.
  • Supply chain: go.etcd.io/bbolt removed from go.mod, reducing native dependency surface.

No new inline comments; nothing met the bar for a documented exploit path with attacker-controlled input to a dangerous sink in this diff.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a11f0a477

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread cli/cmd/migrate.go
Comment on lines 17 to 21
migrateCmd.PersistentFlags().StringVar(&migrationArgs.undoTo, "undo-to", "", "Undo to specific version")
migrateCmd.PersistentFlags().StringVar(&migrationArgs.applyTo, "apply-to", "", "Apply to specific version")
migrateCmd.PersistentFlags().StringVar(&migrationArgs.fromBoltDb, "from-boltdb", "", "Path to boltDB data file")
migrateCmd.PersistentFlags().IntVar(&migrationArgs.errLogSize, "err-log-size", 0, "Error log size")
migrateCmd.PersistentFlags().BoolVar(&migrationArgs.skipTaskOutput, "skip-task-output", false, "Skip task output importing during migration")
migrateCmd.PersistentFlags().BoolVar(&migrationArgs.mergeExistingUsers, "merge-existing-users", false, "Reuse existing users matched by username instead of failing on conflict")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep BoltDB migration flag wired for first-run path

The migrate command no longer defines a --from-boltdb flag, but the Docker entrypoint still invokes semaphore migrate --from-boltdb=... when SEMAPHORE_MIGRATE_FROM_BOLTDB is set (deployment/docker/server/server-wrapper, first-run migration block). In this state, Cobra treats that argument as an unknown flag and aborts, so first-run container startup fails specifically for users trying to migrate existing BoltDB data.

Useful? React with 👍 / 👎.

Comment thread cli/setup/setup.go
Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Stale comment

Security review (automation)

API / data layer: Dropping db.StoreSession around authenticationWithStore, StoreMiddleware, TaskPool, and RemoteJob does not weaken SQL-backed deployments. SqlDb.PermanentConnection() is true, so StoreSession never called Connect/Close for MySQL, PostgreSQL, or SQLite; behavior matches a single long-lived pool.

Dependencies: Removing go.etcd.io/bbolt shrinks the dependency graph (positive).

Supply chain / release integrity: The self-hosted Pro GitHub workflows now clone semaphorepro-module from branch remove_boltdb instead of main. If that change ships on the default branch, beta and tagged release builds would track a feature branch unless reverted—see inline comments. Recommend restoring -b main (or a pinned SHA/tag policy) before merge unless this is an intentional short-lived coordination step with matching governance.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

- name: Add PRO implementation
run: |
git clone -b main https://${{ secrets.GH_TOKEN }}@github.com/semaphoreui/semaphorepro-module.git pro_impl
git clone -b remove_boltdb https://${{ secrets.GH_TOKEN }}@github.com/semaphoreui/semaphorepro-module.git pro_impl
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Medium — supply chain / release integrity

git clone -b remove_boltdb makes beta (and similarly release) builds pull the private semaphorepro-module from a feature branch instead of main. If merged as-is, future tags would not match the usual “build from reviewed main” bar; a compromised or unreviewed change on that branch would ship into self-hosted Pro artifacts without the normal branch policy.

Impact: Weaker provenance and higher risk of unintended or malicious code in released binaries compared to cloning main or an immutable ref.

- name: Add PRO implementation
run: |
git clone -b main https://${{ secrets.GH_TOKEN }}@github.com/semaphoreui/semaphorepro-module.git pro_impl
git clone -b remove_boltdb https://${{ secrets.GH_TOKEN }}@github.com/semaphoreui/semaphorepro-module.git pro_impl
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Medium — supply chain / release integrity

Same as the beta workflow: cloning semaphorepro-module from remove_boltdb instead of main changes what code is eligible to ship in production self-hosted releases. Impact: Release artifacts may diverge from the reviewed main lineage; revert to main or pin an immutable SHA before merge unless explicitly governed.

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Stale comment

Security review (PR #3876 — remove BoltDB)

Outcome: No medium, high, or critical vulnerabilities identified in the added or modified code paths for this diff.

Why prior concerns about StoreSession do not apply: db.StoreSession only invoked Connect/Close when PermanentConnection() was false (BoltDB’s single-writer model). db/sql.SqlDb.PermanentConnection() returns true, so for MySQL, PostgreSQL, and SQLite the callback already ran with no per-request connect/close. Removing StoreSession from api/auth.go, api/router.go, and task services is behavior-equivalent for supported backends and does not remove auth checks or store injection (SetContextValue in cli/cmd/root.go).

Other notes: Dropping go.etcd.io/bbolt is a supply-chain/attack-surface reduction. Config now rejects bolt as a dialect; legacy Bolt-only installs fail closed (availability), not an authz bypass.


Slack summary: PR3876 security pass — no findings (M/H/C). Bolt removal + StoreSession cleanup is neutral for SQL backends; bbolt dependency removed.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

Copy link
Copy Markdown

@cursor cursor Bot left a comment

Choose a reason for hiding this comment

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

Security review (PR #3876 — remove boltdb)

Outcome: No medium, high, or critical vulnerabilities were identified in the added or modified application code reviewed for this PR.

What was checked

  • api/auth.go / api/router.go: db.StoreSession was removed around auth and generic handlers. For SQL stores, SqlDb.PermanentConnection() is always true, so StoreSession did not open or close connections; behavior for supported backends matches the prior no-op path. No change to how credentials, sessions, or API tokens are validated.
  • services/tasks/*: Same removal of StoreSession wrappers; no new attacker-controlled sinks or permission boundaries.
  • util/config.go, db/factory/store.go, Docker/compose docs: Bolt removal and dialect validation; no new injection or secret exposure in these edits.
  • Dependencies: go.etcd.io/bbolt removed — smaller attack surface, no new risky dependency added.

Process note (below reporting threshold): .github/workflows/pro_selfhosted_beta.yml and pro_selfhosted_release.yml clone semaphorepro-module from branch remove_boltdb instead of main. That is reasonable for coordinated work but should be reverted to main before long-lived merge so release/beta builds are not permanently pinned to a feature branch.


Slack summary: PR 3876 (remove boltdb) — no medium+ security findings in reviewed code; optional follow-up: revert Pro-module workflow clone branch to main after integration.

Open in Web View Automation 

Sent by Cursor Automation: Find vulnerabilities

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