fix(dashboard): stop reporting a failed job save as success - #62
Merged
Conversation
Saving a job announced "Saved and sent to the worker." as soon as the control plane returned 200 — which only means the command was written and dispatched. Whether msdb accepted it is decided on the SQL host and arrives seconds later over the hub, so the editor's try/catch could never see it. A save that SQL Server refused outright left a green notice on screen and a row in the database saying failed. The error was never actually lost: it reaches Postgres, the audit log, a notification, and is already rendered on the Commands page. It just never reached the screen the operator was looking at. The job page runs no command query at all, so the SSE invalidation that fires on failure lands and does nothing. For a product whose premise is that the dashboard tells you the truth about the estate, a false "Saved" is the worst failure available. The editor now keeps the command id it was already being given and discarding, waits for a terminal state, and reports what happened. A command still running when we stop waiting is reported as queued rather than done — the worker may simply be offline, which is not failure and will apply on reconnect. Needed GET /api/commands/:id: list() filtered only on state and limit, so there was no way to ask about one command without pulling a hundred. sqlErrorNumber was being written on failure and never selected, so the one field that distinguishes "you do not own this job" from any other SQL error stopped at the database. It is now in the projection and drives the explanation for 14525, which matters because SQL Server's own wording is "Only members of sysadmin role are allowed to update or delete jobs owned by a different login" — which reads as "grant sysadmin" and is the wrong conclusion. Measured on SQL Server 2022, enable and disable still work on jobs the worker does not own, so the message says so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AyYg2j8FVkLjiaVcj5HCkj
semics-tech
added a commit
that referenced
this pull request
Jul 31, 2026
#63) Second of three. Depends on nothing; #62 is independent. `sp_update_job` refuses to touch a job owned by another login unless the caller is `sysadmin` — and on a typical estate most jobs are owned by `sa`. That left `job.write` working only on jobs the worker created itself. ## Why the obvious routes were rejected Both were tested, not assumed: **Reassign ownership to the worker.** Changes the security context every step runs under: ``` TsqlAsSa -> SUCCEEDED :: Executed as user: NT AUTHORITY\NETWORK SERVICE. TsqlAsLow -> FAILED :: Executed as user: lowpriv. User 'guest' does not have permission to run DBCC checkdb for database 'master'. CmdExec -> FAILED :: Non-SysAdmins have been denied permission to run CmdExec job steps without a proxy account. ``` **Grant sysadmin.** Hands the instance to anyone who compromises the control plane. ## The third option Procedures created `WITH EXECUTE AS OWNER` **and** signed by a certificate whose login is a `sysadmin` member. Both halves are required, and each alone is useless: | | TRUSTWORTHY ON | TRUSTWORTHY OFF | |---|---|---| | Signed only | fails — token doesn't survive the nested call | fails | | `EXECUTE AS OWNER` only | works | **fails** — `is_sysadmin=0` | | **Both** | works | **works** | So this does **not** depend on `TRUSTWORTHY`. `msdb` ships with it on, but hardening guides turn it off, and this keeps working when they do. ## Default-deny, per your call The allowlist starts **empty** — installing grants nothing until a DBA names a job. Parameters are enumerated rather than forwarded: - **No `@owner_login_name`** — changing owner changes step execution context - **No `@proxy_name`** — that's a stored credential - **`@subsystem` pinned to `TSQL`** — steps run as the Agent service account, so anything else turns "edit a job" into "run arbitrary commands as that account" - Every call logged with `ORIGINAL_LOGIN()`, which survives the context switch, so the log names the caller not `sa` ## Verified on SQL Server 2022 | | | |---|---| | Default-deny refuses | `Job "WrapJob" is not in ...rsagent_write_allowlist` | | Allowlisted job edits | `description=EDITED BY WORKER \| owner=sa` | | Non-allowlisted refused | yes | | CmdExec step refused | `This wrapper only creates TSQL steps` | | Direct `sp_update_job` still denied | `Msg 14525` | | Worker cannot write its own allowlist | `Msg 229` | | `is_sysadmin` outside the wrapper | `0` | | Works with `TRUSTWORTHY OFF` | yes | | Installer re-runnable | yes, twice | | Uninstall leaves nothing | `rsagent objects left=0` | ## Two bugs found by running it - **The installer wasn't idempotent.** A certificate can't be dropped while anything is signed with it, so the second run failed. Signatures are now dropped first. - **A refused CmdExec step logged `allowed=1`**, because the allowlist check passed before the subsystem check rejected it. An audit row saying "allowed" for something that didn't happen is worse than none. Also: the signing password is generated per-install from `NEWID()` rather than hardcoded, so a public script doesn't ship a known password for a `sysadmin`-mapped certificate. ## Next 3. Worker support — detect the wrapper, prefer it, and pre-flight block edits it can't apply. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01AyYg2j8FVkLjiaVcj5HCkj --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
First of the three fixes from the job-ownership investigation. Independent of the other two.
The bug
Saving a job announced "Saved and sent to the worker." as soon as the control plane returned 200 — which only means the command was written and dispatched. Whether
msdbaccepted it is decided on the SQL host and arrives seconds later over the hub, so the editor'stry/catchcould never see it.A save that SQL Server refused outright left a green notice on screen and a row in the database saying
failed.The error was never lost. It reaches Postgres, the audit log, a notification, and is already rendered on the Commands page. It just never reached the screen the operator was looking at — the job page runs no command query, so the SSE invalidation that fires on failure lands and does nothing.
For a product whose premise is that the dashboard tells you the truth about the estate, a false "Saved" is the worst failure available.
The fix
The editor keeps the command id it was already being handed and discarding, waits for a terminal state, and reports what actually happened.
A command still running when we stop waiting is reported as queued, not done — the worker may simply be offline, which isn't failure and will apply on reconnect. Distinguishing those two mattered enough to word separately.
Two things that had to be unblocked first
GET /api/commands/:id—list()filtered only onstateandlimit, so there was no way to ask about one command without pulling the last hundred.sqlErrorNumberwas written on failure and never selected. The one field that distinguishes "you do not own this job" from any other SQL error was reaching the database and stopping there. It's now in the projection and drives the explanation for 14525, which matters because SQL Server's own wording is:That reads as "grant sysadmin" and is the wrong conclusion. The replacement says the change was refused, that nothing was changed, and that enable/disable still work on jobs the worker doesn't own — which I verified on SQL Server 2022 (
sp_update_job's@enable_only_usedcarve-out).Verification
pnpm lintpnpm typecheckpnpm test:unitpnpm test:integrationNew
packages/dashboard/test/command-failure.test.ts— 5 tests pinning the operator-facing wording, including that a detail-less failure never rendersundefined.Still to come
deploy/sql/worker-write-wrapper.sql— signedEXECUTE AS OWNERprocs with structured parameters, so cross-owner edits work without sysadmin and without ownership changes🤖 Generated with Claude Code
https://claude.ai/code/session_01AyYg2j8FVkLjiaVcj5HCkj