From 2da972105dd31aba195769a6d578a29db0091d13 Mon Sep 17 00:00:00 2001 From: sumit Date: Thu, 20 Aug 2026 15:54:57 +0530 Subject: [PATCH 1/2] fix(brain): authorize every Brain endpoint against its connection (#70) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit BrainController shipped with 93 of its 116 endpoints performing no authorization at all. SecurityConfig only asserts `.anyRequest().authenticated()` and JwtAuthenticationFilter only resolves a principal — neither inspects a connectionId, and there is no filter, interceptor or aspect that does. Connections are private per user (ConnectionAccessService.resolveAccess keys on ownerUsername plus an explicit grant table), so any authenticated user who passed somebody else's connection id to /brain/health-scores/{id}, /brain/data-sensitivity/{id} (which names the PII columns), /brain/cost-attribution/{id}, /brain/ml-overview/{id} and ~90 others got that user's schema, sensitivity, cost and workload intelligence back. Only the first ~15 endpoints had the check. The misses clustered by when a section was written — every later "Phase" block omitted it — not by read/write semantics, so the scalability, brain-score, classification, column-values, insights, workload, config-tuning, statistics, executions, patterns, ml-overview and query-intelligence families were open in full. All 116 now authorize: assertCanReadConnectionContent for GETs, assertCanManageConnectionContent for writes. Endpoints whose path carries some other id resolve the owning connection first, via three new getConnectionId lookups (ScalabilitySimulationService, ConfigTuningService, PlanPatternLibraryService) matching the existing BrainNoteService / BrainTaskService precedent. Two endpoints have no connection scope and are admin-only instead: /column-values/embed-all spans every connection, and /key-columns/anti-pattern/{patternId}/acknowledge is an unimplemented stub whose body never loads the anti-pattern, so there is nothing to authorize against yet. Asserts go inside each handler's try, before the catch-all that returns 500, relying on the existing `catch (ResponseStatusException e) { throw e; }` so a denial surfaces as a real 403 rather than looking like a broken feature. BrainControllerAuthorizationSafetyTest locks both properties structurally — every mapping is authorized, and every inline assert rethrows. Verified it fails on 92 endpoints against the pre-fix file and passes on 0 after, so it catches the regression rather than passing vacuously. Co-Authored-By: Claude Opus 5 (1M context) --- CLAUDE.md | 36 +++++ .../dbaagent/controller/BrainController.java | 102 ++++++++++++++ .../ScalabilitySimulationService.java | 6 + .../brain/config/ConfigTuningService.java | 6 + .../query/PlanPatternLibraryService.java | 6 + ...rainControllerAuthorizationSafetyTest.java | 124 ++++++++++++++++++ 6 files changed, 280 insertions(+) create mode 100644 backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java diff --git a/CLAUDE.md b/CLAUDE.md index 543c223..653c3ff 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -349,6 +349,42 @@ it against a real database — not a theoretical hardening pass. `POST /users/admin/reset` on every install that had run `setup-agent.sh`, since that mints an admin MCP token on each run. +### Endpoint Authorization Rules + +- **Authentication is not authorization.** `SecurityConfig` only asserts + `.anyRequest().authenticated()` and `JwtAuthenticationFilter` only resolves a + principal — neither looks at a `connectionId`. Connections are **private per user** + (`ConnectionAccessService.resolveAccess` keys on `ownerUsername` plus an explicit + grant table), so any endpoint taking a caller-supplied `connectionId` **must** call + `accessControlService.assertCanReadConnectionContent` (reads) or + `assertCanManageConnectionContent` (writes) itself. There is no filter, interceptor + or aspect that does this for you. +- **`BrainController` shipped with 93 of its 116 endpoints unguarded.** Only the first + ~15 (`/understanding`, `/notes/*`, `/tasks/*`, `/key-columns/*`, + `/inferred-relationships/*`) had the check; every later "Phase" block did not — so an + authenticated user could pass someone else's connection id to + `/brain/health-scores/{id}`, `/brain/data-sensitivity/{id}` (which names the PII + columns), `/brain/cost-attribution/{id}`, `/brain/ml-overview/{id}` and ~90 more and + read that user's database intelligence. All 116 are now guarded, and + `BrainControllerAuthorizationSafetyTest` fails the build if a new one is not. The + misses clustered by **when a section was written**, not by read/write semantics — + when adding a controller section, guard it as you write it. +- **When the path carries some other id** (`simulationId`, `experimentId`, `patternId`, + `noteId`, `taskId`), resolve the owning connection first via that service's + `getConnectionId(id)` and assert on the result. Do not skip the check because the + path has no `connectionId` in it. +- **An endpoint with no connection scope at all is admin-only.** + `POST /brain/column-values/embed-all` spans every connection, so it carries + `@PreAuthorize("hasRole('ADMIN')")` — it cannot be authorized against one + connection's grants. `@EnableMethodSecurity(prePostEnabled = true)` is on in + `SecurityConfig`, so `@PreAuthorize` is live. +- **Assert inside the `try`, and rethrow `ResponseStatusException` before the + catch-all.** Every handler in `BrainController` ends with a + `catch (Exception) -> 500`; without the earlier + `catch (ResponseStatusException e) { throw e; }` a 403 is swallowed and reported as a + server error, so a client cannot tell "not yours" from "broken". The safety test + asserts this too. + ### MCP & CLI Release Rules **Whenever you add, rename, or remove an MCP tool or a CLI subcommand, you MUST update all of these in the same commit — they are agent-facing surfaces and drift silently breaks discoverability:** diff --git a/backend/src/main/java/com/dbaagent/controller/BrainController.java b/backend/src/main/java/com/dbaagent/controller/BrainController.java index 9d4560a..1657bb1 100644 --- a/backend/src/main/java/com/dbaagent/controller/BrainController.java +++ b/backend/src/main/java/com/dbaagent/controller/BrainController.java @@ -70,6 +70,7 @@ import lombok.RequiredArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.DeleteMapping; import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; @@ -391,7 +392,14 @@ public ResponseEntity analyzeKeyColumns( } } + /** + * Not implemented. Kept admin-only rather than guarded per connection: the body + * never loads the anti-pattern, so there is no connection to authorize against. + * Whoever implements it must resolve the owning connection from {@code patternId} + * and switch to {@code assertCanManageConnectionContent}. + */ @PostMapping("/key-columns/anti-pattern/{patternId}/acknowledge") + @PreAuthorize("hasRole('ADMIN')") public ResponseEntity> acknowledgeAntiPattern( @PathVariable Long patternId ) { @@ -600,6 +608,7 @@ public ResponseEntity> analyzeQueryAntiPatterns( @PathVariable String connectionId ) { try { + accessControlService.assertCanManageConnectionContent(connectionId); queryQualityAnalysisService.analyzeQueryQuality(connectionId); List patterns = queryQualityAnalysisService.getAllQueryAntiPatterns(connectionId); @@ -631,6 +640,7 @@ public ResponseEntity> getQueryAntiPatternsBySeverity( @PathVariable String severity ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); QueryAntiPattern.Severity severityEnum = QueryAntiPattern.Severity.valueOf(severity.toUpperCase()); // Note: Need to add this method to QueryQualityAnalysisService return ResponseEntity.ok(List.of()); // TODO: Implement in service @@ -651,6 +661,7 @@ public ResponseEntity> getScalabilitySimulations( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(scalabilitySimulationService.getAllSimulations(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -665,6 +676,7 @@ public ResponseEntity getLatestSimulation( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return scalabilitySimulationService.getLatestSimulation(connectionId) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); @@ -682,6 +694,7 @@ public ResponseEntity> runScalabilitySimulation( @RequestParam(required = false) String scenario ) { try { + accessControlService.assertCanManageConnectionContent(connectionId); if (scenario != null && !scenario.isEmpty()) { // Run single scenario ScalabilitySimulation.GrowthScenario scenarioEnum = @@ -708,6 +721,7 @@ public ResponseEntity> getTablePredictions( @PathVariable String simulationId ) { try { + accessControlService.assertCanReadConnectionContent(scalabilitySimulationService.getConnectionId(simulationId)); return ResponseEntity.ok(scalabilitySimulationService.getTablePredictions(simulationId)); } catch (ResponseStatusException e) { throw e; @@ -722,6 +736,7 @@ public ResponseEntity> getHighRiskTables( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(scalabilitySimulationService.getHighRiskTables(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -738,6 +753,7 @@ public ResponseEntity getLatestBrainScore( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return brainScoreService.getLatestBrainScore(connectionId) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); @@ -755,6 +771,7 @@ public ResponseEntity> getBrainScoreHistory( @RequestParam(defaultValue = "10") int limit ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(brainScoreService.getBrainScoreHistory(connectionId, limit)); } catch (ResponseStatusException e) { throw e; @@ -769,6 +786,7 @@ public ResponseEntity calculateBrainScore( @PathVariable String connectionId ) { try { + accessControlService.assertCanManageConnectionContent(connectionId); BrainScore score = brainScoreService.calculateBrainScore(connectionId); return ResponseEntity.ok(score); } catch (ResponseStatusException e) { @@ -786,6 +804,7 @@ public ResponseEntity @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(antiPatternService.detectAntiPatterns(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -814,6 +834,7 @@ public ResponseEntity> get @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(healthScoreService.calculateHealthScores(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -842,6 +864,7 @@ public ResponseEntity> @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(partitionService.evaluatePartitionReadiness(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -884,6 +909,7 @@ public ResponseEntity> getRelationshipClas @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(relationshipClassificationService.getRelationships(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -898,6 +924,7 @@ public ResponseEntity> getIntegrityIssues( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(relationshipClassificationService.getIntegrityIssues(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -912,6 +939,7 @@ public ResponseEntity> getMissingRelations @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(relationshipClassificationService.getMissingIndexes(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -927,6 +955,7 @@ public ResponseEntity> getTablesBySensitivity( @PathVariable String level ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> level.equalsIgnoreCase(tc.getSensitivityLevel())) @@ -946,6 +975,7 @@ public ResponseEntity> getTablesByDomain( @PathVariable String domain ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> domain.equalsIgnoreCase(tc.getBusinessDomain())) @@ -964,6 +994,7 @@ public ResponseEntity> getPartitionCandidates( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tc.getPartitionReadiness() != null && @@ -984,6 +1015,7 @@ public ResponseEntity> getTablesWithAntiPatterns( @RequestParam(required = false) String severity ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tc.getAntiPatternCount() != null && tc.getAntiPatternCount() > 0) @@ -1004,6 +1036,7 @@ public ResponseEntity> getLowHealthTables( @RequestParam(defaultValue = "60") double threshold ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tc.getHealthScore() != null && tc.getHealthScore().doubleValue() < threshold) @@ -1025,6 +1058,7 @@ public ResponseEntity> getCo @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(costService.calculateCostAttribution(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -1109,6 +1148,7 @@ public ResponseEntity> g @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(shardingService.assessShardingReadiness(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -1123,6 +1163,7 @@ public ResponseEntity> get @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(dataQualityService.calculateDataQuality(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -1137,6 +1178,7 @@ public ResponseEntity> get @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return ResponseEntity.ok(growthPredictionService.predictGrowth(connectionId)); } catch (ResponseStatusException e) { throw e; @@ -1168,6 +1211,7 @@ public ResponseEntity> getTablesByLifecycle( @PathVariable String lifecycle ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> lifecycle.equalsIgnoreCase(tc.getDataLifecycle())) @@ -1186,6 +1230,7 @@ public ResponseEntity> getHighCacheValueTables( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> "HIGH_CACHE_VALUE".equalsIgnoreCase(tc.getCacheAffinity())) @@ -1205,6 +1250,7 @@ public ResponseEntity> getTablesBySchemaRisk( @PathVariable String level ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> level.equalsIgnoreCase(tc.getSchemaEvolutionRisk())) @@ -1223,6 +1269,7 @@ public ResponseEntity> getDenormalizationCandidateTabl @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> Boolean.TRUE.equals(tc.getDenormalizationCandidate())) @@ -1241,6 +1288,7 @@ public ResponseEntity> getShardingReadyTables( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> "READY".equalsIgnoreCase(tc.getShardingReadiness())) @@ -1260,6 +1308,7 @@ public ResponseEntity> getLowQualityTables( @RequestParam(defaultValue = "50") double threshold ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tc.getDataQualityScore() != null && tc.getDataQualityScore().doubleValue() < threshold) @@ -1279,6 +1328,7 @@ public ResponseEntity> getHighDependencyCriticalityTab @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> "CRITICAL".equalsIgnoreCase(tc.getDependencyCriticality()) || @@ -1298,6 +1348,7 @@ public ResponseEntity> getRapidGrowthTables( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> "EXPLOSIVE".equalsIgnoreCase(tc.getGrowthCategory()) || @@ -1318,6 +1369,7 @@ public ResponseEntity> getTablesByCostTier( @PathVariable String tier ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List tables = schemaClassificationService.getAllTableClassifications(connectionId) .stream() .filter(tc -> tier.equalsIgnoreCase(tc.getCostTier())) @@ -1343,6 +1395,7 @@ public ResponseEntity> getColumnValues( @RequestParam(required = false) String tableName ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List values = columnValueCollectionService.getCachedColumns(connectionId, tableName); return ResponseEntity.ok(values); } catch (ResponseStatusException e) { @@ -1361,6 +1414,7 @@ public ResponseEntity> getColumnValueStats( @PathVariable String connectionId ) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map stats = columnValueCollectionService.getStatistics(connectionId); return ResponseEntity.ok(stats); } catch (ResponseStatusException e) { @@ -1380,6 +1434,7 @@ public ResponseEntity> refreshColumnValues( @PathVariable String connectionId ) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Manually triggering column value refresh for connection: {}", connectionId); // Run async and return immediately @@ -1409,8 +1464,12 @@ public ResponseEntity> refreshColumnValues( /** * Embed all unembedded column values. * Useful for re-syncing Azure AI Search after migration or data loss. + * + *

Spans every connection with no per-connection scope, so it cannot be + * authorized against a single connection's grants — admin only. */ @PostMapping("/column-values/embed-all") + @PreAuthorize("hasRole('ADMIN')") public ResponseEntity> embedAllColumnValues() { try { int count = columnValueCollectionService.embedAllUnembedded(); @@ -1436,6 +1495,7 @@ public ResponseEntity> embedAllColumnValues() { @PostMapping("/insights/{connectionId}/embed") public ResponseEntity> embedBrainInsights(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Embedding Brain insights for connection: {}", connectionId); Map result = brainInsightEmbeddingService.embedAllInsights(connectionId); return ResponseEntity.ok(result); @@ -1457,6 +1517,7 @@ public ResponseEntity> embedBrainInsights(@PathVariable Stri @PostMapping("/insights/{connectionId}/embed/patterns") public ResponseEntity> embedPlanPatterns(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); int count = brainInsightEmbeddingService.embedPlanPatterns(connectionId); return ResponseEntity.ok(Map.of( "success", true, @@ -1477,6 +1538,7 @@ public ResponseEntity> embedPlanPatterns(@PathVariable Strin @PostMapping("/insights/{connectionId}/embed/workload") public ResponseEntity> embedWorkloadInsight(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); boolean success = brainInsightEmbeddingService.embedWorkloadInsight(connectionId); return ResponseEntity.ok(Map.of( "success", success, @@ -1496,6 +1558,7 @@ public ResponseEntity> embedWorkloadInsight(@PathVariable St @PostMapping("/insights/{connectionId}/embed/cardinality") public ResponseEntity> embedCardinalityInsights(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); int count = brainInsightEmbeddingService.embedCardinalityInsights(connectionId); return ResponseEntity.ok(Map.of( "success", true, @@ -1518,6 +1581,7 @@ public ResponseEntity> embedCardinalityInsights(@PathVariabl @PostMapping("/workload/collect/{connectionId}") public ResponseEntity collectWorkloadMetrics(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Collecting workload metrics for connection: {}", connectionId); WorkloadMetricsSnapshot snapshot = metricsCollectorService.collectMetrics(connectionId); return ResponseEntity.ok(snapshot); @@ -1535,6 +1599,7 @@ public ResponseEntity collectWorkloadMetrics(@PathVaria @PostMapping("/workload/characterize/{connectionId}") public ResponseEntity characterizeWorkload(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Characterizing workload for connection: {}", connectionId); WorkloadProfile profile = workloadCharacterizationService.characterizeWorkload(connectionId); return ResponseEntity.ok(profile); @@ -1552,6 +1617,7 @@ public ResponseEntity characterizeWorkload(@PathVariable String @GetMapping("/workload/profile/{connectionId}") public ResponseEntity getWorkloadProfile(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); return workloadCharacterizationService.getProfile(connectionId) .map(ResponseEntity::ok) .orElse(ResponseEntity.notFound().build()); @@ -1570,6 +1636,7 @@ public ResponseEntity getWorkloadProfile(@PathVariable String c @GetMapping("/workload/status/{connectionId}") public ResponseEntity> getWorkloadStatus(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map status = new java.util.HashMap<>(); // Snapshot count @@ -1620,6 +1687,7 @@ public ResponseEntity> findSimilarWorkloads( @PathVariable String connectionId, @RequestParam(defaultValue = "5") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List similar = workloadCharacterizationService.findSimilarProfiles(connectionId, limit); return ResponseEntity.ok(similar); } catch (ResponseStatusException e) { @@ -1640,6 +1708,7 @@ public ResponseEntity> identifyKnobs( @PathVariable String connectionId, @RequestParam(defaultValue = "LATENCY") KnobRanking.TargetMetric targetMetric) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Identifying knobs for connection: {} targeting: {}", connectionId, targetMetric); List rankings = knobIdentificationService.identifyKnobs(connectionId, targetMetric); return ResponseEntity.ok(rankings); @@ -1660,6 +1729,7 @@ public ResponseEntity> getTopKnobs( @RequestParam(defaultValue = "LATENCY") KnobRanking.TargetMetric targetMetric, @RequestParam(defaultValue = "5") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List topKnobs = knobIdentificationService.getTopKnobs(connectionId, targetMetric, limit); return ResponseEntity.ok(topKnobs); } catch (ResponseStatusException e) { @@ -1676,6 +1746,7 @@ public ResponseEntity> getTopKnobs( @GetMapping("/config/rankings/{connectionId}") public ResponseEntity> getAllKnobRankings(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List rankings = knobIdentificationService.getAllRankings(connectionId); return ResponseEntity.ok(rankings); } catch (ResponseStatusException e) { @@ -1693,6 +1764,7 @@ public ResponseEntity> getAllKnobRankings(@PathVariable String public ResponseEntity> generateConfigRecommendations( @PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Generating ML-based config recommendations for: {}", connectionId); List recommendations = configTuningService.generateRecommendations(connectionId); return ResponseEntity.ok(recommendations); @@ -1712,6 +1784,7 @@ public ResponseEntity startTuningExperiment( @PathVariable String connectionId, @org.springframework.web.bind.annotation.RequestBody ExperimentRequest request) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Starting tuning experiment for: {}", connectionId); TuningExperiment experiment = configTuningService.startExperiment( connectionId, request.getKnobChanges(), request.getRecommendationId()); @@ -1732,6 +1805,7 @@ public ResponseEntity startTuningExperiment( @PostMapping("/config/experiments/{experimentId}/complete") public ResponseEntity completeTuningExperiment(@PathVariable String experimentId) { try { + accessControlService.assertCanManageConnectionContent(configTuningService.getConnectionId(experimentId)); log.info("Completing experiment: {}", experimentId); TuningExperiment experiment = configTuningService.completeExperiment(experimentId); return ResponseEntity.ok(experiment); @@ -1751,6 +1825,7 @@ public ResponseEntity completeTuningExperiment(@PathVariable S @DeleteMapping("/config/experiments/{experimentId}") public ResponseEntity cancelTuningExperiment(@PathVariable String experimentId) { try { + accessControlService.assertCanManageConnectionContent(configTuningService.getConnectionId(experimentId)); log.info("Cancelling experiment: {}", experimentId); configTuningService.cancelExperiment(experimentId); return ResponseEntity.noContent().build(); @@ -1772,6 +1847,7 @@ public ResponseEntity> getExperimentHistory( @PathVariable String connectionId, @RequestParam(defaultValue = "10") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List history = configTuningService.getExperimentHistory(connectionId, limit); return ResponseEntity.ok(history); } catch (ResponseStatusException e) { @@ -1788,6 +1864,7 @@ public ResponseEntity> getExperimentHistory( @GetMapping("/config/experiments/{connectionId}/success-rate") public ResponseEntity> getExperimentSuccessRate(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); double successRate = configTuningService.getExperimentSuccessRate(connectionId); return ResponseEntity.ok(Map.of( "connectionId", connectionId, @@ -1811,6 +1888,7 @@ public ResponseEntity> collectTableStatistics( @PathVariable String connectionId, @PathVariable String tableName) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Collecting statistics for table: {} in connection: {}", tableName, connectionId); List stats = cardinalityEstimationService.collectTableStatistics(connectionId, tableName); return ResponseEntity.ok(stats); @@ -1828,6 +1906,7 @@ public ResponseEntity> collectTableStatistics( @GetMapping("/statistics/{connectionId}") public ResponseEntity> getColumnStatistics(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List stats = cardinalityEstimationService.getStatistics(connectionId); return ResponseEntity.ok(stats); } catch (ResponseStatusException e) { @@ -1846,6 +1925,7 @@ public ResponseEntity> getHighCardinalityColumns( @PathVariable String connectionId, @RequestParam(defaultValue = "1000") long minDistinct) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List columns = cardinalityEstimationService.getHighCardinalityColumns(connectionId, minDistinct); return ResponseEntity.ok(columns); } catch (ResponseStatusException e) { @@ -1862,6 +1942,7 @@ public ResponseEntity> getHighCardinalityColumns( @GetMapping("/statistics/{connectionId}/skewed") public ResponseEntity> getSkewedColumns(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List columns = cardinalityEstimationService.getSkewedColumns(connectionId); return ResponseEntity.ok(columns); } catch (ResponseStatusException e) { @@ -1880,6 +1961,7 @@ public ResponseEntity> estimateCardinality( @PathVariable String connectionId, @org.springframework.web.bind.annotation.RequestBody CardinalityEstimateRequest request) { try { + accessControlService.assertCanManageConnectionContent(connectionId); long estimate; if (request.getLowBound() != null || request.getHighBound() != null) { estimate = cardinalityEstimationService.estimateRange( @@ -1909,6 +1991,7 @@ public ResponseEntity> estimateCardinality( @PostMapping("/statistics/{connectionId}/refresh") public ResponseEntity> refreshStaleStatistics(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); int refreshed = cardinalityEstimationService.refreshStaleStatistics(connectionId); return ResponseEntity.ok(Map.of("refreshedCount", refreshed)); } catch (ResponseStatusException e) { @@ -1926,6 +2009,7 @@ public ResponseEntity> refreshStaleStatistics(@PathVariable @GetMapping("/statistics/{connectionId}/accuracy") public ResponseEntity> getCardinalityAccuracy(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map accuracy = adaptivePlanScoringService.getCardinalityAccuracyStats(connectionId); return ResponseEntity.ok(accuracy); } catch (ResponseStatusException e) { @@ -1946,6 +2030,7 @@ public ResponseEntity recordExecution( @PathVariable String connectionId, @org.springframework.web.bind.annotation.RequestBody ExecutionRecordRequest request) { try { + accessControlService.assertCanManageConnectionContent(connectionId); PlanExecution execution = adaptivePlanScoringService.recordExecution( connectionId, request.getQuery(), request.getActualExecutionMs(), request.getActualRows()); return ResponseEntity.ok(execution); @@ -1965,6 +2050,7 @@ public ResponseEntity> getRecentExecutions( @PathVariable String connectionId, @RequestParam(defaultValue = "20") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List executions = adaptivePlanScoringService.getRecentExecutions(connectionId, limit); return ResponseEntity.ok(executions); } catch (ResponseStatusException e) { @@ -1983,6 +2069,7 @@ public ResponseEntity> getCardinalityErrors( @PathVariable String connectionId, @RequestParam(defaultValue = "10") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List errors = adaptivePlanScoringService.getSignificantCardinalityErrors(connectionId, limit); return ResponseEntity.ok(errors); } catch (ResponseStatusException e) { @@ -1999,6 +2086,7 @@ public ResponseEntity> getCardinalityErrors( @GetMapping("/calibration/{connectionId}") public ResponseEntity> getCalibrationStatus(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map status = adaptivePlanScoringService.getCalibrationStatus(connectionId); return ResponseEntity.ok(status); } catch (ResponseStatusException e) { @@ -2015,6 +2103,7 @@ public ResponseEntity> getCalibrationStatus(@PathVariable St @GetMapping("/executions/{connectionId}/multiple-plans") public ResponseEntity>> getQueriesWithMultiplePlans(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map> result = adaptivePlanScoringService.findQueriesWithMultiplePlans(connectionId); return ResponseEntity.ok(result); } catch (ResponseStatusException e) { @@ -2051,6 +2140,7 @@ public ResponseEntity>> getPatternSuggestions( @PathVariable String connectionId, @org.springframework.web.bind.annotation.RequestBody QueryRequest request) { try { + accessControlService.assertCanManageConnectionContent(connectionId); List> suggestions = planPatternLibraryService.getSuggestions(connectionId, request.getQuery()); return ResponseEntity.ok(suggestions); } catch (ResponseStatusException e) { @@ -2067,6 +2157,7 @@ public ResponseEntity>> getPatternSuggestions( @GetMapping("/patterns/{connectionId}/reliable") public ResponseEntity> getReliablePatterns(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List patterns = planPatternLibraryService.getReliablePatterns(connectionId); return ResponseEntity.ok(patterns); } catch (ResponseStatusException e) { @@ -2085,6 +2176,7 @@ public ResponseEntity> getMostUsedPatterns( @PathVariable String connectionId, @RequestParam(defaultValue = "10") int limit) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List patterns = planPatternLibraryService.getMostUsedPatterns(connectionId, limit); return ResponseEntity.ok(patterns); } catch (ResponseStatusException e) { @@ -2101,6 +2193,7 @@ public ResponseEntity> getMostUsedPatterns( @GetMapping("/patterns/{connectionId}/with-optimizations") public ResponseEntity> getPatternsWithOptimizations(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); List patterns = planPatternLibraryService.getPatternsWithOptimizations(connectionId); return ResponseEntity.ok(patterns); } catch (ResponseStatusException e) { @@ -2117,6 +2210,7 @@ public ResponseEntity> getPatternsWithOptimizations(@PathVaria @GetMapping("/patterns/{connectionId}/stats") public ResponseEntity> getPatternStatistics(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map stats = planPatternLibraryService.getPatternStatistics(connectionId); return ResponseEntity.ok(stats); } catch (ResponseStatusException e) { @@ -2135,6 +2229,7 @@ public ResponseEntity recordPatternFeedback( @PathVariable String patternId, @RequestParam boolean wasSuccessful) { try { + accessControlService.assertCanManageConnectionContent(planPatternLibraryService.getConnectionId(patternId)); planPatternLibraryService.recordFeedback(patternId, wasSuccessful); return ResponseEntity.ok().build(); } catch (ResponseStatusException e) { @@ -2151,6 +2246,7 @@ public ResponseEntity recordPatternFeedback( @PostMapping("/patterns/{connectionId}/cleanup") public ResponseEntity> cleanupPatterns(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); int deleted = planPatternLibraryService.cleanupIneffectivePatterns(connectionId); return ResponseEntity.ok(Map.of("deletedCount", deleted)); } catch (ResponseStatusException e) { @@ -2169,6 +2265,7 @@ public ResponseEntity> cleanupPatterns(@PathVariable String @GetMapping("/ml-overview/{connectionId}") public ResponseEntity> getMlOverview(@PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); Map overview = new java.util.HashMap<>(); // Workload Profile @@ -2211,6 +2308,7 @@ public ResponseEntity> getMlOverview(@PathVariable String co @DeleteMapping("/query-intelligence/{connectionId}/executions") public ResponseEntity> clearExecutions(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); long countBefore = adaptivePlanScoringService.getRecentExecutions(connectionId, 1).isEmpty() ? 0 : adaptivePlanScoringService.getCardinalityAccuracyStats(connectionId).get("totalExecutions") instanceof Number n ? n.longValue() : 0; adaptivePlanScoringService.clearExecutions(connectionId); @@ -2238,6 +2336,7 @@ public ResponseEntity> clearExecutions(@PathVariable String @PostMapping("/query-intelligence/{connectionId}/backfill") public ResponseEntity> backfillQueryIntelligence(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); log.info("Starting Query Intelligence backfill for connection {}", connectionId); // Get history summaries (lightweight) instead of full entities to avoid OOM @@ -2302,6 +2401,7 @@ public ResponseEntity startExplai @PathVariable String connectionId, @RequestParam(defaultValue = "500") int maxQueries) { try { + accessControlService.assertCanManageConnectionContent(connectionId); // Limit max queries to prevent runaway jobs int safeMax = Math.min(maxQueries, 2000); var progress = adaptivePlanScoringService.startExplainBackfill(connectionId, safeMax); @@ -2321,6 +2421,7 @@ public ResponseEntity startExplai public ResponseEntity getExplainProgress( @PathVariable String connectionId) { try { + accessControlService.assertCanReadConnectionContent(connectionId); var progress = adaptivePlanScoringService.getExplainProgress(connectionId); return ResponseEntity.ok(progress); } catch (ResponseStatusException e) { @@ -2337,6 +2438,7 @@ public ResponseEntity getExplainP @PostMapping("/query-intelligence/{connectionId}/explain-cancel") public ResponseEntity cancelExplainBackfill(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); adaptivePlanScoringService.cancelExplainBackfill(connectionId); return ResponseEntity.ok().build(); } catch (ResponseStatusException e) { diff --git a/backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java b/backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java index e2f541b..974ec6e 100644 --- a/backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java +++ b/backend/src/main/java/com/dbaagent/service/brain/analysis/ScalabilitySimulationService.java @@ -504,6 +504,12 @@ public List getTablePredictions(String simulationId) { return predictionRepository.findByScalabilitySimulationId(simulationId); } + public String getConnectionId(String simulationId) { + return simulationRepository.findById(simulationId) + .map(ScalabilitySimulation::getConnectionId) + .orElseThrow(() -> new IllegalArgumentException("Simulation not found")); + } + /** * Get high-risk tables. */ diff --git a/backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java b/backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java index a472e79..1af5306 100644 --- a/backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java +++ b/backend/src/main/java/com/dbaagent/service/brain/config/ConfigTuningService.java @@ -723,6 +723,12 @@ public void cancelExperiment(String experimentId) { experimentRepository.delete(experiment); } + public String getConnectionId(String experimentId) { + return experimentRepository.findById(experimentId) + .map(TuningExperiment::getConnectionId) + .orElseThrow(() -> new IllegalArgumentException("Experiment not found: " + experimentId)); + } + /** * Get experiment history. */ diff --git a/backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java b/backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java index d60cb18..db5abb1 100644 --- a/backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java +++ b/backend/src/main/java/com/dbaagent/service/brain/query/PlanPatternLibraryService.java @@ -194,6 +194,12 @@ public void recordFeedback(String patternId, boolean wasSuccessful) { } } + public String getConnectionId(String patternId) { + return patternRepository.findById(patternId) + .map(PlanPattern::getConnectionId) + .orElseThrow(() -> new IllegalArgumentException("Pattern not found: " + patternId)); + } + /** * Get reliable patterns for a connection. */ diff --git a/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java b/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java new file mode 100644 index 0000000..547267b --- /dev/null +++ b/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java @@ -0,0 +1,124 @@ +package com.dbaagent.controller; + +import org.junit.jupiter.api.Test; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.List; +import java.util.regex.Matcher; +import java.util.regex.Pattern; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Every {@code /brain/**} endpoint must authorize the caller against the connection it + * touches, not merely require a logged-in user. + * + *

This shipped with 93 of 116 endpoints unguarded. {@code SecurityConfig} only asserts + * {@code .anyRequest().authenticated()} and {@code JwtAuthenticationFilter} only resolves a + * principal — neither inspects a {@code connectionId}. Connections are private per user + * ({@code ConnectionAccessService.resolveAccess} keys on {@code ownerUsername} plus an + * explicit grant table), so an authenticated user who passed somebody else's connection id + * to {@code /brain/health-scores/{id}}, {@code /brain/data-sensitivity/{id}} (which names + * the PII columns), {@code /brain/cost-attribution/{id}} and ~90 others got that user's + * database intelligence back. + * + *

Resource-level authorization here is opt-in per method, and the misses clustered by + * when a section was written rather than by read/write semantics — the first ~15 endpoints + * had it and every later "Phase" block did not. That is a defect a reviewer catches once + * and a test catches forever, so it is asserted structurally: scanned as text, because the + * property under test is that no endpoint method body lacks the call, whatever it does. + */ +class BrainControllerAuthorizationSafetyTest { + + private static final Path CONTROLLER = + Path.of("src/main/java/com/dbaagent/controller/BrainController.java"); + + private static final Pattern MAPPING = + Pattern.compile("^\\s*@(Get|Post|Delete|Put|Patch)Mapping\\b"); + + /** A method is authorized by a per-connection assert, or by being admin-only. */ + private static final Pattern AUTHORIZED = Pattern.compile( + "accessControlService\\.assertCan(Read|Manage)ConnectionContent\\(|@PreAuthorize"); + + private record Endpoint(int line, String mapping, String body) {} + + /** + * Slices the controller into one entry per handler: from its mapping annotation to the + * method's closing brace, which at this nesting level is a line that is exactly + * {@code " }"}. + */ + private static List endpoints() throws IOException { + List lines = Files.readAllLines(CONTROLLER); + List endpoints = new ArrayList<>(); + + for (int i = 0; i < lines.size(); i++) { + Matcher matcher = MAPPING.matcher(lines.get(i)); + if (!matcher.find()) { + continue; + } + StringBuilder body = new StringBuilder(); + int end = i; + while (end < lines.size()) { + body.append(lines.get(end)).append('\n'); + if (end > i && lines.get(end).equals(" }")) { + break; + } + end++; + } + endpoints.add(new Endpoint(i + 1, lines.get(i).trim(), body.toString())); + } + return endpoints; + } + + @Test + void everyBrainEndpointAuthorizesTheCallerAgainstTheConnection() throws IOException { + List offenders = new ArrayList<>(); + + for (Endpoint endpoint : endpoints()) { + if (!AUTHORIZED.matcher(endpoint.body()).find()) { + offenders.add(CONTROLLER + ":" + endpoint.line() + " " + endpoint.mapping()); + } + } + + assertThat(offenders) + .as("Each of these BrainController endpoints takes a caller-supplied id and " + + "never authorizes it. Authentication is not authorization: connections " + + "are private per user, so this hands one user another user's schema, " + + "sensitivity, cost and workload intelligence. Add " + + "accessControlService.assertCanReadConnectionContent(connectionId) to " + + "reads and assertCanManageConnectionContent(connectionId) to writes, " + + "resolving the connection id first when the path carries some other id. " + + "An endpoint with no connection scope at all is admin-only (@PreAuthorize).") + .isEmpty(); + } + + /** + * The asserts live inside each handler's {@code try}, and every handler ends with a + * {@code catch (Exception)} that returns 500. Without an earlier + * {@code catch (ResponseStatusException e) { throw e; }} the 403 would be swallowed and + * reported as a server error — the denial would still hold, but it would look like a + * bug in the feature rather than a permission boundary, and a client could not tell + * "not yours" from "broken". + */ + @Test + void authorizationFailuresPropagateAsForbiddenRatherThanServerError() throws IOException { + List offenders = new ArrayList<>(); + + for (Endpoint endpoint : endpoints()) { + String body = endpoint.body(); + boolean guardedInline = body.contains("accessControlService.assertCan"); + if (guardedInline && !body.contains("catch (ResponseStatusException e)")) { + offenders.add(CONTROLLER + ":" + endpoint.line() + " " + endpoint.mapping()); + } + } + + assertThat(offenders) + .as("These endpoints assert access inside a try whose catch-all converts the " + + "403 into a 500. Rethrow it first: " + + "catch (ResponseStatusException e) { throw e; }") + .isEmpty(); + } +} From 724b2e8ef56ab7466ec5ef88c41ab41239185747 Mon Sep 17 00:00:00 2001 From: sumit Date: Thu, 20 Aug 2026 17:16:34 +0530 Subject: [PATCH 2/2] fix(brain): guard the fully-qualified calibration DELETE and 404 bad ids MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by hands-on QA against the running stack, not by reading the diff. DELETE /brain/calibration/{connectionId} was annotated `@org.springframework.web.bind.annotation.DeleteMapping` rather than the bare `@DeleteMapping` every other handler uses. The sweep that added the access checks matched on `^\s*@(Get|Post|Delete|Put)Mapping`, so this one endpoint was skipped — and it is destructive. Verified live: a user holding only a CHAT_EDITOR grant on a *different* connection got 200 from `DELETE /brain/calibration/`; it is 403 for both the read-granted and the ungranted connection now, and still 200 for the owner. BrainControllerAuthorizationSafetyTest shared the blind spot exactly — it reported "116 endpoints, 0 unguarded" while that endpoint was open, which is the vacuous-pass failure mode CLAUDE.md's verification anti-patterns warn about. Its mapping pattern now accepts an optional package qualifier; confirmed it reports 117/1-unguarded against the pre-fix shape and 117/0 after. Also: the three new getConnectionId lookups throw IllegalArgumentException for an unknown id, and two of their handlers had no IllegalArgumentException catch, so a bogus simulation/pattern id returned 500 instead of 404 (observed, with the stack trace in the backend log). Access was still correctly denied — this was a status-code and log-noise defect, not an exposure. Both now 404, matching the pre-existing convention at /inferred-relationships/{id}/validate. Not fixed here, pre-existing and out of scope: POST /brain/tasks and POST /brain/tasks/{taskId}/status have the same missing-404 shape via brainTaskService.getConnectionId, and reproduce on v1.2.0. Co-Authored-By: Claude Opus 5 (1M context) --- .../java/com/dbaagent/controller/BrainController.java | 7 ++++++- .../BrainControllerAuthorizationSafetyTest.java | 11 +++++++++-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/backend/src/main/java/com/dbaagent/controller/BrainController.java b/backend/src/main/java/com/dbaagent/controller/BrainController.java index 1657bb1..636cdc3 100644 --- a/backend/src/main/java/com/dbaagent/controller/BrainController.java +++ b/backend/src/main/java/com/dbaagent/controller/BrainController.java @@ -723,6 +723,8 @@ public ResponseEntity> getTablePredictions( try { accessControlService.assertCanReadConnectionContent(scalabilitySimulationService.getConnectionId(simulationId)); return ResponseEntity.ok(scalabilitySimulationService.getTablePredictions(simulationId)); + } catch (IllegalArgumentException e) { + return ResponseEntity.notFound().build(); } catch (ResponseStatusException e) { throw e; } catch (Exception e) { @@ -2117,9 +2119,10 @@ public ResponseEntity>> getQueriesWithMultiplePlans(@Pa /** * Reset cost calibration. */ - @org.springframework.web.bind.annotation.DeleteMapping("/calibration/{connectionId}") + @DeleteMapping("/calibration/{connectionId}") public ResponseEntity resetCalibration(@PathVariable String connectionId) { try { + accessControlService.assertCanManageConnectionContent(connectionId); adaptivePlanScoringService.resetCalibration(connectionId); return ResponseEntity.ok().build(); } catch (ResponseStatusException e) { @@ -2232,6 +2235,8 @@ public ResponseEntity recordPatternFeedback( accessControlService.assertCanManageConnectionContent(planPatternLibraryService.getConnectionId(patternId)); planPatternLibraryService.recordFeedback(patternId, wasSuccessful); return ResponseEntity.ok().build(); + } catch (IllegalArgumentException e) { + return ResponseEntity.notFound().build(); } catch (ResponseStatusException e) { throw e; } catch (Exception e) { diff --git a/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java b/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java index 547267b..fd76a56 100644 --- a/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java +++ b/backend/src/test/java/com/dbaagent/controller/BrainControllerAuthorizationSafetyTest.java @@ -36,8 +36,15 @@ class BrainControllerAuthorizationSafetyTest { private static final Path CONTROLLER = Path.of("src/main/java/com/dbaagent/controller/BrainController.java"); - private static final Pattern MAPPING = - Pattern.compile("^\\s*@(Get|Post|Delete|Put|Patch)Mapping\\b"); + /** + * Matches a fully-qualified annotation as well as a bare one. A + * {@code @org.springframework.web.bind.annotation.DeleteMapping} on + * {@code DELETE /calibration/{connectionId}} is exactly how the one destructive + * endpoint escaped the first sweep of this fix — a bare-name-only pattern silently + * skips it, so the endpoint reads as "not an endpoint" rather than "unguarded". + */ + private static final Pattern MAPPING = Pattern.compile( + "^\\s*@(?:[\\w.]*\\.)?(Get|Post|Delete|Put|Patch)Mapping\\b"); /** A method is authorized by a per-connection assert, or by being admin-only. */ private static final Pattern AUTHORIZED = Pattern.compile(