B3 Phase 2: always-safe DB-config Apply Fix (ALTER DATABASE SET)#1039
Merged
Conversation
Adds the second privileged "Apply Fix" type — the three always-safe database settings AUTO_SHRINK OFF, AUTO_CLOSE OFF, PAGE_VERIFY CHECKSUM — reusing the Phase-1 remediation framework verbatim. RCSI is excluded (destructive); recovery_model/query_store are out of scope. Apply-only (no un-apply for DB-config). Security invariants enforced: - Identifier safety (§1): the executed ALTER DATABASE statement's ONLY non-constant token is the database identifier, which is (a) validated by a PARAMETERIZED sys.databases existence check (@db NVARCHAR(128)), (b) bracketed by an inline ]-doubling routine in the Dashboard executor (FactRemediation.QuoteName is private in another assembly — m-B), and (c) the SET clause is a hardcoded compile-time literal chosen by a switch over DbConfigSetting. Same-string invariant (M-1): the validated @db string is byte-identical to the bracketed token. - Single-connection self-gating (§2/R2-MOD-1): existence + permission (HAS_PERMS_BY_NAME(@db,'DATABASE','ALTER') wrapped ISNULL(...,0), fail closed) + live freshness read + the ALTER all run on ONE monitoring connection (no InitialCatalog retarget, no elevation). GateSpid/ExecSpid emitted for the SPID-equality proof. - Audit integrity (§7): 02_ create script amended in place (2.12.0 unreleased) — query_id/plan_id NULLABLE, action varchar(16->32), prior_value nvarchar(128) added; writer passes DBNull for null IDs and widens the @action SqlParameter to VarChar(32) (the prior 16 truncated the new taxonomy before the column). - Reachability guard (M-2): DbConfigHandler, SetDatabaseOptionAsync, PreflightDbConfigAsync added to CoreMachineryMarkers. - Deserialize threading (m-A): FromDto passes the deserialized DbConfigTargets to the ctor (round-trip test asserts they survive). - Un-apply fail-safe (m-C): RunAsync short-circuits to a clean report when isUnapply && !SupportsUnapply (never bubbles NotSupportedException). Both drill-down collectors enriched with structured auto_shrink/auto_close /page_verify fields (identical JSON names+types) so the shared extractor reads typed fields, never the human issues strings. Confirm/preview path threaded for DB_CONFIG (BuildConfirmRequest, RenderPreview, confirm-window rows + dispositions, banner). 124 Dashboard tests + 310 Lite tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <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
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.
What shipped
The second privileged Apply Fix type — the three always-safe database settings, applied via gated
ALTER DATABASE <db> SET …:AUTO_SHRINK OFFAUTO_CLOSE OFFPAGE_VERIFY CHECKSUMRCSI is excluded (destructive — never emitted by the extractor);
recovery_model/query_storeare out of scope. Apply-only (no un-apply for DB-config — these have no sensible reverse; the prior value is recorded for manual reversal). Reuses the Phase-1 remediation framework verbatim and adds one handler + the model/extractor/executor/schema/UI deltas. Single PR per plan §10.Implements the approved plan
b3-phase2-dbconfig.md(cleared two adversarial security review rounds).Security invariants enforced (file:line)
ALTER DATABASEstatement's ONLY non-constant token is the DB identifier — (a) validated by a parameterizedsys.databasesexistence check (@dbNVARCHAR(128), no concat) atDashboard/Services/DatabaseService.Remediation.cs:441-452; (b) bracketed by an inline]→]]routineQuoteIdentifierat:325(its OWN routine —FactRemediation.QuoteNameis private inPerformanceMonitor.Analysis, m-B); (c) the SET clause is a hardcoded literal chosen byswitchoverDbConfigSettinginSetClauseForat:333. Same-string invariant (M-1):ValidatedNameis set to the exact@dbvalue only on existence (:465) and is the exact string passed toBuildAlterStatement(:357, executed at:344).HAS_PERMS_BY_NAME(@db,'DATABASE','ALTER')wrappedISNULL(...,0),:448) + live freshness read + the ALTER all on ONE monitoring connection — noInitialCatalogretarget, no re-open, no elevation (SetDatabaseOptionAsync,:300-388).GateSpid/ExecSpidemitted (:385-386).02_create script amended in place (2.12.0 unreleased — no tag, csprojs2.11.0):query_id/plan_id→ NULL,action varchar(16)→(32),prior_value nvarchar(128)added (upgrades/2.11.0-to-2.12.0/02_create_remediation_action_log.sql:56-59). Writer passesDBNullfor null IDs (RemediationAuditWriter.cs:151-152) and widens@actiontoVarChar,32+ adds@prior_value(:155-157).RemediationAuditRecord.QueryId/PlanId → long?+PriorValue(RemediationResults.cs:154-157)."DbConfigHandler","SetDatabaseOptionAsync","PreflightDbConfigAsync"added toCoreMachineryMarkers(Dashboard.Tests/RemediationTests.cs:380-383);CoreMachinery_OnlyReferencedInRemediationCorenow covers the new surface.FromDtodeserializesDbConfigTargetsand passes them to the 4-arg ctor (PerformanceMonitor.Notifications/AlertContext.cs:189-203).RunAsyncshort-circuits to a cleanUnapplyNotSupportedreport whenisUnapply && !handler.SupportsUnapply(RemediationApplyService.cs:171-178) — never bubblesNotSupportedException.Both drill-down collectors enriched with structured
auto_shrink/auto_close/page_verifyfields (identical JSON names+types) so the shared extractor reads typed fields, never the humanissuesstrings (SqlServerDrillDownCollector.cs,Lite/Analysis/DrillDownCollector.cs).Headless test results (real)
dotnet build Dashboard/Dashboard.csproj -c Debug— clean (0 warnings, 0 errors)Dashboard.Tests— 124 passed, 0 failed, 0 skippedLite.Tests— 310 passed, 0 failed, 0 skippedNew tests cover: no-injection/QUOTENAME against the executor's own builder, same-string invariant (trailing-whitespace), extractor (never RCSI / multi-setting / empty-db / RCSI-only→null), render-stability golden, audit-absent hard-block, fail-closed-on-no-ALTER, freshness-skip, DB-not-found, success (null IDs + prior_value + precise taxonomy), applied-but-unlogged, per-target independence, SupportsUnapply, apply-only enforcement, confirm-path threading, AnyActionable, serialization round-trip (DbConfigTargets survive — m-A),
CoreMachineryMarkerscoverage.Maintainer's real-server pre-merge gate (plan §11, steps 1-6)
I did NOT run sql2022 — these fold into your end-to-end pass (same as Phase 1):
AUTO_SHRINK ON→ apply →is_auto_shrink_on=0, audit rowaction='set_auto_shrink_off', result='success', prior_value='ON',generated_sqlmatches executed statement. Re-apply → freshness-skip (skipped, no second ALTER).NONE→CHECKSUM,prior_value='NONE'; separate run fromTORN_PAGE_DETECTIONconfirms distinct prior.PermissionDenied, no ALTER, grant guidance.Blocked, no ALTER.02_via an Installer built at 2.12.0: table created withquery_id/plan_idNULLABLE,action varchar(32),prior_value nvarchar(128); aset_page_verify_checksumrow stores untruncated; re-run is a no-op. O1 boundary: ifv2.12.0tags before this lands, switch to a03_ALTER in2.12.0-to-2.13.0/.🤖 Generated with Claude Code